| Message ID | 1473415834-98576-1-git-send-email-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(+)