[PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Uladzislau Rezki (Sony)]

Currently "nfakewriters" parameter can be set to any value but
there is no possibility to adjust it automatically based on how
many CPUs a system has where a test is run on.

To address this, if the "nfakewriters" is set to negative it will
be adjusted to num_online_cpus() during torture initialization.

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index d98b3bd6d91f..f376262532ce 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");
 
 static int nrealnocbers;
 static int nrealreaders;
+static int nrealfakewriters;
 static struct task_struct *writer_task;
 static struct task_struct **fakewriter_tasks;
 static struct task_struct **reader_tasks;
@@ -1763,7 +1764,7 @@ rcu_torture_fakewriter(void *arg)
 	do {
 		torture_hrtimeout_jiffies(torture_random(&rand) % 10, &rand);
 		if (cur_ops->cb_barrier != NULL &&
-		    torture_random(&rand) % (nfakewriters * 8) == 0) {
+		    torture_random(&rand) % (nrealfakewriters * 8) == 0) {
 			cur_ops->cb_barrier();
 		} else {
 			switch (synctype[torture_random(&rand) % nsynctypes]) {
@@ -2568,7 +2569,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
 		 "nocbs_nthreads=%d nocbs_toggle=%d "
 		 "test_nmis=%d "
 		 "preempt_duration=%d preempt_interval=%d\n",
-		 torture_type, tag, nrealreaders, nfakewriters,
+		 torture_type, tag, nrealreaders, nrealfakewriters,
 		 stat_interval, verbose, test_no_idle_hz, shuffle_interval,
 		 stutter, irqreader, fqs_duration, fqs_holdoff, fqs_stutter,
 		 test_boost, cur_ops->can_boost,
@@ -3644,7 +3645,7 @@ rcu_torture_cleanup(void)
 	rcu_torture_reader_mbchk = NULL;
 
 	if (fakewriter_tasks) {
-		for (i = 0; i < nfakewriters; i++)
+		for (i = 0; i < nrealfakewriters; i++)
 			torture_stop_kthread(rcu_torture_fakewriter,
 					     fakewriter_tasks[i]);
 		kfree(fakewriter_tasks);
@@ -4066,6 +4067,14 @@ rcu_torture_init(void)
 
 	rcu_torture_init_srcu_lockdep();
 
+	if (nfakewriters >= 0) {
+		nrealfakewriters = nfakewriters;
+	} else {
+		nrealfakewriters = num_online_cpus() - 2 - nfakewriters;
+		if (nrealfakewriters <= 0)
+			nrealfakewriters = 1;
+	}
+
 	if (nreaders >= 0) {
 		nrealreaders = nreaders;
 	} else {
@@ -4122,8 +4131,9 @@ rcu_torture_init(void)
 					  writer_task);
 	if (torture_init_error(firsterr))
 		goto unwind;
-	if (nfakewriters > 0) {
-		fakewriter_tasks = kcalloc(nfakewriters,
+
+	if (nrealfakewriters > 0) {
+		fakewriter_tasks = kcalloc(nrealfakewriters,
 					   sizeof(fakewriter_tasks[0]),
 					   GFP_KERNEL);
 		if (fakewriter_tasks == NULL) {
@@ -4132,7 +4142,7 @@ rcu_torture_init(void)
 			goto unwind;
 		}
 	}
-	for (i = 0; i < nfakewriters; i++) {
+	for (i = 0; i < nrealfakewriters; i++) {
 		firsterr = torture_create_kthread(rcu_torture_fakewriter,
 						  NULL, fakewriter_tasks[i]);
 		if (torture_init_error(firsterr))
-- 
2.39.5

[PATCH v3 2/3] rcu: Update TREE05.boot to test normal synchronize_rcu()

[Uladzislau Rezki (Sony)]

Add extra parameters for rcutorture module. One is the "nfakewriters"
which is set -1. There will be created number of test-kthreads which
correspond to number of CPUs in a test system. Those threads randomly
invoke synchronize_rcu() call.

Apart of that "rcu_normal" is set to 1, because it is specifically for
a normal synchronize_rcu() testing, also a newly added parameter which
is "rcu_normal_wake_from_gp" is set to 1 also. That prevents interaction
with other callbacks in a system.

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 tools/testing/selftests/rcutorture/configs/rcu/TREE05.boot | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TREE05.boot b/tools/testing/selftests/rcutorture/configs/rcu/TREE05.boot
index c419cac233ee..54f5c9053474 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TREE05.boot
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TREE05.boot
@@ -2,3 +2,9 @@ rcutree.gp_preinit_delay=3
 rcutree.gp_init_delay=3
 rcutree.gp_cleanup_delay=3
 rcupdate.rcu_self_test=1
+
+# This part is for synchronize_rcu() testing
+rcutorture.nfakewriters=-1
+rcutorture.gp_sync=1
+rcupdate.rcu_normal=1
+rcutree.rcu_normal_wake_from_gp=1
-- 
2.39.5

[PATCH v3 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Uladzislau Rezki (Sony)]

Switch for using of get_state_synchronize_rcu_full() and
poll_state_synchronize_rcu_full() pair for debug a normal
synchronize_rcu() call.

Just using "not" full APIs to identify if a grace period
is passed or not might lead to a false kernel splat.

Link: https://lore.kernel.org/lkml/Z5ikQeVmVdsWQrdD@pc636/T/
Fixes: 988f569ae041 ("rcu: Reduce synchronize_rcu() latency")
Reported-by: cheung wall <zzqq0103.hey@gmail.com>
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 include/linux/rcupdate_wait.h | 3 +++
 kernel/rcu/tree.c             | 8 +++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
index f9bed3d3f78d..4c92d4291cce 100644
--- a/include/linux/rcupdate_wait.h
+++ b/include/linux/rcupdate_wait.h
@@ -16,6 +16,9 @@
 struct rcu_synchronize {
 	struct rcu_head head;
 	struct completion completion;
+
+	/* This is for debugging. */
+	struct rcu_gp_oldstate oldstate;
 };
 void wakeme_after_rcu(struct rcu_head *head);
 
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8625f616c65a..48384fa2eaeb 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1632,12 +1632,10 @@ static void rcu_sr_normal_complete(struct llist_node *node)
 {
 	struct rcu_synchronize *rs = container_of(
 		(struct rcu_head *) node, struct rcu_synchronize, head);
-	unsigned long oldstate = (unsigned long) rs->head.func;
 
 	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) &&
-		!poll_state_synchronize_rcu(oldstate),
-		"A full grace period is not passed yet: %lu",
-		rcu_seq_diff(get_state_synchronize_rcu(), oldstate));
+		!poll_state_synchronize_rcu_full(&rs->oldstate),
+		"A full grace period is not passed yet!\n");
 
 	/* Finally. */
 	complete(&rs->completion);
@@ -3247,7 +3245,7 @@ static void synchronize_rcu_normal(void)
 	 * snapshot before adding a request.
 	 */
 	if (IS_ENABLED(CONFIG_PROVE_RCU))
-		rs.head.func = (void *) get_state_synchronize_rcu();
+		get_state_synchronize_rcu_full(&rs.oldstate);
 
 	rcu_sr_normal_add_req(&rs);
 
-- 
2.39.5

Re: [PATCH v3 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Paul E. McKenney]

On Tue, Feb 25, 2025 at 12:00:20PM +0100, Uladzislau Rezki (Sony) wrote:
> Switch for using of get_state_synchronize_rcu_full() and
> poll_state_synchronize_rcu_full() pair for debug a normal
> synchronize_rcu() call.
> 
> Just using "not" full APIs to identify if a grace period
> is passed or not might lead to a false kernel splat.
> 
> Link: https://lore.kernel.org/lkml/Z5ikQeVmVdsWQrdD@pc636/T/
> Fixes: 988f569ae041 ("rcu: Reduce synchronize_rcu() latency")
> Reported-by: cheung wall <zzqq0103.hey@gmail.com>
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

This one passes light testing, so:

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  include/linux/rcupdate_wait.h | 3 +++
>  kernel/rcu/tree.c             | 8 +++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
> index f9bed3d3f78d..4c92d4291cce 100644
> --- a/include/linux/rcupdate_wait.h
> +++ b/include/linux/rcupdate_wait.h
> @@ -16,6 +16,9 @@
>  struct rcu_synchronize {
>  	struct rcu_head head;
>  	struct completion completion;
> +
> +	/* This is for debugging. */
> +	struct rcu_gp_oldstate oldstate;
>  };
>  void wakeme_after_rcu(struct rcu_head *head);
>  
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8625f616c65a..48384fa2eaeb 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1632,12 +1632,10 @@ static void rcu_sr_normal_complete(struct llist_node *node)
>  {
>  	struct rcu_synchronize *rs = container_of(
>  		(struct rcu_head *) node, struct rcu_synchronize, head);
> -	unsigned long oldstate = (unsigned long) rs->head.func;
>  
>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) &&
> -		!poll_state_synchronize_rcu(oldstate),
> -		"A full grace period is not passed yet: %lu",
> -		rcu_seq_diff(get_state_synchronize_rcu(), oldstate));
> +		!poll_state_synchronize_rcu_full(&rs->oldstate),
> +		"A full grace period is not passed yet!\n");
>  
>  	/* Finally. */
>  	complete(&rs->completion);
> @@ -3247,7 +3245,7 @@ static void synchronize_rcu_normal(void)
>  	 * snapshot before adding a request.
>  	 */
>  	if (IS_ENABLED(CONFIG_PROVE_RCU))
> -		rs.head.func = (void *) get_state_synchronize_rcu();
> +		get_state_synchronize_rcu_full(&rs.oldstate);
>  
>  	rcu_sr_normal_add_req(&rs);
>  
> -- 
> 2.39.5
> 

Re: [PATCH v3 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Uladzislau Rezki]

On Tue, Feb 25, 2025 at 07:48:55AM -0800, Paul E. McKenney wrote:
> On Tue, Feb 25, 2025 at 12:00:20PM +0100, Uladzislau Rezki (Sony) wrote:
> > Switch for using of get_state_synchronize_rcu_full() and
> > poll_state_synchronize_rcu_full() pair for debug a normal
> > synchronize_rcu() call.
> > 
> > Just using "not" full APIs to identify if a grace period
> > is passed or not might lead to a false kernel splat.
> > 
> > Link: https://lore.kernel.org/lkml/Z5ikQeVmVdsWQrdD@pc636/T/
> > Fixes: 988f569ae041 ("rcu: Reduce synchronize_rcu() latency")
> > Reported-by: cheung wall <zzqq0103.hey@gmail.com>
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> This one passes light testing, so:
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> 
Thank you!

--
Uladzislau Rezki

Re: [PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Joel Fernandes]

On Tue, Feb 25, 2025 at 12:00:18PM +0100, Uladzislau Rezki (Sony) wrote:
> Currently "nfakewriters" parameter can be set to any value but
> there is no possibility to adjust it automatically based on how
> many CPUs a system has where a test is run on.
> 
> To address this, if the "nfakewriters" is set to negative it will
> be adjusted to num_online_cpus() during torture initialization.
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index d98b3bd6d91f..f376262532ce 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");

IMO, this should also be updated to reflect the possibily to set it negative
and hence to number CPUs:

torture_param(int, nfakewriters, 4, "Number of RCU fake writer threads");

thanks,

 - Joel


>  
>  static int nrealnocbers;
>  static int nrealreaders;
> +static int nrealfakewriters;
>  static struct task_struct *writer_task;
>  static struct task_struct **fakewriter_tasks;
>  static struct task_struct **reader_tasks;
> @@ -1763,7 +1764,7 @@ rcu_torture_fakewriter(void *arg)
>  	do {
>  		torture_hrtimeout_jiffies(torture_random(&rand) % 10, &rand);
>  		if (cur_ops->cb_barrier != NULL &&
> -		    torture_random(&rand) % (nfakewriters * 8) == 0) {
> +		    torture_random(&rand) % (nrealfakewriters * 8) == 0) {
>  			cur_ops->cb_barrier();
>  		} else {
>  			switch (synctype[torture_random(&rand) % nsynctypes]) {
> @@ -2568,7 +2569,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
>  		 "nocbs_nthreads=%d nocbs_toggle=%d "
>  		 "test_nmis=%d "
>  		 "preempt_duration=%d preempt_interval=%d\n",
> -		 torture_type, tag, nrealreaders, nfakewriters,
> +		 torture_type, tag, nrealreaders, nrealfakewriters,
>  		 stat_interval, verbose, test_no_idle_hz, shuffle_interval,
>  		 stutter, irqreader, fqs_duration, fqs_holdoff, fqs_stutter,
>  		 test_boost, cur_ops->can_boost,
> @@ -3644,7 +3645,7 @@ rcu_torture_cleanup(void)
>  	rcu_torture_reader_mbchk = NULL;
>  
>  	if (fakewriter_tasks) {
> -		for (i = 0; i < nfakewriters; i++)
> +		for (i = 0; i < nrealfakewriters; i++)
>  			torture_stop_kthread(rcu_torture_fakewriter,
>  					     fakewriter_tasks[i]);
>  		kfree(fakewriter_tasks);
> @@ -4066,6 +4067,14 @@ rcu_torture_init(void)
>  
>  	rcu_torture_init_srcu_lockdep();
>  
> +	if (nfakewriters >= 0) {
> +		nrealfakewriters = nfakewriters;
> +	} else {
> +		nrealfakewriters = num_online_cpus() - 2 - nfakewriters;
> +		if (nrealfakewriters <= 0)
> +			nrealfakewriters = 1;
> +	}
> +
>  	if (nreaders >= 0) {
>  		nrealreaders = nreaders;
>  	} else {
> @@ -4122,8 +4131,9 @@ rcu_torture_init(void)
>  					  writer_task);
>  	if (torture_init_error(firsterr))
>  		goto unwind;
> -	if (nfakewriters > 0) {
> -		fakewriter_tasks = kcalloc(nfakewriters,
> +
> +	if (nrealfakewriters > 0) {
> +		fakewriter_tasks = kcalloc(nrealfakewriters,
>  					   sizeof(fakewriter_tasks[0]),
>  					   GFP_KERNEL);
>  		if (fakewriter_tasks == NULL) {
> @@ -4132,7 +4142,7 @@ rcu_torture_init(void)
>  			goto unwind;
>  		}
>  	}
> -	for (i = 0; i < nfakewriters; i++) {
> +	for (i = 0; i < nrealfakewriters; i++) {
>  		firsterr = torture_create_kthread(rcu_torture_fakewriter,
>  						  NULL, fakewriter_tasks[i]);
>  		if (torture_init_error(firsterr))
> -- 
> 2.39.5
> 

Re: [PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Uladzislau Rezki]

On Tue, Feb 25, 2025 at 04:24:09PM -0500, Joel Fernandes wrote:
> On Tue, Feb 25, 2025 at 12:00:18PM +0100, Uladzislau Rezki (Sony) wrote:
> > Currently "nfakewriters" parameter can be set to any value but
> > there is no possibility to adjust it automatically based on how
> > many CPUs a system has where a test is run on.
> > 
> > To address this, if the "nfakewriters" is set to negative it will
> > be adjusted to num_online_cpus() during torture initialization.
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> >  kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > index d98b3bd6d91f..f376262532ce 100644
> > --- a/kernel/rcu/rcutorture.c
> > +++ b/kernel/rcu/rcutorture.c
> > @@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");
> 
> IMO, this should also be updated to reflect the possibily to set it negative
> and hence to number CPUs:
> 
> torture_param(int, nfakewriters, 4, "Number of RCU fake writer threads");
> 
You can set it to a negative as well as to number of CPUs or any other
number.

--
Uladzislau Rezki

Re: [PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Paul E. McKenney]

On Wed, Feb 26, 2025 at 03:29:09PM +0100, Uladzislau Rezki wrote:
> On Tue, Feb 25, 2025 at 04:24:09PM -0500, Joel Fernandes wrote:
> > On Tue, Feb 25, 2025 at 12:00:18PM +0100, Uladzislau Rezki (Sony) wrote:
> > > Currently "nfakewriters" parameter can be set to any value but
> > > there is no possibility to adjust it automatically based on how
> > > many CPUs a system has where a test is run on.
> > > 
> > > To address this, if the "nfakewriters" is set to negative it will
> > > be adjusted to num_online_cpus() during torture initialization.
> > > 
> > > Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > ---
> > >  kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
> > >  1 file changed, 16 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > > index d98b3bd6d91f..f376262532ce 100644
> > > --- a/kernel/rcu/rcutorture.c
> > > +++ b/kernel/rcu/rcutorture.c
> > > @@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");
> > 
> > IMO, this should also be updated to reflect the possibily to set it negative
> > and hence to number CPUs:
> > 
> > torture_param(int, nfakewriters, 4, "Number of RCU fake writer threads");
> > 
> You can set it to a negative as well as to number of CPUs or any other
> number.

Joel does have a good point, though.  Could I interest one of you in
updating the Documentation/admin-guide/kernel-parameters.txt entry for
rcutorture.nfakewriters?  The rcutorture.nreaders entry is a decent guide.

							Thanx, Paul

Re: [PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Uladzislau Rezki]

On Wed, Feb 26, 2025 at 06:50:02AM -0800, Paul E. McKenney wrote:
> On Wed, Feb 26, 2025 at 03:29:09PM +0100, Uladzislau Rezki wrote:
> > On Tue, Feb 25, 2025 at 04:24:09PM -0500, Joel Fernandes wrote:
> > > On Tue, Feb 25, 2025 at 12:00:18PM +0100, Uladzislau Rezki (Sony) wrote:
> > > > Currently "nfakewriters" parameter can be set to any value but
> > > > there is no possibility to adjust it automatically based on how
> > > > many CPUs a system has where a test is run on.
> > > > 
> > > > To address this, if the "nfakewriters" is set to negative it will
> > > > be adjusted to num_online_cpus() during torture initialization.
> > > > 
> > > > Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > > ---
> > > >  kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
> > > >  1 file changed, 16 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > > > index d98b3bd6d91f..f376262532ce 100644
> > > > --- a/kernel/rcu/rcutorture.c
> > > > +++ b/kernel/rcu/rcutorture.c
> > > > @@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");
> > > 
> > > IMO, this should also be updated to reflect the possibily to set it negative
> > > and hence to number CPUs:
> > > 
> > > torture_param(int, nfakewriters, 4, "Number of RCU fake writer threads");
> > > 
> > You can set it to a negative as well as to number of CPUs or any other
> > number.
> 
> Joel does have a good point, though.  Could I interest one of you in
> updating the Documentation/admin-guide/kernel-parameters.txt entry for
> rcutorture.nfakewriters?  The rcutorture.nreaders entry is a decent guide.
> 
OK. Then i misunderstood Joel. It was about updating a documentation
about this!

Good point :)

--
Uladzislau Rezki

Re: [PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Joel Fernandes]

On 2/26/2025 9:29 AM, Uladzislau Rezki wrote:
> On Tue, Feb 25, 2025 at 04:24:09PM -0500, Joel Fernandes wrote:
>> On Tue, Feb 25, 2025 at 12:00:18PM +0100, Uladzislau Rezki (Sony) wrote:
>>> Currently "nfakewriters" parameter can be set to any value but
>>> there is no possibility to adjust it automatically based on how
>>> many CPUs a system has where a test is run on.
>>>
>>> To address this, if the "nfakewriters" is set to negative it will
>>> be adjusted to num_online_cpus() during torture initialization.
>>>
>>> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
>>> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
>>> ---
>>>  kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
>>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
>>> index d98b3bd6d91f..f376262532ce 100644
>>> --- a/kernel/rcu/rcutorture.c
>>> +++ b/kernel/rcu/rcutorture.c
>>> @@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");
>>
>> IMO, this should also be updated to reflect the possibily to set it negative
>> and hence to number CPUs:
>>
>> torture_param(int, nfakewriters, 4, "Number of RCU fake writer threads");
>>
> You can set it to a negative as well as to number of CPUs or any other
> number.
Sorry I just meant amend the description to something like "Number of RCU fake
writer threads (or set to -1 for NR_CPUs)", so user does not have to read code
to know that (and update the kernel cmdline params document as well).

thanks,

 - Joel

Re: [PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Paul E. McKenney]

On Wed, Feb 26, 2025 at 12:49:41PM -0500, Joel Fernandes wrote:
> 
> 
> On 2/26/2025 9:29 AM, Uladzislau Rezki wrote:
> > On Tue, Feb 25, 2025 at 04:24:09PM -0500, Joel Fernandes wrote:
> >> On Tue, Feb 25, 2025 at 12:00:18PM +0100, Uladzislau Rezki (Sony) wrote:
> >>> Currently "nfakewriters" parameter can be set to any value but
> >>> there is no possibility to adjust it automatically based on how
> >>> many CPUs a system has where a test is run on.
> >>>
> >>> To address this, if the "nfakewriters" is set to negative it will
> >>> be adjusted to num_online_cpus() during torture initialization.
> >>>
> >>> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> >>> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> >>> ---
> >>>  kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
> >>>  1 file changed, 16 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> >>> index d98b3bd6d91f..f376262532ce 100644
> >>> --- a/kernel/rcu/rcutorture.c
> >>> +++ b/kernel/rcu/rcutorture.c
> >>> @@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");
> >>
> >> IMO, this should also be updated to reflect the possibily to set it negative
> >> and hence to number CPUs:
> >>
> >> torture_param(int, nfakewriters, 4, "Number of RCU fake writer threads");
> >>
> > You can set it to a negative as well as to number of CPUs or any other
> > number.
> Sorry I just meant amend the description to something like "Number of RCU fake
> writer threads (or set to -1 for NR_CPUs)", so user does not have to read code
> to know that (and update the kernel cmdline params document as well).

Should we also adjust the string for nreaders?

							Thanx, Paul

Re: [PATCH v3 1/3] rcutorture: Allow a negative value for nfakewriters

[Uladzislau Rezki]

On Wed, Feb 26, 2025 at 10:04:35AM -0800, Paul E. McKenney wrote:
> On Wed, Feb 26, 2025 at 12:49:41PM -0500, Joel Fernandes wrote:
> > 
> > 
> > On 2/26/2025 9:29 AM, Uladzislau Rezki wrote:
> > > On Tue, Feb 25, 2025 at 04:24:09PM -0500, Joel Fernandes wrote:
> > >> On Tue, Feb 25, 2025 at 12:00:18PM +0100, Uladzislau Rezki (Sony) wrote:
> > >>> Currently "nfakewriters" parameter can be set to any value but
> > >>> there is no possibility to adjust it automatically based on how
> > >>> many CPUs a system has where a test is run on.
> > >>>
> > >>> To address this, if the "nfakewriters" is set to negative it will
> > >>> be adjusted to num_online_cpus() during torture initialization.
> > >>>
> > >>> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> > >>> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > >>> ---
> > >>>  kernel/rcu/rcutorture.c | 22 ++++++++++++++++------
> > >>>  1 file changed, 16 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > >>> index d98b3bd6d91f..f376262532ce 100644
> > >>> --- a/kernel/rcu/rcutorture.c
> > >>> +++ b/kernel/rcu/rcutorture.c
> > >>> @@ -148,6 +148,7 @@ MODULE_PARM_DESC(torture_type, "Type of RCU to torture (rcu, srcu, ...)");
> > >>
> > >> IMO, this should also be updated to reflect the possibily to set it negative
> > >> and hence to number CPUs:
> > >>
> > >> torture_param(int, nfakewriters, 4, "Number of RCU fake writer threads");
> > >>
> > > You can set it to a negative as well as to number of CPUs or any other
> > > number.
> > Sorry I just meant amend the description to something like "Number of RCU fake
> > writer threads (or set to -1 for NR_CPUs)", so user does not have to read code
> > to know that (and update the kernel cmdline params document as well).
> 
> Should we also adjust the string for nreaders?
> 
Makes sense. Both options are adjusted in same way.

--
Uladzislau Rezki

Re: [PATCH v3 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Boqun Feng]

Hi Ulad,

On Tue, Feb 25, 2025 at 12:00:20PM +0100, Uladzislau Rezki (Sony) wrote:
> Switch for using of get_state_synchronize_rcu_full() and
> poll_state_synchronize_rcu_full() pair for debug a normal
> synchronize_rcu() call.
> 
> Just using "not" full APIs to identify if a grace period
> is passed or not might lead to a false kernel splat.
> 

Could you provide detailed explanation on this? I.e. why is _full() is
needed? I find the current commit message is a bit vague.

Regards,
Boqun

> Link: https://lore.kernel.org/lkml/Z5ikQeVmVdsWQrdD@pc636/T/
> Fixes: 988f569ae041 ("rcu: Reduce synchronize_rcu() latency")
> Reported-by: cheung wall <zzqq0103.hey@gmail.com>
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  include/linux/rcupdate_wait.h | 3 +++
>  kernel/rcu/tree.c             | 8 +++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
> index f9bed3d3f78d..4c92d4291cce 100644
> --- a/include/linux/rcupdate_wait.h
> +++ b/include/linux/rcupdate_wait.h
> @@ -16,6 +16,9 @@
>  struct rcu_synchronize {
>  	struct rcu_head head;
>  	struct completion completion;
> +
> +	/* This is for debugging. */
> +	struct rcu_gp_oldstate oldstate;
>  };
>  void wakeme_after_rcu(struct rcu_head *head);
>  
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8625f616c65a..48384fa2eaeb 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1632,12 +1632,10 @@ static void rcu_sr_normal_complete(struct llist_node *node)
>  {
>  	struct rcu_synchronize *rs = container_of(
>  		(struct rcu_head *) node, struct rcu_synchronize, head);
> -	unsigned long oldstate = (unsigned long) rs->head.func;
>  
>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) &&
> -		!poll_state_synchronize_rcu(oldstate),
> -		"A full grace period is not passed yet: %lu",
> -		rcu_seq_diff(get_state_synchronize_rcu(), oldstate));
> +		!poll_state_synchronize_rcu_full(&rs->oldstate),
> +		"A full grace period is not passed yet!\n");
>  
>  	/* Finally. */
>  	complete(&rs->completion);
> @@ -3247,7 +3245,7 @@ static void synchronize_rcu_normal(void)
>  	 * snapshot before adding a request.
>  	 */
>  	if (IS_ENABLED(CONFIG_PROVE_RCU))
> -		rs.head.func = (void *) get_state_synchronize_rcu();
> +		get_state_synchronize_rcu_full(&rs.oldstate);
>  
>  	rcu_sr_normal_add_req(&rs);
>  
> -- 
> 2.39.5
> 

Re: [PATCH v3 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Uladzislau Rezki]

Hello, Boqun.

> Hi Ulad,
> 
> On Tue, Feb 25, 2025 at 12:00:20PM +0100, Uladzislau Rezki (Sony) wrote:
> > Switch for using of get_state_synchronize_rcu_full() and
> > poll_state_synchronize_rcu_full() pair for debug a normal
> > synchronize_rcu() call.
> > 
> > Just using "not" full APIs to identify if a grace period
> > is passed or not might lead to a false kernel splat.
> > 
> 
> Could you provide detailed explanation on this? I.e. why is _full() is
> needed? I find the current commit message is a bit vague.
>
<snip>
rcu: Use _full() API to debug synchronize_rcu()

Switch for using of get_state_synchronize_rcu_full() and
poll_state_synchronize_rcu_full() pair to debug a normal
synchronize_rcu() call.

Just using "not" full APIs to identify if a grace period is
passed or not might lead to a false-positive kernel splat.

It can happen, because get_state_synchronize_rcu() compresses
both normal and expedited states into one single unsigned long
value, so a poll_state_synchronize_rcu() can miss GP-completion
when synchronize_rcu()/synchronize_rcu_expedited() concurrently
run.

To address this, switch to poll_state_synchronize_rcu_full() and
get_state_synchronize_rcu_full() APIs, which use separate variables
for expedited and normal states.
<snip>

Does it look better?

--
Uladzislau Rezki

Re: [PATCH v3 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Boqun Feng]

On Wed, Feb 26, 2025 at 09:48:52PM +0100, Uladzislau Rezki wrote:
> Hello, Boqun.
> 
> > Hi Ulad,
> > 
> > On Tue, Feb 25, 2025 at 12:00:20PM +0100, Uladzislau Rezki (Sony) wrote:
> > > Switch for using of get_state_synchronize_rcu_full() and
> > > poll_state_synchronize_rcu_full() pair for debug a normal
> > > synchronize_rcu() call.
> > > 
> > > Just using "not" full APIs to identify if a grace period
> > > is passed or not might lead to a false kernel splat.
> > > 
> > 
> > Could you provide detailed explanation on this? I.e. why is _full() is
> > needed? I find the current commit message is a bit vague.
> >
> <snip>
> rcu: Use _full() API to debug synchronize_rcu()
> 
> Switch for using of get_state_synchronize_rcu_full() and
> poll_state_synchronize_rcu_full() pair to debug a normal
> synchronize_rcu() call.
> 
> Just using "not" full APIs to identify if a grace period is
> passed or not might lead to a false-positive kernel splat.
> 
> It can happen, because get_state_synchronize_rcu() compresses
> both normal and expedited states into one single unsigned long
> value, so a poll_state_synchronize_rcu() can miss GP-completion
> when synchronize_rcu()/synchronize_rcu_expedited() concurrently
> run.
> 
> To address this, switch to poll_state_synchronize_rcu_full() and
> get_state_synchronize_rcu_full() APIs, which use separate variables
> for expedited and normal states.
> <snip>
> 
> Does it look better?
> 

Yes, that looks good to me. Thanks!

Regards,
Boqun

> --
> Uladzislau Rezki
>