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

["Radim Krčmář" <rkrcmar@redhat.com>]

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.