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

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

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 v2 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 v2 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 | 4 ++++
 kernel/rcu/tree.c             | 8 +++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
index f9bed3d3f78d..a16fc2a9a7d7 100644
--- a/include/linux/rcupdate_wait.h
+++ b/include/linux/rcupdate_wait.h
@@ -16,6 +16,10 @@
 struct rcu_synchronize {
 	struct rcu_head head;
 	struct completion completion;
+#ifdef CONFIG_PROVE_RCU
+	/* This is for testing. */
+	struct rcu_gp_oldstate oldstate;
+#endif
 };
 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 v2 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Paul E. McKenney]

On Mon, Feb 24, 2025 at 02:36:59PM +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>
> ---
>  include/linux/rcupdate_wait.h | 4 ++++
>  kernel/rcu/tree.c             | 8 +++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
> index f9bed3d3f78d..a16fc2a9a7d7 100644
> --- a/include/linux/rcupdate_wait.h
> +++ b/include/linux/rcupdate_wait.h
> @@ -16,6 +16,10 @@
>  struct rcu_synchronize {
>  	struct rcu_head head;
>  	struct completion completion;
> +#ifdef CONFIG_PROVE_RCU
> +	/* This is for testing. */
> +	struct rcu_gp_oldstate oldstate;
> +#endif

This causes the build to fail on TREE01.  One way to make the build
succeed is to remove the #ifdefs above.  Another way would be to add
#ifdefs to the WARN_ONCE() below.  I suspect that removing the #ifdefs
is best, at least until such time as people start passing many tens
of SRCU instances to synchronize_rcu_mult() or some such (which seems
quite unlikely).

Thoughts?

							Thanx, Paul

>  };
>  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 v2 3/3] rcu: Use _full() API to debug synchronize_rcu()

[Uladzislau Rezki]

On Mon, Feb 24, 2025 at 11:06:01AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 24, 2025 at 02:36:59PM +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>
> > ---
> >  include/linux/rcupdate_wait.h | 4 ++++
> >  kernel/rcu/tree.c             | 8 +++-----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
> > index f9bed3d3f78d..a16fc2a9a7d7 100644
> > --- a/include/linux/rcupdate_wait.h
> > +++ b/include/linux/rcupdate_wait.h
> > @@ -16,6 +16,10 @@
> >  struct rcu_synchronize {
> >  	struct rcu_head head;
> >  	struct completion completion;
> > +#ifdef CONFIG_PROVE_RCU
> > +	/* This is for testing. */
> > +	struct rcu_gp_oldstate oldstate;
> > +#endif
> 
> This causes the build to fail on TREE01.  One way to make the build
> succeed is to remove the #ifdefs above.  Another way would be to add
> #ifdefs to the WARN_ONCE() below.  I suspect that removing the #ifdefs
> is best, at least until such time as people start passing many tens
> of SRCU instances to synchronize_rcu_mult() or some such (which seems
> quite unlikely).
> 
> Thoughts?
> 
Right, i agree. I will repost this series.

Thank you for checking and testing :)

--
Uladzislau Rezki

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

[Paul E. McKenney]

On Mon, Feb 24, 2025 at 02:36:57PM +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.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

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

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

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

[Uladzislau Rezki]

On Mon, Feb 24, 2025 at 10:41:03AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 24, 2025 at 02:36:57PM +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.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> 
Applied and i will resend a v3 :)

Thank you!

--
Uladzislau Rezki