Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check

[Paolo Bonzini]

On 25/02/20 13:43, Vitaly Kuznetsov wrote:
> If I'm not mistaken, the logic this function was supposed to implement
> is: change the requested bit to the requested state and, if
> kvm_apicv_activated() changed (we set the first bit or cleared the
> last), proceed with KVM_REQ_APICV_UPDATE. What if we re-write it like
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2103101eca78..b97b8ff4a789 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8027,19 +8027,19 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
>   */
>  void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>  {
> +       bool apicv_was_activated = kvm_apicv_activated(kvm);
> +
>         if (!kvm_x86_ops->check_apicv_inhibit_reasons ||
>             !kvm_x86_ops->check_apicv_inhibit_reasons(bit))
>                 return;
>  
> -       if (activate) {
> -               if (!test_and_clear_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
> -                   !kvm_apicv_activated(kvm))
> -                       return;
> -       } else {
> -               if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
> -                   kvm_apicv_activated(kvm))
> -                       return;
> -       }
> +       if (activate)
> +               clear_bit(bit, &kvm->arch.apicv_inhibit_reasons);
> +       else
> +               set_bit(bit, &kvm->arch.apicv_inhibit_reasons);
> +
> +       if (kvm_apicv_activated(kvm) == apicv_was_activated)
> +               return;

Yes, I got to the same conclusion before seeing you message.  Another
possibility is to use cmpxchg, which I slightly prefer because if there
are multiple concurrent updates it has some possibilities of avoiding
the atomic operation and consequent cacheline bouncing.  I've sent a patch.

Paolo