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 kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
