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

[Wanpeng Li <wanpeng.li@linux.intel.com>]

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