Messages in this thread | | | From | Yury Norov <> | Date | Wed, 27 Jan 2021 16:47:50 -0800 | Subject | Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct |
| |
On Wed, Jan 27, 2021 at 12:02 AM Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > > [Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct] On 26/01/2021 (Tue 23:16) Andy Shevchenko wrote: > > > On Tue, Jan 26, 2021 at 12:11:36PM -0500, Paul Gortmaker wrote: > > > This will reduce parameter passing and enable using nbits as part > > > of future dynamic region parameter parsing. > > > > One nit below, nevertheless > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > Cc: Yury Norov <yury.norov@gmail.com> > > > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Suggested-by: Yury Norov <yury.norov@gmail.com> > > > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> > > > --- > > > lib/bitmap.c | 19 ++++++++++--------- > > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > > > > diff --git a/lib/bitmap.c b/lib/bitmap.c > > > index 75006c4036e9..162e2850c622 100644 > > > --- a/lib/bitmap.c > > > +++ b/lib/bitmap.c > > > @@ -487,24 +487,24 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf); > > > > > > /* > > > * Region 9-38:4/10 describes the following bitmap structure: > > > - * 0 9 12 18 38 > > > - * .........****......****......****...... > > > - * ^ ^ ^ ^ > > > - * start off group_len end > > > + * 0 9 12 18 38 N > > > + * .........****......****......****.................. > > > + * ^ ^ ^ ^ ^ > > > + * start off group_len end nbits > > > */ > > > struct region { > > > unsigned int start; > > > unsigned int off; > > > unsigned int group_len; > > > unsigned int end; > > > + unsigned int nbits; > > > }; > > > > > > -static int bitmap_set_region(const struct region *r, > > > - unsigned long *bitmap, int nbits) > > > +static int bitmap_set_region(const struct region *r, unsigned long *bitmap) > > > { > > > unsigned int start; > > > > > > - if (r->end >= nbits) > > > + if (r->end >= r->nbits) > > > return -ERANGE; > > > > > > for (start = r->start; start <= r->end; start += r->group_len) > > > @@ -640,7 +640,8 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) > > > struct region r; > > > long ret; > > > > > > - bitmap_zero(maskp, nmaskbits); > > > + r.nbits = nmaskbits; > > > > > + bitmap_zero(maskp, r.nbits); > > > > This sounds not right from style perspective. > > You have completely uninitialized r on stack, then you assign only one value > > for immediate use here and... > > So, this change was added because Yury suggested that I "..store > nmaskbits in the struct region, and avoid passing nmaskbits as a > parameter." > > To which I originally noted "I considered that and went with the param > so as to not open the door to someone possibly using an uninitialized > struct value later."
struct region is purely internal structure. It's declared on stack and filled field-by-field using helpers. 'Someone' misusing the structure doesn't exist because the structure doesn't exist out of the scope.
> https://lore.kernel.org/lkml/20210122044357.GS16838@windriver.com/ > > Looking back, I had a similar thought as to yours, it seems... > > I am also thinking more and more that nbits doesn't belong in the > region anyway - yes, a region gets validated against a specific nbits > eventually, but it doesn't need an nbits field to be a complete > specification. The region "0-3" is a complete specification for "the > 1st four cores" and is as valid on a 4 core machine as it is on a 64 core > machine -- a validation we do when we deploy the region on that machine. > > I will set this change aside and get the nbits value to getnum() another > way, and leave the region struct as it was -- without a nbits field. > > This will also resolve having the macro handling of region that you were > not really liking. > > Paul.
Region is a convenient structure. Adding nbits into it helps to remove validation logic from bitmap_set_region(), so it's worth doing this.
Can you please have it unchanged?
Thanks, Yury
| |