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

[PATCH v3 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 [6/8] 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.
---

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

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

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 more start-end:offset/len tests
  lib: bitmap: fold nbits into region struct
  lib: bitmap: move ERANGE check from set_region to check_region
  lib: bitmap_getnum: separate arg into region and field
  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         |  2 +
 .../admin-guide/kernel-parameters.txt         |  4 +-
 kernel/rcu/tree_plugin.h                      |  6 +--
 lib/bitmap.c                                  | 46 ++++++++++--------
 lib/test_bitmap.c                             | 48 ++++++++++++++++---
 5 files changed, 72 insertions(+), 34 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,
and then add tests specific for ERANGE (no syntax errors, just doing
32bit op on 8 bit width, plus a typical 9-on-8 fencepost error).

Note that the remaining "10-1" on 8 is left as-is, since that captures
the fact that we check for (r->start > r->end) ---> EINVAL before we
check for (r->end >= nbits) ---> ERANGE.  If the code is ever re-ordered
then this test will pick up the mismatched errno value.

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 | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 4425a1dd4ef1..3d2cd3b1de84 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -337,13 +337,15 @@ 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, "10-1", NULL, 8, 0},	/* (start > end) ; also ERANGE */
+	{-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},
+	{-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 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>
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 3d2cd3b1de84..807d1e8dd59c 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},	/* (start > end) ; also ERANGE */
-- 
2.17.1

[PATCH 3/8] lib: bitmap: fold nbits into region struct

[Paul Gortmaker]

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>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/bitmap.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 75006c4036e9..162e2850c622 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -487,24 +487,24 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
 
 /*
  * Region 9-38:4/10 describes the following bitmap structure:
- * 0	   9  12    18			38
- * .........****......****......****......
- *	    ^  ^     ^			 ^
- *      start  off   group_len	       end
+ * 0	   9  12    18			38	     N
+ * .........****......****......****..................
+ *	    ^  ^     ^			 ^	     ^
+ *      start  off   group_len	       end	 nbits
  */
 struct region {
 	unsigned int start;
 	unsigned int off;
 	unsigned int group_len;
 	unsigned int end;
+	unsigned int nbits;
 };
 
-static int bitmap_set_region(const struct region *r,
-				unsigned long *bitmap, int nbits)
+static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
 {
 	unsigned int start;
 
-	if (r->end >= nbits)
+	if (r->end >= r->nbits)
 		return -ERANGE;
 
 	for (start = r->start; start <= r->end; start += r->group_len)
@@ -640,7 +640,8 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 	struct region r;
 	long ret;
 
-	bitmap_zero(maskp, nmaskbits);
+	r.nbits = nmaskbits;
+	bitmap_zero(maskp, r.nbits);
 
 	while (buf) {
 		buf = bitmap_find_region(buf);
@@ -655,7 +656,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 		if (ret)
 			return ret;
 
-		ret = bitmap_set_region(&r, maskp, nmaskbits);
+		ret = bitmap_set_region(&r, maskp);
 		if (ret)
 			return ret;
 	}
-- 
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>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 lib/bitmap.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 162e2850c622..833f152a2c43 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -500,17 +500,12 @@ struct region {
 	unsigned int nbits;
 };
 
-static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
+static void bitmap_set_region(const struct region *r, unsigned long *bitmap)
 {
 	unsigned int start;
 
-	if (r->end >= r->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)
@@ -518,6 +513,9 @@ static int bitmap_check_region(const struct region *r)
 	if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
 		return -EINVAL;
 
+	if (r->end >= r->nbits)
+		return -ERANGE;
+
 	return 0;
 }
 
@@ -656,9 +654,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 		if (ret)
 			return ret;
 
-		ret = bitmap_set_region(&r, maskp);
-		if (ret)
-			return ret;
+		bitmap_set_region(&r, maskp);
 	}
 
 	return 0;
-- 
2.17.1

[PATCH 5/8] lib: bitmap_getnum: separate arg into region and field

[Paul Gortmaker]

The bitmap_getnum is only used on a region's start/end/off/group_len
field.  Trivially decouple the region from the field so that the region
pointer is available for a pending change.

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/bitmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 833f152a2c43..f65be2f148fd 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -533,6 +533,7 @@ static const char *bitmap_getnum(const char *str, unsigned int *num)
 	*num = n;
 	return str + len;
 }
+#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))
 
 static inline bool end_of_str(char c)
 {
@@ -571,7 +572,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end
 
 static const char *bitmap_parse_region(const char *str, struct region *r)
 {
-	str = bitmap_getnum(str, &r->start);
+	str = bitmap_getrnum(str, r, start);
 	if (IS_ERR(str))
 		return str;
 
@@ -581,7 +582,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_getrnum(str + 1, r, end);
 	if (IS_ERR(str))
 		return str;
 
@@ -591,14 +592,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_getrnum(str + 1, r, off);
 	if (IS_ERR(str))
 		return str;
 
 	if (*str != '/')
 		return ERR_PTR(-EINVAL);
 
-	return bitmap_getnum(str + 1, &r->group_len);
+	return bitmap_getrnum(str + 1, r, group_len);
 
 no_end:
 	r->end = r->start;
-- 
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>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 Documentation/admin-guide/kernel-parameters.rst |  2 ++
 lib/bitmap.c                                    | 12 ++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index 682ab28b5c94..850917f19476 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -68,6 +68,8 @@ 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.
 
 
 This document may not be entirely up to date and comprehensive. The command
diff --git a/lib/bitmap.c b/lib/bitmap.c
index f65be2f148fd..2fdd00b312c3 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -519,11 +519,17 @@ static int bitmap_check_region(const struct region *r)
 	return 0;
 }
 
-static const char *bitmap_getnum(const char *str, unsigned int *num)
+static const char *__bitmap_getnum(const char *str, unsigned int nbits,
+				    unsigned int *num)
 {
 	unsigned long long n;
 	unsigned int len;
 
+	if (str[0] == 'N') {
+		*num = nbits - 1;
+		return str + 1;
+	}
+
 	len = _parse_integer(str, 10, &n);
 	if (!len)
 		return ERR_PTR(-EINVAL);
@@ -533,7 +539,7 @@ static const char *bitmap_getnum(const char *str, unsigned int *num)
 	*num = n;
 	return str + len;
 }
-#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))
+#define bitmap_getrnum(s, r, pos) __bitmap_getnum(s, r->nbits, &(r->pos))
 
 static inline bool end_of_str(char c)
 {
@@ -626,6 +632,8 @@ 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).
  *
  * Returns: 0 on success, -errno on invalid input strings. Error values:
  *
-- 
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 807d1e8dd59c..2bcea2517c03 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 1/8] lib: test_bitmap: clearly separate ERANGE from EINVAL tests.

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 12:11:34PM -0500, Paul Gortmaker wrote:
> 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,
> and then add tests specific for ERANGE (no syntax errors, just doing
> 32bit op on 8 bit width, plus a typical 9-on-8 fencepost error).
> 
> Note that the remaining "10-1" on 8 is left as-is, since that captures
> the fact that we check for (r->start > r->end) ---> EINVAL before we
> check for (r->end >= nbits) ---> ERANGE.  If the code is ever re-ordered
> then this test will pick up the mismatched errno value.

I didn't get the last statement. You meant code in the bitmap library itself,
and not in the test cases? Please, clarify this somehow.

I don't really much care, since it's not a tricky commit, but it might be split
to two or three separated ones. Anyway, feel free to add
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 | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 4425a1dd4ef1..3d2cd3b1de84 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -337,13 +337,15 @@ 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, "10-1", NULL, 8, 0},	/* (start > end) ; also ERANGE */
> +	{-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},
> +	{-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
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 12:11:35PM -0500, Paul Gortmaker wrote:
> 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.

Makes sense.
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 | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 3d2cd3b1de84..807d1e8dd59c 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},	/* (start > end) ; also ERANGE */
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 12:11:36PM -0500, Paul Gortmaker wrote:
> This will reduce parameter passing and enable using nbits as part
> of future dynamic region parameter parsing.

One nit below, nevertheless
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>
> Suggested-by: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  lib/bitmap.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 75006c4036e9..162e2850c622 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -487,24 +487,24 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
>  
>  /*
>   * Region 9-38:4/10 describes the following bitmap structure:
> - * 0	   9  12    18			38
> - * .........****......****......****......
> - *	    ^  ^     ^			 ^
> - *      start  off   group_len	       end
> + * 0	   9  12    18			38	     N
> + * .........****......****......****..................
> + *	    ^  ^     ^			 ^	     ^
> + *      start  off   group_len	       end	 nbits
>   */
>  struct region {
>  	unsigned int start;
>  	unsigned int off;
>  	unsigned int group_len;
>  	unsigned int end;
> +	unsigned int nbits;
>  };
>  
> -static int bitmap_set_region(const struct region *r,
> -				unsigned long *bitmap, int nbits)
> +static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
>  {
>  	unsigned int start;
>  
> -	if (r->end >= nbits)
> +	if (r->end >= r->nbits)
>  		return -ERANGE;
>  
>  	for (start = r->start; start <= r->end; start += r->group_len)
> @@ -640,7 +640,8 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>  	struct region r;
>  	long ret;
>  
> -	bitmap_zero(maskp, nmaskbits);
> +	r.nbits = nmaskbits;

> +	bitmap_zero(maskp, r.nbits);

This sounds not right from style perspective.
You have completely uninitialized r on stack, then you assign only one value
for immediate use here and...

>  	while (buf) {
>  		buf = bitmap_find_region(buf);
> @@ -655,7 +656,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>  		if (ret)
>  			return ret;
>  
> -		ret = bitmap_set_region(&r, maskp, nmaskbits);
> +		ret = bitmap_set_region(&r, maskp);

...hiding this fact here. Which I would expect that &r may be rewritten here.

I would leave these unchanged and simple assign the value in
bitmap_set_region().

>  		if (ret)
>  			return ret;
>  	}
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 11:16:25PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 26, 2021 at 12:11:36PM -0500, Paul Gortmaker wrote:
> > This will reduce parameter passing and enable using nbits as part
> > of future dynamic region parameter parsing.

...

> >  	struct region r;
> >  	long ret;
> >  
> > -	bitmap_zero(maskp, nmaskbits);
> > +	r.nbits = nmaskbits;

Alternatively (though I personally don't prefer it) you can clarify in the
definition block the initial values.

	struct region r = { .nbist = nmaskbits };

> > +	bitmap_zero(maskp, r.nbits);
> 
> This sounds not right from style perspective.
> You have completely uninitialized r on stack, then you assign only one value
> for immediate use here and...
> 
> >  	while (buf) {
> >  		buf = bitmap_find_region(buf);
> > @@ -655,7 +656,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> >  		if (ret)
> >  			return ret;
> >  
> > -		ret = bitmap_set_region(&r, maskp, nmaskbits);
> > +		ret = bitmap_set_region(&r, maskp);
> 
> ...hiding this fact here. Which I would expect that &r may be rewritten here.
> 
> I would leave these unchanged and simple assign the value in
> bitmap_set_region().

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 12:11:37PM -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...

Like it!
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/bitmap.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 162e2850c622..833f152a2c43 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -500,17 +500,12 @@ struct region {
>  	unsigned int nbits;
>  };
>  
> -static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
> +static void bitmap_set_region(const struct region *r, unsigned long *bitmap)
>  {
>  	unsigned int start;
>  
> -	if (r->end >= r->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)
> @@ -518,6 +513,9 @@ static int bitmap_check_region(const struct region *r)
>  	if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
>  		return -EINVAL;
>  
> +	if (r->end >= r->nbits)
> +		return -ERANGE;
> +
>  	return 0;
>  }
>  
> @@ -656,9 +654,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>  		if (ret)
>  			return ret;
>  
> -		ret = bitmap_set_region(&r, maskp);
> -		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_getnum: separate arg into region and field

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 12:11:38PM -0500, Paul Gortmaker wrote:
> The bitmap_getnum is only used on a region's start/end/off/group_len
> field.  Trivially decouple the region from the field so that the region
> pointer is available for a pending change.

Honestly, I don't like this macro trick. It's bad in couple of ways:
 - it hides what actually is done with the fields of r structure
   (after you get that they are fields!)
 - it breaks possibility to compile time (type) checks

I will listen what others say, but I'm in favour not to proceed like this.

> 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/bitmap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 833f152a2c43..f65be2f148fd 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -533,6 +533,7 @@ static const char *bitmap_getnum(const char *str, unsigned int *num)
>  	*num = n;
>  	return str + len;
>  }
> +#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))
>  
>  static inline bool end_of_str(char c)
>  {
> @@ -571,7 +572,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end
>  
>  static const char *bitmap_parse_region(const char *str, struct region *r)
>  {
> -	str = bitmap_getnum(str, &r->start);
> +	str = bitmap_getrnum(str, r, start);
>  	if (IS_ERR(str))
>  		return str;
>  
> @@ -581,7 +582,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_getrnum(str + 1, r, end);
>  	if (IS_ERR(str))
>  		return str;
>  
> @@ -591,14 +592,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_getrnum(str + 1, r, off);
>  	if (IS_ERR(str))
>  		return str;
>  
>  	if (*str != '/')
>  		return ERR_PTR(-EINVAL);
>  
> -	return bitmap_getnum(str + 1, &r->group_len);
> +	return bitmap_getrnum(str + 1, r, group_len);
>  
>  no_end:
>  	r->end = r->start;
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Yury Norov]

On Tue, Jan 26, 2021 at 9:12 AM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> 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 */

I think 'all' and 'none' is a good idea. It's simple and convenient.
But if you don't
like it, can you please at least put this comment in system log using
WARN_ON_ONCE(). It's quite possible that Linux users don't read source code
comments.

>                 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

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:
> While this is done for all bitmaps, the original use case in mind was
> for CPU masks and cpulist_parse() as described below.
> 
> It seems that a common configuration is to use the 1st couple cores for
> housekeeping tasks.  This tends to leave the remaining ones to form a
> pool of similarly configured cores to take on the real workload of
> interest to the user.
> 
> So on machine A - with 32 cores, it could be 0-3 for "system" and then
> 4-31 being used in boot args like nohz_full=, or rcu_nocbs= as part of
> setting up the worker pool of CPUs.
> 
> But then newer machine B is added, and it has 48 cores, and so while
> the 0-3 part remains unchanged, the pool setup cpu list becomes 4-47.
> 
> Multiple deployment becomes easier when we can just simply replace 31
> and 47 with "N" and let the system substitute in the actual number at
> boot; a number that it knows better than we do.

I would accept lower 'n' as well.

...

> -static const char *bitmap_getnum(const char *str, unsigned int *num)
> +static const char *__bitmap_getnum(const char *str, unsigned int nbits,
> +				    unsigned int *num)
>  {
>  	unsigned long long n;
>  	unsigned int len;
>  
> +	if (str[0] == 'N') {
> +		*num = nbits - 1;
> +		return str + 1;
> +	}

But locating it here makes possible to enter a priori invalid input, like N for
start of the region.

I think this should be separate helper which is called in places where it makes
sense.

>  	len = _parse_integer(str, 10, &n);
>  	if (!len)
>  		return ERR_PTR(-EINVAL);


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 26, 2021 at 11:37:30PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:

...

> > +	if (str[0] == 'N') {
> > +		*num = nbits - 1;
> > +		return str + 1;
> > +	}
> 
> But locating it here makes possible to enter a priori invalid input, like N for
> start of the region.
> 
> I think this should be separate helper which is called in places where it makes
> sense.

Okay, N is 31 on 32 core system... It is a bit counter intuitive, because it's
rather _L_ast than _N_umber of CPUs.

Changing letter?

-- 
With Best Regards,
Andy Shevchenko

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

[Paul E. McKenney]

On Tue, Jan 26, 2021 at 01:36:23PM -0800, Yury Norov wrote:
> On Tue, Jan 26, 2021 at 9:12 AM Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> >
> > 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 */
> 
> I think 'all' and 'none' is a good idea. It's simple and convenient.
> But if you don't
> like it, can you please at least put this comment in system log using
> WARN_ON_ONCE(). It's quite possible that Linux users don't read source code
> comments.

Please leave it silent.  This has been available to RCU users for
quite some time, so suddenly spewing warnings at all of them is a bit
unfriendly.  The extra code is negligible, and the documentation will
guide people more gently in the right direction.  Plus I am the one who
would end up receiving complaints about the warnings, and I have much
better things to do with my time.

							Thanx, Paul

> >                 cpumask_setall(rcu_nocb_mask);
> >         else
> >                 if (cpulist_parse(str, rcu_nocb_mask)) {
> > --
> > 2.17.1
> >

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

[Yury Norov]

On Tue, Jan 26, 2021 at 9:12 AM Paul Gortmaker
<paul.gortmaker@windriver.com> 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.
>
> 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 [6/8] 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.

I think it's worth moving this sentence to the Documentation. Another
caveat with
N is that users' config may surprisingly become invalid, like if user
says 32-N, and
on some machine with a smaller bitmap this config fails to boot.

It doesn't mean of course that I'm against 'N'. I think it's very
useful especially in
such common cases like "N", "0-N", "1-N".

Would it make sense to treat the mask "32-N" when N < 32 as N-N,
and bark something in dmesg?

> 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.]
>  https://lore.kernel.org/lkml/20210121223355.59780-1-paul.gortmaker@windriver.com/
>
> [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. ]
>
> 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 more start-end:offset/len tests
>   lib: bitmap: fold nbits into region struct
>   lib: bitmap: move ERANGE check from set_region to check_region
>   lib: bitmap_getnum: separate arg into region and field
>   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         |  2 +
>  .../admin-guide/kernel-parameters.txt         |  4 +-
>  kernel/rcu/tree_plugin.h                      |  6 +--
>  lib/bitmap.c                                  | 46 ++++++++++--------
>  lib/test_bitmap.c                             | 48 ++++++++++++++++---
>  5 files changed, 72 insertions(+), 34 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and field

[Yury Norov]

On Tue, Jan 26, 2021 at 1:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Jan 26, 2021 at 12:11:38PM -0500, Paul Gortmaker wrote:
> > The bitmap_getnum is only used on a region's start/end/off/group_len
> > field.  Trivially decouple the region from the field so that the region
> > pointer is available for a pending change.
>
> Honestly, I don't like this macro trick. It's bad in couple of ways:
>  - it hides what actually is done with the fields of r structure
>    (after you get that they are fields!)
>  - it breaks possibility to compile time (type) checks
>
> I will listen what others say, but I'm in favour not to proceed like this.

Agree. Would be better to drop the patch. Paul, what kind of pending
change do you mean here? All the following patches are not related to
parsing machinery.

> > 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/bitmap.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index 833f152a2c43..f65be2f148fd 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -533,6 +533,7 @@ static const char *bitmap_getnum(const char *str, unsigned int *num)
> >       *num = n;
> >       return str + len;
> >  }
> > +#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))
> >
> >  static inline bool end_of_str(char c)
> >  {
> > @@ -571,7 +572,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end
> >
> >  static const char *bitmap_parse_region(const char *str, struct region *r)
> >  {
> > -     str = bitmap_getnum(str, &r->start);
> > +     str = bitmap_getrnum(str, r, start);
> >       if (IS_ERR(str))
> >               return str;
> >
> > @@ -581,7 +582,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_getrnum(str + 1, r, end);
> >       if (IS_ERR(str))
> >               return str;
> >
> > @@ -591,14 +592,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_getrnum(str + 1, r, off);
> >       if (IS_ERR(str))
> >               return str;
> >
> >       if (*str != '/')
> >               return ERR_PTR(-EINVAL);
> >
> > -     return bitmap_getnum(str + 1, &r->group_len);
> > +     return bitmap_getrnum(str + 1, r, group_len);
> >
> >  no_end:
> >       r->end = r->start;
> > --
> > 2.17.1
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

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

[Yury Norov]

On Tue, Jan 26, 2021 at 9:12 AM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> 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>
> 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 3d2cd3b1de84..807d1e8dd59c 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},  /* (start > end) ; also ERANGE */
> --
> 2.17.1

Acked-by: Yury Norov <yury.norov@gmail.com>

Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct

[Yury Norov]

On Tue, Jan 26, 2021 at 9:12 AM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> 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>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  lib/bitmap.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 75006c4036e9..162e2850c622 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -487,24 +487,24 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
>
>  /*
>   * Region 9-38:4/10 describes the following bitmap structure:
> - * 0      9  12    18                  38
> - * .........****......****......****......
> - *         ^  ^     ^                   ^
> - *      start  off   group_len        end
> + * 0      9  12    18                  38           N
> + * .........****......****......****..................
> + *         ^  ^     ^                   ^           ^
> + *      start  off   group_len        end       nbits
>   */
>  struct region {
>         unsigned int start;
>         unsigned int off;
>         unsigned int group_len;
>         unsigned int end;
> +       unsigned int nbits;
>  };
>
> -static int bitmap_set_region(const struct region *r,
> -                               unsigned long *bitmap, int nbits)
> +static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
>  {
>         unsigned int start;
>
> -       if (r->end >= nbits)
> +       if (r->end >= r->nbits)
>                 return -ERANGE;
>
>         for (start = r->start; start <= r->end; start += r->group_len)
> @@ -640,7 +640,8 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>         struct region r;
>         long ret;
>
> -       bitmap_zero(maskp, nmaskbits);
> +       r.nbits = nmaskbits;
> +       bitmap_zero(maskp, r.nbits);
>
>         while (buf) {
>                 buf = bitmap_find_region(buf);
> @@ -655,7 +656,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>                 if (ret)
>                         return ret;
>
> -               ret = bitmap_set_region(&r, maskp, nmaskbits);
> +               ret = bitmap_set_region(&r, maskp);
>                 if (ret)
>                         return ret;
>         }
> --
> 2.17.1

Acked-by: Yury Norov <yury.norov@gmail.com>

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

[Yury Norov]

On Tue, Jan 26, 2021 at 9:12 AM Paul Gortmaker
<paul.gortmaker@windriver.com> 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...
>
> 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/bitmap.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 162e2850c622..833f152a2c43 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -500,17 +500,12 @@ struct region {
>         unsigned int nbits;
>  };
>
> -static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
> +static void bitmap_set_region(const struct region *r, unsigned long *bitmap)
>  {
>         unsigned int start;
>
> -       if (r->end >= r->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)
> @@ -518,6 +513,9 @@ static int bitmap_check_region(const struct region *r)
>         if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
>                 return -EINVAL;
>
> +       if (r->end >= r->nbits)
> +               return -ERANGE;
> +
>         return 0;
>  }
>
> @@ -656,9 +654,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>                 if (ret)
>                         return ret;
>
> -               ret = bitmap_set_region(&r, maskp);
> -               if (ret)
> -                       return ret;
> +               bitmap_set_region(&r, maskp);
>         }
>
>         return 0;
> --
> 2.17.1

Acked-by: Yury Norov <yury.norov@gmail.com>

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

[Paul Gortmaker]

[Re: [PATCH 1/8] lib: test_bitmap: clearly separate ERANGE from EINVAL tests.] On 26/01/2021 (Tue 23:04) Andy Shevchenko wrote:

> On Tue, Jan 26, 2021 at 12:11:34PM -0500, Paul Gortmaker wrote:
> > 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,
> > and then add tests specific for ERANGE (no syntax errors, just doing
> > 32bit op on 8 bit width, plus a typical 9-on-8 fencepost error).
> > 
> > Note that the remaining "10-1" on 8 is left as-is, since that captures
> > the fact that we check for (r->start > r->end) ---> EINVAL before we
> > check for (r->end >= nbits) ---> ERANGE.  If the code is ever re-ordered
> > then this test will pick up the mismatched errno value.
> 
> I didn't get the last statement. You meant code in the bitmap library itself,
> and not in the test cases? Please, clarify this somehow.

It probably wasn't worth a mention at all, as that test in question was
left unchanged;  but yes - it was a reference to the ordering of the
sanity checks in the bitmap code itself and not the test order.   I'll
simply delete the confusing "10-1" paragraph/comment. 

> I don't really much care, since it's not a tricky commit, but it might be split
> to two or three separated ones. Anyway, feel free to add
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Since you mentioned it, I assume you would prefer it.  So I will make
the 8 --> 32 change in one commit, and add the two new ERANGE tests in
another subsequent commit.

Thanks,
Paul.
--

> 
> > 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 | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> > index 4425a1dd4ef1..3d2cd3b1de84 100644
> > --- a/lib/test_bitmap.c
> > +++ b/lib/test_bitmap.c
> > @@ -337,13 +337,15 @@ 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, "10-1", NULL, 8, 0},	/* (start > end) ; also ERANGE */
> > +	{-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},
> > +	{-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
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct

[Paul Gortmaker]

[Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct] On 26/01/2021 (Tue 23:16) Andy Shevchenko wrote:

> On Tue, Jan 26, 2021 at 12:11:36PM -0500, Paul Gortmaker wrote:
> > This will reduce parameter passing and enable using nbits as part
> > of future dynamic region parameter parsing.
> 
> One nit below, nevertheless
> 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>
> > Suggested-by: Yury Norov <yury.norov@gmail.com>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > ---
> >  lib/bitmap.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index 75006c4036e9..162e2850c622 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -487,24 +487,24 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
> >  
> >  /*
> >   * Region 9-38:4/10 describes the following bitmap structure:
> > - * 0	   9  12    18			38
> > - * .........****......****......****......
> > - *	    ^  ^     ^			 ^
> > - *      start  off   group_len	       end
> > + * 0	   9  12    18			38	     N
> > + * .........****......****......****..................
> > + *	    ^  ^     ^			 ^	     ^
> > + *      start  off   group_len	       end	 nbits
> >   */
> >  struct region {
> >  	unsigned int start;
> >  	unsigned int off;
> >  	unsigned int group_len;
> >  	unsigned int end;
> > +	unsigned int nbits;
> >  };
> >  
> > -static int bitmap_set_region(const struct region *r,
> > -				unsigned long *bitmap, int nbits)
> > +static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
> >  {
> >  	unsigned int start;
> >  
> > -	if (r->end >= nbits)
> > +	if (r->end >= r->nbits)
> >  		return -ERANGE;
> >  
> >  	for (start = r->start; start <= r->end; start += r->group_len)
> > @@ -640,7 +640,8 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> >  	struct region r;
> >  	long ret;
> >  
> > -	bitmap_zero(maskp, nmaskbits);
> > +	r.nbits = nmaskbits;
> 
> > +	bitmap_zero(maskp, r.nbits);
> 
> This sounds not right from style perspective.
> You have completely uninitialized r on stack, then you assign only one value
> for immediate use here and...

So, this change was added because Yury suggested that I "..store
nmaskbits in the struct region, and avoid passing nmaskbits as a
parameter."

To which I originally noted "I considered that and went with the param
so as to not open the door to someone possibly using an uninitialized
struct value later."

https://lore.kernel.org/lkml/20210122044357.GS16838@windriver.com/

Looking back, I had a similar thought as to yours, it seems...

I am also thinking more and more that nbits doesn't belong in the
region anyway - yes, a region gets validated against a specific nbits
eventually, but it doesn't need an nbits field to be a complete
specification.  The region "0-3" is a complete specification for "the
1st four cores" and is as valid on a 4 core machine as it is on a 64 core
machine -- a validation we do when we deploy the region on that machine.

I will set this change aside and get the nbits value to getnum() another
way, and leave the region struct as it was -- without a nbits field.

This will also resolve having the macro handling of region that you were
not really liking.

Paul.
--

> >  	while (buf) {
> >  		buf = bitmap_find_region(buf);
> > @@ -655,7 +656,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> >  		if (ret)
> >  			return ret;
> >  
> > -		ret = bitmap_set_region(&r, maskp, nmaskbits);
> > +		ret = bitmap_set_region(&r, maskp);
> 
> ...hiding this fact here. Which I would expect that &r may be rewritten here.
> 
> I would leave these unchanged and simple assign the value in
> bitmap_set_region().
> 
> >  		if (ret)
> >  			return ret;
> >  	}
> > -- 
> > 2.17.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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 26/01/2021 (Tue 23:37) Andy Shevchenko wrote:

> On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:
> > While this is done for all bitmaps, the original use case in mind was
> > for CPU masks and cpulist_parse() as described below.
> > 
> > It seems that a common configuration is to use the 1st couple cores for
> > housekeeping tasks.  This tends to leave the remaining ones to form a
> > pool of similarly configured cores to take on the real workload of
> > interest to the user.
> > 
> > So on machine A - with 32 cores, it could be 0-3 for "system" and then
> > 4-31 being used in boot args like nohz_full=, or rcu_nocbs= as part of
> > setting up the worker pool of CPUs.
> > 
> > But then newer machine B is added, and it has 48 cores, and so while
> > the 0-3 part remains unchanged, the pool setup cpu list becomes 4-47.
> > 
> > Multiple deployment becomes easier when we can just simply replace 31
> > and 47 with "N" and let the system substitute in the actual number at
> > boot; a number that it knows better than we do.
> 
> I would accept lower 'n' as well.
> 
> ...
> 
> > -static const char *bitmap_getnum(const char *str, unsigned int *num)
> > +static const char *__bitmap_getnum(const char *str, unsigned int nbits,
> > +				    unsigned int *num)
> >  {
> >  	unsigned long long n;
> >  	unsigned int len;
> >  
> > +	if (str[0] == 'N') {
> > +		*num = nbits - 1;
> > +		return str + 1;
> > +	}
> 
> But locating it here makes possible to enter a priori invalid input, like N for
> start of the region.

Actually, no.  N can be valid input for start of the region - or for any
field in the region.  I was originally thinking like you -- that N
was only valid as the end of the region, but Yury made a compelling
argument that N should be treated exactly as any other number is.

Skip down to where Yury says:
     So, when I do echo N-N > cpuset.cpus, I want it to work as
     if I do echo 15-15 > cpuset.cpus.

  https://lore.kernel.org/lkml/20210126171811.GC23530@windriver.com/

You weren't Cc'd at that point as I'd not added any self-test changes
to the series yet - so you didn't probably see that discussion.

This is why you'll see "N-N:N/N" added as a self-test that works.  It
doesn't make any more sense than using 15-15:15/15 does (vs. "15") but
both are equally valid inputs, in that they don't trigger an error.

Thanks,
Paul.
--

> 
> I think this should be separate helper which is called in places where it makes
> sense.
> 
> >  	len = _parse_integer(str, 10, &n);
> >  	if (!len)
> >  		return ERR_PTR(-EINVAL);
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and field

[Paul Gortmaker]

[Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and field] On 26/01/2021 (Tue 18:58) Yury Norov wrote:

> On Tue, Jan 26, 2021 at 1:22 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Jan 26, 2021 at 12:11:38PM -0500, Paul Gortmaker wrote:
> > > The bitmap_getnum is only used on a region's start/end/off/group_len
> > > field.  Trivially decouple the region from the field so that the region
> > > pointer is available for a pending change.
> >
> > Honestly, I don't like this macro trick. It's bad in couple of ways:
> >  - it hides what actually is done with the fields of r structure
> >    (after you get that they are fields!)
> >  - it breaks possibility to compile time (type) checks
> >
> > I will listen what others say, but I'm in favour not to proceed like this.
> 
> Agree. Would be better to drop the patch. Paul, what kind of pending
> change do you mean here? All the following patches are not related to
> parsing machinery.

It was directly related, because...

> 
> > > 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/bitmap.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > > index 833f152a2c43..f65be2f148fd 100644
> > > --- a/lib/bitmap.c
> > > +++ b/lib/bitmap.c
> > > @@ -533,6 +533,7 @@ static const char *bitmap_getnum(const char *str, unsigned int *num)
> > >       *num = n;
> > >       return str + len;
> > >  }
> > > +#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))

...this one line above opened the door to then do [in 6/8]:

   -#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))
   +#define bitmap_getrnum(s, r, pos) __bitmap_getnum(s, r->nbits, &(r->pos))

which gets nbits down into bitmap_getnum so we can handle N in there as
the placement you'd specifically requested for treating N as just a number.

In any case, I've decided against putting nbits into the region struct
and have got the nbits value down into getnum() another way for v4,
without using this commit or similar macros.

Paul.
--

> > >
> > >  static inline bool end_of_str(char c)
> > >  {
> > > @@ -571,7 +572,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end
> > >
> > >  static const char *bitmap_parse_region(const char *str, struct region *r)
> > >  {
> > > -     str = bitmap_getnum(str, &r->start);
> > > +     str = bitmap_getrnum(str, r, start);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > > @@ -581,7 +582,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_getrnum(str + 1, r, end);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > > @@ -591,14 +592,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_getrnum(str + 1, r, off);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > >       if (*str != '/')
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     return bitmap_getnum(str + 1, &r->group_len);
> > > +     return bitmap_getrnum(str + 1, r, group_len);
> > >
> > >  no_end:
> > >       r->end = r->start;
> > > --
> > > 2.17.1
> > >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

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

[Paul Gortmaker]

[Re: [PATCH v3 0/8] support for bitmap (and hence CPU) list "N" abbreviation] On 26/01/2021 (Tue 14:27) Yury Norov wrote:

> On Tue, Jan 26, 2021 at 9:12 AM Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> >
> > 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.
> 
> I think it's worth moving this sentence to the Documentation. Another

Dynamic nature comment added to Documentation

> caveat with
> N is that users' config may surprisingly become invalid, like if user
> says 32-N, and
> on some machine with a smaller bitmap this config fails to boot.

Updated example to indicate that "16-N" becomes invalid if moved from 32
core system to quad core.  I'm not currently able to think of an example
where boot will fail -- vs. a subsystem getting -EINVAL from bitmap code
and printing a subsystem error instead.

> It doesn't mean of course that I'm against 'N'. I think it's very
> useful especially in
> such common cases like "N", "0-N", "1-N".
> 
> Would it make sense to treat the mask "32-N" when N < 32 as N-N,
> and bark something in dmesg?

I don't think so.  For the same reasons you used to convince me -- that N
should be treated as just another number and not have special rules.

If I boot now, with "important_cpu="32-3" on a quad core then I get what
I get for being stupid.   We don't special case that and subsitute in a
"3-3" (which would then be "3") -- and nor should we!

Sticking to the CPU example, we have no idea what the caller's use case
is -- we don't know if NUMA stuff might be present and whether having
the single CPU #3 in that set is better or worse than EINVAL and no CPUs
in the set.   Expand that to bitmaps in general and we have no idea what
the "right" reaction to garbage input is.

The context of the caller could be simply test_bitmap.c itself -- which
would be expecting the EINVAL, and not some kind of "hot patching" of
the region in order to make it valid.

The only sane option is for the bitmap code to return EINVAL and let the
calling subsystem (with the appropriate context/info) make the decision
as to what to do next.  Which is what the series does now.

Paul.
--

> 
> > 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.]
> >  https://lore.kernel.org/lkml/20210121223355.59780-1-paul.gortmaker@windriver.com/
> >
> > [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. ]
> >
> > 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 more start-end:offset/len tests
> >   lib: bitmap: fold nbits into region struct
> >   lib: bitmap: move ERANGE check from set_region to check_region
> >   lib: bitmap_getnum: separate arg into region and field
> >   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         |  2 +
> >  .../admin-guide/kernel-parameters.txt         |  4 +-
> >  kernel/rcu/tree_plugin.h                      |  6 +--
> >  lib/bitmap.c                                  | 46 ++++++++++--------
> >  lib/test_bitmap.c                             | 48 ++++++++++++++++---
> >  5 files changed, 72 insertions(+), 34 deletions(-)
> >
> > --
> > 2.17.1
> >

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

[Yury Norov]

On Tue, Jan 26, 2021 at 1:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Jan 26, 2021 at 11:37:30PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 26, 2021 at 12:11:39PM -0500, Paul Gortmaker wrote:
>
> ...
>
> > > +   if (str[0] == 'N') {
> > > +           *num = nbits - 1;
> > > +           return str + 1;
> > > +   }
> >
> > But locating it here makes possible to enter a priori invalid input, like N for
> > start of the region.
> >
> > I think this should be separate helper which is called in places where it makes
> > sense.
>
> Okay, N is 31 on 32 core system... It is a bit counter intuitive, because it's
> rather _L_ast than _N_umber of CPUs.
>
> Changing letter?

No objections.

Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct

[Yury Norov]

On Wed, Jan 27, 2021 at 12:02 AM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> [Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct] On 26/01/2021 (Tue 23:16) Andy Shevchenko wrote:
>
> > On Tue, Jan 26, 2021 at 12:11:36PM -0500, Paul Gortmaker wrote:
> > > This will reduce parameter passing and enable using nbits as part
> > > of future dynamic region parameter parsing.
> >
> > One nit below, nevertheless
> > 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>
> > > Suggested-by: Yury Norov <yury.norov@gmail.com>
> > > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > > ---
> > >  lib/bitmap.c | 19 ++++++++++---------
> > >  1 file changed, 10 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > > index 75006c4036e9..162e2850c622 100644
> > > --- a/lib/bitmap.c
> > > +++ b/lib/bitmap.c
> > > @@ -487,24 +487,24 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
> > >
> > >  /*
> > >   * Region 9-38:4/10 describes the following bitmap structure:
> > > - * 0          9  12    18                  38
> > > - * .........****......****......****......
> > > - *     ^  ^     ^                   ^
> > > - *      start  off   group_len            end
> > > + * 0          9  12    18                  38           N
> > > + * .........****......****......****..................
> > > + *     ^  ^     ^                   ^           ^
> > > + *      start  off   group_len            end       nbits
> > >   */
> > >  struct region {
> > >     unsigned int start;
> > >     unsigned int off;
> > >     unsigned int group_len;
> > >     unsigned int end;
> > > +   unsigned int nbits;
> > >  };
> > >
> > > -static int bitmap_set_region(const struct region *r,
> > > -                           unsigned long *bitmap, int nbits)
> > > +static int bitmap_set_region(const struct region *r, unsigned long *bitmap)
> > >  {
> > >     unsigned int start;
> > >
> > > -   if (r->end >= nbits)
> > > +   if (r->end >= r->nbits)
> > >             return -ERANGE;
> > >
> > >     for (start = r->start; start <= r->end; start += r->group_len)
> > > @@ -640,7 +640,8 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> > >     struct region r;
> > >     long ret;
> > >
> > > -   bitmap_zero(maskp, nmaskbits);
> > > +   r.nbits = nmaskbits;
> >
> > > +   bitmap_zero(maskp, r.nbits);
> >
> > This sounds not right from style perspective.
> > You have completely uninitialized r on stack, then you assign only one value
> > for immediate use here and...
>
> So, this change was added because Yury suggested that I "..store
> nmaskbits in the struct region, and avoid passing nmaskbits as a
> parameter."
>
> To which I originally noted "I considered that and went with the param
> so as to not open the door to someone possibly using an uninitialized
> struct value later."

struct region is purely internal structure. It's declared on stack and filled
field-by-field using helpers. 'Someone' misusing the structure doesn't exist
because the structure doesn't exist out of the scope.

> https://lore.kernel.org/lkml/20210122044357.GS16838@windriver.com/
>
> Looking back, I had a similar thought as to yours, it seems...
>
> I am also thinking more and more that nbits doesn't belong in the
> region anyway - yes, a region gets validated against a specific nbits
> eventually, but it doesn't need an nbits field to be a complete
> specification.  The region "0-3" is a complete specification for "the
> 1st four cores" and is as valid on a 4 core machine as it is on a 64 core
> machine -- a validation we do when we deploy the region on that machine.
>
> I will set this change aside and get the nbits value to getnum() another
> way, and leave the region struct as it was -- without a nbits field.
>
> This will also resolve having the macro handling of region that you were
> not really liking.
>
> Paul.

Region is a convenient structure. Adding nbits into it helps to remove
validation
logic from bitmap_set_region(), so it's worth doing this.

Can you please have it unchanged?

Thanks,
Yury

Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 2:52 AM Yury Norov <yury.norov@gmail.com> wrote:
> On Wed, Jan 27, 2021 at 12:02 AM Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:

...

> > So, this change was added because Yury suggested that I "..store
> > nmaskbits in the struct region, and avoid passing nmaskbits as a
> > parameter."
> >
> > To which I originally noted "I considered that and went with the param
> > so as to not open the door to someone possibly using an uninitialized
> > struct value later."
>
> struct region is purely internal structure. It's declared on stack and filled
> field-by-field using helpers. 'Someone' misusing the structure doesn't exist
> because the structure doesn't exist out of the scope.
>
> > https://lore.kernel.org/lkml/20210122044357.GS16838@windriver.com/
> >
> > Looking back, I had a similar thought as to yours, it seems...
> >
> > I am also thinking more and more that nbits doesn't belong in the
> > region anyway - yes, a region gets validated against a specific nbits
> > eventually, but it doesn't need an nbits field to be a complete
> > specification.  The region "0-3" is a complete specification for "the
> > 1st four cores" and is as valid on a 4 core machine as it is on a 64 core
> > machine -- a validation we do when we deploy the region on that machine.
> >
> > I will set this change aside and get the nbits value to getnum() another
> > way, and leave the region struct as it was -- without a nbits field.
> >
> > This will also resolve having the macro handling of region that you were
> > not really liking.

> Region is a convenient structure. Adding nbits into it helps to remove
> validation
> logic from bitmap_set_region(), so it's worth doing this.
>
> Can you please have it unchanged?

I have a compromise proposal here, i.e. why not to create a wrapper
structure like

struct bitmap_region {
  unsigned int nbits;
  struct region r;
};

?

At least it will solve my concern and still be a local structure on
the stack without adding new parameters to called functions.

-- 
With Best Regards,
Andy Shevchenko