From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liang Chen Subject: Re: [PATCH] Using the tlb flush util function where applicable Date: Mon, 15 Sep 2014 18:49:10 -0400 Message-ID: <54176CE6.3080006@gmail.com> References: <1410556005-15639-1-git-send-email-liangchen.linux@gmail.com> <20140915193351.GA3356@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: pbonzini@redhat.com, kvm@vger.kernel.org To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: Received: from mail-qg0-f44.google.com ([209.85.192.44]:37596 "EHLO mail-qg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756898AbaIOWtL (ORCPT ); Mon, 15 Sep 2014 18:49:11 -0400 Received: by mail-qg0-f44.google.com with SMTP id f51so4829332qge.3 for ; Mon, 15 Sep 2014 15:49:10 -0700 (PDT) In-Reply-To: <20140915193351.GA3356@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/15/2014 03:33 PM, Radim Kr=C4=8Dm=C3=A1=C5=99 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 >> --- > 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 *vcp= u, int cpu) >> struct desc_ptr *gdt =3D &__get_cpu_var(host_gdt); >> unsigned long sysenter_esp; >> =20 >> - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); >> + kvm_mmu_flush_tlb(vcpu); >> local_irq_disable(); >> crash_disable_local_vmclear(cpu); >> =20 > 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 solu= tion to make sure the counter really shows the number of tlb flushes - not the = time=20 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=C4=8Dm=C3=A1=C5=99 > --- > (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_v= cpu *vcpu, > =20 > 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 *v= cpu) > } > 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 =3D KVM_EXIT_TPR_ACCESS; > r =3D 0;