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

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

On Tue, Nov 17, 2020 at 07:57:40AM +0100, Hannes Reinecke wrote:
> On 11/17/20 3:10 AM, Ming Lei wrote:
> > 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.
> > 
> No argument about that. Just wanted to be clear that this is by intention.
> 
> > > 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.
> > 
> So can you please state this in the patch description?

Sure.

Thanks, 
Ming