Message-ID: <YCQOmkfEH0lLJY8l@smile.fi.intel.com>
Date:   Wed, 10 Feb 2021 18:49:30 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Paul Gortmaker <paul.gortmaker@...driver.com>
Cc:     Yury Norov <yury.norov@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Li Zefan <lizefan@...wei.com>, Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Josh Triplett <josh@...htriplett.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of
 bitmap

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@...driver.com> 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.
> 
> 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'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.

> +/*
> + * 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;
> 

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists