[PATCH/RFC] KVM: do early exit in kvm_check_request

[PATCH/RFC] KVM: do early exit in kvm_check_request

[Christian Borntraeger]

By checking vcpu->requests we can do an early exit and allow gcc
to optimize multiple kvm_check_request into one block for the
common case (no requests).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/linux/kvm_host.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 1c9c973..b15b460 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1115,6 +1115,8 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
 
 static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
 {
+	if (likely(!vcpu->requests))
+		return false;
 	if (test_bit(req, &vcpu->requests)) {
 		clear_bit(req, &vcpu->requests);
 
-- 
2.5.5

Re: [PATCH/RFC] KVM: do early exit in kvm_check_request

[Paolo Bonzini]

On 09/09/2016 12:10, Christian Borntraeger wrote:
> By checking vcpu->requests we can do an early exit and allow gcc
> to optimize multiple kvm_check_request into one block for the
> common case (no requests).
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  include/linux/kvm_host.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1c9c973..b15b460 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1115,6 +1115,8 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
>  
>  static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
>  {
> +	if (likely(!vcpu->requests))
> +		return false;
>  	if (test_bit(req, &vcpu->requests)) {
>  		clear_bit(req, &vcpu->requests);

I'm not sure -- due to asm in test_bit and to -fno-strict-aliasing, I'm
afraid that each kvm_check_request will have its own zero check.
kvm_check_request should be rare, but it does show up in microbenchmarks
so perhaps it's best to keep those two lines of code duplicated across
the architectures.

Paolo

Re: [PATCH/RFC] KVM: do early exit in kvm_check_request

[Christian Borntraeger]

On 09/09/2016 06:35 PM, Paolo Bonzini wrote:
> 
> 
> On 09/09/2016 12:10, Christian Borntraeger wrote:
>> By checking vcpu->requests we can do an early exit and allow gcc
>> to optimize multiple kvm_check_request into one block for the
>> common case (no requests).
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  include/linux/kvm_host.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 1c9c973..b15b460 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1115,6 +1115,8 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
>>  
>>  static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
>>  {
>> +	if (likely(!vcpu->requests))
>> +		return false;
>>  	if (test_bit(req, &vcpu->requests)) {
>>  		clear_bit(req, &vcpu->requests);
> 
> I'm not sure -- due to asm in test_bit and to -fno-strict-aliasing, I'm
> afraid that each kvm_check_request will have its own zero check.
> kvm_check_request should be rare, but it does show up in microbenchmarks
> so perhaps it's best to keep those two lines of code duplicated across
> the architectures.

Not sure either, it seems to work like above (but does not work when I combine both ifs..)
So yes, maybe its just too unreliable and we keep things as is.

Re: [PATCH/RFC] KVM: do early exit in kvm_check_request

[Nadav Amit]

Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 09/09/2016 12:10, Christian Borntraeger wrote:
>> By checking vcpu->requests we can do an early exit and allow gcc
>> to optimize multiple kvm_check_request into one block for the
>> common case (no requests).
>> 
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> include/linux/kvm_host.h | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 1c9c973..b15b460 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1115,6 +1115,8 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
>> 
>> static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
>> {
>> +	if (likely(!vcpu->requests))
>> +		return false;
>> 	if (test_bit(req, &vcpu->requests)) {
>> 		clear_bit(req, &vcpu->requests);
> 
> I'm not sure -- due to asm in test_bit and to -fno-strict-aliasing, I'm
> afraid that each kvm_check_request will have its own zero check.
> kvm_check_request should be rare, but it does show up in microbenchmarks
> so perhaps it's best to keep those two lines of code duplicated across
> the architectures.

Interesting. I don’t think that this test_bit usually goes through the asm
part (since usually req is constant). However, it seems that due to the
indirection to vcpu->requests, the compiler does not use a register to cache
vcpu->requests. It is shameful in places like vcpu_enter_guest. Perhaps
checking the bit directly in kvm_check_request is wiser?

Nadav