Re: tlb_flush stat on Intel/AMD

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Mon, Jul 20, 2020 at 12:08:05PM -0700, Jacob Xu wrote:
> Hi all,
> 
> I wrote a kvm selftest to enter a 1-vcpu guest VM running a nop in one
> thread and check the VM's debugfs entry for # of tlb flushes while
> that was running. Installing the latest upstream kernel and running
> this on an intel host showed a tlb_flush count of 30, while running it
> on an amd host shows the tlb_flush count at 0.
> 
> Do we have an inconsistency between Intel and AMD in how VCPU_STAT is
> incremented?

Yes, you can even drop "between Intel and AMD" and just state "we have
inconsistencies in how VCPU_STAT is incremented".

> From browsing the code, we see that the stat only gets incremented in
> the kvm_ wrappers of the x86_ops functions tlb_flush_all,
> tlb_flush_current, and tlb_flush_guest. These wrappers are only called
> via the KVM request api (referred to as deferred flushes in some other
> threads), and there other instances of calling the x86_ops tlb_flush
> methods directly (immediate flush).
> 
> It looks like most of the tlb flush calls are deferred, but there are
> a few instances using the immediate flush where it's not counted
> (kvm_mmu_load, svm_set_cr4, vmx_set_apic_access_page,
> nested_prepare_vmcb_control). Is there a guideline on when to
> deferred/immediate tlb_flush?

The rule of thumb is to defer the flush whenever possible so that KVM
doesn't flush multiple times in a single VM-Exit.   However, of the above
three, svm_set_cr4() is the only one that can be deferred, but only
because kvm_mmu_load() and vmx_set_apic_access_page() are reachable after
KVM_REQ_TLB_FLUSH_CURRENT is processed in vcpu_enter_guest().

The VMX APIC flush could be deferred by hoisting KVM_REQ_APIC_PAGE_RELOAD
up.  I think that's safe?  But it's a very infrequent operation so I'm not
exactly chomping at the bit to get it fixed.

> Could this be a cause for the lower tlb_flush stat seen on an AMD
> host? Or perhaps there's another reason for the difference due to the
> (too) simple selftest?

Yes, but it's likely not due to any of the paths listed above.   Obviously
accounting the flush in kvm_mmu_load() will elevate the count, but it will
affect VMX and SVM identically.

Given that you see 0 on SVM and a low number on VMX, my money is on the
difference being that VMX accounts the TLB flush that occurs on vCPU
migration.  vmx_vcpu_load_vmcs() makes a KVM_REQ_TLB_FLUSH request, whereas
svm_vcpu_load() resets asid_generation but doesn't increment the stats.
 
> In the case of svm_tlb_flush, it seems like the tlb flush is deferred
> anyway since the response to setting a tlb flush control bit in the
> VMCB is not acted upon until entering the guest. So it seems we could
> count tlb flushes on svm more easily by incrementing the counter by
> checking the control bit before KVM_RUN. Though perhaps there's
> another case we'd like to count as tlb flush when the guest switches
> ASID (where would we track this?).
>
> Would switching to this alternative for incrementing tlb_flush stat in
> svm be much different than what we do right now?

I think a better option is to keep the current accounting, defer flushes
when possible to naturally fix accounting, and then fix the remaining one
off cases, e.g. kvm_mmu_load() and svm_vcpu_load().

I can prep a small series unless you want the honors?