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

Hannes Reinecke
2020-11-17T06:57:45Z

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?

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer