Message ID | 20201116090737.50989-6-ming.lei@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | blk-mq/scsi: tracking device queue depth via sbitmap | expand |
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
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
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
On 16/11/2020 09:07, Ming Lei wrote: > + > +/** > + * 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); > + Hi Ming, Just a nit on the comment - I think that "real bits set" is vague. How about "set and not cleared bits", "busy bits", "net set bits" or "aggregate bits set"? Thanks, John
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);