Re: [PATCH 5/6] rcu/nocb: Allow empty "rcu_nocbs" kernel parameter

Re: [PATCH 5/6] rcu/nocb: Allow empty "rcu_nocbs" kernel parameter

[Yury Norov]

On Wed, Nov 24, 2021 at 04:47:20PM -0800, Paul E. McKenney wrote:
> On Tue, Nov 23, 2021 at 01:37:07AM +0100, Frederic Weisbecker wrote:
> > If a user wants to boot without any CPU in offloaded mode initially but
> > with the possibility to offload them later using cpusets, provide a way
> > to simply pass an empty "rcu_nocbs" kernel parameter which will enforce
> > the creation of dormant nocb kthreads.
> 
> Huh.  This would have been a use for Yury Norov's "none" bitmask
> specifier.  ;-)
> 
> I pulled this one in with the usual wordsmithing.
> 
> 							Thanx, Paul

I think 'rcu_nocbs=,' should work as 'none'. But I admit that it looks
awkward. The following patch adds clear 'none' semantics to the parser.
If you like it, I think you may drop non-documentation part of this
patch.

From e3a9cfe4830141c88aa5d8a93eae3512b2ae2882 Mon Sep 17 00:00:00 2001
From: Yury Norov <yury.norov@gmail.com>
Date: Wed, 24 Nov 2021 19:34:05 -0800
Subject: [PATCH] lib/bitmap: add 'none' specifier for bitmap_parselist()

Currently bitmap_parselist() has no clear notation to specify empty bitmap.
The format allows ',' or '0:0-N' for doing this, but it looks hacky.

Frederic Weisbecker needs to pass an empty rcu_nocbs to the kernel, and
without such a notation has to hack his own code:

https://lore.kernel.org/rcu/20211125005526.GA490855@lothringen/

This patch adds 'none' to the bitmap_parselist, so that no such hacks would
be needed. 'None' is case-insensitive and doesn't support group semantics
('none:1/2' is meaningless and wouldn't work).

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 Documentation/admin-guide/kernel-parameters.rst |  7 +++++--
 lib/bitmap.c                                    | 12 +++++++++++-
 lib/test_bitmap.c                               | 13 +++++++++++++
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index 01ba293a2d70..af261018bab5 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -76,11 +76,14 @@ 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).
 
+The special case-tolerant group name "none" has a meaning of selecting no CPUs,
+so that "rcu_nocps=none" would allow to disable offloading mode for all CPUs.
+
 The special case-tolerant group name "all" has a meaning of selecting all CPUs,
 so that "nohz_full=all" is the equivalent of "nohz_full=0-N".
 
-The semantics of "N" and "all" is supported on a level of bitmaps and holds for
-all users of bitmap_parse().
+The semantics of "none", "N" and "all" is supported on a level of bitmaps and
+holds for all users of bitmap_parse().
 
 This document may not be entirely up to date and comprehensive. The command
 "modinfo -p ${modulename}" shows a current list of all parameters of a loadable
diff --git a/lib/bitmap.c b/lib/bitmap.c
index d7b80a069819..bcb38d055ec1 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -771,6 +771,16 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
 		goto check_pattern;
 	}
 
+	if (!strncasecmp(str, "none", 4)) {
+		r->start = 0;
+		r->end = 0;
+		r->off = 0;
+		r->group_len = r->nbits;
+		str += 4;
+
+		goto out;
+	}
+
 	str = bitmap_getnum(str, &r->start, lastbit);
 	if (IS_ERR(str))
 		return str;
@@ -806,7 +816,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
 no_pattern:
 	r->off = r->end + 1;
 	r->group_len = r->end + 1;
-
+out:
 	return end_of_str(*str) ? NULL : str;
 }
 
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 0c82f07f74fc..1111d0d0df5f 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -351,18 +351,26 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{0, ",,  ,,  , ,  ,",		&exp1[12 * step], 8, 0},
 	{0, " ,  ,,  , ,   ",		&exp1[12 * step], 8, 0},
 	{0, " ,  ,,  , ,   \n",		&exp1[12 * step], 8, 0},
+	{0, "none",             	&exp1[12 * step], 8, 0},
+	{0, " , NONE ,,  , ,   \n",	&exp1[12 * step], 8, 0},
+	{0, " ,  ,none,  , ,   \n",	&exp1[12 * step], 8, 0},
+	{0, " ,  ,,  , ,none   \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, "31-31,none",		&exp1[14 * step], 32, 0},
 
 	{0, "0-0:0/1",			&exp1[12 * step], 32, 0},
+	{0, "0-0:0/1,none",		&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, "0-15:16/31,none",		&exp1[2 * step], 32, 0},
+	{0, "none,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},
@@ -381,6 +389,7 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{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},
+	{0, "1-10:8/12,none,8-31:24/29,0-31:0/3",	&exp1[9 * step], 32, 0},
 
 	{0,	  "all",		&exp1[8 * step], 32, 0},
 	{0,	  "0, 1, all,  ",	&exp1[8 * step], 32, 0},
@@ -388,6 +397,10 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{0,	  "ALL:1/2",		&exp1[4 * step], 32, 0},
 	{-EINVAL, "al", NULL, 8, 0},
 	{-EINVAL, "alll", NULL, 8, 0},
+	{-EINVAL, "non", NULL, 8, 0},
+	{-EINVAL, "one", NULL, 8, 0},
+	{-EINVAL, "NONEE", NULL, 8, 0},
+	{-EINVAL, "NONE:1/2", NULL, 8, 0},
 
 	{-EINVAL, "-1",	NULL, 8, 0},
 	{-EINVAL, "-0",	NULL, 8, 0},
-- 
2.25.1

Re: [PATCH 5/6] rcu/nocb: Allow empty "rcu_nocbs" kernel parameter

[Andy Shevchenko]

On Wed, Nov 24, 2021 at 08:41:32PM -0800, Yury Norov wrote:
> On Wed, Nov 24, 2021 at 04:47:20PM -0800, Paul E. McKenney wrote:
> > On Tue, Nov 23, 2021 at 01:37:07AM +0100, Frederic Weisbecker wrote:

...

> +	if (!strncasecmp(str, "none", 4)) {
> +		r->start = 0;
> +		r->end = 0;
> +		r->off = 0;
> +		r->group_len = r->nbits;
> +		str += 4;

> +		goto out;

Side remark, I would make the name of the label a bit more verbose on what it's
going to be done there, "out_end_of_region:" or alike.

> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] rcu/nocb: Allow empty "rcu_nocbs" kernel parameter

[Frederic Weisbecker]

On Wed, Nov 24, 2021 at 08:41:32PM -0800, Yury Norov wrote:
> On Wed, Nov 24, 2021 at 04:47:20PM -0800, Paul E. McKenney wrote:
> > On Tue, Nov 23, 2021 at 01:37:07AM +0100, Frederic Weisbecker wrote:
> > > If a user wants to boot without any CPU in offloaded mode initially but
> > > with the possibility to offload them later using cpusets, provide a way
> > > to simply pass an empty "rcu_nocbs" kernel parameter which will enforce
> > > the creation of dormant nocb kthreads.
> > 
> > Huh.  This would have been a use for Yury Norov's "none" bitmask
> > specifier.  ;-)
> > 
> > I pulled this one in with the usual wordsmithing.
> > 
> > 							Thanx, Paul
> 
> I think 'rcu_nocbs=,' should work as 'none'. But I admit that it looks
> awkward. The following patch adds clear 'none' semantics to the parser.
> If you like it, I think you may drop non-documentation part of this
> patch.

I don't have real objection, but I fear that "rcu_nocbs=none" might be
interpretated as rcu_nocbs is entirely deactivated, whereas "rcu_nocbs"
alone makes it clear that we are turning something on.

We can support both though.

Thanks.

Re: [PATCH 5/6] rcu/nocb: Allow empty "rcu_nocbs" kernel parameter

[Paul E. McKenney]

On Thu, Nov 25, 2021 at 02:28:53PM +0100, Frederic Weisbecker wrote:
> On Wed, Nov 24, 2021 at 08:41:32PM -0800, Yury Norov wrote:
> > On Wed, Nov 24, 2021 at 04:47:20PM -0800, Paul E. McKenney wrote:
> > > On Tue, Nov 23, 2021 at 01:37:07AM +0100, Frederic Weisbecker wrote:
> > > > If a user wants to boot without any CPU in offloaded mode initially but
> > > > with the possibility to offload them later using cpusets, provide a way
> > > > to simply pass an empty "rcu_nocbs" kernel parameter which will enforce
> > > > the creation of dormant nocb kthreads.
> > > 
> > > Huh.  This would have been a use for Yury Norov's "none" bitmask
> > > specifier.  ;-)
> > > 
> > > I pulled this one in with the usual wordsmithing.
> > > 
> > > 							Thanx, Paul
> > 
> > I think 'rcu_nocbs=,' should work as 'none'. But I admit that it looks
> > awkward. The following patch adds clear 'none' semantics to the parser.
> > If you like it, I think you may drop non-documentation part of this
> > patch.
> 
> I don't have real objection, but I fear that "rcu_nocbs=none" might be
> interpretated as rcu_nocbs is entirely deactivated, whereas "rcu_nocbs"
> alone makes it clear that we are turning something on.

How about "rcu_nocbs=,"?  ;-)

							Thanx, Paul

> We can support both though.
> 
> Thanks.