[PATCH 0/2] rcutorture: Some PREEMPT_RT fixlets

[PATCH 0/2] rcutorture: Some PREEMPT_RT fixlets

[Valentin Schneider]

Hi folks,

I've been meaning to run RCU torture under v5.13-rt1 to validate my hacking
around the RCU offloaded state [1], but have hit some warnings.


The second patch clearly isn't a thing of beauty, but FWIW it lets me run RCU
torture tests without any extra steps.

As mentioned over IRC, this started as a .setup() callback for the kosftirqd
threads gated behind CONFIG_RCU_BOOST && CONFIG_RCU_TORTURE_TEST, but I figured
keeping RCU torture specific stuff within rcutorture.c would be a tad smarter.
I hate either version, but here it is regardless.

[1]: https://lore.kernel.org/lkml/20210728220137.GD293265@lothringen/

Cheers,
Valentin

Valentin Schneider (2):
  rcutorture: Don't disable softirqs with preemption disabled when
    PREEMPT_RT
  rcutorture: Nudge ksoftirqd priority for RCU boost testing

 kernel/rcu/rcutorture.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--
2.25.1

[PATCH 1/2] rcutorture: Don't disable softirqs with preemption disabled when PREEMPT_RT

[Valentin Schneider]

Running RCU torture with CONFIG_PREEMPT_RT under v5.13-rt1 triggers:

[   10.821700] DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt))
[   10.821716] WARNING: CPU: 5 PID: 137 at kernel/softirq.c:173 __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
[   10.821739] Modules linked in:
[   10.821749] CPU: 5 PID: 137 Comm: rcu_torture_rea Not tainted 5.13.0-rt1-00005-g08bbda29766a #129
[   10.821759] Hardware name: ARM Juno development board (r0) (DT)
[   10.821765] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[   10.821938] Call trace:
[   10.821941] __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
[   10.821950] rcutorture_one_extend (./include/linux/rcupdate.h:274 ./include/linux/rcupdate.h:737 kernel/rcu/rcutorture.c:1443)
[   10.821960] rcu_torture_one_read (kernel/rcu/rcutorture.c:1590 kernel/rcu/rcutorture.c:1638)
[   10.821968] rcu_torture_reader (kernel/rcu/rcutorture.c:1730)
[   10.821976] kthread (kernel/kthread.c:321)
[   10.821986] ret_from_fork (arch/arm64/kernel/entry.S:1005)
[   10.821997] irq event stamp: 478635
[   10.822001] hardirqs last enabled at (478635): _raw_spin_unlock_irq (./arch/arm64/include/asm/irqflags.h:35 ./include/linux/spinlock_api_smp.h:168 kernel/locking/spinlock.c:202)
[   10.822016] hardirqs last disabled at (478634): __schedule (kernel/sched/core.c:5154 (discriminator 1))
[   10.822029] softirqs last enabled at (478626): __local_bh_enable_ip (./arch/arm64/include/asm/irqflags.h:85 kernel/softirq.c:262)
[   10.822040] softirqs last disabled at (478622): rcutorture_one_extend (./include/linux/bottom_half.h:19 kernel/rcu/rcutorture.c:1441)

Per this warning, softirqs cannot be disabled in a non-preemptible region
under CONFIG_PREEMPT_RT. Adjust RCU torture accordingly.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 kernel/rcu/rcutorture.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 6096a7d14342..680f66b65f14 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -1537,6 +1537,8 @@ rcutorture_extend_mask(int oldmask, struct torture_random_state *trsp)
 	 * them on non-RT.
 	 */
 	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
+		/* Can't disable bh in atomic context under PREEMPT_RT */
+		mask &= ~(RCUTORTURE_RDR_ATOM_BH | RCUTORTURE_RDR_ATOM_RBH);
 		/*
 		 * Can't release the outermost rcu lock in an irq disabled
 		 * section without preemption also being disabled, if irqs
-- 
2.25.1

Re: [PATCH 1/2] rcutorture: Don't disable softirqs with preemption disabled when PREEMPT_RT

[Paul E. McKenney]

On Tue, Aug 03, 2021 at 11:54:36PM +0100, Valentin Schneider wrote:
> Running RCU torture with CONFIG_PREEMPT_RT under v5.13-rt1 triggers:
> 
> [   10.821700] DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt))
> [   10.821716] WARNING: CPU: 5 PID: 137 at kernel/softirq.c:173 __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
> [   10.821739] Modules linked in:
> [   10.821749] CPU: 5 PID: 137 Comm: rcu_torture_rea Not tainted 5.13.0-rt1-00005-g08bbda29766a #129
> [   10.821759] Hardware name: ARM Juno development board (r0) (DT)
> [   10.821765] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
> [   10.821938] Call trace:
> [   10.821941] __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
> [   10.821950] rcutorture_one_extend (./include/linux/rcupdate.h:274 ./include/linux/rcupdate.h:737 kernel/rcu/rcutorture.c:1443)
> [   10.821960] rcu_torture_one_read (kernel/rcu/rcutorture.c:1590 kernel/rcu/rcutorture.c:1638)
> [   10.821968] rcu_torture_reader (kernel/rcu/rcutorture.c:1730)
> [   10.821976] kthread (kernel/kthread.c:321)
> [   10.821986] ret_from_fork (arch/arm64/kernel/entry.S:1005)
> [   10.821997] irq event stamp: 478635
> [   10.822001] hardirqs last enabled at (478635): _raw_spin_unlock_irq (./arch/arm64/include/asm/irqflags.h:35 ./include/linux/spinlock_api_smp.h:168 kernel/locking/spinlock.c:202)
> [   10.822016] hardirqs last disabled at (478634): __schedule (kernel/sched/core.c:5154 (discriminator 1))
> [   10.822029] softirqs last enabled at (478626): __local_bh_enable_ip (./arch/arm64/include/asm/irqflags.h:85 kernel/softirq.c:262)
> [   10.822040] softirqs last disabled at (478622): rcutorture_one_extend (./include/linux/bottom_half.h:19 kernel/rcu/rcutorture.c:1441)
> 
> Per this warning, softirqs cannot be disabled in a non-preemptible region
> under CONFIG_PREEMPT_RT. Adjust RCU torture accordingly.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  kernel/rcu/rcutorture.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index 6096a7d14342..680f66b65f14 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -1537,6 +1537,8 @@ rcutorture_extend_mask(int oldmask, struct torture_random_state *trsp)
>  	 * them on non-RT.
>  	 */
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {

This depends on some rcutorture patches in -rt that are not yet in
-rcu.  Would -rt be a good place for this one, or are those patches
now ready for -rcu?

							Thanx, Paul

> +		/* Can't disable bh in atomic context under PREEMPT_RT */
> +		mask &= ~(RCUTORTURE_RDR_ATOM_BH | RCUTORTURE_RDR_ATOM_RBH);
>  		/*
>  		 * Can't release the outermost rcu lock in an irq disabled
>  		 * section without preemption also being disabled, if irqs
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] rcutorture: Don't disable softirqs with preemption disabled when PREEMPT_RT

[Valentin Schneider]

+Cc Scott, Sebastian

On 03/08/21 16:43, Paul E. McKenney wrote:
> On Tue, Aug 03, 2021 at 11:54:36PM +0100, Valentin Schneider wrote:
>> Running RCU torture with CONFIG_PREEMPT_RT under v5.13-rt1 triggers:
>>
>> [   10.821700] DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt))
>> [   10.821716] WARNING: CPU: 5 PID: 137 at kernel/softirq.c:173 __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
>> [   10.821739] Modules linked in:
>> [   10.821749] CPU: 5 PID: 137 Comm: rcu_torture_rea Not tainted 5.13.0-rt1-00005-g08bbda29766a #129
>> [   10.821759] Hardware name: ARM Juno development board (r0) (DT)
>> [   10.821765] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
>> [   10.821938] Call trace:
>> [   10.821941] __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
>> [   10.821950] rcutorture_one_extend (./include/linux/rcupdate.h:274 ./include/linux/rcupdate.h:737 kernel/rcu/rcutorture.c:1443)
>> [   10.821960] rcu_torture_one_read (kernel/rcu/rcutorture.c:1590 kernel/rcu/rcutorture.c:1638)
>> [   10.821968] rcu_torture_reader (kernel/rcu/rcutorture.c:1730)
>> [   10.821976] kthread (kernel/kthread.c:321)
>> [   10.821986] ret_from_fork (arch/arm64/kernel/entry.S:1005)
>> [   10.821997] irq event stamp: 478635
>> [   10.822001] hardirqs last enabled at (478635): _raw_spin_unlock_irq (./arch/arm64/include/asm/irqflags.h:35 ./include/linux/spinlock_api_smp.h:168 kernel/locking/spinlock.c:202)
>> [   10.822016] hardirqs last disabled at (478634): __schedule (kernel/sched/core.c:5154 (discriminator 1))
>> [   10.822029] softirqs last enabled at (478626): __local_bh_enable_ip (./arch/arm64/include/asm/irqflags.h:85 kernel/softirq.c:262)
>> [   10.822040] softirqs last disabled at (478622): rcutorture_one_extend (./include/linux/bottom_half.h:19 kernel/rcu/rcutorture.c:1441)
>>
>> Per this warning, softirqs cannot be disabled in a non-preemptible region
>> under CONFIG_PREEMPT_RT. Adjust RCU torture accordingly.
>>
>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>> ---
>>  kernel/rcu/rcutorture.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
>> index 6096a7d14342..680f66b65f14 100644
>> --- a/kernel/rcu/rcutorture.c
>> +++ b/kernel/rcu/rcutorture.c
>> @@ -1537,6 +1537,8 @@ rcutorture_extend_mask(int oldmask, struct torture_random_state *trsp)
>>       * them on non-RT.
>>       */
>>      if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
>
> This depends on some rcutorture patches in -rt that are not yet in
> -rcu.  Would -rt be a good place for this one, or are those patches
> now ready for -rcu?
>

Right, this goes along with

  72d6f4f680bf ("rcutorture: Avoid problematic critical section nesting on RT")

(from v5.13-rt1), which seems to apply cleanly on top of the current
mainline. So if we want this to go straight into -rcu, the above needs to
go along with it.

Thomas et al, any objections?

>                                                       Thanx, Paul
>
>> +		/* Can't disable bh in atomic context under PREEMPT_RT */
>> +		mask &= ~(RCUTORTURE_RDR_ATOM_BH | RCUTORTURE_RDR_ATOM_RBH);
>>              /*
>>               * Can't release the outermost rcu lock in an irq disabled
>>               * section without preemption also being disabled, if irqs
>> --
>> 2.25.1
>>

Re: [PATCH 1/2] rcutorture: Don't disable softirqs with preemption disabled when PREEMPT_RT

[Paul E. McKenney]

On Wed, Aug 04, 2021 at 11:17:33AM +0100, Valentin Schneider wrote:
> 
> +Cc Scott, Sebastian
> 
> On 03/08/21 16:43, Paul E. McKenney wrote:
> > On Tue, Aug 03, 2021 at 11:54:36PM +0100, Valentin Schneider wrote:
> >> Running RCU torture with CONFIG_PREEMPT_RT under v5.13-rt1 triggers:
> >>
> >> [   10.821700] DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt))
> >> [   10.821716] WARNING: CPU: 5 PID: 137 at kernel/softirq.c:173 __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
> >> [   10.821739] Modules linked in:
> >> [   10.821749] CPU: 5 PID: 137 Comm: rcu_torture_rea Not tainted 5.13.0-rt1-00005-g08bbda29766a #129
> >> [   10.821759] Hardware name: ARM Juno development board (r0) (DT)
> >> [   10.821765] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
> >> [   10.821938] Call trace:
> >> [   10.821941] __local_bh_disable_ip (kernel/softirq.c:173 (discriminator 31))
> >> [   10.821950] rcutorture_one_extend (./include/linux/rcupdate.h:274 ./include/linux/rcupdate.h:737 kernel/rcu/rcutorture.c:1443)
> >> [   10.821960] rcu_torture_one_read (kernel/rcu/rcutorture.c:1590 kernel/rcu/rcutorture.c:1638)
> >> [   10.821968] rcu_torture_reader (kernel/rcu/rcutorture.c:1730)
> >> [   10.821976] kthread (kernel/kthread.c:321)
> >> [   10.821986] ret_from_fork (arch/arm64/kernel/entry.S:1005)
> >> [   10.821997] irq event stamp: 478635
> >> [   10.822001] hardirqs last enabled at (478635): _raw_spin_unlock_irq (./arch/arm64/include/asm/irqflags.h:35 ./include/linux/spinlock_api_smp.h:168 kernel/locking/spinlock.c:202)
> >> [   10.822016] hardirqs last disabled at (478634): __schedule (kernel/sched/core.c:5154 (discriminator 1))
> >> [   10.822029] softirqs last enabled at (478626): __local_bh_enable_ip (./arch/arm64/include/asm/irqflags.h:85 kernel/softirq.c:262)
> >> [   10.822040] softirqs last disabled at (478622): rcutorture_one_extend (./include/linux/bottom_half.h:19 kernel/rcu/rcutorture.c:1441)
> >>
> >> Per this warning, softirqs cannot be disabled in a non-preemptible region
> >> under CONFIG_PREEMPT_RT. Adjust RCU torture accordingly.
> >>
> >> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> >> ---
> >>  kernel/rcu/rcutorture.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> >> index 6096a7d14342..680f66b65f14 100644
> >> --- a/kernel/rcu/rcutorture.c
> >> +++ b/kernel/rcu/rcutorture.c
> >> @@ -1537,6 +1537,8 @@ rcutorture_extend_mask(int oldmask, struct torture_random_state *trsp)
> >>       * them on non-RT.
> >>       */
> >>      if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> >
> > This depends on some rcutorture patches in -rt that are not yet in
> > -rcu.  Would -rt be a good place for this one, or are those patches
> > now ready for -rcu?
> >
> 
> Right, this goes along with
> 
>   72d6f4f680bf ("rcutorture: Avoid problematic critical section nesting on RT")
> 
> (from v5.13-rt1), which seems to apply cleanly on top of the current
> mainline. So if we want this to go straight into -rcu, the above needs to
> go along with it.

Ah, this one needs to have its changes conditioned on PREEMPT_RT.
The situations that this patch excludes really can happen in mainline,
so they still need to be tested in mainline.

It all comes back now.  ;-)

							Thanx, Paul

> Thomas et al, any objections?
> 
> >                                                       Thanx, Paul
> >
> >> +		/* Can't disable bh in atomic context under PREEMPT_RT */
> >> +		mask &= ~(RCUTORTURE_RDR_ATOM_BH | RCUTORTURE_RDR_ATOM_RBH);
> >>              /*
> >>               * Can't release the outermost rcu lock in an irq disabled
> >>               * section without preemption also being disabled, if irqs
> >> --
> >> 2.25.1
> >>

[PATCH 2/2] rcutorture: Nudge ksoftirqd priority for RCU boost testing

[Valentin Schneider]

As pointed out by commit 5e59fba573e6 ("rcutorture: Fix testing of RCU
priority boosting"), timer expiry needs to run at a priority higher than
that of the rcu_torture_boost threads (FIFO1) for RCU boost testing to
function. If that's not the case, the rcu_torture_boost threads will
prevent the wakeup of the RCU grace-period kthread, which means no boosting
will be initiated.

Instead of setting this up manually, check the priority of ksoftirqd before
starting the RCU boost test and nudge if required.

Note that this does not attempt to save and restore the scheduler
parameters of ksoftirqd.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 kernel/rcu/rcutorture.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 680f66b65f14..3dd5fa75f469 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -948,12 +948,26 @@ static int rcu_torture_boost(void *arg)
 	unsigned long endtime;
 	unsigned long oldstarttime;
 	struct rcu_boost_inflight rbi = { .inflight = 0 };
+	struct task_struct *ksoftirqd = this_cpu_ksoftirqd();
 
 	VERBOSE_TOROUT_STRING("rcu_torture_boost started");
 
 	/* Set real-time priority. */
 	sched_set_fifo_low(current);
 
+	/*
+	 * Boost testing requires TIMER_SOFTIRQ to run at a higher priority
+	 * than the CPU-hogging torture kthreads, otherwise said threads
+	 * will never let timer expiry for the RCU GP kthread happen, which will
+	 * prevent any boosting.
+	 */
+	if (current->normal_prio < ksoftirqd->normal_prio) {
+		struct sched_param sp = { .sched_priority = 2 };
+
+		pr_alert("%s(): Adjusting %s priority\n", __func__, ksoftirqd->comm);
+		sched_setscheduler_nocheck(ksoftirqd, SCHED_FIFO, &sp);
+	}
+
 	init_rcu_head_on_stack(&rbi.rcu);
 	/* Each pass through the following loop does one boost-test cycle. */
 	do {
-- 
2.25.1

Re: [PATCH 2/2] rcutorture: Nudge ksoftirqd priority for RCU boost testing

[Paul E. McKenney]

On Tue, Aug 03, 2021 at 11:54:37PM +0100, Valentin Schneider wrote:
> As pointed out by commit 5e59fba573e6 ("rcutorture: Fix testing of RCU
> priority boosting"), timer expiry needs to run at a priority higher than
> that of the rcu_torture_boost threads (FIFO1) for RCU boost testing to
> function. If that's not the case, the rcu_torture_boost threads will
> prevent the wakeup of the RCU grace-period kthread, which means no boosting
> will be initiated.
> 
> Instead of setting this up manually, check the priority of ksoftirqd before
> starting the RCU boost test and nudge if required.
> 
> Note that this does not attempt to save and restore the scheduler
> parameters of ksoftirqd.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  kernel/rcu/rcutorture.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index 680f66b65f14..3dd5fa75f469 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -948,12 +948,26 @@ static int rcu_torture_boost(void *arg)
>  	unsigned long endtime;
>  	unsigned long oldstarttime;
>  	struct rcu_boost_inflight rbi = { .inflight = 0 };
> +	struct task_struct *ksoftirqd = this_cpu_ksoftirqd();
>  
>  	VERBOSE_TOROUT_STRING("rcu_torture_boost started");
>  
>  	/* Set real-time priority. */
>  	sched_set_fifo_low(current);
>  
> +	/*
> +	 * Boost testing requires TIMER_SOFTIRQ to run at a higher priority
> +	 * than the CPU-hogging torture kthreads, otherwise said threads
> +	 * will never let timer expiry for the RCU GP kthread happen, which will
> +	 * prevent any boosting.
> +	 */
> +	if (current->normal_prio < ksoftirqd->normal_prio) {

Would it make sense to add IS_ENABLED(CONFIG_PREEMPT_RT) to the above
condition?

							Thanx, Paul

> +		struct sched_param sp = { .sched_priority = 2 };
> +
> +		pr_alert("%s(): Adjusting %s priority\n", __func__, ksoftirqd->comm);
> +		sched_setscheduler_nocheck(ksoftirqd, SCHED_FIFO, &sp);
> +	}
> +
>  	init_rcu_head_on_stack(&rbi.rcu);
>  	/* Each pass through the following loop does one boost-test cycle. */
>  	do {
> -- 
> 2.25.1
> 

Re: [PATCH 2/2] rcutorture: Nudge ksoftirqd priority for RCU boost testing

[Valentin Schneider]

On 03/08/21 16:42, Paul E. McKenney wrote:
> On Tue, Aug 03, 2021 at 11:54:37PM +0100, Valentin Schneider wrote:
>> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
>> index 680f66b65f14..3dd5fa75f469 100644
>> --- a/kernel/rcu/rcutorture.c
>> +++ b/kernel/rcu/rcutorture.c
>> @@ -948,12 +948,26 @@ static int rcu_torture_boost(void *arg)
>>      unsigned long endtime;
>>      unsigned long oldstarttime;
>>      struct rcu_boost_inflight rbi = { .inflight = 0 };
>> +	struct task_struct *ksoftirqd = this_cpu_ksoftirqd();
>>
>>      VERBOSE_TOROUT_STRING("rcu_torture_boost started");
>>
>>      /* Set real-time priority. */
>>      sched_set_fifo_low(current);
>>
>> +	/*
>> +	 * Boost testing requires TIMER_SOFTIRQ to run at a higher priority
>> +	 * than the CPU-hogging torture kthreads, otherwise said threads
>> +	 * will never let timer expiry for the RCU GP kthread happen, which will
>> +	 * prevent any boosting.
>> +	 */
>> +	if (current->normal_prio < ksoftirqd->normal_prio) {
>
> Would it make sense to add IS_ENABLED(CONFIG_PREEMPT_RT) to the above
> condition?
>

Hm so v5.13-rt1 has this commit:

  5e59fba573e6 ("rcutorture: Fix testing of RCU priority boosting")

which gates RCU boost torture testing under CONFIG_PREEMPT_RT. Now, AFAICT
the TIMER_SOFTIRQ priority problem is there regardless of
CONFIG_PREEMPT_RT, so this patch would (should?) make sense even on
!CONFIG_PREEMPT_RT.

>                                                       Thanx, Paul
>
>> +		struct sched_param sp = { .sched_priority = 2 };
>> +
>> +		pr_alert("%s(): Adjusting %s priority\n", __func__, ksoftirqd->comm);
>> +		sched_setscheduler_nocheck(ksoftirqd, SCHED_FIFO, &sp);
>> +	}
>> +
>>      init_rcu_head_on_stack(&rbi.rcu);
>>      /* Each pass through the following loop does one boost-test cycle. */
>>      do {
>> --
>> 2.25.1
>>

Re: [PATCH 2/2] rcutorture: Nudge ksoftirqd priority for RCU boost testing

[Paul E. McKenney]

On Wed, Aug 04, 2021 at 11:18:11AM +0100, Valentin Schneider wrote:
> On 03/08/21 16:42, Paul E. McKenney wrote:
> > On Tue, Aug 03, 2021 at 11:54:37PM +0100, Valentin Schneider wrote:
> >> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> >> index 680f66b65f14..3dd5fa75f469 100644
> >> --- a/kernel/rcu/rcutorture.c
> >> +++ b/kernel/rcu/rcutorture.c
> >> @@ -948,12 +948,26 @@ static int rcu_torture_boost(void *arg)
> >>      unsigned long endtime;
> >>      unsigned long oldstarttime;
> >>      struct rcu_boost_inflight rbi = { .inflight = 0 };
> >> +	struct task_struct *ksoftirqd = this_cpu_ksoftirqd();
> >>
> >>      VERBOSE_TOROUT_STRING("rcu_torture_boost started");
> >>
> >>      /* Set real-time priority. */
> >>      sched_set_fifo_low(current);
> >>
> >> +	/*
> >> +	 * Boost testing requires TIMER_SOFTIRQ to run at a higher priority
> >> +	 * than the CPU-hogging torture kthreads, otherwise said threads
> >> +	 * will never let timer expiry for the RCU GP kthread happen, which will
> >> +	 * prevent any boosting.
> >> +	 */
> >> +	if (current->normal_prio < ksoftirqd->normal_prio) {
> >
> > Would it make sense to add IS_ENABLED(CONFIG_PREEMPT_RT) to the above
> > condition?
> >
> 
> Hm so v5.13-rt1 has this commit:
> 
>   5e59fba573e6 ("rcutorture: Fix testing of RCU priority boosting")
> 
> which gates RCU boost torture testing under CONFIG_PREEMPT_RT. Now, AFAICT
> the TIMER_SOFTIRQ priority problem is there regardless of
> CONFIG_PREEMPT_RT, so this patch would (should?) make sense even on
> !CONFIG_PREEMPT_RT.

What rcutorture scenario TREE03 does is to boot with tree.use_softirq=0
and threadirqs.  I see your point about timers and softirq, but this
does run reliably for me.

Ah, I see why.  Commit ea6d962e80b6 ("rcutorture: Judge RCU priority
boosting on grace periods, not callbacks") includes boosting the priority
of the ksoftirqd kthreads.  But only when running rcutorture builtin,
not as a module.  Here is the code in rcu_torture_init():

		// Testing RCU priority boosting requires rcutorture do
		// some serious abuse.  Counter this by running ksoftirqd
		// at higher priority.
		if (IS_BUILTIN(CONFIG_RCU_TORTURE_TEST)) {
			for_each_online_cpu(cpu) {
				struct sched_param sp;
				struct task_struct *t;

				t = per_cpu(ksoftirqd, cpu);
				WARN_ON_ONCE(!t);
				sp.sched_priority = 2;
				sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
			}
		}

I take it that you were running rcutorture as a module?

This describes how to run it built-in, if that works for you:

https://paulmck.livejournal.com/61432.html

More specifically: https://paulmck.livejournal.com/57769.html

Alternatively, the "IS_BUILTIN(CONFIG_RCU_TORTURE_TEST)" check could be
removed in the above code, and the ksoftirqd kthreads could have their
original priority restored in rcu_torture_cleanup().

Thoughts?

							Thanx, Paul

> >> +		struct sched_param sp = { .sched_priority = 2 };
> >> +
> >> +		pr_alert("%s(): Adjusting %s priority\n", __func__, ksoftirqd->comm);
> >> +		sched_setscheduler_nocheck(ksoftirqd, SCHED_FIFO, &sp);
> >> +	}
> >> +
> >>      init_rcu_head_on_stack(&rbi.rcu);
> >>      /* Each pass through the following loop does one boost-test cycle. */
> >>      do {
> >> --
> >> 2.25.1
> >>

Re: [PATCH 2/2] rcutorture: Nudge ksoftirqd priority for RCU boost testing

[Valentin Schneider]

On 04/08/21 15:53, Paul E. McKenney wrote:
> On Wed, Aug 04, 2021 at 11:18:11AM +0100, Valentin Schneider wrote:
>>
>> Hm so v5.13-rt1 has this commit:
>>
>>   5e59fba573e6 ("rcutorture: Fix testing of RCU priority boosting")
>>
>> which gates RCU boost torture testing under CONFIG_PREEMPT_RT. Now, AFAICT
>> the TIMER_SOFTIRQ priority problem is there regardless of
>> CONFIG_PREEMPT_RT, so this patch would (should?) make sense even on
>> !CONFIG_PREEMPT_RT.
>
> What rcutorture scenario TREE03 does is to boot with tree.use_softirq=0
> and threadirqs.  I see your point about timers and softirq, but this
> does run reliably for me.
>
> Ah, I see why.  Commit ea6d962e80b6 ("rcutorture: Judge RCU priority
> boosting on grace periods, not callbacks") includes boosting the priority
> of the ksoftirqd kthreads.  But only when running rcutorture builtin,
> not as a module.  Here is the code in rcu_torture_init():
>
>               // Testing RCU priority boosting requires rcutorture do
>               // some serious abuse.  Counter this by running ksoftirqd
>               // at higher priority.
>               if (IS_BUILTIN(CONFIG_RCU_TORTURE_TEST)) {
>                       for_each_online_cpu(cpu) {
>                               struct sched_param sp;
>                               struct task_struct *t;
>
>                               t = per_cpu(ksoftirqd, cpu);
>                               WARN_ON_ONCE(!t);
>                               sp.sched_priority = 2;
>                               sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
>                       }
>               }
>
> I take it that you were running rcutorture as a module?
>
> This describes how to run it built-in, if that works for you:
>
> https://paulmck.livejournal.com/61432.html
>
> More specifically: https://paulmck.livejournal.com/57769.html
>
> Alternatively, the "IS_BUILTIN(CONFIG_RCU_TORTURE_TEST)" check could be
> removed in the above code, and the ksoftirqd kthreads could have their
> original priority restored in rcu_torture_cleanup().
>
> Thoughts?
>

I actually run rcutorture as a builtin, but from what I can tell the above
patch came in v5.14-rc1, and ofc I'm running my tests on v5.13-rt1... I
should have paid closer attention to what was in the latest mainline,
apologies for the noise. 

FWIW tweaking ksoftirqd priority only when the torture module is builtin
makes sense to me.

Re: [PATCH 2/2] rcutorture: Nudge ksoftirqd priority for RCU boost testing

[Paul E. McKenney]

On Thu, Aug 05, 2021 at 12:51:48PM +0100, Valentin Schneider wrote:
> On 04/08/21 15:53, Paul E. McKenney wrote:
> > On Wed, Aug 04, 2021 at 11:18:11AM +0100, Valentin Schneider wrote:
> >>
> >> Hm so v5.13-rt1 has this commit:
> >>
> >>   5e59fba573e6 ("rcutorture: Fix testing of RCU priority boosting")
> >>
> >> which gates RCU boost torture testing under CONFIG_PREEMPT_RT. Now, AFAICT
> >> the TIMER_SOFTIRQ priority problem is there regardless of
> >> CONFIG_PREEMPT_RT, so this patch would (should?) make sense even on
> >> !CONFIG_PREEMPT_RT.
> >
> > What rcutorture scenario TREE03 does is to boot with tree.use_softirq=0
> > and threadirqs.  I see your point about timers and softirq, but this
> > does run reliably for me.
> >
> > Ah, I see why.  Commit ea6d962e80b6 ("rcutorture: Judge RCU priority
> > boosting on grace periods, not callbacks") includes boosting the priority
> > of the ksoftirqd kthreads.  But only when running rcutorture builtin,
> > not as a module.  Here is the code in rcu_torture_init():
> >
> >               // Testing RCU priority boosting requires rcutorture do
> >               // some serious abuse.  Counter this by running ksoftirqd
> >               // at higher priority.
> >               if (IS_BUILTIN(CONFIG_RCU_TORTURE_TEST)) {
> >                       for_each_online_cpu(cpu) {
> >                               struct sched_param sp;
> >                               struct task_struct *t;
> >
> >                               t = per_cpu(ksoftirqd, cpu);
> >                               WARN_ON_ONCE(!t);
> >                               sp.sched_priority = 2;
> >                               sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
> >                       }
> >               }
> >
> > I take it that you were running rcutorture as a module?
> >
> > This describes how to run it built-in, if that works for you:
> >
> > https://paulmck.livejournal.com/61432.html
> >
> > More specifically: https://paulmck.livejournal.com/57769.html
> >
> > Alternatively, the "IS_BUILTIN(CONFIG_RCU_TORTURE_TEST)" check could be
> > removed in the above code, and the ksoftirqd kthreads could have their
> > original priority restored in rcu_torture_cleanup().
> >
> > Thoughts?
> 
> I actually run rcutorture as a builtin, but from what I can tell the above
> patch came in v5.14-rc1, and ofc I'm running my tests on v5.13-rt1... I
> should have paid closer attention to what was in the latest mainline,
> apologies for the noise. 

Not a problem, and thank you for giving rcutorture a try!

> FWIW tweaking ksoftirqd priority only when the torture module is builtin
> makes sense to me.

Very good, I will stick with the status quo, then.

							Thanx, Paul