[PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

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

Originally I did this at the CPU subsys level, but Yury suggested it
be moved down further to bitmap level itself, which made the core 
implementation smaller and less complex, but the series longer.

New self tests are added to better exercise what bitmap range/region
currently supports, and new tests are added for the new "N" support.

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

   root@hackbox:~# cat /proc/cmdline 
   BOOT_IMAGE=/boot/bzImage root=/dev/sda1 rcu_nocbs=2,3,8-N:1/2
   root@hackbox:~# dmesg|grep Offl
   rcu:     Offload RCU callbacks from CPUs: 2-3,8,10,12,14.

   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 N-N:N/N > cpuset.cpus
   root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
   15

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

I've intentionally not gone down the rabbit hole of whether N or Z or
L is the better letter to mark the end of a mathematical set in the
hope that we can stay focused, and get this closed out here in v4.

Aside from that, I believe all other feedback has been responded to
in one way or another.  Note that I didn't add Reviewed/Ack tags to
anything that changed significantly from what was reviewed in v3.

[v4: pair nbits with region, instead of inside it.  Split EINVAL and
 ERANGE tests.  Don't handle start/end/offset within a macro to
 abstract away nbits usage.  Added some Reviwed-by/Ack tags.]

[v3: Allow "N" to be used anywhere in the region spec, i.e. "N-N:N/N" vs.
 just being allowed at end of range like "0-N".  Add new self-tests.  Drop
 "all" and "none" aliases as redundant and not worth the extra complication. ]
 https://lore.kernel.org/lkml/20210126171141.122639-1-paul.gortmaker@windriver.com

[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.]
 https://lore.kernel.org/lkml/20210121223355.59780-1-paul.gortmaker@windriver.com/

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

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


Paul Gortmaker (8):
  lib: test_bitmap: clearly separate ERANGE from EINVAL tests.
  lib: test_bitmap: add tests to trigger ERANGE case.
  lib: test_bitmap: add more start-end:offset/len tests
  lib: bitmap: move ERANGE check from set_region to check_region
  lib: bitmap: pair nbits value with region struct
  lib: bitmap: support "N" as an alias for size of bitmap
  lib: test_bitmap: add tests for "N" alias
  rcu: deprecate "all" option to rcu_nocbs=

 .../admin-guide/kernel-parameters.rst         |  7 +++
 .../admin-guide/kernel-parameters.txt         |  4 +-
 kernel/rcu/tree_plugin.h                      |  6 +-
 lib/bitmap.c                                  | 62 +++++++++++++------
 lib/test_bitmap.c                             | 46 ++++++++++++--
 5 files changed, 93 insertions(+), 32 deletions(-)

-- 
2.17.1

[PATCH 1/8] lib: test_bitmap: clearly separate ERANGE from EINVAL tests.

[Paul Gortmaker]

This block of tests was meant to find/flag incorrect use of the ":"
and "/" separators (syntax errors) and invalid (zero) group len.

However they were specified with an 8 bit width and 32 bit operations,
so they really contained two errors (EINVAL and ERANGE).

Promote them to 32 bit so it is clear what they are meant to target.
Then we can add tests specific for ERANGE (no syntax errors, just
doing 32bit op on 8 bit width, plus a typical 9-on-8 fencepost error).

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/test_bitmap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 4425a1dd4ef1..589f2a34ceba 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -338,12 +338,12 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{-EINVAL, "-1",	NULL, 8, 0},
 	{-EINVAL, "-0",	NULL, 8, 0},
 	{-EINVAL, "10-1", NULL, 8, 0},
-	{-EINVAL, "0-31:", NULL, 8, 0},
-	{-EINVAL, "0-31:0", NULL, 8, 0},
-	{-EINVAL, "0-31:0/", NULL, 8, 0},
-	{-EINVAL, "0-31:0/0", NULL, 8, 0},
-	{-EINVAL, "0-31:1/0", NULL, 8, 0},
-	{-EINVAL, "0-31:10/1", NULL, 8, 0},
+	{-EINVAL, "0-31:", NULL, 32, 0},
+	{-EINVAL, "0-31:0", NULL, 32, 0},
+	{-EINVAL, "0-31:0/", NULL, 32, 0},
+	{-EINVAL, "0-31:0/0", NULL, 32, 0},
+	{-EINVAL, "0-31:1/0", NULL, 32, 0},
+	{-EINVAL, "0-31:10/1", NULL, 32, 0},
 	{-EOVERFLOW, "0-98765432123456789:10/1", NULL, 8, 0},
 
 	{-EINVAL, "a-31", NULL, 8, 0},
-- 
2.17.1

[PATCH 2/8] lib: test_bitmap: add tests to trigger ERANGE case.

[Paul Gortmaker]

Add tests that specify a valid range, but one that is outside the
width of the bitmap for which it is to be applied to.  These should
trigger an -ERANGE response from the code.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/test_bitmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 589f2a34ceba..172ffbfa83c4 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -338,6 +338,8 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{-EINVAL, "-1",	NULL, 8, 0},
 	{-EINVAL, "-0",	NULL, 8, 0},
 	{-EINVAL, "10-1", NULL, 8, 0},
+	{-ERANGE, "8-8", NULL, 8, 0},
+	{-ERANGE, "0-31", NULL, 8, 0},
 	{-EINVAL, "0-31:", NULL, 32, 0},
 	{-EINVAL, "0-31:0", NULL, 32, 0},
 	{-EINVAL, "0-31:0/", NULL, 32, 0},
-- 
2.17.1

[PATCH 3/8] lib: test_bitmap: add more start-end:offset/len tests

[Paul Gortmaker]

There are inputs to bitmap_parselist() that would probably never
be entered manually by a person, but might result from some kind of
automated input generator.  Things like ranges of length 1, or group
lengths longer than nbits, overlaps, or offsets of zero.

Adding these tests serve two purposes:

1) document what might seem odd but nonetheless valid input.

2) don't regress from what we currently accept as valid.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Yury Norov <yury.norov@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/test_bitmap.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 172ffbfa83c4..9c6a88c480c1 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -35,6 +35,8 @@ static const unsigned long exp1[] __initconst = {
 	BITMAP_FROM_U64(0x3333333311111111ULL),
 	BITMAP_FROM_U64(0xffffffff77777777ULL),
 	BITMAP_FROM_U64(0),
+	BITMAP_FROM_U64(0x00008000),
+	BITMAP_FROM_U64(0x80000000),
 };
 
 static const unsigned long exp2[] __initconst = {
@@ -335,6 +337,26 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{0, " ,  ,,  , ,   ",		&exp1[12 * step], 8, 0},
 	{0, " ,  ,,  , ,   \n",		&exp1[12 * step], 8, 0},
 
+	{0, "0-0",			&exp1[0], 32, 0},
+	{0, "1-1",			&exp1[1 * step], 32, 0},
+	{0, "15-15",			&exp1[13 * step], 32, 0},
+	{0, "31-31",			&exp1[14 * step], 32, 0},
+
+	{0, "0-0:0/1",			&exp1[12 * step], 32, 0},
+	{0, "0-0:1/1",			&exp1[0], 32, 0},
+	{0, "0-0:1/31",			&exp1[0], 32, 0},
+	{0, "0-0:31/31",		&exp1[0], 32, 0},
+	{0, "1-1:1/1",			&exp1[1 * step], 32, 0},
+	{0, "0-15:16/31",		&exp1[2 * step], 32, 0},
+	{0, "15-15:1/2",		&exp1[13 * step], 32, 0},
+	{0, "15-15:31/31",		&exp1[13 * step], 32, 0},
+	{0, "15-31:1/31",		&exp1[13 * step], 32, 0},
+	{0, "16-31:16/31",		&exp1[3 * step], 32, 0},
+	{0, "31-31:31/31",		&exp1[14 * step], 32, 0},
+
+	{0, "0-31:1/3,1-31:1/3,2-31:1/3",	&exp1[8 * step], 32, 0},
+	{0, "1-10:8/12,8-31:24/29,0-31:0/3",	&exp1[9 * step], 32, 0},
+
 	{-EINVAL, "-1",	NULL, 8, 0},
 	{-EINVAL, "-0",	NULL, 8, 0},
 	{-EINVAL, "10-1", NULL, 8, 0},
-- 
2.17.1

[PATCH 4/8] lib: bitmap: move ERANGE check from set_region to check_region

[Paul Gortmaker]

It makes sense to do all the checks in check_region() and not 1/2
in check_region and 1/2 in set_region.

Since set_region is called immediately after check_region, the net
effect on runtime is zero, but it gets rid of an if (...) return...

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/bitmap.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 75006c4036e9..9596ba53c36b 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -499,25 +499,22 @@ struct region {
 	unsigned int end;
 };
 
-static int bitmap_set_region(const struct region *r,
-				unsigned long *bitmap, int nbits)
+static void bitmap_set_region(const struct region *r, unsigned long *bitmap)
 {
 	unsigned int start;
 
-	if (r->end >= nbits)
-		return -ERANGE;
-
 	for (start = r->start; start <= r->end; start += r->group_len)
 		bitmap_set(bitmap, start, min(r->end - start + 1, r->off));
-
-	return 0;
 }
 
-static int bitmap_check_region(const struct region *r)
+static int bitmap_check_region(const struct region *r, int nbits)
 {
 	if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
 		return -EINVAL;
 
+	if (r->end >= nbits)
+		return -ERANGE;
+
 	return 0;
 }
 
@@ -651,13 +648,11 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 		if (IS_ERR(buf))
 			return PTR_ERR(buf);
 
-		ret = bitmap_check_region(&r);
+		ret = bitmap_check_region(&r, nmaskbits);
 		if (ret)
 			return ret;
 
-		ret = bitmap_set_region(&r, maskp, nmaskbits);
-		if (ret)
-			return ret;
+		bitmap_set_region(&r, maskp);
 	}
 
 	return 0;
-- 
2.17.1

[PATCH 5/8] lib: bitmap: pair nbits value with region struct

[Paul Gortmaker]

A region is a standalone entity to some degree, but it needs to
be paired with a bitmap width in order to set context and determine
if the region even fits into the width of the bitmap.

This will reduce parameter passing and enable using nbits as part
of future dynamic region parameter parsing.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Yury Norov <yury.norov@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/bitmap.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 9596ba53c36b..6b568f98af3d 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -499,6 +499,16 @@ struct region {
 	unsigned int end;
 };
 
+/*
+ * The region "0-3" is a complete specification, i.e. "the 1st four cores"
+ * for a CPU map, but it needs to be paired to a width in order to have a
+ * meaningful and valid context. (i.e. 4 core region on 4+ core machine...)
+ */
+struct bitmap_region {
+	struct region *r;
+	unsigned int nbits;
+};
+
 static void bitmap_set_region(const struct region *r, unsigned long *bitmap)
 {
 	unsigned int start;
@@ -507,12 +517,14 @@ static void bitmap_set_region(const struct region *r, unsigned long *bitmap)
 		bitmap_set(bitmap, start, min(r->end - start + 1, r->off));
 }
 
-static int bitmap_check_region(const struct region *r, int nbits)
+static int bitmap_check_region(const struct bitmap_region *br)
 {
+	struct region *r = br->r;
+
 	if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
 		return -EINVAL;
 
-	if (r->end >= nbits)
+	if (r->end >= br->nbits)
 		return -ERANGE;
 
 	return 0;
@@ -635,8 +647,12 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
 int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 {
 	struct region r;
+	struct bitmap_region br;
 	long ret;
 
+	br.r = &r;
+	br.nbits = nmaskbits;
+
 	bitmap_zero(maskp, nmaskbits);
 
 	while (buf) {
@@ -648,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 		if (IS_ERR(buf))
 			return PTR_ERR(buf);
 
-		ret = bitmap_check_region(&r, nmaskbits);
+		ret = bitmap_check_region(&br);
 		if (ret)
 			return ret;
 
-- 
2.17.1

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

[Paul Gortmaker]

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 <yury.norov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Yury Norov <yury.norov@gmail.com> # move it from CPU code
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 .../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)
 {
 	unsigned long long n;
 	unsigned int len;
 
+	if (str[0] == 'N') {
+		*num = lastbit;
+		return str + 1;
+	}
+
 	len = _parse_integer(str, 10, &n);
 	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)
 {
-	str = bitmap_getnum(str, &r->start);
+	struct region *r = br->r;
+	unsigned int lastbit = br->nbits - 1;
+
+	str = bitmap_getnum(str, &r->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, &r->end);
+	str = bitmap_getnum(str + 1, &r->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, &r->off);
+	str = bitmap_getnum(str + 1, &r->off, lastbit);
 	if (IS_ERR(str))
 		return str;
 
 	if (*str != '/')
 		return ERR_PTR(-EINVAL);
 
-	return bitmap_getnum(str + 1, &r->group_len);
+	return bitmap_getnum(str + 1, &r->group_len, lastbit);
 
 no_end:
 	r->end = r->start;
@@ -636,6 +645,10 @@ 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
+ * The value 'N' can be used as a dynamically substituted token for the
+ * maximum allowed value; i.e (nmaskbits - 1).  Keep in mind that it is
+ * dynamic, so if system changes cause the bitmap width to change, such
+ * as more cores in a CPU list, then any ranges using N will also change.
  *
  * Returns: 0 on success, -errno on invalid input strings. Error values:
  *
@@ -660,7 +673,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, &br);
 		if (IS_ERR(buf))
 			return PTR_ERR(buf);
 
-- 
2.17.1

[PATCH 7/8] lib: test_bitmap: add tests for "N" alias

[Paul Gortmaker]

These are copies of existing tests, with just 31 --> N.  This ensures
the recently added "N" alias transparently works in any normally
numeric fields of a region specification.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/test_bitmap.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 9c6a88c480c1..a6048278d027 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -354,6 +354,16 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{0, "16-31:16/31",		&exp1[3 * step], 32, 0},
 	{0, "31-31:31/31",		&exp1[14 * step], 32, 0},
 
+	{0, "N-N",			&exp1[14 * step], 32, 0},
+	{0, "0-0:1/N",			&exp1[0], 32, 0},
+	{0, "0-0:N/N",			&exp1[0], 32, 0},
+	{0, "0-15:16/N",		&exp1[2 * step], 32, 0},
+	{0, "15-15:N/N",		&exp1[13 * step], 32, 0},
+	{0, "15-N:1/N",			&exp1[13 * step], 32, 0},
+	{0, "16-N:16/N",		&exp1[3 * step], 32, 0},
+	{0, "N-N:N/N",			&exp1[14 * step], 32, 0},
+
+	{0, "0-N:1/3,1-N:1/3,2-N:1/3",		&exp1[8 * step], 32, 0},
 	{0, "0-31:1/3,1-31:1/3,2-31:1/3",	&exp1[8 * step], 32, 0},
 	{0, "1-10:8/12,8-31:24/29,0-31:0/3",	&exp1[9 * step], 32, 0},
 
-- 
2.17.1

[PATCH 8/8] rcu: deprecate "all" option to rcu_nocbs=

[Paul Gortmaker]

With the core bitmap support now accepting "N" as a placeholder for
the end of the bitmap, "all" can be represented as "0-N" and has the
advantage of not being specific to RCU (or any other subsystem).

So deprecate the use of "all" by removing documentation references
to it.  The support itself needs to remain for now, since we don't
know how many people out there are using it currently, but since it
is in an __init area anyway, it isn't worth losing sleep over.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 4 +---
 kernel/rcu/tree_plugin.h                        | 6 ++----
 2 files changed, 3 insertions(+), 7 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..56788dfde922 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -1463,14 +1463,12 @@ 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 (!strcasecmp(str, "all"))		/* legacy: use "0-N" instead */
 		cpumask_setall(rcu_nocb_mask);
 	else
 		if (cpulist_parse(str, rcu_nocb_mask)) {
-- 
2.17.1

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

[Yury Norov]

On Tue, Feb 9, 2021 at 3:01 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() 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 <yury.norov@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Yury Norov <yury.norov@gmail.com> # move it from CPU code
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  .../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, &n);
>         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, &r->start);
> +       struct region *r = br->r;
> +       unsigned int lastbit = br->nbits - 1;
> +
> +       str = bitmap_getnum(str, &r->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, &r->end);
> +       str = bitmap_getnum(str + 1, &r->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, &r->off);
> +       str = bitmap_getnum(str + 1, &r->off, lastbit);
>         if (IS_ERR(str))
>                 return str;
>
>         if (*str != '/')
>                 return ERR_PTR(-EINVAL);
>
> -       return bitmap_getnum(str + 1, &r->group_len);
> +       return bitmap_getnum(str + 1, &r->group_len, lastbit);
>
>  no_end:
>         r->end = r->start;
> @@ -636,6 +645,10 @@ 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
> + * The value 'N' can be used as a dynamically substituted token for the
> + * maximum allowed value; i.e (nmaskbits - 1).  Keep in mind that it is
> + * dynamic, so if system changes cause the bitmap width to change, such
> + * as more cores in a CPU list, then any ranges using N will also change.
>   *
>   * Returns: 0 on success, -errno on invalid input strings. Error values:
>   *
> @@ -660,7 +673,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, &br);
>                 if (IS_ERR(buf))
>                         return PTR_ERR(buf);
>
> --
> 2.17.1
>

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 09/02/2021 (Tue 15:16) Yury Norov wrote:

> On Tue, Feb 9, 2021 at 3:01 PM Paul Gortmaker
> <paul.gortmaker@windriver.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;

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Andy Shevchenko]

On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
> 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.

I thought we kinda agreed that N is confusing and L is better.
N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
sounds better as *last available CPU number*.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/8] lib: test_bitmap: add tests to trigger ERANGE case.

[Andy Shevchenko]

On Tue, Feb 09, 2021 at 05:59:01PM -0500, Paul Gortmaker wrote:
> Add tests that specify a valid range, but one that is outside the
> width of the bitmap for which it is to be applied to.  These should
> trigger an -ERANGE response from the code.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Yury Norov <yury.norov@gmail.com>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  lib/test_bitmap.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 589f2a34ceba..172ffbfa83c4 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -338,6 +338,8 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
>  	{-EINVAL, "-1",	NULL, 8, 0},
>  	{-EINVAL, "-0",	NULL, 8, 0},
>  	{-EINVAL, "10-1", NULL, 8, 0},
> +	{-ERANGE, "8-8", NULL, 8, 0},
> +	{-ERANGE, "0-31", NULL, 8, 0},
>  	{-EINVAL, "0-31:", NULL, 32, 0},
>  	{-EINVAL, "0-31:0", NULL, 32, 0},
>  	{-EINVAL, "0-31:0/", NULL, 32, 0},
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/8] lib: bitmap: move ERANGE check from set_region to check_region

[Andy Shevchenko]

On Tue, Feb 09, 2021 at 05:59:03PM -0500, Paul Gortmaker wrote:
> It makes sense to do all the checks in check_region() and not 1/2
> in check_region and 1/2 in set_region.
> 
> Since set_region is called immediately after check_region, the net
> effect on runtime is zero, but it gets rid of an if (...) return...

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Yury Norov <yury.norov@gmail.com>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  lib/bitmap.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 75006c4036e9..9596ba53c36b 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -499,25 +499,22 @@ struct region {
>  	unsigned int end;
>  };
>  
> -static int bitmap_set_region(const struct region *r,
> -				unsigned long *bitmap, int nbits)
> +static void bitmap_set_region(const struct region *r, unsigned long *bitmap)
>  {
>  	unsigned int start;
>  
> -	if (r->end >= nbits)
> -		return -ERANGE;
> -
>  	for (start = r->start; start <= r->end; start += r->group_len)
>  		bitmap_set(bitmap, start, min(r->end - start + 1, r->off));
> -
> -	return 0;
>  }
>  
> -static int bitmap_check_region(const struct region *r)
> +static int bitmap_check_region(const struct region *r, int nbits)
>  {
>  	if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
>  		return -EINVAL;
>  
> +	if (r->end >= nbits)
> +		return -ERANGE;
> +
>  	return 0;
>  }
>  
> @@ -651,13 +648,11 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>  		if (IS_ERR(buf))
>  			return PTR_ERR(buf);
>  
> -		ret = bitmap_check_region(&r);
> +		ret = bitmap_check_region(&r, nmaskbits);
>  		if (ret)
>  			return ret;
>  
> -		ret = bitmap_set_region(&r, maskp, nmaskbits);
> -		if (ret)
> -			return ret;
> +		bitmap_set_region(&r, maskp);
>  	}
>  
>  	return 0;
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] lib: bitmap: pair nbits value with region struct

[Andy Shevchenko]

On Tue, Feb 09, 2021 at 05:59:04PM -0500, Paul Gortmaker wrote:
> A region is a standalone entity to some degree, but it needs to
> be paired with a bitmap width in order to set context and determine
> if the region even fits into the width of the bitmap.
> 
> This will reduce parameter passing and enable using nbits as part
> of future dynamic region parameter parsing.

...

> +struct bitmap_region {
> +	struct region *r;

Why do we need it as a pointer?

> +	unsigned int nbits;
> +};

...

>  	struct region r;
> +	struct bitmap_region br;

> +	br.r = &r;
> +	br.nbits = nmaskbits;

I thought about simply

	struct bitmap_region br;

	br.nbits = nmaskbits;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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@windriver.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

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Paul E. McKenney]

On Wed, Feb 10, 2021 at 06:26:54PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
> > 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.
> 
> I thought we kinda agreed that N is confusing and L is better.
> N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
> sounds better as *last available CPU number*.

The advantage of "N" is that people will automatically recognize it as
"last thing" or number of things" because "N" has long been used in
both senses.  In contrast, someone seeing "0-L" for the first time is
likely to go "What???".

Besides, why would someone interpret "N" as "number of CPUs" when doing
that almost always gets you an invalid CPU number?

							Thanx, Paul

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Yury Norov]

On Wed, Feb 10, 2021 at 9:57 AM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Wed, Feb 10, 2021 at 06:26:54PM +0200, Andy Shevchenko wrote:
> > On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
> > > 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.
> >
> > I thought we kinda agreed that N is confusing and L is better.
> > N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
> > sounds better as *last available CPU number*.
>
> The advantage of "N" is that people will automatically recognize it as
> "last thing" or number of things" because "N" has long been used in
> both senses.  In contrast, someone seeing "0-L" for the first time is
> likely to go "What???".
>
> Besides, why would someone interpret "N" as "number of CPUs" when doing
> that almost always gets you an invalid CPU number?
>
>                                                         Thanx, Paul

I have no strong opinion about a letter, but I like Andy's idea to make it
case-insensitive.

There is another comment from the previous iteration not addressed so far.

This idea of the N notation is to make the bitmap list interface more robust
when we share the configs between different machines. What we have now
is definitely a good thing, but not completely portable except for cases
'N', '0-N' and 'N-N'.

For example, if one user adds rcu_nocbs= '4-N', and it works perfectly fine for
him, another user with s NR_CPUS == 2 will fail to boot with such a config.

This is not a problem of course in case of absolute values because nobody
guaranteed robustness. But this N feature would be barely useful in practice,
except for 'N', '0-N' and 'N-N' as I mentioned before, because there's always
a chance to end up with a broken config.

We can improve on robustness a lot if we take care about this case.For me,
the more reliable interface would look like this:
1. chunks without N work as before.
2. if 'a-N' is passed where a>=N, we drop chunk and print warning message
3. if 'a-N' is passed where a>=N together with a control key, we set last bit
and print warning.

For example, on 2-core CPU:
"4-2" --> error
"4-4" --> error
"4-N" --> drop and warn
"X, 4-N" --> set last bit and warn

Any comments?

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Paul E. McKenney]

On Wed, Feb 10, 2021 at 03:50:07PM -0800, Yury Norov wrote:
> On Wed, Feb 10, 2021 at 9:57 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Wed, Feb 10, 2021 at 06:26:54PM +0200, Andy Shevchenko wrote:
> > > On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
> > > > 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.
> > >
> > > I thought we kinda agreed that N is confusing and L is better.
> > > N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
> > > sounds better as *last available CPU number*.
> >
> > The advantage of "N" is that people will automatically recognize it as
> > "last thing" or number of things" because "N" has long been used in
> > both senses.  In contrast, someone seeing "0-L" for the first time is
> > likely to go "What???".
> >
> > Besides, why would someone interpret "N" as "number of CPUs" when doing
> > that almost always gets you an invalid CPU number?
> >
> >                                                         Thanx, Paul
> 
> I have no strong opinion about a letter, but I like Andy's idea to make it
> case-insensitive.
> 
> There is another comment from the previous iteration not addressed so far.
> 
> This idea of the N notation is to make the bitmap list interface more robust
> when we share the configs between different machines. What we have now
> is definitely a good thing, but not completely portable except for cases
> 'N', '0-N' and 'N-N'.
> 
> For example, if one user adds rcu_nocbs= '4-N', and it works perfectly fine for
> him, another user with s NR_CPUS == 2 will fail to boot with such a config.
> 
> This is not a problem of course in case of absolute values because nobody
> guaranteed robustness. But this N feature would be barely useful in practice,
> except for 'N', '0-N' and 'N-N' as I mentioned before, because there's always
> a chance to end up with a broken config.
> 
> We can improve on robustness a lot if we take care about this case.For me,
> the more reliable interface would look like this:
> 1. chunks without N work as before.
> 2. if 'a-N' is passed where a>=N, we drop chunk and print warning message
> 3. if 'a-N' is passed where a>=N together with a control key, we set last bit
> and print warning.
> 
> For example, on 2-core CPU:
> "4-2" --> error
> "4-4" --> error
> "4-N" --> drop and warn
> "X, 4-N" --> set last bit and warn
> 
> Any comments?

We really don't know the user's intent, and we cannot have complete
portability without knowing the user's intent.  For example, "4-N" means
"all but the first four CPUs", in which case an error is appropriate
because "4-N" makes no more sense on a 2-CPU system than does "4-1".
I could see a potential desire for some notation for "the last two CPUs",
but let's please have a real need for such a thing before overengineering
this patch series any further.

To get the level of portability you seem to be looking for, we need some
higher-level automation that knows how many CPUs there are and what
the intent is.  That automation can then generate the cpumasks for a
given system.  But for more typical situations, what Paul has now will
work fine.

Paul Gortmaker's patch series is doing something useful.  We should
not let potential future desires prevent us from taking a very useful
step forward.

							Thanx, Paul

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Rasmus Villemoes]

On 10/02/2021 18.57, Paul E. McKenney wrote:
> On Wed, Feb 10, 2021 at 06:26:54PM +0200, Andy Shevchenko wrote:
>> On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
>>> 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.
>>
>> I thought we kinda agreed that N is confusing and L is better.
>> N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
>> sounds better as *last available CPU number*.
> 
> The advantage of "N" is that people will automatically recognize it as
> "last thing" or number of things" because "N" has long been used in
> both senses.  In contrast, someone seeing "0-L" for the first time is
> likely to go "What???".

Completely agree. The patch that introduces this even updates
Documentation/ at the same time, and if people are confused just because
they don't RTFM, xkcd#293 applies. So let's please just paint the
bikeshed N. (As for case insensitivity, I don't see the point, it just
makes documentation and implementation more cumbersome and confusing.
Just document and implement _one_ way of doing this.)

As for a future syntax for "last 4 cpus", it's common to accept a
negative index to mean count from the end, so unless we already accept
-4 as a shorthand for 0-4 (haven't checked), that could be -4-N. But
regardless, I also agree with Paul on this point, that's for a future
time when the need arises.

Rasmus

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Yury Norov]

On Wed, Feb 10, 2021 at 04:23:09PM -0800, Paul E. McKenney wrote:
> On Wed, Feb 10, 2021 at 03:50:07PM -0800, Yury Norov wrote:
> > On Wed, Feb 10, 2021 at 9:57 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > >
> > > On Wed, Feb 10, 2021 at 06:26:54PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
> > > > > 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.
> > > >
> > > > I thought we kinda agreed that N is confusing and L is better.
> > > > N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
> > > > sounds better as *last available CPU number*.
> > >
> > > The advantage of "N" is that people will automatically recognize it as
> > > "last thing" or number of things" because "N" has long been used in
> > > both senses.  In contrast, someone seeing "0-L" for the first time is
> > > likely to go "What???".
> > >
> > > Besides, why would someone interpret "N" as "number of CPUs" when doing
> > > that almost always gets you an invalid CPU number?
> > >
> > >                                                         Thanx, Paul
> > 
> > I have no strong opinion about a letter, but I like Andy's idea to make it
> > case-insensitive.
> > 
> > There is another comment from the previous iteration not addressed so far.
> > 
> > This idea of the N notation is to make the bitmap list interface more robust
> > when we share the configs between different machines. What we have now
> > is definitely a good thing, but not completely portable except for cases
> > 'N', '0-N' and 'N-N'.
> > 
> > For example, if one user adds rcu_nocbs= '4-N', and it works perfectly fine for
> > him, another user with s NR_CPUS == 2 will fail to boot with such a config.
> > 
> > This is not a problem of course in case of absolute values because nobody
> > guaranteed robustness. But this N feature would be barely useful in practice,
> > except for 'N', '0-N' and 'N-N' as I mentioned before, because there's always
> > a chance to end up with a broken config.
> > 
> > We can improve on robustness a lot if we take care about this case.For me,
> > the more reliable interface would look like this:
> > 1. chunks without N work as before.
> > 2. if 'a-N' is passed where a>=N, we drop chunk and print warning message
> > 3. if 'a-N' is passed where a>=N together with a control key, we set last bit
> > and print warning.
> > 
> > For example, on 2-core CPU:
> > "4-2" --> error
> > "4-4" --> error
> > "4-N" --> drop and warn
> > "X, 4-N" --> set last bit and warn
> > 
> > Any comments?
> 
> We really don't know the user's intent, and we cannot have complete
> portability without knowing the user's intent.  For example, "4-N" means
> "all but the first four CPUs", in which case an error is appropriate
> because "4-N" makes no more sense on a 2-CPU system than does "4-1".
> I could see a potential desire for some notation for "the last two CPUs",
> but let's please have a real need for such a thing before overengineering
> this patch series any further.
> 
> To get the level of portability you seem to be looking for, we need some
> higher-level automation that knows how many CPUs there are and what
> the intent is.  That automation can then generate the cpumasks for a
> given system.  But for more typical situations, what Paul has now will
> work fine.
> 
> Paul Gortmaker's patch series is doing something useful.  We should
> not let potential future desires prevent us from taking a very useful
> step forward.
> 
> 							Thanx, Paul

No problem, we can do it later if it will become a real concern. 

Can you please remove this series from linux-next unless we finish
the review? It prevents me from applying the series from the LKML.

Yury

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Paul E. McKenney]

On Thu, Feb 11, 2021 at 04:23:39PM -0800, Yury Norov wrote:
> On Wed, Feb 10, 2021 at 04:23:09PM -0800, Paul E. McKenney wrote:
> > On Wed, Feb 10, 2021 at 03:50:07PM -0800, Yury Norov wrote:
> > > On Wed, Feb 10, 2021 at 9:57 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 10, 2021 at 06:26:54PM +0200, Andy Shevchenko wrote:
> > > > > On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
> > > > > > 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.
> > > > >
> > > > > I thought we kinda agreed that N is confusing and L is better.
> > > > > N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
> > > > > sounds better as *last available CPU number*.
> > > >
> > > > The advantage of "N" is that people will automatically recognize it as
> > > > "last thing" or number of things" because "N" has long been used in
> > > > both senses.  In contrast, someone seeing "0-L" for the first time is
> > > > likely to go "What???".
> > > >
> > > > Besides, why would someone interpret "N" as "number of CPUs" when doing
> > > > that almost always gets you an invalid CPU number?
> > > >
> > > >                                                         Thanx, Paul
> > > 
> > > I have no strong opinion about a letter, but I like Andy's idea to make it
> > > case-insensitive.
> > > 
> > > There is another comment from the previous iteration not addressed so far.
> > > 
> > > This idea of the N notation is to make the bitmap list interface more robust
> > > when we share the configs between different machines. What we have now
> > > is definitely a good thing, but not completely portable except for cases
> > > 'N', '0-N' and 'N-N'.
> > > 
> > > For example, if one user adds rcu_nocbs= '4-N', and it works perfectly fine for
> > > him, another user with s NR_CPUS == 2 will fail to boot with such a config.
> > > 
> > > This is not a problem of course in case of absolute values because nobody
> > > guaranteed robustness. But this N feature would be barely useful in practice,
> > > except for 'N', '0-N' and 'N-N' as I mentioned before, because there's always
> > > a chance to end up with a broken config.
> > > 
> > > We can improve on robustness a lot if we take care about this case.For me,
> > > the more reliable interface would look like this:
> > > 1. chunks without N work as before.
> > > 2. if 'a-N' is passed where a>=N, we drop chunk and print warning message
> > > 3. if 'a-N' is passed where a>=N together with a control key, we set last bit
> > > and print warning.
> > > 
> > > For example, on 2-core CPU:
> > > "4-2" --> error
> > > "4-4" --> error
> > > "4-N" --> drop and warn
> > > "X, 4-N" --> set last bit and warn
> > > 
> > > Any comments?
> > 
> > We really don't know the user's intent, and we cannot have complete
> > portability without knowing the user's intent.  For example, "4-N" means
> > "all but the first four CPUs", in which case an error is appropriate
> > because "4-N" makes no more sense on a 2-CPU system than does "4-1".
> > I could see a potential desire for some notation for "the last two CPUs",
> > but let's please have a real need for such a thing before overengineering
> > this patch series any further.
> > 
> > To get the level of portability you seem to be looking for, we need some
> > higher-level automation that knows how many CPUs there are and what
> > the intent is.  That automation can then generate the cpumasks for a
> > given system.  But for more typical situations, what Paul has now will
> > work fine.
> > 
> > Paul Gortmaker's patch series is doing something useful.  We should
> > not let potential future desires prevent us from taking a very useful
> > step forward.
> > 
> > 							Thanx, Paul
> 
> No problem, we can do it later if it will become a real concern. 
> 
> Can you please remove this series from linux-next unless we finish
> the review? It prevents me from applying the series from the LKML.

That will happen shortly, but in the meantime, just do the following on
top of -next before applying Paul's latest series:

	git revert b3c314b ed78166 1e792c4 e831c73

							Thanx, Paul

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

[Yury Norov]

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@windriver.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 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 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

Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation

[Paul Gortmaker]

[Re: [PATCH v4 0/8] support for bitmap (and hence CPU) list "N" abbreviation] On 10/02/2021 (Wed 15:50) Yury Norov wrote:

> On Wed, Feb 10, 2021 at 9:57 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Wed, Feb 10, 2021 at 06:26:54PM +0200, Andy Shevchenko wrote:
> > > On Tue, Feb 09, 2021 at 05:58:59PM -0500, Paul Gortmaker wrote:
> > > > 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.
> > >
> > > I thought we kinda agreed that N is confusing and L is better.
> > > N to me is equal to 32 on 32 core system as *number of cores / CPUs*. While L
> > > sounds better as *last available CPU number*.
> >
> > The advantage of "N" is that people will automatically recognize it as
> > "last thing" or number of things" because "N" has long been used in
> > both senses.  In contrast, someone seeing "0-L" for the first time is
> > likely to go "What???".
> >
> > Besides, why would someone interpret "N" as "number of CPUs" when doing
> > that almost always gets you an invalid CPU number?
> >
> >                                                         Thanx, Paul
> 
> I have no strong opinion about a letter, but I like Andy's idea to make it
> case-insensitive.

It is trivial to add later if someone can prove a genuine need for it,
but it is impossible to remove later if we add it now for no reason.

> 
> There is another comment from the previous iteration not addressed so far.

Actually, no - it was addressed in detail already:

https://lore.kernel.org/lkml/20210127091238.GH23530@windriver.com/

> This idea of the N notation is to make the bitmap list interface more robust
> when we share the configs between different machines. What we have now
> is definitely a good thing, but not completely portable except for cases
> 'N', '0-N' and 'N-N'.
> 
> For example, if one user adds rcu_nocbs= '4-N', and it works perfectly fine for
> him, another user with s NR_CPUS == 2 will fail to boot with such a config.

Firstly there is no "fail to boot" from "rcu_nocbs=<invalid>" -- that
just doesn't happen.   In any case, as you can see, I added in v4 the
documentation (as you requested) for this case - in several places.

And I explained in the thread above why any attempt to do some kind of
mapping policy was doomed to just add confusion and end up doing the
wrong thing.  And the discussion ended with that.

So I'm not clear why it was brought up again here as if I just ignored
your "broken config" concerns and never addressed them.

In any case as others have indicated, it serves no immediate purpose to
over-think this and start adding corner case reactions to use cases that
simply don't exist and probably never will.

Thanks,
Paul.
--

> 
> This is not a problem of course in case of absolute values because nobody
> guaranteed robustness. But this N feature would be barely useful in practice,
> except for 'N', '0-N' and 'N-N' as I mentioned before, because there's always
> a chance to end up with a broken config.
> 
> We can improve on robustness a lot if we take care about this case.For me,
> the more reliable interface would look like this:
> 1. chunks without N work as before.
> 2. if 'a-N' is passed where a>=N, we drop chunk and print warning message
> 3. if 'a-N' is passed where a>=N together with a control key, we set last bit
> and print warning.
> 
> For example, on 2-core CPU:
> "4-2" --> error
> "4-4" --> error
> "4-N" --> drop and warn
> "X, 4-N" --> set last bit and warn
> 
> Any comments?

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@windriver.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.

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

[PATCH 2/8] lib: test_bitmap: add tests to trigger ERANGE case.

[Paul Gortmaker]

Add tests that specify a valid range, but one that is outside the
width of the bitmap for which it is to be applied to.  These should
trigger an -ERANGE response from the code.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/test_bitmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 589f2a34ceba..172ffbfa83c4 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -338,6 +338,8 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{-EINVAL, "-1",	NULL, 8, 0},
 	{-EINVAL, "-0",	NULL, 8, 0},
 	{-EINVAL, "10-1", NULL, 8, 0},
+	{-ERANGE, "8-8", NULL, 8, 0},
+	{-ERANGE, "0-31", NULL, 8, 0},
 	{-EINVAL, "0-31:", NULL, 32, 0},
 	{-EINVAL, "0-31:0", NULL, 32, 0},
 	{-EINVAL, "0-31:0/", NULL, 32, 0},
-- 
2.30.0