Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap

[Paul Gortmaker]

[Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap] On 11/02/2021 (Thu 17:24) Yury Norov wrote:

> On Wed, Feb 10, 2021 at 06:49:30PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 10, 2021 at 10:58:25AM -0500, Paul Gortmaker wrote:
> > > [Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap] On 09/02/2021 (Tue 15:16) Yury Norov wrote:
> > > 
> > > > On Tue, Feb 9, 2021 at 3:01 PM Paul Gortmaker
> > > > <paul.gortmaker@xxxxxxxxxxxxx> wrote:
> > > 
> > > [...]
> > > 
> > > > > -static const char *bitmap_getnum(const char *str, unsigned int *num)
> > > > > +static const char *bitmap_getnum(const char *str, unsigned int *num,
> > > > > +                                unsigned int lastbit)
> > > > 
> > > > The idea of struct bitmap_region is avoid passing the lastbit to the functions.
> > > > But here you do pass. Can you please be consistent? Or if I misunderstand
> > > > the idea of struct bitmap_region, can you please clarify it?
> > > > 
> > > > Also, I don't think that in this specific case it's worth it to create
> > > > a hierarchy of
> > > > structures. Just adding lastbits to struct region will be simpler and more
> > > > transparent.
> > > 
> > > I'm getting mixed messages from different people as to what is wanted here.
> > > 
> > > Here is what the code looks like now; only relevant lines shown:
> > > 
> > >  -------------------------------
> > > int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> > > {
> > > 
> > >         struct region r;
> > > 
> > >         bitmap_parse_region(buf, &r);       <-----------
> > >         bitmap_check_region(&r);
> > >         bitmap_set_region(&r, maskp, nmaskbits);
> > > }
> > > 
> > > static const char *bitmap_parse_region(const char *str, struct region *r)
> > > {
> > >         bitmap_getnum(str, &r->start);
> > >         bitmap_getnum(str + 1, &r->end);
> > >         bitmap_getnum(str + 1, &r->off);
> > >         bitmap_getnum(str + 1, &r->group_len);
> > > }
> > > 
> > > static const char *bitmap_getnum(const char *str, unsigned int *num)
> > > {
> > > 	/* PG: We need nmaskbits here for N processing. */
> > > }
> > >  -------------------------------
> > > 
> > > 
> > > Note the final function - the one where you asked to locate the N
> > > processing into -- does not take a region.  So even if we bundle nbits
> > > into the region struct, it doesn't get the data to where we need it.

Yury - you asked why there was an arg passed -- "lastbit"  -- and from
your reply, I don't think you fully read my answer - or at least missed
the three key sentences above as to why "lastbit" was passed.

> > > 
> > > Choices:
> > > 
> > > 1) pass in nbits just like bitmap_set_region() does currently.
> > > 
> > > 2) add nbits to region and pass full region instead of start/end/off.
> > > 
> > > 2a) add nbits to region and pass full region and also start/end/off.
> > > 
> > > 3) use *num as a bi-directional data path and initialize with nbits.
> > > 
> > > 
> > > Yury doesn't want us add any function args -- i.e. not to do #1.
> > > 
> > > Andy didn't like #2 because it "hides" that we are writing to r.
> > > 
> > > I ruled out sending 2a -- bitmap_getnum(str, r, &r->end)  because
> > > it adds an arg, AND seems rather redundant to pass r and r->field.
> > > 
> > > The #3 is the smallest change - but seems like we are trying to be
> > > too clever just to save a line of code or a couple bytes. (see below)
> > > 
> > > Yury - in your reply to patch 5, you indicate you wrote the region
> > > code and want me to go back to putting nbits into region directly.
> > > 
> > > Can you guys please clarify who is maintainer and hence exactly how
> > > you want this relatively minor detail handled?  I'll gladly do it
> > > in whatever way the maintainer wants just to get this finally done.
> > 
> > Funny that there is no maintainer of the code.
> > That said, I consider #1 or #3 is good enough. Rationale for
> > - #1: it doesn't touch purity of getnum(), I think it's good enough not to know
> >   region details
> > - #3 (as you posted below): I like how it looks like (one nit below, though)
> > 
> > But let's put this way, I think Yury had done a lot in the area, let's listen
> > more to him than to me.
> 
> I think that the simplest approach is the best. For me, the simplest
> thing is to add a field to existing structure, like this:
> 
>         struct region {
>                 unsigned int start;
>                 unsigned int off; 
>                 unsigned int group_len;
>                 unsigned int end; 
>                 unsigned int nbits; 
>         };

This is what was in v3 of this series, and I have put it back to this.

But as stated above, it doesn't get nbits to where it is needed, so
there still needs to be this one change you explicitly asked about:

 -static const char *bitmap_getnum(const char *str, unsigned int *num)
 +static const char *bitmap_getnum(const char *str, unsigned int *num,
 +                                 unsigned int lastbit)

There is no region here, so there is no access to nbits via region.
Hence the extra arg. So that is what v5 of the series will do - just
like v4 did.

Paul.
--

> 
> Of course we can do like:
>  
>         struct region {
>                 struct boundaries;
>                 struct periodic_part;
>         };
> 
>         struct bitmap_region {
>                 struct region;
>                 unsigned int nbits; 
>         };
> 
> But it would be nothing better than simple flat structure.
> 
> > > I'd rather not keep going in circles and guessing and annoying everyone
> > > else on the Cc: list by filling their inbox any more than I already have.
> > > 
> > > That would help a lot in getting this finished.
> > 
> > Agree!
> > 
> > > Example #3 -- not sent..
> > > 
> > > +#define DECLARE_REGION(rname, initval) \
> > > +struct region rname = {                \
> > > +       .start = initval,               \
> > > +       .off = initval,                 \
> > > +       .group_len = initval,           \
> > > +       .end = initval,                 \
> > > +}
> > > 
> > > [...]
> > > 
> > > -       struct region r;
> > > +       DECLARE_REGION(r, nmaskbits - 1);       /* "N-N:N/N" */
> >
> > I would initialize with nmaskbits to be sure the value is invalid, but it will
> > add some code, below, so up to you, guys.
> 
> We don't need to initialize r because it's purely internal and helpers
> don't expect that r is initialized, so it's waste of cycles.
>  
> > > +/*
> > > + * Seeing 'N' tells us to leave the value of "num" unchanged (which will
> > > + * be the max value for the width of the bitmap, set via DECLARE_REGION).
> > > + */
> > >  static const char *bitmap_getnum(const char *str, unsigned int *num)
> > >  {
> > >         unsigned long long n;
> > >         unsigned int len;
> > >  
> > > +       if (str[0] == 'N')      /* nothing to do, just advance str */
> > > +               return str + 1;
> 
> Is my understanding correct that this is an attempt to avoid adding
> the nbits to struct region? It would probably work, but I'd prefer
> to extend the structure. For me it's more readable and maintainable
> approach.
> 
> Yury