[PATCH v2] KVM: x86: count actual tlb flushes

[PATCH v2] KVM: x86: count actual tlb flushes

[Liang Chen]

- we count KVM_REQ_TLB_FLUSH requests, not actual flushes
(KVM can have multiple requests for one flush)
- flushes from kvm_flush_remote_tlbs aren't counted
- it's easy to make a direct request by mistake

Solve these by postponing the counting to kvm_check_request(),
and refactor the code to use kvm_make_request again.

Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
---
Changes since v2:

* Instead of calling kvm_mmu_flush_tlb everywhere to make sure the
stat is always incremented, postponing the counting to
kvm_check_request.

(The idea comes from Radim. Much of the work is indeed done by him
and is included in this patch, otherwise I couldn't start working
on the followup work as I promised early. As I'm new to kvm
development, please let me know if I am doing wrong here.)

 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/mmu.c              | 17 +++++------------
 arch/x86/kvm/vmx.c              |  2 +-
 arch/x86/kvm/x86.c              | 10 ++++++++--
 4 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 7c492ed..77ade89 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -917,7 +917,6 @@ void kvm_inject_nmi(struct kvm_vcpu *vcpu);
 
 int fx_init(struct kvm_vcpu *vcpu);
 
-void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu);
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes);
 int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9314678..acc2d0c5 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1749,7 +1749,7 @@ static int __kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 		return 1;
 	}
 
-	kvm_mmu_flush_tlb(vcpu);
+	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 	return 0;
 }
 
@@ -1802,7 +1802,7 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 
 	kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list);
 	if (flush)
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 }
 
 struct mmu_page_path {
@@ -2536,7 +2536,7 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	      true, host_writable)) {
 		if (write_fault)
 			*emulate = 1;
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 	}
 
 	if (unlikely(is_mmio_spte(*sptep) && emulate))
@@ -3450,13 +3450,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
 	context->nx = false;
 }
 
-void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
-{
-	++vcpu->stat.tlb_flush;
-	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
-}
-EXPORT_SYMBOL_GPL(kvm_mmu_flush_tlb);
-
 void kvm_mmu_new_cr3(struct kvm_vcpu *vcpu)
 {
 	mmu_free_roots(vcpu);
@@ -3962,7 +3955,7 @@ static void mmu_pte_write_flush_tlb(struct kvm_vcpu *vcpu, bool zap_page,
 	if (remote_flush)
 		kvm_flush_remote_tlbs(vcpu->kvm);
 	else if (local_flush)
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 }
 
 static u64 mmu_pte_write_fetch_gpte(struct kvm_vcpu *vcpu, gpa_t *gpa,
@@ -4223,7 +4216,7 @@ EXPORT_SYMBOL_GPL(kvm_mmu_page_fault);
 void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva)
 {
 	vcpu->arch.mmu.invlpg(vcpu, gva);
-	kvm_mmu_flush_tlb(vcpu);
+	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 	++vcpu->stat.invlpg;
 }
 EXPORT_SYMBOL_GPL(kvm_mmu_invlpg);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index bfe11cf..bb0a7ab 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6617,7 +6617,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 	switch (type) {
 	case VMX_EPT_EXTENT_GLOBAL:
 		kvm_mmu_sync_roots(vcpu);
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		nested_vmx_succeed(vcpu);
 		break;
 	default:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8f1e22d..f9fca52f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -726,7 +726,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 {
 	if (cr3 == kvm_read_cr3(vcpu) && !pdptrs_changed(vcpu)) {
 		kvm_mmu_sync_roots(vcpu);
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		return 0;
 	}
 
@@ -5989,6 +5989,12 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
 	kvm_apic_update_tmr(vcpu, tmr);
 }
 
+static void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
+{
+	++vcpu->stat.tlb_flush;
+	kvm_x86_ops->tlb_flush(vcpu);
+}
+
 /*
  * Returns 1 to let __vcpu_run() continue the guest execution loop without
  * exiting to the userspace.  Otherwise, the value will be returned to the
@@ -6018,7 +6024,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
 			kvm_mmu_sync_roots(vcpu);
 		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
-			kvm_x86_ops->tlb_flush(vcpu);
+			kvm_mmu_flush_tlb(vcpu);
 		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
 			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
 			r = 0;
-- 
1.9.1

Re: [PATCH v2] KVM: x86: count actual tlb flushes

[Xiao Guangrong]

On 09/18/2014 02:35 AM, Liang Chen wrote:
> - we count KVM_REQ_TLB_FLUSH requests, not actual flushes
> (KVM can have multiple requests for one flush)
> - flushes from kvm_flush_remote_tlbs aren't counted
> - it's easy to make a direct request by mistake
> 
> Solve these by postponing the counting to kvm_check_request(),

It's good.

> and refactor the code to use kvm_make_request again.

Why this refactor is needed? It's really a bad idea using
raw-bit-set instead of meaningful name.

[ Btw, maybe kvm_mmu_flush_local_tlb is a better name than
kvm_mmu_flush_tlb() in the current code. ]

Re: [PATCH v2] KVM: x86: count actual tlb flushes

[Paolo Bonzini]

Il 18/09/2014 07:05, Xiao Guangrong ha scritto:
> On 09/18/2014 02:35 AM, Liang Chen wrote:
>> - we count KVM_REQ_TLB_FLUSH requests, not actual flushes
>> (KVM can have multiple requests for one flush)
>> - flushes from kvm_flush_remote_tlbs aren't counted
>> - it's easy to make a direct request by mistake
>>
>> Solve these by postponing the counting to kvm_check_request(),
> 
> It's good.
> 
>> and refactor the code to use kvm_make_request again.
> 
> Why this refactor is needed? It's really a bad idea using
> raw-bit-set instead of meaningful name.

set_bit is worse than kvm_make_request, but adding a one-line wrapper
around kvm_make_request is not particularly useful.

We have the following requests:

- used multiple times:
        kvm_make_request(KVM_REQ_APF_HALT, vcpu);
        kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_EVENT, vcpu);
        kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
        kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
        kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);

- used once:
        kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
        kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
        kvm_make_request(KVM_REQ_NMI, vcpu);
        kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
        kvm_make_request(KVM_REQ_PMI, pmc->vcpu);
        kvm_make_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu);

So I'm pretty much ambivalent about that.  However, I agree with this
suggestion:

> [ Btw, maybe kvm_mmu_flush_local_tlb is a better name than
> kvm_mmu_flush_tlb() in the current code. ]

Or even better, call it kvm_vcpu_flush_tlb since it is all within
x86.c/vmx.c/svm.c and the MMU is not involved at all.

Paolo

Re: [PATCH v2] KVM: x86: count actual tlb flushes

[Liang Chen]

On 09/18/2014 08:47 AM, Paolo Bonzini wrote:
> Il 18/09/2014 07:05, Xiao Guangrong ha scritto:
>> On 09/18/2014 02:35 AM, Liang Chen wrote:
>>> - we count KVM_REQ_TLB_FLUSH requests, not actual flushes
>>> (KVM can have multiple requests for one flush)
>>> - flushes from kvm_flush_remote_tlbs aren't counted
>>> - it's easy to make a direct request by mistake
>>>
>>> Solve these by postponing the counting to kvm_check_request(),
>> It's good.
>>
>>> and refactor the code to use kvm_make_request again.
>> Why this refactor is needed? It's really a bad idea using
>> raw-bit-set instead of meaningful name.
> set_bit is worse than kvm_make_request, but adding a one-line wrapper
> around kvm_make_request is not particularly useful.
>
> We have the following requests:
>
> - used multiple times:
>         kvm_make_request(KVM_REQ_APF_HALT, vcpu);
>         kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_EVENT, vcpu);
>         kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>         kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
>         kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>
> - used once:
>         kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
>         kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>         kvm_make_request(KVM_REQ_NMI, vcpu);
>         kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
>         kvm_make_request(KVM_REQ_PMI, pmc->vcpu);
>         kvm_make_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu);
>
> So I'm pretty much ambivalent about that.  However, I agree with this
> suggestion:
>
>> [ Btw, maybe kvm_mmu_flush_local_tlb is a better name than
>> kvm_mmu_flush_tlb() in the current code. ]
> Or even better, call it kvm_vcpu_flush_tlb since it is all within
> x86.c/vmx.c/svm.c and the MMU is not involved at all.
>
> Paolo

Thanks for the quick feedback! I will incorporate the name change into the
next iteration of the patch.

Thanks!
Liang Chen

Re: [PATCH v2] KVM: x86: count actual tlb flushes

[Radim Krčmář]

2014-09-17 14:35-0400, Liang Chen:
> - we count KVM_REQ_TLB_FLUSH requests, not actual flushes
> (KVM can have multiple requests for one flush)
> - flushes from kvm_flush_remote_tlbs aren't counted
> - it's easy to make a direct request by mistake
> 
> Solve these by postponing the counting to kvm_check_request(),
> and refactor the code to use kvm_make_request again.
> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> ---
> Changes since v2:
> 
> * Instead of calling kvm_mmu_flush_tlb everywhere to make sure the
> stat is always incremented, postponing the counting to
> kvm_check_request.
> 
> (The idea comes from Radim. Much of the work is indeed done by him
> and is included in this patch, otherwise I couldn't start working
> on the followup work as I promised early. As I'm new to kvm
> development, please let me know if I am doing wrong here.)

I found (shame on me) Documentation/development-process/ when looking
how to help and it looks really good.
(If you read it, the rest of my mail will be obsolete :)

You usually want to Cc linux-kernel@vger.kernel.org.
(I've heard that someone actually reads it directly and it is a good
 archive otherwise.  It allows people to `git blame` your code and find
 the discussion in their preferred mail reader.)

The hard part about posting a patch is splitting it ...
You want to separate logical changes to make the code maintainable:
For this patch, I would create at least two-part series (cover letter!)

 - change the meaning of tlb_flush
 - refactor code

And see if it would make sense to split the refactoring further or if it
breaks when only a first part of the whole series is applied.

It's not a problem if your code depends on unmerged patches, you can
include someone else's code in the series as long as it isn't modified.
(Which probably is better than just mentioning that your code depends on
 some other patches from the list, but I'm not applying it ... Paolo?)

Re: [PATCH v2] KVM: x86: count actual tlb flushes

[Radim Krčmář]

2014-09-18 14:47+0200, Paolo Bonzini:
> Il 18/09/2014 07:05, Xiao Guangrong ha scritto:
> > On 09/18/2014 02:35 AM, Liang Chen wrote:
> >> - we count KVM_REQ_TLB_FLUSH requests, not actual flushes
> >> (KVM can have multiple requests for one flush)
> >> - flushes from kvm_flush_remote_tlbs aren't counted
> >> - it's easy to make a direct request by mistake
> >>
> >> Solve these by postponing the counting to kvm_check_request(),
> > 
> > It's good.
> > 
> >> and refactor the code to use kvm_make_request again.
> > 
> > Why this refactor is needed? It's really a bad idea using
> > raw-bit-set instead of meaningful name.
> 
> set_bit is worse than kvm_make_request, but adding a one-line wrapper
> around kvm_make_request is not particularly useful.
> 
> We have the following requests:
> 
> - used multiple times:
>         kvm_make_request(KVM_REQ_APF_HALT, vcpu);
>         kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_EVENT, vcpu);
>         kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>         kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
>         kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> 
> - used once:
>         kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
>         kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>         kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>         kvm_make_request(KVM_REQ_NMI, vcpu);
>         kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
>         kvm_make_request(KVM_REQ_PMI, pmc->vcpu);
>         kvm_make_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu);
> 
> So I'm pretty much ambivalent about that.

I love abstractions, but consistency is above that, so I'd rather use
kvm_make_requests unless we modified all of them ... which is a lot of
work for the gain.  (make_request is just clunky)

> However, I agree with this > suggestion:
> 
> > [ Btw, maybe kvm_mmu_flush_local_tlb is a better name than
> > kvm_mmu_flush_tlb() in the current code. ]
> 
> Or even better, call it kvm_vcpu_flush_tlb since it is all within
> x86.c/vmx.c/svm.c and the MMU is not involved at all.

Good ideas.

Re: [PATCH v2] KVM: x86: count actual tlb flushes

[Liang Chen]

On 09/18/2014 10:00 AM, Radim Krčmář wrote:
> 2014-09-17 14:35-0400, Liang Chen:
>> - we count KVM_REQ_TLB_FLUSH requests, not actual flushes
>> (KVM can have multiple requests for one flush)
>> - flushes from kvm_flush_remote_tlbs aren't counted
>> - it's easy to make a direct request by mistake
>>
>> Solve these by postponing the counting to kvm_check_request(),
>> and refactor the code to use kvm_make_request again.
>>
>> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
>> ---
>> Changes since v2:
>>
>> * Instead of calling kvm_mmu_flush_tlb everywhere to make sure the
>> stat is always incremented, postponing the counting to
>> kvm_check_request.
>>
>> (The idea comes from Radim. Much of the work is indeed done by him
>> and is included in this patch, otherwise I couldn't start working
>> on the followup work as I promised early. As I'm new to kvm
>> development, please let me know if I am doing wrong here.)
> I found (shame on me) Documentation/development-process/ when looking
> how to help and it looks really good.
> (If you read it, the rest of my mail will be obsolete :)
>
> You usually want to Cc linux-kernel@vger.kernel.org.
> (I've heard that someone actually reads it directly and it is a good
>  archive otherwise.  It allows people to `git blame` your code and find
>  the discussion in their preferred mail reader.)
>
> The hard part about posting a patch is splitting it ...
> You want to separate logical changes to make the code maintainable:
> For this patch, I would create at least two-part series (cover letter!)
>
>  - change the meaning of tlb_flush
>  - refactor code
>
> And see if it would make sense to split the refactoring further or if it
> breaks when only a first part of the whole series is applied.
>
> It's not a problem if your code depends on unmerged patches, you can
> include someone else's code in the series as long as it isn't modified.
> (Which probably is better than just mentioning that your code depends on
>  some other patches from the list, but I'm not applying it ... Paolo?)

Thank you very much for the help! Creating a patch series and including
your patch intact as the first one sound to be the best ;)

Thanks,
Liang