Re: [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/01/2016 02:47, Takuya Yoshikawa wrote:
> 
> Since some request handlers can make other requests which need to be
> checked in the following path, this young number is valuable. In this
> sense, I agree. Your patch looks good to me.
> 
>> This patch can go in for 4.5.  Regarding the other patch,
>> KVM_REQ_MCLOCK_INPROGRESS is indeed not really necessary, see
>> http://www.spinics.net/lists/kvm/msg95944.html and the follow-up.  Were
>> you thinking of the same?  If so I would prefer to have some comments.
> 
> I did not notice that discussion.
> 
> What I have in mind is:
>   http://www.spinics.net/lists/kvm/msg80477.html
> 
> We have more requests than that time and the overhead of
> requests checking has become much worse: Hyper-V code may
> still add some more if-branches, which will always be false
> for many guests.

True, on the other hand they will be well predicted.  Any time I tried
reordering kvm_check_request or other similar micro-optimizations,
everything became slower.  Same for replacing all the clear_bit with a
single xchg and then testing the bits on the value that xchg returned.

Even "obvious" replacements such as

-	if (vcpu->requests)
+	if (vcpu->requests & ~(1 << KVM_REQ_EVENT))

weren't clear winners!  But anyway, this is where I would start from;
not for_each_set_bit or __ffs.

There are other optimizations that are easily done, unrelated to
vcpu->requests: lapic_in_kernel is a dup of kvm_vcpu_has_lapic, but
without the static key optimization.  I prefer the lapic_in_kernel name,
but it has the less efficient implementation.  They ought to be merged.

If you really want to free bits, a possible optimization would be to
move KVM_REQ_REPORT_TPR_ACCESS, KVM_REQ_TRIPLE_FAULT, KVM_REQ_HV_CRASH,
KVM_REQ_HV_RESET, and KVM_REQ_HV_EXIT to a separate
vcpu->vmexit_requests field.  You could either check it on every vmexit,
or just consolidate them into a KVM_REQ_VMEXIT bit.

> Anyway, your patches make enough room for now, so I do not need
> to remove KVM_REQ_MCLOCK_INPROGRESS dummy request for that. It
> depends just on our preference.

I've never liked the dummy request really, but I've never felt like
arguing for its removal either. :)

Paolo