linux-kernel - Re: [PATCH 07/49] KVM: x86: replace bitmap_weight with bitmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YgaaqJNPyGvw9uKi@google.com>
Date:   Fri, 11 Feb 2022 17:19:36 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     Yury Norov <yury.norov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        David Laight <David.Laight@...lab.com>,
        Joe Perches <joe@...ches.com>, Dennis Zhou <dennis@...nel.org>,
        Emil Renner Berthing <kernel@...il.dk>,
        Nicholas Piggin <npiggin@...il.com>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Alexey Klimov <aklimov@...hat.com>,
        linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH 07/49] KVM: x86: replace bitmap_weight with bitmap_empty
 where appropriate

On Fri, Feb 11, 2022, Christophe JAILLET wrote:
> Le 10/02/2022 à 23:48, Yury Norov a écrit :
> > In some places kvm/hyperv.c code calls bitmap_weight() to check if any bit
> > of a given bitmap is set. It's better to use bitmap_empty() in that case
> > because bitmap_empty() stops traversing the bitmap as soon as it finds
> > first set bit, while bitmap_weight() counts all bits unconditionally.
> > 
> > Signed-off-by: Yury Norov <yury.norov@...il.com>
> > Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> > ---
> >   arch/x86/kvm/hyperv.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 6e38a7d22e97..06c2a5603123 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -90,7 +90,7 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >   {
> >   	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
> >   	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
> > -	int auto_eoi_old, auto_eoi_new;
> > +	bool auto_eoi_old, auto_eoi_new;
> >   	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
> >   		return;
> > @@ -100,16 +100,16 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >   	else
> >   		__clear_bit(vector, synic->vec_bitmap);
> > -	auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256);
> > +	auto_eoi_old = !bitmap_empty(synic->auto_eoi_bitmap, 256);
> 
> I think that you can also remove the "!" here, ...
> 
> >   	if (synic_has_vector_auto_eoi(synic, vector))
> >   		__set_bit(vector, synic->auto_eoi_bitmap);
> >   	else
> >   		__clear_bit(vector, synic->auto_eoi_bitmap);
> > -	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
> > +	auto_eoi_new = !bitmap_empty(synic->auto_eoi_bitmap, 256);
> 
> ... and there...
> 
> > -	if (!!auto_eoi_old == !!auto_eoi_new)
> > +	if (auto_eoi_old == auto_eoi_new)
> 
> ... because this test would still give the same result.

It would give the same result, but the variable names would be inverted as they
track if "auto EOI" is being used.  So yes, it's technically unnecessary, but
also very deliberate.