[PATCH] Using the tlb flush util function where applicable

[PATCH] Using the tlb flush util function where applicable

[Liang Chen]

Using kvm_mmu_flush_tlb as the other places to make sure vcpu
 stat is incremented

Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index bfe11cf..439682e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
 		unsigned long sysenter_esp;
 
-		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
+		kvm_mmu_flush_tlb(vcpu);
 		local_irq_disable();
 		crash_disable_local_vmclear(cpu);
 
-- 
1.9.1

Re: [PATCH] Using the tlb flush util function where applicable

[Radim Krčmář]

2014-09-12 17:06-0400, Liang Chen:
> Using kvm_mmu_flush_tlb as the other places to make sure vcpu
>  stat is incremented
> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> ---

Good catch.

>  arch/x86/kvm/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index bfe11cf..439682e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
>  		unsigned long sysenter_esp;
>  
> -		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> +		kvm_mmu_flush_tlb(vcpu);
>  		local_irq_disable();
>  		crash_disable_local_vmclear(cpu);
>  

And to hijack this thread ...
I noticed three other deficits in stat.tlb_flush, patch below.

Do you prefer the current behavior?

--- 8< ---
KVM: x86: count actual tlb flushes

- 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().

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
(And what about a possible followup patch that replaces
 kvm_mmu_flush_tlb() with kvm_make_request() again?
 It would free the namespace a bit and we could call something
 similarly named from vcpu_enter_guest() to do the job.)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9314678..b41fd97 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
 
 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);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 916e895..0f0ad08 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6041,8 +6041,10 @@ 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))
+		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
+			++vcpu->stat.tlb_flush;
 			kvm_x86_ops->tlb_flush(vcpu);
+		}
 		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
 			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
 			r = 0;

Re: [PATCH] Using the tlb flush util function where applicable

[Liang Chen]

On 09/15/2014 03:33 PM, Radim Krčmář wrote:
> 2014-09-12 17:06-0400, Liang Chen:
>> Using kvm_mmu_flush_tlb as the other places to make sure vcpu
>>  stat is incremented
>>
>> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
>> ---
> Good catch.
>
>>  arch/x86/kvm/vmx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index bfe11cf..439682e 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
>>  		unsigned long sysenter_esp;
>>  
>> -		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
>> +		kvm_mmu_flush_tlb(vcpu);
>>  		local_irq_disable();
>>  		crash_disable_local_vmclear(cpu);
>>  
> And to hijack this thread ...
> I noticed three other deficits in stat.tlb_flush, patch below.
>
> Do you prefer the current behavior?

Yeah, you are right. I think the patch below gives a more thorough solution to
make sure the counter really shows the number of tlb flushes - not the time 
kvm_mmu_flush_tlb is called. Thanks!

> --- 8< ---
> KVM: x86: count actual tlb flushes
>
> - 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().
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> (And what about a possible followup patch that replaces
>  kvm_mmu_flush_tlb() with kvm_make_request() again?
>  It would free the namespace a bit and we could call something
>  similarly named from vcpu_enter_guest() to do the job.)

That seems good. I can take the labor to make the followup patch ;)

Thanks,
Liang Chen

> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 9314678..b41fd97 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
>  
>  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);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 916e895..0f0ad08 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6041,8 +6041,10 @@ 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))
> +		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
> +			++vcpu->stat.tlb_flush;
>  			kvm_x86_ops->tlb_flush(vcpu);
> +		}
>  		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
>  			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
>  			r = 0;

Re: [PATCH] Using the tlb flush util function where applicable

[Paolo Bonzini]

Il 16/09/2014 00:49, Liang Chen ha scritto:
>> > ---
>> > (And what about a possible followup patch that replaces
>> >  kvm_mmu_flush_tlb() with kvm_make_request() again?
>> >  It would free the namespace a bit and we could call something
>> >  similarly named from vcpu_enter_guest() to do the job.)
> That seems good. I can take the labor to make the followup patch ;)

Ok, so I'll not apply your first patch.

Thanks!

Paolo

Re: [PATCH] Using the tlb flush util function where applicable

[Wanpeng Li]

Hi Radim,
On Mon, Sep 15, 2014 at 09:33:52PM +0200, Radim Krčmář wrote:
>2014-09-12 17:06-0400, Liang Chen:
>> Using kvm_mmu_flush_tlb as the other places to make sure vcpu
>>  stat is incremented
>> 
>> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
>> ---
>
>Good catch.
>
>>  arch/x86/kvm/vmx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index bfe11cf..439682e 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
>>  		unsigned long sysenter_esp;
>>  
>> -		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
>> +		kvm_mmu_flush_tlb(vcpu);
>>  		local_irq_disable();
>>  		crash_disable_local_vmclear(cpu);
>>  
>
>And to hijack this thread ...
>I noticed three other deficits in stat.tlb_flush, patch below.
>
>Do you prefer the current behavior?
>
>--- 8< ---
>KVM: x86: count actual tlb flushes
>
>- we count KVM_REQ_TLB_FLUSH requests, not actual flushes

So there maybe multiple requests accumulated at the point of kvm_check_request, 
if your patch account these accumulations correctly?

Regards,
Wanpeng Li 

>  (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().
>
>Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>---
>(And what about a possible followup patch that replaces
> kvm_mmu_flush_tlb() with kvm_make_request() again?
> It would free the namespace a bit and we could call something
> similarly named from vcpu_enter_guest() to do the job.)
>
>diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>index 9314678..b41fd97 100644
>--- a/arch/x86/kvm/mmu.c
>+++ b/arch/x86/kvm/mmu.c
>@@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
> 
> 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);
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 916e895..0f0ad08 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -6041,8 +6041,10 @@ 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))
>+		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
>+			++vcpu->stat.tlb_flush;
> 			kvm_x86_ops->tlb_flush(vcpu);
>+		}
> 		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
> 			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
> 			r = 0;
>--
>To unsubscribe from this list: send the line "unsubscribe kvm" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Using the tlb flush util function where applicable

[Radim Krčmář]

2014-09-17 08:15+0800, Wanpeng Li:
> Hi Radim,
> On Mon, Sep 15, 2014 at 09:33:52PM +0200, Radim Krčmář wrote:
> >Do you prefer the current behavior?
> >
> >--- 8< ---
> >KVM: x86: count actual tlb flushes
> >
> >- we count KVM_REQ_TLB_FLUSH requests, not actual flushes
> 
> So there maybe multiple requests accumulated at the point of kvm_check_request, 
> if your patch account these accumulations correctly?

It will ignore request accumulations and count it as one TLB flush,
we have to decide what is correct (the value is just statistics)

 a) count local KVM_REQ_TLB_FLUSH requests
 b) count all TLB flushes
 c) both (a) and (b)

I was thinking that when you look at /sys/kernel/debug/kvm/tlb_flushes,
you are interested in the number of TLB flushes that VMs did, not
requests, so you won't have to add remote_tlb_flushes multiplied by
maximal vcpu count and guess their amount from this upper bound.

And that we don't even care about requests, so (c) is just complication.

---
I tried to get an idea about the number of coalesced requests and added
a counter called tlb_flush_real, making option (c).
After a night of reading virtio-rng (different experiment) on 1 VCPU VM:

 # cat /sys/kernel/debug/kvm/{tlb_flush{,_real},remote_tlb_flush}
 5927
 5206
 0

Re: [PATCH] Using the tlb flush util function where applicable

[Paolo Bonzini]

Il 17/09/2014 12:45, Radim Krčmář ha scritto:
>  a) count local KVM_REQ_TLB_FLUSH requests
>  b) count all TLB flushes
>  c) both (a) and (b)
> 
> I was thinking that when you look at /sys/kernel/debug/kvm/tlb_flushes,
> you are interested in the number of TLB flushes that VMs did, not
> requests, so you won't have to add remote_tlb_flushes multiplied by
> maximal vcpu count and guess their amount from this upper bound.

I agree.  The difference isn't too big, but if two requests are
coalesced they will only cause a hit once.

Paolo

> And that we don't even care about requests, so (c) is just complication.