Messages in this thread | | | Date | Fri, 19 Sep 2014 21:35:56 +0800 | From | Xiao Guangrong <> | Subject | Re: [PATCH v3 2/2] KVM: x86: directly use kvm_make_request again |
| |
On 09/19/2014 08:25 PM, Radim Krčmář wrote:
>>> * Returns 1 to let __vcpu_run() continue the guest execution loop without >>> * exiting to the userspace. Otherwise, the value will be returned to the >>> @@ -6018,8 +6024,7 @@ 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)) { >>> - ++vcpu->stat.tlb_flush; >>> - kvm_x86_ops->tlb_flush(vcpu); >>> + kvm_vcpu_flush_tlb(vcpu); >> >> NACK! >> >> Do not understand why you have to introduce a meaningful name >> here - it's used just inner a function, which can not help to >> improve a readability of the code at all. > > I prefer the new hunk > - it makes the parent function simpler (not everyone wants to read how > we do tlb flushes when looking at vcpu_enter_guest)
Using one line instead of two lines does not simplify parent function much.
> - the function is properly named
kvm_x86_ops->tlb_flush(vcpu) is also a good hit to tell the reader it is doing tlb flush. :)
> - we do a similar thing with kvm_gen_kvmclock_update
I understand this raw-bit-set style is largely used in current kvm code, however, it does not mean it's a best way do it. It may be turned off someday as it is be used in more and more places.
Anyway, the meaningful name wrapping raw-bit-set is a right direction and let's keep this right direction.
> >> What i suggested is renaming kvm_mmu_flush_tlb() since it's a >> API used in multiple files - a good name helps developer to >> know what it's doing and definitely easier typing. > > I think it is a good idea. > The proposed name is definitely better than what we have now. > > You can see reasons that led me to prefer raw request below. > (Preferring something else is no way means that I'm against your idea.)
I understand that, Radim! :)
> > --- > I'm always trying to reach some ideal code in my mind, which makes me > seemingly oppose good proposals because I see how it could be even > better ... and I opt not to do them. > (Pushing minor refactoring patches upstream is hard!) > > My issues with kvm_mmu_flush_tlb: > > - 'kvm_flush_remote_tlbs()' calls tlb request directly; > our wrapper thus cannot be extended with features, which makes it a > poor abstraction
kvm_flush_remote_tlbs does not only set tlb request but also handles memory order and syncs the tlb state.
I guess you wanted to say kvm_mmu_flush_tlb here, it is a API name and let it be easily used in other files. It's not worth committing a patch doing nothing except reverting the meaningful name.
> - we don't do this for other requests
See above.
> - direct request isn't absolutely horrible to read and write > (I totally agree that it is bad.) > - we call one function 'kvm_mmu_flush_tlb()' and the second one > 'kvm_flush_remote_tlbs()' and I'd need to look why
Yeah, this is why i suggested to rename kvm_mmu_flush_tlb since which clarifies things better: - kvm_flush_remote_tlbs: flush tlb in all vcpus - kvm_vcpu_flush_tlb: only flush tlb on the vcpu specified by @vcpu.
> > Which is why just removing it solves more problems for me :)
Thank you for raising this question and letting me know the patch's history. :)
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |