| Message ID | 20201116090737.50989-6-ming.lei@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V4,01/12] sbitmap: remove sbitmap_clear_bit_unlock | expand |
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
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
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); void sbitmap_show(struct sbitmap *sb, struct seq_file *m) { seq_printf(m, "depth=%u\n", sb->depth); - seq_printf(m, "busy=%u\n", sbitmap_weight(sb) - sbitmap_cleared(sb)); + seq_printf(m, "busy=%u\n", sbitmap_weight(sb)); seq_printf(m, "cleared=%u\n", sbitmap_cleared(sb)); seq_printf(m, "bits_per_word=%u\n", 1U << sb->shift); seq_printf(m, "map_nr=%u\n", sb->map_nr);