Re: [PATCH V4 05/12] sbitmap: export sbitmap_weight

[Ming Lei <ming.lei@redhat.com>]

On Mon, Nov 16, 2020 at 10:38:58AM +0100, Hannes Reinecke wrote:
> On 11/16/20 10:07 AM, Ming Lei wrote:
> > SCSI's .device_busy will be converted to sbitmap, and sbitmap_weight
> > is needed, so export the helper.
> > 
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> > Cc: Sumanesh Samanta <sumanesh.samanta@broadcom.com>
> > Cc: Ewan D. Milne <emilne@redhat.com>
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Tested-by: Sumanesh Samanta <sumanesh.samanta@broadcom.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   include/linux/sbitmap.h |  9 +++++++++
> >   lib/sbitmap.c           | 11 ++++++-----
> >   2 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/sbitmap.h b/include/linux/sbitmap.h
> > index 103b41c03311..34343ce3ef6c 100644
> > --- a/include/linux/sbitmap.h
> > +++ b/include/linux/sbitmap.h
> > @@ -346,6 +346,15 @@ static inline int sbitmap_test_bit(struct sbitmap *sb, unsigned int bitnr)
> >    */
> >   void sbitmap_show(struct sbitmap *sb, struct seq_file *m);
> > +
> > +/**
> > + * sbitmap_weight() - Return how many real bits set in a &struct sbitmap.
> > + * @sb: Bitmap to check.
> > + *
> > + * Return: How many real bits set
> > + */
> > +unsigned int sbitmap_weight(const struct sbitmap *sb);
> > +
> >   /**
> >    * sbitmap_bitmap_show() - Write a hex dump of a &struct sbitmap to a &struct
> >    * seq_file.
> > diff --git a/lib/sbitmap.c b/lib/sbitmap.c
> > index dcd6a89b4d2f..fb1d3c2f70a2 100644
> > --- a/lib/sbitmap.c
> > +++ b/lib/sbitmap.c
> > @@ -342,20 +342,21 @@ static unsigned int __sbitmap_weight(const struct sbitmap *sb, bool set)
> >   	return weight;
> >   }
> > -static unsigned int sbitmap_weight(const struct sbitmap *sb)
> > +static unsigned int sbitmap_cleared(const struct sbitmap *sb)
> >   {
> > -	return __sbitmap_weight(sb, true);
> > +	return __sbitmap_weight(sb, false);
> >   }
> > -static unsigned int sbitmap_cleared(const struct sbitmap *sb)
> > +unsigned int sbitmap_weight(const struct sbitmap *sb)
> >   {
> > -	return __sbitmap_weight(sb, false);
> > +	return __sbitmap_weight(sb, true) - sbitmap_cleared(sb);
> >   }
> > +EXPORT_SYMBOL_GPL(sbitmap_weight);
> That is extremely confusing. Why do you change the meaning of
> 'sbitmap_weight' from __sbitmap_weight(sb, true) to
> __sbitmap_weight(sb, true) - __sbitmap_weight(sb, false)?

Because the only user of sbitmap_weight() just uses the following way:

	sbitmap_weight(sb) - sbitmap_cleared(sb)

Frankly, I think sbitmap_weight(sb) should return real busy bits.

> Does this mean that the original definition was wrong?
> Or does this mean that this patch implies a different meaning of
> 'sbitmap_weight'?

Yeah, this patch changes meaning of sbitmap_weight(), now it is
exported, and we should make it more accurate/readable from user view.


Thanks, 
Ming