Date:   Wed, 10 Feb 2021 10:58:25 -0500
From:   Paul Gortmaker <paul.gortmaker@...driver.com>
To:     Yury Norov <yury.norov@...il.com>
Cc:     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>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of
 bitmap

[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.

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.

Thanks,
Paul.
--

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" */

[...]

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

Powered by blists - more mailing lists