Re: [PATCH 07/49] KVM: x86: replace bitmap_weight with bitmap_empty where appropriate

[Sean Christopherson]

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@xxxxxxxxx>
> > Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > ---
> >   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.