[PATCH v2 0/3] :support for bitmap (and hence CPU) list abbreviations

[PATCH v2 0/3] :support for bitmap (and hence CPU) list abbreviations

[Paul Gortmaker]

The basic objective here was to add support for "nohz_full=8-N" and/or
"rcu_nocbs="4-N" -- essentially introduce "N" as a portable reference
to the last core, evaluated at boot for anything using a CPU list.

The thinking behind this, is that people carve off a few early CPUs to
support housekeeping tasks, and perhaps dedicate one to a busy I/O
peripheral, and then the remaining pool of CPUs out to the end are a
part of a commonly configured pool used for the real work the user
cares about.

Extend that logic out to a fleet of machines - some new, and some
nearing EOL, and you've probably got a wide range of core counts to
contend with - even though the early number of cores dedicated to the
system overhead probably doesn't vary.

This change would enable sysadmins to have a common bootarg across all
such systems, and would also avoid any off-by-one fencepost errors that
happen for users who might briefly forget that core counts start at
zero.

Looking around before starting, I noticed RCU already had a short-form
abbreviation "all" -- but if we want to treat CPU lists in a uniform
matter, then tokens shouldn't be implemented at a subsystem level and
hence be subsystem specific; each with their own variations.

So I moved "all" to global use - for boot args, and for cgroups.  Then
I added the inverse "none" and finally, the one I wanted -- "N".

Originally I did this at the CPU subsys level, but Yury suggested it
be moved down further to bitmap level itself, and that was good advice.
Things got smaller and less complex.

The use of "N" isn't a standalone word like "all" or "none".  It will
be a part of a complete range specification, possibly with CSV separate
ranges before and after; like "nohz_full=5,6,8-N" or "nohz_full=2-N:3/4"

Also tested the post-boot cgroup use case as per below:

   root@hackbox:/sys/fs/cgroup/cpuset# mkdir foo
   root@hackbox:/sys/fs/cgroup/cpuset# cd foo
   root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
   
   root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 10-N > cpuset.cpus
   root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
   10-15
   root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all > cpuset.cpus
   root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
   0-15
   root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none > cpuset.cpus
   root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
   
   root@hackbox:/sys/fs/cgroup/cpuset/foo#

This was on a 16 core machine with CONFIG_NR_CPUS=16 in .config file.

Note that "N" is a dynamic quantity, and can change scope if the bitmap
is changed in size.  So at the risk of stating the obvious, don't use it
for "burn_eFuse=128-N" or "secure_erase_firmware=32-N" type stuff.

Paul.
---

[v1: https://lore.kernel.org/lkml/20210106004850.GA11682@paulmck-ThinkPad-P72/

[v2: push code down from cpu subsys to core bitmap code as per
 Yury's comments.  Change "last" to simply be "N" as per PeterZ.

 Combination of the two got rid of needing strword() and greatly
 reduced complexity and footprint of the change -- thanks! ]

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>


Paul Gortmaker (3):
  lib: add "all" and "none" as valid ranges to bitmap_parselist()
  rcu: dont special case "all" handling; let bitmask deal with it
  lib: support N as end of range in bitmap_parselist()

 .../admin-guide/kernel-parameters.rst         | 15 +++++++++++
 .../admin-guide/kernel-parameters.txt         |  4 +--
 kernel/rcu/tree_plugin.h                      | 13 +++------
 lib/bitmap.c                                  | 27 +++++++++++++++----
 4 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist()

[Paul Gortmaker]

The use of "all" was originally RCU specific - I'd pushed it down to
being used for any CPU lists -- then Yuri suggested pushing it down
further to be used by any bitmap, which is done here.

As a trivial one line extension, we also accept the inverse "none"
as a valid alias.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 Documentation/admin-guide/kernel-parameters.rst | 11 +++++++++++
 lib/bitmap.c                                    |  9 +++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index 682ab28b5c94..5e080080b058 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -68,7 +68,18 @@ For example one can add to the command line following parameter:
 
 where the final item represents CPUs 100,101,125,126,150,151,...
 
+The following convenience aliases are also accepted and used:
 
+        foo_cpus=all
+
+will provide an full/all-set cpu mask for the associated boot argument.
+
+        foo_cpus=none
+
+will provide an empty/cleared cpu mask for the associated boot argument.
+
+Note that "all" and "none" are not necessarily valid/sensible input values
+for each available boot parameter expecting a CPU list.
 
 This document may not be entirely up to date and comprehensive. The command
 "modinfo -p ${modulename}" shows a current list of all parameters of a loadable
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 75006c4036e9..a1010646fbe5 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -627,6 +627,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
  * From each group will be used only defined amount of bits.
  * Syntax: range:used_size/group_size
  * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
+ * Optionally the self-descriptive "all" or "none" can be used.
  *
  * Returns: 0 on success, -errno on invalid input strings. Error values:
  *
@@ -640,8 +641,16 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 	struct region r;
 	long ret;
 
+	if (!strcmp(buf, "all")) {
+		bitmap_fill(maskp, nmaskbits);
+		return 0;
+	}
+
 	bitmap_zero(maskp, nmaskbits);
 
+	if (!strcmp(buf, "none"))
+		return 0;
+
 	while (buf) {
 		buf = bitmap_find_region(buf);
 		if (buf == NULL)
-- 
2.17.1

Re: [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist()

[Yury Norov]

On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> The use of "all" was originally RCU specific - I'd pushed it down to
> being used for any CPU lists -- then Yuri suggested pushing it down
> further to be used by any bitmap, which is done here.
>
> As a trivial one line extension, we also accept the inverse "none"
> as a valid alias.
>
> Cc: Yury Norov <yury.norov@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  Documentation/admin-guide/kernel-parameters.rst | 11 +++++++++++
>  lib/bitmap.c                                    |  9 +++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
> index 682ab28b5c94..5e080080b058 100644
> --- a/Documentation/admin-guide/kernel-parameters.rst
> +++ b/Documentation/admin-guide/kernel-parameters.rst
> @@ -68,7 +68,18 @@ For example one can add to the command line following parameter:
>
>  where the final item represents CPUs 100,101,125,126,150,151,...
>
> +The following convenience aliases are also accepted and used:
>
> +        foo_cpus=all
> +
> +will provide an full/all-set cpu mask for the associated boot argument.
> +
> +        foo_cpus=none
> +
> +will provide an empty/cleared cpu mask for the associated boot argument.
> +
> +Note that "all" and "none" are not necessarily valid/sensible input values
> +for each available boot parameter expecting a CPU list.

My question from v1 is still there: what about the line like
"none,all", ok ",all,"
or similar? If it's not legal, it should be mentioned in the comment,
if it is legal,
the corresponding code should go to bitmap_parse_region(), just like for "N".

My personal preference is the latter option.

>  This document may not be entirely up to date and comprehensive. The command
>  "modinfo -p ${modulename}" shows a current list of all parameters of a loadable
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 75006c4036e9..a1010646fbe5 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -627,6 +627,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
>   * From each group will be used only defined amount of bits.
>   * Syntax: range:used_size/group_size
>   * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
> + * Optionally the self-descriptive "all" or "none" can be used.
>   *
>   * Returns: 0 on success, -errno on invalid input strings. Error values:
>   *
> @@ -640,8 +641,16 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>         struct region r;
>         long ret;
>
> +       if (!strcmp(buf, "all")) {
> +               bitmap_fill(maskp, nmaskbits);
> +               return 0;
> +       }
> +
>         bitmap_zero(maskp, nmaskbits);
>
> +       if (!strcmp(buf, "none"))
> +               return 0;
> +
>         while (buf) {
>                 buf = bitmap_find_region(buf);
>                 if (buf == NULL)
> --
> 2.17.1
>

Re: [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist()

[Paul Gortmaker]

[Re: [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist()] On 21/01/2021 (Thu 16:07) Yury Norov wrote:

> On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> >
> > The use of "all" was originally RCU specific - I'd pushed it down to
> > being used for any CPU lists -- then Yuri suggested pushing it down
> > further to be used by any bitmap, which is done here.
> >
> > As a trivial one line extension, we also accept the inverse "none"
> > as a valid alias.
> >
> > Cc: Yury Norov <yury.norov@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > ---
> >  Documentation/admin-guide/kernel-parameters.rst | 11 +++++++++++
> >  lib/bitmap.c                                    |  9 +++++++++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
> > index 682ab28b5c94..5e080080b058 100644
> > --- a/Documentation/admin-guide/kernel-parameters.rst
> > +++ b/Documentation/admin-guide/kernel-parameters.rst
> > @@ -68,7 +68,18 @@ For example one can add to the command line following parameter:
> >
> >  where the final item represents CPUs 100,101,125,126,150,151,...
> >
> > +The following convenience aliases are also accepted and used:
> >
> > +        foo_cpus=all
> > +
> > +will provide an full/all-set cpu mask for the associated boot argument.
> > +
> > +        foo_cpus=none
> > +
> > +will provide an empty/cleared cpu mask for the associated boot argument.
> > +
> > +Note that "all" and "none" are not necessarily valid/sensible input values
> > +for each available boot parameter expecting a CPU list.
> 
> My question from v1 is still there: what about the line like
> "none,all", ok ",all,"

Apologies - I must have overlooked that somehow.  Let me address it now.

> or similar? If it's not legal, it should be mentioned in the comment,

OK, it is not legal.  So if desired, I can do this in the code...

 - * Optionally the self-descriptive "all" or "none" can be used.
 + * Optionally the self-descriptive stand alone "all" or "none" can be used.

...and a similar "stand alone" addition in kernel-parameters.rst above?

> if it is legal,
> the corresponding code should go to bitmap_parse_region(), just like for "N".

Non-standalone is not legal.  The strcmp ensures the "all" or "none" are
stand-alone.  And as can be seen in the testing below, any attempt to
combine them with commas or ranges or repeated instances is -EINVAL.
(And I'll look at adding such tests to bitmap_test.c as requested.)

> My personal preference is the latter option.

I'm a bit confused as to the value in adding code for supporting things
like ",all,none,all,,none" and then having to define some policy, like
"last processed takes precedence" or similar.   A strict stand-alone
"all" or "none" and everything else as -EINVAL as per below seems
logical.   Maybe I'm missing something and you can elaborate?

Thanks
Paul.
--

root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all,none,all > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none,all > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all,all > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all, > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all > cpuset.cpus
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo ,none > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none > cpuset.cpus
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 1,3,5,7,9,11,13,15 > cpuset.cpus
root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
1,3,5,7,9,11,13,15
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 1,3,5,7,9,11,13,all > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none,3,5,7,9,11,13,15 > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# 

[PATCH 2/3] rcu: dont special case "all" handling; let bitmask deal with it

[Paul Gortmaker]

Now that the core bitmap parse code respects the "all" parameter, there
is no need for RCU to have its own special check for it.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 +---
 kernel/rcu/tree_plugin.h                        | 13 ++++---------
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..a116c0ff0a91 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4037,9 +4037,7 @@
 				see CONFIG_RAS_CEC help text.
 
 	rcu_nocbs=	[KNL]
-			The argument is a cpu list, as described above,
-			except that the string "all" can be used to
-			specify every CPU on the system.
+			The argument is a cpu list, as described above.
 
 			In kernels built with CONFIG_RCU_NOCB_CPU=y, set
 			the specified list of CPUs to be no-callback CPUs.
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 7e291ce0a1d6..642ebd6569c7 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -1463,20 +1463,15 @@ static void rcu_cleanup_after_idle(void)
 
 /*
  * Parse the boot-time rcu_nocb_mask CPU list from the kernel parameters.
- * The string after the "rcu_nocbs=" is either "all" for all CPUs, or a
- * comma-separated list of CPUs and/or CPU ranges.  If an invalid list is
- * given, a warning is emitted and all CPUs are offloaded.
+ * If the list is invalid, a warning is emitted and all CPUs are offloaded.
  */
 static int __init rcu_nocb_setup(char *str)
 {
 	alloc_bootmem_cpumask_var(&rcu_nocb_mask);
-	if (!strcasecmp(str, "all"))
+	if (cpulist_parse(str, rcu_nocb_mask)) {
+		pr_warn("rcu_nocbs= bad CPU range, all CPUs set\n");
 		cpumask_setall(rcu_nocb_mask);
-	else
-		if (cpulist_parse(str, rcu_nocb_mask)) {
-			pr_warn("rcu_nocbs= bad CPU range, all CPUs set\n");
-			cpumask_setall(rcu_nocb_mask);
-		}
+	}
 	return 1;
 }
 __setup("rcu_nocbs=", rcu_nocb_setup);
-- 
2.17.1

[PATCH 3/3] lib: support N as end of range in bitmap_parselist()

[Paul Gortmaker]

While this is done for all bitmaps, the original use case in mind was
for CPU masks and cpulist_parse().  Credit to Yury who suggested to
push it down from CPU subsys to bitmap - it simplified things a lot.

It seems that a common configuration is to use the 1st couple cores
for housekeeping tasks, and or driving a busy peripheral that generates
a lot of interrupts, or something similar.

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.

Deployment would be easier if we could 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.

No need to have custom boot args per node, no need to do a trial boot
in order to snoop /proc/cpuinfo and/or /sys/devices/system/cpu - no
more fencepost errors of using 32 and 48 instead of 31 and 47.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 .../admin-guide/kernel-parameters.rst          |  4 ++++
 lib/bitmap.c                                   | 18 +++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index 5e080080b058..668f0b69fb4f 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -68,6 +68,10 @@ 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 as the end of a range, 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.
+
 The following convenience aliases are also accepted and used:
 
         foo_cpus=all
diff --git a/lib/bitmap.c b/lib/bitmap.c
index a1010646fbe5..d498ea9d526b 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -571,7 +571,7 @@ 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 region *r, int nmaskbits)
 {
 	str = bitmap_getnum(str, &r->start);
 	if (IS_ERR(str))
@@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
 	if (*str != '-')
 		return ERR_PTR(-EINVAL);
 
-	str = bitmap_getnum(str + 1, &r->end);
-	if (IS_ERR(str))
-		return str;
+	str++;
+	if (*str == 'N') {
+		r->end = nmaskbits - 1;
+		str++;
+	} else {
+		str = bitmap_getnum(str, &r->end);
+		if (IS_ERR(str))
+			return str;
+	}
 
 	if (end_of_region(*str))
 		goto no_pattern;
@@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
  * Syntax: range:used_size/group_size
  * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
  * Optionally the self-descriptive "all" or "none" can be used.
+ * The value 'N' can be used as the end of a range to indicate the maximum
+ * allowed value; i.e (nmaskbits - 1).
  *
  * Returns: 0 on success, -errno on invalid input strings. Error values:
  *
@@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 		if (buf == NULL)
 			return 0;
 
-		buf = bitmap_parse_region(buf, &r);
+		buf = bitmap_parse_region(buf, &r, nmaskbits);
 		if (IS_ERR(buf))
 			return PTR_ERR(buf);
 
-- 
2.17.1

Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()

[Yury Norov]

On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> While this is done for all bitmaps, the original use case in mind was
> for CPU masks and cpulist_parse().  Credit to Yury who suggested to
> push it down from CPU subsys to bitmap - it simplified things a lot.

Can you convert your credit to Suggested-by or Reviewed-by? :)

> It seems that a common configuration is to use the 1st couple cores
> for housekeeping tasks, and or driving a busy peripheral that generates
> a lot of interrupts, or something similar.
>
> 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.
>
> Deployment would be easier if we could 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.
>
> No need to have custom boot args per node, no need to do a trial boot
> in order to snoop /proc/cpuinfo and/or /sys/devices/system/cpu - no
> more fencepost errors of using 32 and 48 instead of 31 and 47.
>
> Cc: Yury Norov <yury.norov@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  .../admin-guide/kernel-parameters.rst          |  4 ++++
>  lib/bitmap.c                                   | 18 +++++++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
> index 5e080080b058..668f0b69fb4f 100644
> --- a/Documentation/admin-guide/kernel-parameters.rst
> +++ b/Documentation/admin-guide/kernel-parameters.rst
> @@ -68,6 +68,10 @@ 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 as the end of a range, 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.
> +
>  The following convenience aliases are also accepted and used:
>
>          foo_cpus=all
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index a1010646fbe5..d498ea9d526b 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -571,7 +571,7 @@ 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 region *r, int nmaskbits)
>  {

in bitmap_parselist() you can store nmaskbits in the struct region, and avoid
passing nmaskbits as a parameter.

>         str = bitmap_getnum(str, &r->start);
>         if (IS_ERR(str))
> @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
>         if (*str != '-')
>                 return ERR_PTR(-EINVAL);
>
> -       str = bitmap_getnum(str + 1, &r->end);
> -       if (IS_ERR(str))
> -               return str;
> +       str++;
> +       if (*str == 'N') {
> +               r->end = nmaskbits - 1;
> +               str++;
> +       } else {
> +               str = bitmap_getnum(str, &r->end);
> +               if (IS_ERR(str))
> +                       return str;
> +       }

Indeed it's much simpler. But I don't like that you increase the nesting level.
Can you keep bitmap_parse_region() a single-tab style function?

What about group size? Are you going to support N there, like "0-N:5/N"?
What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()?

I would also like to see tests covering new functionality. As a user of "N",
I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including
error codes that the parser returns. Otherwise it will be hard to maintain the
transition.

>         if (end_of_region(*str))
>                 goto no_pattern;
> @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
>   * Syntax: range:used_size/group_size
>   * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
>   * Optionally the self-descriptive "all" or "none" can be used.
> + * The value 'N' can be used as the end of a range to indicate the maximum
> + * allowed value; i.e (nmaskbits - 1).
>   *
>   * Returns: 0 on success, -errno on invalid input strings. Error values:
>   *
> @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>                 if (buf == NULL)
>                         return 0;
>
> -               buf = bitmap_parse_region(buf, &r);
> +               buf = bitmap_parse_region(buf, &r, nmaskbits);
>                 if (IS_ERR(buf))
>                         return PTR_ERR(buf);
>
> --
> 2.17.1
>

Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()

[Paul Gortmaker]

[Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 21/01/2021 (Thu 16:29) Yury Norov wrote:

> On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> >
> > While this is done for all bitmaps, the original use case in mind was
> > for CPU masks and cpulist_parse().  Credit to Yury who suggested to
> > push it down from CPU subsys to bitmap - it simplified things a lot.
> 
> Can you convert your credit to Suggested-by or Reviewed-by? :)

Sure, of course.

[...]

> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index a1010646fbe5..d498ea9d526b 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -571,7 +571,7 @@ 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 region *r, int nmaskbits)
> >  {
> 
> in bitmap_parselist() you can store nmaskbits in the struct region, and avoid
> passing nmaskbits as a parameter.

OK.   FWIW, I considered that and went with the param so as to not open
the door to someone possibly using an uninitialized struct value later.

> >         str = bitmap_getnum(str, &r->start);
> >         if (IS_ERR(str))
> > @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> >         if (*str != '-')
> >                 return ERR_PTR(-EINVAL);
> >
> > -       str = bitmap_getnum(str + 1, &r->end);
> > -       if (IS_ERR(str))
> > -               return str;
> > +       str++;
> > +       if (*str == 'N') {
> > +               r->end = nmaskbits - 1;
> > +               str++;
> > +       } else {
> > +               str = bitmap_getnum(str, &r->end);
> > +               if (IS_ERR(str))
> > +                       return str;
> > +       }
> 
> Indeed it's much simpler. But I don't like that you increase the nesting level.
> Can you keep bitmap_parse_region() a single-tab style function?

Rather a strict coding style, but we can replace with:

       if (*str == 'N') {
               r->end = nmaskbits - 1;
               str++;
       } else {
               str = bitmap_getnum(str, &r->end);
       }

       if (IS_ERR(str))
               return str;

Is that what you were after?

> What about group size? Are you going to support N there, like "0-N:5/N"?

No.  I would think that the group size has to be less than 1/2 of
the nmaskbits or you get the rather pointless case of just one group.
Plus conflating "end of range" with "group size" just adds confusion.
So it is currently not legal:

root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/4 > cpuset.cpus
root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
4-5,8-9,12-13
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/N > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo#

> What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()?

The "N-N" is also not supported/legal.  The allowed use is listed as
being for the end of a range only.  The code enforces this by ensuring
the char previous is a '-'  ; hence a leading N is invalid:

root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus
/bin/echo: write error: Invalid argument
root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus
root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
0-15
root@hackbox:/sys/fs/cgroup/cpuset/foo#

I think "use for end of range only" makes sense in the mathematical
sense most of us have seen during school:  {0, 1, 2, ...  N-1, N} as
used in the end point of a range of numbers.  I could make the "only"
part more explicit and concrete in the comments/docs if desired.

I'm not sure I see the value in complicating things in order to add
or extend support to non-intuitive use cases beyond that - to me that
seems to just make things more confusing for end users.  But again
if you've something in mind that I'm simply missing, then by all
means please elaborate.

> I would also like to see tests covering new functionality. As a user of "N",
> I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including
> error codes that the parser returns. Otherwise it will be hard to maintain the
> transition.

That is a reasonable request.  I will look into adding "N" based type
tests to the existing bitmap test cases in a separate commit.

Thanks,
Paul.
--

> 
> >         if (end_of_region(*str))
> >                 goto no_pattern;
> > @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> >   * Syntax: range:used_size/group_size
> >   * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
> >   * Optionally the self-descriptive "all" or "none" can be used.
> > + * The value 'N' can be used as the end of a range to indicate the maximum
> > + * allowed value; i.e (nmaskbits - 1).
> >   *
> >   * Returns: 0 on success, -errno on invalid input strings. Error values:
> >   *
> > @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> >                 if (buf == NULL)
> >                         return 0;
> >
> > -               buf = bitmap_parse_region(buf, &r);
> > +               buf = bitmap_parse_region(buf, &r, nmaskbits);
> >                 if (IS_ERR(buf))
> >                         return PTR_ERR(buf);
> >
> > --
> > 2.17.1
> >

Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()

[Yury Norov]

On Thu, Jan 21, 2021 at 8:44 PM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> [Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 21/01/2021 (Thu 16:29) Yury Norov wrote:
>
> > On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker
> > <paul.gortmaker@windriver.com> wrote:
> > >
> > > While this is done for all bitmaps, the original use case in mind was
> > > for CPU masks and cpulist_parse().  Credit to Yury who suggested to
> > > push it down from CPU subsys to bitmap - it simplified things a lot.
> >
> > Can you convert your credit to Suggested-by or Reviewed-by? :)
>
> Sure, of course.
>
> [...]
>
> > > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > > index a1010646fbe5..d498ea9d526b 100644
> > > --- a/lib/bitmap.c
> > > +++ b/lib/bitmap.c
> > > @@ -571,7 +571,7 @@ 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 region *r, int nmaskbits)
> > >  {
> >
> > in bitmap_parselist() you can store nmaskbits in the struct region, and avoid
> > passing nmaskbits as a parameter.
>
> OK.   FWIW, I considered that and went with the param so as to not open
> the door to someone possibly using an uninitialized struct value later.
>
> > >         str = bitmap_getnum(str, &r->start);
> > >         if (IS_ERR(str))
> > > @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > >         if (*str != '-')
> > >                 return ERR_PTR(-EINVAL);
> > >
> > > -       str = bitmap_getnum(str + 1, &r->end);
> > > -       if (IS_ERR(str))
> > > -               return str;
> > > +       str++;
> > > +       if (*str == 'N') {
> > > +               r->end = nmaskbits - 1;
> > > +               str++;
> > > +       } else {
> > > +               str = bitmap_getnum(str, &r->end);
> > > +               if (IS_ERR(str))
> > > +                       return str;
> > > +       }
> >
> > Indeed it's much simpler. But I don't like that you increase the nesting level.
> > Can you keep bitmap_parse_region() a single-tab style function?
>
> Rather a strict coding style, but we can replace with:
>
>        if (*str == 'N') {
>                r->end = nmaskbits - 1;
>                str++;
>        } else {
>                str = bitmap_getnum(str, &r->end);
>        }
>
>        if (IS_ERR(str))
>                return str;
>
> Is that what you were after?
>
> > What about group size? Are you going to support N there, like "0-N:5/N"?
>
> No.  I would think that the group size has to be less than 1/2 of
> the nmaskbits or you get the rather pointless case of just one group.
> Plus conflating "end of range" with "group size" just adds confusion.
> So it is currently not legal:
>
> root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/4 > cpuset.cpus
> root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 4-5,8-9,12-13
> root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/N > cpuset.cpus
> /bin/echo: write error: Invalid argument
> root@hackbox:/sys/fs/cgroup/cpuset/foo#
>
> > What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()?
>
> The "N-N" is also not supported/legal.  The allowed use is listed as
> being for the end of a range only.  The code enforces this by ensuring
> the char previous is a '-'  ; hence a leading N is invalid:
>
> root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus
> /bin/echo: write error: Invalid argument
> root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus
> root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 0-15
> root@hackbox:/sys/fs/cgroup/cpuset/foo#
>
> I think "use for end of range only" makes sense in the mathematical
> sense most of us have seen during school:  {0, 1, 2, ...  N-1, N} as
> used in the end point of a range of numbers.  I could make the "only"
> part more explicit and concrete in the comments/docs if desired.
>
> I'm not sure I see the value in complicating things in order to add
> or extend support to non-intuitive use cases beyond that - to me that
> seems to just make things more confusing for end users.  But again
> if you've something in mind that I'm simply missing, then by all
> means please elaborate.

OK, let me share my view on this. As you said in the patch description,
N is substitution to the number of the last CPU, in your example sort of
#define N (15).

So, when I do echo N-N > cpuset.cpus, I want it to work as if I do
echo 15-15 > cpuset.cpus. Why? Because in my terribly huge config
I just want to do s/15/N.

Now let's check how it works .

root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 15-15>cpuset.cpus
root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
15
root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-0>cpuset.cpus
root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
0

OK, works as expected. And what about N?

> root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus
> root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 0-15

OK, looks good.

> root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus
> /bin/echo: write error: Invalid argument

Why? If N is 15, it should work exactly as 15-15, but it doesn't...
This is the source
of confusion and unneeded refactoring of user scripts. (In practice of course
nobody will use "N" because it's broken.) Documentation says nothing about
this limitation, and this is a real example of "complicating things".

You can do better - parse "N" in bitmap_getnum() and avoid all that confusion.

Same logic regarding all/none: all is the equivalent of 0-15, none is something
like ", ,". Now let's try with "0-15,0-3, ," (imagine it's a result of
merging configs).

root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-15,0-3, , >cpuset.cpus
root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
0-15

OK, works. But if I do 's/0-15/all' and 's/ /none', things get broken. This
again brings a special case where it can be avoided - just  parse all/none
at the beginning of bitmap_parse_region().

After reading the description, one can think that you introduce simple and
convenient extensions to existing interface. In fact this is a new interface
which is mostly incompatible with the existing one.

> > I would also like to see tests covering new functionality. As a user of "N",
> > I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including
> > error codes that the parser returns. Otherwise it will be hard to maintain the
> > transition.
>
> That is a reasonable request.  I will look into adding "N" based type
> tests to the existing bitmap test cases in a separate commit.
>
> Thanks,
> Paul.
> --
>
> >
> > >         if (end_of_region(*str))
> > >                 goto no_pattern;
> > > @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > >   * Syntax: range:used_size/group_size
> > >   * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
> > >   * Optionally the self-descriptive "all" or "none" can be used.
> > > + * The value 'N' can be used as the end of a range to indicate the maximum
> > > + * allowed value; i.e (nmaskbits - 1).
> > >   *
> > >   * Returns: 0 on success, -errno on invalid input strings. Error values:
> > >   *
> > > @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> > >                 if (buf == NULL)
> > >                         return 0;
> > >
> > > -               buf = bitmap_parse_region(buf, &r);
> > > +               buf = bitmap_parse_region(buf, &r, nmaskbits);
> > >                 if (IS_ERR(buf))
> > >                         return PTR_ERR(buf);
> > >
> > > --
> > > 2.17.1
> > >

Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()

[Paul Gortmaker]

[Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 22/01/2021 (Fri 15:08) Yury Norov wrote:

> On Thu, Jan 21, 2021 at 8:44 PM Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> >
> > [Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 21/01/2021 (Thu 16:29) Yury Norov wrote:
> >
> > > On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker
> > > <paul.gortmaker@windriver.com> wrote:
> > > >
> > > > While this is done for all bitmaps, the original use case in mind was
> > > > for CPU masks and cpulist_parse().  Credit to Yury who suggested to
> > > > push it down from CPU subsys to bitmap - it simplified things a lot.
> > >
> > > Can you convert your credit to Suggested-by or Reviewed-by? :)
> >
> > Sure, of course.

Now done for v3.

> >
> > [...]
> >
> > > > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > > > index a1010646fbe5..d498ea9d526b 100644
> > > > --- a/lib/bitmap.c
> > > > +++ b/lib/bitmap.c
> > > > @@ -571,7 +571,7 @@ 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 region *r, int nmaskbits)
> > > >  {
> > >
> > > in bitmap_parselist() you can store nmaskbits in the struct region, and avoid
> > > passing nmaskbits as a parameter.
> >
> > OK.   FWIW, I considered that and went with the param so as to not open
> > the door to someone possibly using an uninitialized struct value later.

Also now done - reduces parameter passing and enables moving a sanity
check from set_region into check_region where it IMHO belongs.

> >
> > > >         str = bitmap_getnum(str, &r->start);
> > > >         if (IS_ERR(str))
> > > > @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > > >         if (*str != '-')
> > > >                 return ERR_PTR(-EINVAL);
> > > >
> > > > -       str = bitmap_getnum(str + 1, &r->end);
> > > > -       if (IS_ERR(str))
> > > > -               return str;
> > > > +       str++;
> > > > +       if (*str == 'N') {
> > > > +               r->end = nmaskbits - 1;
> > > > +               str++;
> > > > +       } else {
> > > > +               str = bitmap_getnum(str, &r->end);
> > > > +               if (IS_ERR(str))
> > > > +                       return str;
> > > > +       }
> > >
> > > Indeed it's much simpler. But I don't like that you increase the nesting level.
> > > Can you keep bitmap_parse_region() a single-tab style function?

No increase in nesting level in v3.

> >
> > Rather a strict coding style, but we can replace with:
> >
> >        if (*str == 'N') {
> >                r->end = nmaskbits - 1;
> >                str++;
> >        } else {
> >                str = bitmap_getnum(str, &r->end);
> >        }
> >
> >        if (IS_ERR(str))
> >                return str;
> >
> > Is that what you were after?
> >
> > > What about group size? Are you going to support N there, like "0-N:5/N"?
> >
> > No.  I would think that the group size has to be less than 1/2 of
> > the nmaskbits or you get the rather pointless case of just one group.
> > Plus conflating "end of range" with "group size" just adds confusion.
> > So it is currently not legal:
> >
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/4 > cpuset.cpus
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> > 4-5,8-9,12-13
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/N > cpuset.cpus
> > /bin/echo: write error: Invalid argument
> > root@hackbox:/sys/fs/cgroup/cpuset/foo#
> >
> > > What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()?
> >
> > The "N-N" is also not supported/legal.  The allowed use is listed as
> > being for the end of a range only.  The code enforces this by ensuring
> > the char previous is a '-'  ; hence a leading N is invalid:
> >
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus
> > /bin/echo: write error: Invalid argument
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> > 0-15
> > root@hackbox:/sys/fs/cgroup/cpuset/foo#
> >
> > I think "use for end of range only" makes sense in the mathematical
> > sense most of us have seen during school:  {0, 1, 2, ...  N-1, N} as
> > used in the end point of a range of numbers.  I could make the "only"
> > part more explicit and concrete in the comments/docs if desired.
> >
> > I'm not sure I see the value in complicating things in order to add
> > or extend support to non-intuitive use cases beyond that - to me that
> > seems to just make things more confusing for end users.  But again
> > if you've something in mind that I'm simply missing, then by all
> > means please elaborate.
> 
> OK, let me share my view on this. As you said in the patch description,
> N is substitution to the number of the last CPU, in your example sort of
> #define N (15).
> 
> So, when I do echo N-N > cpuset.cpus, I want it to work as if I do
> echo 15-15 > cpuset.cpus. Why? Because in my terribly huge config
> I just want to do s/15/N.

Okay, That might seem like a simple point, but it helped a lot in
letting me understand how you were looking at this and in turn what
kinds of "interesting" parameter combinations are currently accepted.

> 
> Now let's check how it works .
> 
> root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 15-15>cpuset.cpus
> root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 15
> root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-0>cpuset.cpus
> root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 0
> 
> OK, works as expected. And what about N?
> 
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> > 0-15
> 
> OK, looks good.
> 
> > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus
> > /bin/echo: write error: Invalid argument
> 
> Why? If N is 15, it should work exactly as 15-15, but it doesn't...
> This is the source
> of confusion and unneeded refactoring of user scripts. (In practice of course
> nobody will use "N" because it's broken.) Documentation says nothing about
> this limitation, and this is a real example of "complicating things".

So the disconnect was that I was thinking "where does N make sense to
use" and you were thinking "N is just a number - let it be used anywhere
and let the sanity checks take care of the rest".  I can see that now.

I have added self-tests for some of these interesting (but yet valid)
inputs, like "15-15:15/15" as a way to document what is currently
supported.  I then copied those (i.e "N-N:N/N") to ensure that N is
consistent with being "just a number" as you outlined above.

> You can do better - parse "N" in bitmap_getnum() and avoid all that confusion.

N is now handled in getnum in v3 of the series and treated as simply
just another number, with no special cases related to where it appears.

> Same logic regarding all/none: all is the equivalent of 0-15, none is something
> like ", ,". Now let's try with "0-15,0-3, ," (imagine it's a result of
> merging configs).
> 
> root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-15,0-3, , >cpuset.cpus
> root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 0-15

Given that "none" has no users (now or envisioned) and basically amounts
to being a no-op as ", ," it seems to make sense to just drop it.  Which
made me reconsider implementing "all" as well.  Since "0-N" is the same
strlen as "all" and achieves exactly the same result, I've dropped the
idea of accepting "all" as well in the interest of keeping it simple.

Please have a look at v3 if you have the time and see if there are any
remaining things you'd like to see implemented differently, or things
I've inadvertently missed.  There are more commits (added tests) but
each change is hopefully a small, easy to review, standalone item.

It seems like vger/lkml is having some issues, but assuming the mails
eventually get through, the v3 series should get archived at:

https://lore.kernel.org/lkml/20210126171141.122639-1-paul.gortmaker@windriver.com/

Thanks,
Paul.
--

> 
> OK, works. But if I do 's/0-15/all' and 's/ /none', things get broken. This
> again brings a special case where it can be avoided - just  parse all/none
> at the beginning of bitmap_parse_region().
> 
> After reading the description, one can think that you introduce simple and
> convenient extensions to existing interface. In fact this is a new interface
> which is mostly incompatible with the existing one.
> 
> > > I would also like to see tests covering new functionality. As a user of "N",
> > > I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including
> > > error codes that the parser returns. Otherwise it will be hard to maintain the
> > > transition.
> >
> > That is a reasonable request.  I will look into adding "N" based type
> > tests to the existing bitmap test cases in a separate commit.
> >
> > Thanks,
> > Paul.
> > --
> >
> > >
> > > >         if (end_of_region(*str))
> > > >                 goto no_pattern;
> > > > @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > > >   * Syntax: range:used_size/group_size
> > > >   * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
> > > >   * Optionally the self-descriptive "all" or "none" can be used.
> > > > + * The value 'N' can be used as the end of a range to indicate the maximum
> > > > + * allowed value; i.e (nmaskbits - 1).
> > > >   *
> > > >   * Returns: 0 on success, -errno on invalid input strings. Error values:
> > > >   *
> > > > @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> > > >                 if (buf == NULL)
> > > >                         return 0;
> > > >
> > > > -               buf = bitmap_parse_region(buf, &r);
> > > > +               buf = bitmap_parse_region(buf, &r, nmaskbits);
> > > >                 if (IS_ERR(buf))
> > > >                         return PTR_ERR(buf);
> > > >
> > > > --
> > > > 2.17.1
> > > >