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
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
> > > > 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, ); <---
> > > bitmap_check_region();
> > > bitmap_set_region(, maskp, nmaskbits);
> > > }
> > >
> > > static const char *bitmap_parse_region(const char *str, struct region *r)
> > > {
> > > bitmap_getnum(str, >start);
> > > bitmap_getnum(str + 1, >end);
> > > bitmap_getnum(str + 1, >off);
> > > bitmap_getnum(str + 1, >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, >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
> > kn
Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap
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
> > > 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, ); <---
> > bitmap_check_region();
> > bitmap_set_region(, maskp, nmaskbits);
> > }
> >
> > static const char *bitmap_parse_region(const char *str, struct region *r)
> > {
> > bitmap_getnum(str, >start);
> > bitmap_getnum(str + 1, >end);
> > bitmap_getnum(str + 1, >off);
> > bitmap_getnum(str + 1, >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, >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;
};
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 struct
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
> > 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, ); <---
> bitmap_check_region();
> bitmap_set_region(, maskp, nmaskbits);
> }
>
> static const char *bitmap_parse_region(const char *str, struct region *r)
> {
> bitmap_getnum(str, >start);
> bitmap_getnum(str + 1, >end);
> bitmap_getnum(str + 1, >off);
> bitmap_getnum(str + 1, >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, >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
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
> 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, ); <---
bitmap_check_region();
bitmap_set_region(, maskp, nmaskbits);
}
static const char *bitmap_parse_region(const char *str, struct region *r)
{
bitmap_getnum(str, >start);
bitmap_getnum(str + 1, >end);
bitmap_getnum(str + 1, >off);
bitmap_getnum(str + 1, >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, >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;
Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap
On Tue, Feb 9, 2021 at 3:01 PM Paul Gortmaker
wrote:
>
> While this is done for all bitmaps, the original use case in mind was
> for CPU masks and cpulist_parse() as described below.
>
> It seems that a common configuration is to use the 1st couple cores for
> housekeeping tasks. This tends to leave the remaining ones to form a
> pool of similarly configured cores to take on the real workload of
> interest to the user.
>
> So on machine A - with 32 cores, it could be 0-3 for "system" and then
> 4-31 being used in boot args like nohz_full=, or rcu_nocbs= as part of
> setting up the worker pool of CPUs.
>
> But then newer machine B is added, and it has 48 cores, and so while
> the 0-3 part remains unchanged, the pool setup cpu list becomes 4-47.
>
> Multiple deployment becomes easier when we can just simply replace 31
> and 47 with "N" and let the system substitute in the actual number at
> boot; a number that it knows better than we do.
>
> Cc: Yury Norov
> Cc: Peter Zijlstra
> Cc: "Paul E. McKenney"
> Cc: Rasmus Villemoes
> Cc: Andy Shevchenko
> Suggested-by: Yury Norov # move it from CPU code
> Signed-off-by: Paul Gortmaker
> ---
> .../admin-guide/kernel-parameters.rst | 7 +
> lib/bitmap.c | 27 ++-
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.rst
> b/Documentation/admin-guide/kernel-parameters.rst
> index 682ab28b5c94..7733a773f5f8 100644
> --- a/Documentation/admin-guide/kernel-parameters.rst
> +++ b/Documentation/admin-guide/kernel-parameters.rst
> @@ -68,6 +68,13 @@ For example one can add to the command line following
> parameter:
>
> where the final item represents CPUs 100,101,125,126,150,151,...
>
> +The value "N" can be used to represent the numerically last CPU on the
> system,
> +i.e "foo_cpus=16-N" would be equivalent to "16-31" on a 32 core system.
> +
> +Keep in mind that "N" is dynamic, so if system changes cause the bitmap width
> +to change, such as less cores in the CPU list, then N and any ranges using N
> +will also change. Use the same on a small 4 core system, and "16-N" becomes
> +"16-3" and now the same boot input will be flagged as invalid (start > end).
>
>
> This document may not be entirely up to date and comprehensive. The command
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 6b568f98af3d..cc7cb1fca1ac 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -530,11 +530,17 @@ static int bitmap_check_region(const struct
> bitmap_region *br)
> return 0;
> }
>
> -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.
> {
> unsigned long long n;
> unsigned int len;
>
> + if (str[0] == 'N') {
> + *num = lastbit;
> + return str + 1;
> + }
> +
> len = _parse_integer(str, 10, );
> if (!len)
> return ERR_PTR(-EINVAL);
> @@ -580,9 +586,12 @@ static const char *bitmap_find_region_reverse(const char
> *start, const char *end
> return end;
> }
>
> -static const char *bitmap_parse_region(const char *str, struct region *r)
> +static const char *bitmap_parse_region(const char *str, struct bitmap_region
> *br)
It seems like you declare struct bitmap_region in patch 8, but use it
in patch 6.
Can you please reorder your patches and resubmit?
> {
> - str = bitmap_getnum(str, >start);
> + struct region *r = br->r;
> + unsigned int lastbit = br->nbits - 1;
> +
> + str = bitmap_getnum(str, >start, lastbit);
> if (IS_ERR(str))
> return str;
>
> @@ -592,7 +601,7 @@ static const char *bitmap_parse_region(const char *str,
> struct region *r)
> if (*str != '-')
> return ERR_PTR(-EINVAL);
>
> - str = bitmap_getnum(str + 1, >end);
> + str = bitmap_getnum(str + 1, >end, lastbit);
> if (IS_ERR(str))
> return str;
>
> @@ -602,14 +611,14 @@ static const char *bitmap_parse_region(const char *str,
> struct region *r)
> if (*str != ':')
> return ERR_PTR(-EINVAL);
>
> - str = bitmap_getnum(str + 1, >off);
> + str = bitmap_getnum(str + 1, >off, lastbit);
> if (IS_ERR(str))
> return str;
>
> if (*str != '/')
> return ERR_PTR(-EINVAL);
>
> - return bitmap_getnum(str + 1, >group_len);
> + return bitmap_getnum(str + 1, >group_len,
Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap
On Tue, Jan 26, 2021 at 1:40 PM Andy Shevchenko
wrote:
>
> On Tue, Jan 26, 2021 at 11:37:30PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:
>
> ...
>
> > > + if (str[0] == 'N') {
> > > + *num = nbits - 1;
> > > + return str + 1;
> > > + }
> >
> > But locating it here makes possible to enter a priori invalid input, like N
> > for
> > start of the region.
> >
> > I think this should be separate helper which is called in places where it
> > makes
> > sense.
>
> Okay, N is 31 on 32 core system... It is a bit counter intuitive, because it's
> rather _L_ast than _N_umber of CPUs.
>
> Changing letter?
No objections.
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
26/01/2021 (Tue 23:37) Andy Shevchenko wrote:
> On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:
> > While this is done for all bitmaps, the original use case in mind was
> > for CPU masks and cpulist_parse() as described below.
> >
> > It seems that a common configuration is to use the 1st couple cores for
> > housekeeping tasks. This tends to leave the remaining ones to form a
> > pool of similarly configured cores to take on the real workload of
> > interest to the user.
> >
> > So on machine A - with 32 cores, it could be 0-3 for "system" and then
> > 4-31 being used in boot args like nohz_full=, or rcu_nocbs= as part of
> > setting up the worker pool of CPUs.
> >
> > But then newer machine B is added, and it has 48 cores, and so while
> > the 0-3 part remains unchanged, the pool setup cpu list becomes 4-47.
> >
> > Multiple deployment becomes easier when we can just simply replace 31
> > and 47 with "N" and let the system substitute in the actual number at
> > boot; a number that it knows better than we do.
>
> I would accept lower 'n' as well.
>
> ...
>
> > -static const char *bitmap_getnum(const char *str, unsigned int *num)
> > +static const char *__bitmap_getnum(const char *str, unsigned int nbits,
> > + unsigned int *num)
> > {
> > unsigned long long n;
> > unsigned int len;
> >
> > + if (str[0] == 'N') {
> > + *num = nbits - 1;
> > + return str + 1;
> > + }
>
> But locating it here makes possible to enter a priori invalid input, like N
> for
> start of the region.
Actually, no. N can be valid input for start of the region - or for any
field in the region. I was originally thinking like you -- that N
was only valid as the end of the region, but Yury made a compelling
argument that N should be treated exactly as any other number is.
Skip down to where Yury says:
So, when I do echo N-N > cpuset.cpus, I want it to work as
if I do echo 15-15 > cpuset.cpus.
https://lore.kernel.org/lkml/20210126171811.gc23...@windriver.com/
You weren't Cc'd at that point as I'd not added any self-test changes
to the series yet - so you didn't probably see that discussion.
This is why you'll see "N-N:N/N" added as a self-test that works. It
doesn't make any more sense than using 15-15:15/15 does (vs. "15") but
both are equally valid inputs, in that they don't trigger an error.
Thanks,
Paul.
--
>
> I think this should be separate helper which is called in places where it
> makes
> sense.
>
> > len = _parse_integer(str, 10, );
> > if (!len)
> > return ERR_PTR(-EINVAL);
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap
On Tue, Jan 26, 2021 at 11:37:30PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:
...
> > + if (str[0] == 'N') {
> > + *num = nbits - 1;
> > + return str + 1;
> > + }
>
> But locating it here makes possible to enter a priori invalid input, like N
> for
> start of the region.
>
> I think this should be separate helper which is called in places where it
> makes
> sense.
Okay, N is 31 on 32 core system... It is a bit counter intuitive, because it's
rather _L_ast than _N_umber of CPUs.
Changing letter?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap
On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:
> While this is done for all bitmaps, the original use case in mind was
> for CPU masks and cpulist_parse() as described below.
>
> It seems that a common configuration is to use the 1st couple cores for
> housekeeping tasks. This tends to leave the remaining ones to form a
> pool of similarly configured cores to take on the real workload of
> interest to the user.
>
> So on machine A - with 32 cores, it could be 0-3 for "system" and then
> 4-31 being used in boot args like nohz_full=, or rcu_nocbs= as part of
> setting up the worker pool of CPUs.
>
> But then newer machine B is added, and it has 48 cores, and so while
> the 0-3 part remains unchanged, the pool setup cpu list becomes 4-47.
>
> Multiple deployment becomes easier when we can just simply replace 31
> and 47 with "N" and let the system substitute in the actual number at
> boot; a number that it knows better than we do.
I would accept lower 'n' as well.
...
> -static const char *bitmap_getnum(const char *str, unsigned int *num)
> +static const char *__bitmap_getnum(const char *str, unsigned int nbits,
> + unsigned int *num)
> {
> unsigned long long n;
> unsigned int len;
>
> + if (str[0] == 'N') {
> + *num = nbits - 1;
> + return str + 1;
> + }
But locating it here makes possible to enter a priori invalid input, like N for
start of the region.
I think this should be separate helper which is called in places where it makes
sense.
> len = _parse_integer(str, 10, );
> if (!len)
> return ERR_PTR(-EINVAL);
--
With Best Regards,
Andy Shevchenko
