[PATCH 1/2] rcu: Assign higher priority to RCU threads if its rcutorture

[PATCH 1/2] rcu: Assign higher priority to RCU threads if its rcutorture

[Joel Fernandes]

From: "Joel Fernandes (Google)" <joel@joelfernandes.org>

rcutorture boost tests fail even with CONFIG_RCU_BOOST set because
rcutorture's threads are equal priority to the default RCU kthreads (RT
class with priority of 1).

This patch checks if RCU torture is built into the kernel and if so,
assigns a higher priority to the RCU threads. With this the rcutorture
boost tests pass.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index deb2508be923..a141d6314622 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -171,7 +171,7 @@ static void rcu_report_exp_rdp(struct rcu_state *rsp,
 static void sync_sched_exp_online_cleanup(int cpu);
 
 /* rcuc/rcub kthread realtime priority */
-static int kthread_prio = IS_ENABLED(CONFIG_RCU_BOOST) ? 1 : 0;
+static int kthread_prio;
 module_param(kthread_prio, int, 0644);
 
 /* Delay in jiffies for grace-period initialization delays, debug only. */
@@ -3884,12 +3884,16 @@ static int __init rcu_spawn_gp_kthread(void)
 	struct task_struct *t;
 
 	/* Force priority into range. */
-	if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 1)
+	if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 2
+	    && IS_BUILTIN(CONFIG_RCU_TORTURE_TEST))
+		kthread_prio = 2;
+	else if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 1)
 		kthread_prio = 1;
 	else if (kthread_prio < 0)
 		kthread_prio = 0;
 	else if (kthread_prio > 99)
 		kthread_prio = 99;
+
 	if (kthread_prio != kthread_prio_in)
 		pr_alert("rcu_spawn_gp_kthread(): Limited prio to %d from %d\n",
 			 kthread_prio, kthread_prio_in);
-- 
2.18.0.rc1.244.gcf134e6275-goog

[PATCH 2/2] rcutorture: Fix rcu_barrier successes counter

[Joel Fernandes]

From: "Joel Fernandes (Google)" <joel@joelfernandes.org>

rcutorture currently increments both successes and error counters for
the rcu_barrier test incase of errors. It should only increment the
error counter incase of errors so make it do so.

Test: Introduced rcu_barrier errors by returning from the barrier
callback without incrementing the callback counter.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/rcutorture.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index f2cde4dd432d..3faaabc99b60 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -1722,8 +1722,9 @@ static int rcu_torture_barrier(void *arg)
 			       atomic_read(&barrier_cbs_invoked),
 			       n_barrier_cbs);
 			WARN_ON_ONCE(1);
+		} else {
+			n_barrier_successes++;
 		}
-		n_barrier_successes++;
 		schedule_timeout_interruptible(HZ / 10);
 	} while (!torture_must_stop());
 	torture_kthread_stopping("rcu_torture_barrier");
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH 1/2] rcu: Assign higher priority to RCU threads if its rcutorture

[Joel Fernandes]

On Mon, Jun 18, 2018 at 11:22:14PM -0700, Joel Fernandes wrote:
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> 
> rcutorture boost tests fail even with CONFIG_RCU_BOOST set because
> rcutorture's threads are equal priority to the default RCU kthreads (RT
> class with priority of 1).

Sorry for the weird subject line, I meant "rcu: Assign higher prio if
rcutorture is built into kernel". I have included the patch with the subject
line fixed up below (if you prefer to take that instead).

Also one question, incase rcutorture is a module, we can't raise the priority
of the kthreads because it would be too late to do at module load time. In
this case, do you have any ideas on what we can do? I was thinking we can
access the kernel command line from within rcutorture module and check if
'rcutree.kthread_prio' was passed. And if it is and isn't sufficiently high,
then we avoid testing boost feature at all (and print a nice message telling
the user about the issue).

OTOH, we can just let rcutorture module loaders fail the test if you feel
very few automation tests do the module loading way of rcutorture and so a
boost test failure there is tolerable. For me, I will likely be running
rcutorture only as a built-in so I am Ok with not special casing it within
rcutorture.

thanks!

 - Joel

-----8<---------

From 8cb7c2ac98e510abac35fdf2419a3212a587095a Mon Sep 17 00:00:00 2001
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Date: Mon, 18 Jun 2018 15:13:10 -0700
Subject: [PATCH 1/2] rcu: Assign higher prio if rcutorture is built into kernel

rcutorture boost tests fail even with CONFIG_RCU_BOOST set because
rcutorture's threads are equal priority to the default RCU kthreads (RT
class with priority of 1).

This patch checks if RCU torture is built into the kernel and if so,
assigns a higher priority to the RCU threads. With this the rcutorture
boost tests pass.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index deb2508be923..a141d6314622 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -171,7 +171,7 @@ static void rcu_report_exp_rdp(struct rcu_state *rsp,
 static void sync_sched_exp_online_cleanup(int cpu);
 
 /* rcuc/rcub kthread realtime priority */
-static int kthread_prio = IS_ENABLED(CONFIG_RCU_BOOST) ? 1 : 0;
+static int kthread_prio;
 module_param(kthread_prio, int, 0644);
 
 /* Delay in jiffies for grace-period initialization delays, debug only. */
@@ -3884,12 +3884,16 @@ static int __init rcu_spawn_gp_kthread(void)
 	struct task_struct *t;
 
 	/* Force priority into range. */
-	if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 1)
+	if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 2
+	    && IS_BUILTIN(CONFIG_RCU_TORTURE_TEST))
+		kthread_prio = 2;
+	else if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 1)
 		kthread_prio = 1;
 	else if (kthread_prio < 0)
 		kthread_prio = 0;
 	else if (kthread_prio > 99)
 		kthread_prio = 99;
+
 	if (kthread_prio != kthread_prio_in)
 		pr_alert("rcu_spawn_gp_kthread(): Limited prio to %d from %d\n",
 			 kthread_prio, kthread_prio_in);
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH 2/2] rcutorture: Fix rcu_barrier successes counter

[Joel Fernandes]

On Mon, Jun 18, 2018 at 11:22:15PM -0700, Joel Fernandes wrote:
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> 
> rcutorture currently increments both successes and error counters for
> the rcu_barrier test incase of errors. It should only increment the
> error counter incase of errors so make it do so.
> 
> Test: Introduced rcu_barrier errors by returning from the barrier
> callback without incrementing the callback counter.

Hi Paul,
Think some more about this counter, I think you mean 'successes' as in
'successful attempts' than 'successful test' ? If so, then perhaps you can
drop this patch. It wasn't clear to me what the 'successes' meant so I may
have been a bit misled into changing its meaning. If on the other hand, it
means 'successful test', then yes this patch would be Ok then. thanks! -Joel
 

Re: [PATCH 2/2] rcutorture: Fix rcu_barrier successes counter

[Steven Rostedt]

On Tue, 19 Jun 2018 00:31:15 -0700
Joel Fernandes <joelaf@google.com> wrote:

> Hi Paul,
> Think some more about this counter, I think you mean 'successes' as in
> 'successful attempts' than 'successful test' ? If so, then perhaps you can
> drop this patch. It wasn't clear to me what the 'successes' meant so I may
> have been a bit misled into changing its meaning. If on the other hand, it
> means 'successful test', then yes this patch would be Ok then. thanks! -Joel
>  

In either case, it sounds like a comment should be added to clarify
what n_barrier_successes actually means ;-)

-- Steve

Re: [PATCH 2/2] rcutorture: Fix rcu_barrier successes counter

[Paul E. McKenney]

On Tue, Jun 19, 2018 at 09:12:23AM -0400, Steven Rostedt wrote:
> On Tue, 19 Jun 2018 00:31:15 -0700
> Joel Fernandes <joelaf@google.com> wrote:
> 
> > Hi Paul,
> > Think some more about this counter, I think you mean 'successes' as in
> > 'successful attempts' than 'successful test' ? If so, then perhaps you can
> > drop this patch. It wasn't clear to me what the 'successes' meant so I may
> > have been a bit misled into changing its meaning. If on the other hand, it
> > means 'successful test', then yes this patch would be Ok then. thanks! -Joel
> 
> In either case, it sounds like a comment should be added to clarify
> what n_barrier_successes actually means ;-)

Or change the name to n_barrier_attempts.  Except that there already
is an n_barrier_attempts, and it is incremented on each attempt.

So perhaps the original patch is on-point.  ;-)

							Thanx, Paul

Re: [PATCH 1/2] rcu: Assign higher priority to RCU threads if its rcutorture

[Paul E. McKenney]

On Mon, Jun 18, 2018 at 11:34:21PM -0700, Joel Fernandes wrote:
> On Mon, Jun 18, 2018 at 11:22:14PM -0700, Joel Fernandes wrote:
> > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > 
> > rcutorture boost tests fail even with CONFIG_RCU_BOOST set because
> > rcutorture's threads are equal priority to the default RCU kthreads (RT
> > class with priority of 1).
> 
> Sorry for the weird subject line, I meant "rcu: Assign higher prio if
> rcutorture is built into kernel". I have included the patch with the subject
> line fixed up below (if you prefer to take that instead).
> 
> Also one question, incase rcutorture is a module, we can't raise the priority
> of the kthreads because it would be too late to do at module load time. In
> this case, do you have any ideas on what we can do? I was thinking we can
> access the kernel command line from within rcutorture module and check if
> 'rcutree.kthread_prio' was passed. And if it is and isn't sufficiently high,
> then we avoid testing boost feature at all (and print a nice message telling
> the user about the issue).

I do like the idea of checking and printing the message in this case.

Another alternative would be to allow rcutree.kthread_prio to be changed
at runtime.  But one caution: rcutree.kthread_prio applies to a number
of kthreads, not just the boost kthreads, so this approach might have
some unwelcome side-effects.

> OTOH, we can just let rcutorture module loaders fail the test if you feel
> very few automation tests do the module loading way of rcutorture and so a
> boost test failure there is tolerable. For me, I will likely be running
> rcutorture only as a built-in so I am Ok with not special casing it within
> rcutorture.

I don't know of anyone using the module-loading approach.  Probably
someone somewhere does it from time to time, though.

							Thanx, Paul

> thanks!
> 
>  - Joel
> 
> -----8<---------
> 
> >From 8cb7c2ac98e510abac35fdf2419a3212a587095a Mon Sep 17 00:00:00 2001
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Date: Mon, 18 Jun 2018 15:13:10 -0700
> Subject: [PATCH 1/2] rcu: Assign higher prio if rcutorture is built into kernel
> 
> rcutorture boost tests fail even with CONFIG_RCU_BOOST set because
> rcutorture's threads are equal priority to the default RCU kthreads (RT
> class with priority of 1).
> 
> This patch checks if RCU torture is built into the kernel and if so,
> assigns a higher priority to the RCU threads. With this the rcutorture
> boost tests pass.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index deb2508be923..a141d6314622 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -171,7 +171,7 @@ static void rcu_report_exp_rdp(struct rcu_state *rsp,
>  static void sync_sched_exp_online_cleanup(int cpu);
> 
>  /* rcuc/rcub kthread realtime priority */
> -static int kthread_prio = IS_ENABLED(CONFIG_RCU_BOOST) ? 1 : 0;
> +static int kthread_prio;
>  module_param(kthread_prio, int, 0644);
> 
>  /* Delay in jiffies for grace-period initialization delays, debug only. */
> @@ -3884,12 +3884,16 @@ static int __init rcu_spawn_gp_kthread(void)
>  	struct task_struct *t;
> 
>  	/* Force priority into range. */
> -	if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 1)
> +	if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 2
> +	    && IS_BUILTIN(CONFIG_RCU_TORTURE_TEST))
> +		kthread_prio = 2;
> +	else if (IS_ENABLED(CONFIG_RCU_BOOST) && kthread_prio < 1)
>  		kthread_prio = 1;
>  	else if (kthread_prio < 0)
>  		kthread_prio = 0;
>  	else if (kthread_prio > 99)
>  		kthread_prio = 99;
> +
>  	if (kthread_prio != kthread_prio_in)
>  		pr_alert("rcu_spawn_gp_kthread(): Limited prio to %d from %d\n",
>  			 kthread_prio, kthread_prio_in);
> -- 
> 2.18.0.rc1.244.gcf134e6275-goog
> 

Re: [PATCH 2/2] rcutorture: Fix rcu_barrier successes counter

[Joel Fernandes]

On Tue, Jun 19, 2018 at 08:41:39AM -0700, Paul E. McKenney wrote:
> On Tue, Jun 19, 2018 at 09:12:23AM -0400, Steven Rostedt wrote:
> > On Tue, 19 Jun 2018 00:31:15 -0700
> > Joel Fernandes <joelaf@google.com> wrote:
> > 
> > > Hi Paul,
> > > Think some more about this counter, I think you mean 'successes' as in
> > > 'successful attempts' than 'successful test' ? If so, then perhaps you can
> > > drop this patch. It wasn't clear to me what the 'successes' meant so I may
> > > have been a bit misled into changing its meaning. If on the other hand, it
> > > means 'successful test', then yes this patch would be Ok then. thanks! -Joel
> > 
> > In either case, it sounds like a comment should be added to clarify
> > what n_barrier_successes actually means ;-)
> 
> Or change the name to n_barrier_attempts.  Except that there already
> is an n_barrier_attempts, and it is incremented on each attempt.
> 
> So perhaps the original patch is on-point.  ;-)

Cool, added a comment just to clarify it and resent the updated patch.

thanks!

 - Joel

Re: [PATCH 1/2] rcu: Assign higher priority to RCU threads if its rcutorture

[Joel Fernandes]

On Tue, Jun 19, 2018 at 09:00:24AM -0700, Paul E. McKenney wrote:
> On Mon, Jun 18, 2018 at 11:34:21PM -0700, Joel Fernandes wrote:
> > On Mon, Jun 18, 2018 at 11:22:14PM -0700, Joel Fernandes wrote:
> > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > 
> > > rcutorture boost tests fail even with CONFIG_RCU_BOOST set because
> > > rcutorture's threads are equal priority to the default RCU kthreads (RT
> > > class with priority of 1).
> > 
> > Sorry for the weird subject line, I meant "rcu: Assign higher prio if
> > rcutorture is built into kernel". I have included the patch with the subject
> > line fixed up below (if you prefer to take that instead).
> > 
> > Also one question, incase rcutorture is a module, we can't raise the priority
> > of the kthreads because it would be too late to do at module load time. In
> > this case, do you have any ideas on what we can do? I was thinking we can
> > access the kernel command line from within rcutorture module and check if
> > 'rcutree.kthread_prio' was passed. And if it is and isn't sufficiently high,
> > then we avoid testing boost feature at all (and print a nice message telling
> > the user about the issue).
> 
> I do like the idea of checking and printing the message in this case.

Cool, I used this approach in the series I just sent. I agree its good to
report it.

> Another alternative would be to allow rcutree.kthread_prio to be changed
> at runtime.  But one caution: rcutree.kthread_prio applies to a number
> of kthreads, not just the boost kthreads, so this approach might have
> some unwelcome side-effects.

Yes, I was trying to avoid those complexities just because of rcutorture.

> > OTOH, we can just let rcutorture module loaders fail the test if you feel
> > very few automation tests do the module loading way of rcutorture and so a
> > boost test failure there is tolerable. For me, I will likely be running
> > rcutorture only as a built-in so I am Ok with not special casing it within
> > rcutorture.
> 
> I don't know of anyone using the module-loading approach.  Probably
> someone somewhere does it from time to time, though.

Oh Ok :). Then I guess the warning print from rcutorture is sufficient as I did.

thanks!

 - Joel