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

Hannes Reinecke
2020-11-16T10:15:41Z

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)?
Does this mean that the original definition was wrong?
Or does this mean that this patch implies a different meaning of
'sbitmap_weight'?
In either case, it's not 'just' an export, so it does warrant an
explanation.

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