[PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Joel Fernandes (Google)]

rcu_read_unlock() can incur an infrequent deadlock in
sched_core_balance(). Fix this by using the RCU-sched flavor instead.

This fixes the following spinlock recursion observed when testing the
core scheduling patches on PREEMPT=y kernel on ChromeOS:

[    3.240891] BUG: spinlock recursion on CPU#2, swapper/2/0
[    3.240900]  lock: 0xffff9cd1eeb28e40, .magic: dead4ead, .owner: swapper/2/0, .owner_cpu: 2
[    3.240905] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.4.22htcore #4
[    3.240908] Hardware name: Google Eve/Eve, BIOS Google_Eve.9584.174.0 05/29/2018
[    3.240910] Call Trace:
[    3.240919]  dump_stack+0x97/0xdb
[    3.240924]  ? spin_bug+0xa4/0xb1
[    3.240927]  do_raw_spin_lock+0x79/0x98
[    3.240931]  try_to_wake_up+0x367/0x61b
[    3.240935]  rcu_read_unlock_special+0xde/0x169
[    3.240938]  ? sched_core_balance+0xd9/0x11e
[    3.240941]  __rcu_read_unlock+0x48/0x4a
[    3.240945]  __balance_callback+0x50/0xa1
[    3.240949]  __schedule+0x55a/0x61e
[    3.240952]  schedule_idle+0x21/0x2d
[    3.240956]  do_idle+0x1d5/0x1f8
[    3.240960]  cpu_startup_entry+0x1d/0x1f
[    3.240964]  start_secondary+0x159/0x174
[    3.240967]  secondary_startup_64+0xa4/0xb0
[   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]

Cc: vpillai <vpillai@digitalocean.com>
Cc: Aaron Lu <aaron.lwe@gmail.com>
Cc: Aubrey Li <aubrey.intel@gmail.com>
Cc: peterz@infradead.org
Cc: paulmck@kernel.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/sched/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3045bd50e249..037e8f2e2686 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4735,7 +4735,7 @@ static void sched_core_balance(struct rq *rq)
 	struct sched_domain *sd;
 	int cpu = cpu_of(rq);
 
-	rcu_read_lock();
+	rcu_read_lock_sched();
 	raw_spin_unlock_irq(rq_lockp(rq));
 	for_each_domain(cpu, sd) {
 		if (!(sd->flags & SD_LOAD_BALANCE))
@@ -4748,7 +4748,7 @@ static void sched_core_balance(struct rq *rq)
 			break;
 	}
 	raw_spin_lock_irq(rq_lockp(rq));
-	rcu_read_unlock();
+	rcu_read_unlock_sched();
 }
 
 static DEFINE_PER_CPU(struct callback_head, core_balance_head);
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Paul E. McKenney]

On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
> rcu_read_unlock() can incur an infrequent deadlock in
> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
> 
> This fixes the following spinlock recursion observed when testing the
> core scheduling patches on PREEMPT=y kernel on ChromeOS:
> 
> [    3.240891] BUG: spinlock recursion on CPU#2, swapper/2/0
> [    3.240900]  lock: 0xffff9cd1eeb28e40, .magic: dead4ead, .owner: swapper/2/0, .owner_cpu: 2
> [    3.240905] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.4.22htcore #4
> [    3.240908] Hardware name: Google Eve/Eve, BIOS Google_Eve.9584.174.0 05/29/2018
> [    3.240910] Call Trace:
> [    3.240919]  dump_stack+0x97/0xdb
> [    3.240924]  ? spin_bug+0xa4/0xb1
> [    3.240927]  do_raw_spin_lock+0x79/0x98
> [    3.240931]  try_to_wake_up+0x367/0x61b
> [    3.240935]  rcu_read_unlock_special+0xde/0x169
> [    3.240938]  ? sched_core_balance+0xd9/0x11e
> [    3.240941]  __rcu_read_unlock+0x48/0x4a
> [    3.240945]  __balance_callback+0x50/0xa1
> [    3.240949]  __schedule+0x55a/0x61e
> [    3.240952]  schedule_idle+0x21/0x2d
> [    3.240956]  do_idle+0x1d5/0x1f8
> [    3.240960]  cpu_startup_entry+0x1d/0x1f
> [    3.240964]  start_secondary+0x159/0x174
> [    3.240967]  secondary_startup_64+0xa4/0xb0
> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
> 
> Cc: vpillai <vpillai@digitalocean.com>
> Cc: Aaron Lu <aaron.lwe@gmail.com>
> Cc: Aubrey Li <aubrey.intel@gmail.com>
> Cc: peterz@infradead.org
> Cc: paulmck@kernel.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

The original could indeed deadlock, and this would avoid that deadlock.
(The commit to solve this deadlock is sadly not yet in mainline.)

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

> ---
>  kernel/sched/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3045bd50e249..037e8f2e2686 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4735,7 +4735,7 @@ static void sched_core_balance(struct rq *rq)
>  	struct sched_domain *sd;
>  	int cpu = cpu_of(rq);
>  
> -	rcu_read_lock();
> +	rcu_read_lock_sched();
>  	raw_spin_unlock_irq(rq_lockp(rq));
>  	for_each_domain(cpu, sd) {
>  		if (!(sd->flags & SD_LOAD_BALANCE))
> @@ -4748,7 +4748,7 @@ static void sched_core_balance(struct rq *rq)
>  			break;
>  	}
>  	raw_spin_lock_irq(rq_lockp(rq));
> -	rcu_read_unlock();
> +	rcu_read_unlock_sched();
>  }
>  
>  static DEFINE_PER_CPU(struct callback_head, core_balance_head);
> -- 
> 2.25.1.481.gfbce0eb801-goog
> 

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Li, Aubrey]

On 2020/3/14 8:30, Paul E. McKenney wrote:
> On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
>> rcu_read_unlock() can incur an infrequent deadlock in
>> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
>>
>> This fixes the following spinlock recursion observed when testing the
>> core scheduling patches on PREEMPT=y kernel on ChromeOS:
>>
>> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
>>
> 
> The original could indeed deadlock, and this would avoid that deadlock.
> (The commit to solve this deadlock is sadly not yet in mainline.)
> 
> Acked-by: Paul E. McKenney <paulmck@kernel.org>

I saw this in dmesg with this patch, is it expected?

Thanks,
-Aubrey

[  117.000905] =============================
[  117.000907] WARNING: suspicious RCU usage
[  117.000911] 5.5.7+ #160 Not tainted
[  117.000913] -----------------------------
[  117.000916] kernel/sched/core.c:4747 suspicious rcu_dereference_check() usage!
[  117.000918] 
               other info that might help us debug this:

[  117.000921] 
               rcu_scheduler_active = 2, debug_locks = 1
[  117.000923] 1 lock held by swapper/52/0:
[  117.000925]  #0: ffffffff82670960 (rcu_read_lock_sched){....}, at: sched_core_balance+0x5/0x700
[  117.000937] 
               stack backtrace:
[  117.000940] CPU: 52 PID: 0 Comm: swapper/52 Kdump: loaded Not tainted 5.5.7+ #160
[  117.000943] Hardware name: Intel Corporation S2600WFD/S2600WFD, BIOS SE5C620.86B.01.00.0412.020920172159 02/09/2017
[  117.000945] Call Trace:
[  117.000955]  dump_stack+0x86/0xcb
[  117.000962]  sched_core_balance+0x634/0x700
[  117.000982]  __balance_callback+0x49/0xa0
[  117.000990]  __schedule+0x1416/0x1620
[  117.001000]  ? lockdep_hardirqs_off+0xa0/0xe0
[  117.001005]  ? _raw_spin_unlock_irqrestore+0x41/0x70
[  117.001024]  schedule_idle+0x28/0x40
[  117.001030]  do_idle+0x17e/0x2a0
[  117.001041]  cpu_startup_entry+0x19/0x20
[  117.001048]  start_secondary+0x16c/0x1c0
[  117.001055]  secondary_startup_64+0xa4/0xb0

> 
>> ---
>>  kernel/sched/core.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 3045bd50e249..037e8f2e2686 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -4735,7 +4735,7 @@ static void sched_core_balance(struct rq *rq)
>>  	struct sched_domain *sd;
>>  	int cpu = cpu_of(rq);
>>  
>> -	rcu_read_lock();
>> +	rcu_read_lock_sched();
>>  	raw_spin_unlock_irq(rq_lockp(rq));
>>  	for_each_domain(cpu, sd) {
>>  		if (!(sd->flags & SD_LOAD_BALANCE))
>> @@ -4748,7 +4748,7 @@ static void sched_core_balance(struct rq *rq)
>>  			break;
>>  	}
>>  	raw_spin_lock_irq(rq_lockp(rq));
>> -	rcu_read_unlock();
>> +	rcu_read_unlock_sched();
>>  }
>>  
>>  static DEFINE_PER_CPU(struct callback_head, core_balance_head);
>> -- 
>> 2.25.1.481.gfbce0eb801-goog
>>

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Joel Fernandes]

On Mon, Mar 23, 2020 at 02:58:18PM +0800, Li, Aubrey wrote:
> On 2020/3/14 8:30, Paul E. McKenney wrote:
> > On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
> >> rcu_read_unlock() can incur an infrequent deadlock in
> >> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
> >>
> >> This fixes the following spinlock recursion observed when testing the
> >> core scheduling patches on PREEMPT=y kernel on ChromeOS:
> >>
> >> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
> >>
> > 
> > The original could indeed deadlock, and this would avoid that deadlock.
> > (The commit to solve this deadlock is sadly not yet in mainline.)
> > 
> > Acked-by: Paul E. McKenney <paulmck@kernel.org>
> 
> I saw this in dmesg with this patch, is it expected?
> 
> [  117.000905] =============================
> [  117.000907] WARNING: suspicious RCU usage
> [  117.000911] 5.5.7+ #160 Not tainted
> [  117.000913] -----------------------------
> [  117.000916] kernel/sched/core.c:4747 suspicious rcu_dereference_check() usage!
> [  117.000918] 
>                other info that might help us debug this:

Sigh, this is because for_each_domain() expects rcu_read_lock(). From an RCU
PoV, the code is correct (warning doesn't cause any issue).

To silence warning, we could replace the rcu_read_lock_sched() in my patch with:
preempt_disable();
rcu_read_lock();

and replace the unlock with:

rcu_read_unlock();
preempt_enable();

That should both take care of both the warning and the scheduler-related
deadlock. Thoughts?

Does that fix the warning for you? 

thanks,

 - Joel

> 
> [  117.000921] 
>                rcu_scheduler_active = 2, debug_locks = 1
> [  117.000923] 1 lock held by swapper/52/0:
> [  117.000925]  #0: ffffffff82670960 (rcu_read_lock_sched){....}, at: sched_core_balance+0x5/0x700
> [  117.000937] 
>                stack backtrace:
> [  117.000940] CPU: 52 PID: 0 Comm: swapper/52 Kdump: loaded Not tainted 5.5.7+ #160
> [  117.000943] Hardware name: Intel Corporation S2600WFD/S2600WFD, BIOS SE5C620.86B.01.00.0412.020920172159 02/09/2017
> [  117.000945] Call Trace:
> [  117.000955]  dump_stack+0x86/0xcb
> [  117.000962]  sched_core_balance+0x634/0x700
> [  117.000982]  __balance_callback+0x49/0xa0
> [  117.000990]  __schedule+0x1416/0x1620
> [  117.001000]  ? lockdep_hardirqs_off+0xa0/0xe0
> [  117.001005]  ? _raw_spin_unlock_irqrestore+0x41/0x70
> [  117.001024]  schedule_idle+0x28/0x40
> [  117.001030]  do_idle+0x17e/0x2a0
> [  117.001041]  cpu_startup_entry+0x19/0x20
> [  117.001048]  start_secondary+0x16c/0x1c0
> [  117.001055]  secondary_startup_64+0xa4/0xb0
> 
> > 
> >> ---
> >>  kernel/sched/core.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index 3045bd50e249..037e8f2e2686 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -4735,7 +4735,7 @@ static void sched_core_balance(struct rq *rq)
> >>  	struct sched_domain *sd;
> >>  	int cpu = cpu_of(rq);
> >>  
> >> -	rcu_read_lock();
> >> +	rcu_read_lock_sched();
> >>  	raw_spin_unlock_irq(rq_lockp(rq));
> >>  	for_each_domain(cpu, sd) {
> >>  		if (!(sd->flags & SD_LOAD_BALANCE))
> >> @@ -4748,7 +4748,7 @@ static void sched_core_balance(struct rq *rq)
> >>  			break;
> >>  	}
> >>  	raw_spin_lock_irq(rq_lockp(rq));
> >> -	rcu_read_unlock();
> >> +	rcu_read_unlock_sched();
> >>  }
> >>  
> >>  static DEFINE_PER_CPU(struct callback_head, core_balance_head);
> >> -- 
> >> 2.25.1.481.gfbce0eb801-goog
> >>
> 

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Li, Aubrey]

On 2020/3/23 23:21, Joel Fernandes wrote:
> On Mon, Mar 23, 2020 at 02:58:18PM +0800, Li, Aubrey wrote:
>> On 2020/3/14 8:30, Paul E. McKenney wrote:
>>> On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
>>>> rcu_read_unlock() can incur an infrequent deadlock in
>>>> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
>>>>
>>>> This fixes the following spinlock recursion observed when testing the
>>>> core scheduling patches on PREEMPT=y kernel on ChromeOS:
>>>>
>>>> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
>>>>
>>>
>>> The original could indeed deadlock, and this would avoid that deadlock.
>>> (The commit to solve this deadlock is sadly not yet in mainline.)
>>>
>>> Acked-by: Paul E. McKenney <paulmck@kernel.org>
>>
>> I saw this in dmesg with this patch, is it expected?
>>
>> [  117.000905] =============================
>> [  117.000907] WARNING: suspicious RCU usage
>> [  117.000911] 5.5.7+ #160 Not tainted
>> [  117.000913] -----------------------------
>> [  117.000916] kernel/sched/core.c:4747 suspicious rcu_dereference_check() usage!
>> [  117.000918] 
>>                other info that might help us debug this:
> 
> Sigh, this is because for_each_domain() expects rcu_read_lock(). From an RCU
> PoV, the code is correct (warning doesn't cause any issue).
> 
> To silence warning, we could replace the rcu_read_lock_sched() in my patch with:
> preempt_disable();
> rcu_read_lock();
> 
> and replace the unlock with:
> 
> rcu_read_unlock();
> preempt_enable();
> 
> That should both take care of both the warning and the scheduler-related
> deadlock. Thoughts?
> 

How about this?

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a01df3e..7ff694e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4743,7 +4743,6 @@ static void sched_core_balance(struct rq *rq)
 	int cpu = cpu_of(rq);
 
 	rcu_read_lock();
-	raw_spin_unlock_irq(rq_lockp(rq));
 	for_each_domain(cpu, sd) {
 		if (!(sd->flags & SD_LOAD_BALANCE))
 			break;
@@ -4754,7 +4753,6 @@ static void sched_core_balance(struct rq *rq)
 		if (steal_cookie_task(cpu, sd))
 			break;
 	}
-	raw_spin_lock_irq(rq_lockp(rq));
 	rcu_read_unlock();
 }

Thanks,
-Aubrey

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Paul E. McKenney]

On Tue, Mar 24, 2020 at 11:01:27AM +0800, Li, Aubrey wrote:
> On 2020/3/23 23:21, Joel Fernandes wrote:
> > On Mon, Mar 23, 2020 at 02:58:18PM +0800, Li, Aubrey wrote:
> >> On 2020/3/14 8:30, Paul E. McKenney wrote:
> >>> On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
> >>>> rcu_read_unlock() can incur an infrequent deadlock in
> >>>> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
> >>>>
> >>>> This fixes the following spinlock recursion observed when testing the
> >>>> core scheduling patches on PREEMPT=y kernel on ChromeOS:
> >>>>
> >>>> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
> >>>>
> >>>
> >>> The original could indeed deadlock, and this would avoid that deadlock.
> >>> (The commit to solve this deadlock is sadly not yet in mainline.)
> >>>
> >>> Acked-by: Paul E. McKenney <paulmck@kernel.org>
> >>
> >> I saw this in dmesg with this patch, is it expected?
> >>
> >> [  117.000905] =============================
> >> [  117.000907] WARNING: suspicious RCU usage
> >> [  117.000911] 5.5.7+ #160 Not tainted
> >> [  117.000913] -----------------------------
> >> [  117.000916] kernel/sched/core.c:4747 suspicious rcu_dereference_check() usage!
> >> [  117.000918] 
> >>                other info that might help us debug this:
> > 
> > Sigh, this is because for_each_domain() expects rcu_read_lock(). From an RCU
> > PoV, the code is correct (warning doesn't cause any issue).
> > 
> > To silence warning, we could replace the rcu_read_lock_sched() in my patch with:
> > preempt_disable();
> > rcu_read_lock();
> > 
> > and replace the unlock with:
> > 
> > rcu_read_unlock();
> > preempt_enable();
> > 
> > That should both take care of both the warning and the scheduler-related
> > deadlock. Thoughts?
> > 
> 
> How about this?
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index a01df3e..7ff694e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4743,7 +4743,6 @@ static void sched_core_balance(struct rq *rq)
>  	int cpu = cpu_of(rq);
>  
>  	rcu_read_lock();
> -	raw_spin_unlock_irq(rq_lockp(rq));
>  	for_each_domain(cpu, sd) {
>  		if (!(sd->flags & SD_LOAD_BALANCE))
>  			break;
> @@ -4754,7 +4753,6 @@ static void sched_core_balance(struct rq *rq)
>  		if (steal_cookie_task(cpu, sd))
>  			break;
>  	}
> -	raw_spin_lock_irq(rq_lockp(rq));
>  	rcu_read_unlock();
>  }

As an alternative, I am backporting the -rcu commit 2b5e19e20fc2 ("rcu:
Make rcu_read_unlock_special() safe for rq/pi locks") to v5.6-rc6 and
testing it.  The other support for this is already in mainline.  I just
now started testing it, and will let you know how it goes.

If that works for you, and if the bug you are looking to fix is also
in v5.5 or earlier, please let me know so that we can work out how to
deal with the older releases.

							Thanx, Paul

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Paul E. McKenney]

On Tue, Mar 24, 2020 at 06:30:12AM -0700, Paul E. McKenney wrote:
> On Tue, Mar 24, 2020 at 11:01:27AM +0800, Li, Aubrey wrote:
> > On 2020/3/23 23:21, Joel Fernandes wrote:
> > > On Mon, Mar 23, 2020 at 02:58:18PM +0800, Li, Aubrey wrote:
> > >> On 2020/3/14 8:30, Paul E. McKenney wrote:
> > >>> On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
> > >>>> rcu_read_unlock() can incur an infrequent deadlock in
> > >>>> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
> > >>>>
> > >>>> This fixes the following spinlock recursion observed when testing the
> > >>>> core scheduling patches on PREEMPT=y kernel on ChromeOS:
> > >>>>
> > >>>> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
> > >>>>
> > >>>
> > >>> The original could indeed deadlock, and this would avoid that deadlock.
> > >>> (The commit to solve this deadlock is sadly not yet in mainline.)
> > >>>
> > >>> Acked-by: Paul E. McKenney <paulmck@kernel.org>
> > >>
> > >> I saw this in dmesg with this patch, is it expected?
> > >>
> > >> [  117.000905] =============================
> > >> [  117.000907] WARNING: suspicious RCU usage
> > >> [  117.000911] 5.5.7+ #160 Not tainted
> > >> [  117.000913] -----------------------------
> > >> [  117.000916] kernel/sched/core.c:4747 suspicious rcu_dereference_check() usage!
> > >> [  117.000918] 
> > >>                other info that might help us debug this:
> > > 
> > > Sigh, this is because for_each_domain() expects rcu_read_lock(). From an RCU
> > > PoV, the code is correct (warning doesn't cause any issue).
> > > 
> > > To silence warning, we could replace the rcu_read_lock_sched() in my patch with:
> > > preempt_disable();
> > > rcu_read_lock();
> > > 
> > > and replace the unlock with:
> > > 
> > > rcu_read_unlock();
> > > preempt_enable();
> > > 
> > > That should both take care of both the warning and the scheduler-related
> > > deadlock. Thoughts?
> > > 
> > 
> > How about this?
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index a01df3e..7ff694e 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -4743,7 +4743,6 @@ static void sched_core_balance(struct rq *rq)
> >  	int cpu = cpu_of(rq);
> >  
> >  	rcu_read_lock();
> > -	raw_spin_unlock_irq(rq_lockp(rq));
> >  	for_each_domain(cpu, sd) {
> >  		if (!(sd->flags & SD_LOAD_BALANCE))
> >  			break;
> > @@ -4754,7 +4753,6 @@ static void sched_core_balance(struct rq *rq)
> >  		if (steal_cookie_task(cpu, sd))
> >  			break;
> >  	}
> > -	raw_spin_lock_irq(rq_lockp(rq));
> >  	rcu_read_unlock();
> >  }
> 
> As an alternative, I am backporting the -rcu commit 2b5e19e20fc2 ("rcu:
> Make rcu_read_unlock_special() safe for rq/pi locks") to v5.6-rc6 and
> testing it.  The other support for this is already in mainline.  I just
> now started testing it, and will let you know how it goes.
> 
> If that works for you, and if the bug you are looking to fix is also
> in v5.5 or earlier, please let me know so that we can work out how to
> deal with the older releases.

And the backported patch below passes moderate rcutorture testing.
But the big question...  Does it work for you?

							Thanx, Paul

------------------------------------------------------------------------

commit 0ed0648e23f6aca2a6543fee011d74ab674e08e8
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Tue Mar 24 06:20:14 2020 -0700

    rcu: Make rcu_read_unlock_special() safe for rq/pi locks
    
    The scheduler is currently required to hold rq/pi locks across the entire
    RCU read-side critical section or not at all.  This is inconvenient and
    leaves traps for the unwary, including the author of this commit.
    
    But now that excessively ong grace periods enable scheduling-clock
    interrupts for holdout nohz_full CPUs, the nohz_full rescue logic in
    rcu_read_unlock_special() can be dispensed with.  In other words, the
    rcu_read_unlock_special() function can refrain from doing wakeups unless
    such wakeups are guaranteed safe.
    
    This commit therefore avoids unsafe wakeups, freeing the scheduler to
    hold rq/pi locks across rcu_read_unlock() even if the corresponding RCU
    read-side critical section might have been preempted.
    
    This commit is inspired by a patch from Lai Jiangshan:
    https://lore.kernel.org/lkml/20191102124559.1135-2-laijs@linux.alibaba.com
    This commit is further intended to be a step towards his goal of permitting
    the inlining of RCU-preempt's rcu_read_lock() and rcu_read_unlock().
    
    Cc: Lai Jiangshan <laijs@linux.alibaba.com>
    [ paulmck: Backported to v5.6-rc6. ]
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index c6ea81cd4189..2e1bfa0cc393 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -614,18 +614,16 @@ static void rcu_read_unlock_special(struct task_struct *t)
 		struct rcu_node *rnp = rdp->mynode;
 
 		exp = (t->rcu_blocked_node && t->rcu_blocked_node->exp_tasks) ||
-		      (rdp->grpmask & READ_ONCE(rnp->expmask)) ||
-		      tick_nohz_full_cpu(rdp->cpu);
+		      (rdp->grpmask & READ_ONCE(rnp->expmask));
 		// Need to defer quiescent state until everything is enabled.
-		if (irqs_were_disabled && use_softirq &&
-		    (in_interrupt() ||
-		     (exp && !t->rcu_read_unlock_special.b.deferred_qs))) {
-			// Using softirq, safe to awaken, and we get
-			// no help from enabling irqs, unlike bh/preempt.
+		if (use_softirq && (in_irq() || (exp && !irqs_were_disabled))) {
+			// Using softirq, safe to awaken, and either the
+			// wakeup is free or there is an expedited GP.
 			raise_softirq_irqoff(RCU_SOFTIRQ);
 		} else {
 			// Enabling BH or preempt does reschedule, so...
-			// Also if no expediting or NO_HZ_FULL, slow is OK.
+			// Also if no expediting, slow is OK.
+			// Plus nohz_full CPUs eventually get tick enabled.
 			set_tsk_need_resched(current);
 			set_preempt_need_resched();
 			if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Joel Fernandes]

On Tue, Mar 24, 2020 at 11:01:27AM +0800, Li, Aubrey wrote:
> On 2020/3/23 23:21, Joel Fernandes wrote:
> > On Mon, Mar 23, 2020 at 02:58:18PM +0800, Li, Aubrey wrote:
> >> On 2020/3/14 8:30, Paul E. McKenney wrote:
> >>> On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
> >>>> rcu_read_unlock() can incur an infrequent deadlock in
> >>>> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
> >>>>
> >>>> This fixes the following spinlock recursion observed when testing the
> >>>> core scheduling patches on PREEMPT=y kernel on ChromeOS:
> >>>>
> >>>> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
> >>>>
> >>>
> >>> The original could indeed deadlock, and this would avoid that deadlock.
> >>> (The commit to solve this deadlock is sadly not yet in mainline.)
> >>>
> >>> Acked-by: Paul E. McKenney <paulmck@kernel.org>
> >>
> >> I saw this in dmesg with this patch, is it expected?
> >>
> >> [  117.000905] =============================
> >> [  117.000907] WARNING: suspicious RCU usage
> >> [  117.000911] 5.5.7+ #160 Not tainted
> >> [  117.000913] -----------------------------
> >> [  117.000916] kernel/sched/core.c:4747 suspicious rcu_dereference_check() usage!
> >> [  117.000918] 
> >>                other info that might help us debug this:
> > 
> > Sigh, this is because for_each_domain() expects rcu_read_lock(). From an RCU
> > PoV, the code is correct (warning doesn't cause any issue).
> > 
> > To silence warning, we could replace the rcu_read_lock_sched() in my patch with:
> > preempt_disable();
> > rcu_read_lock();
> > 
> > and replace the unlock with:
> > 
> > rcu_read_unlock();
> > preempt_enable();
> > 
> > That should both take care of both the warning and the scheduler-related
> > deadlock. Thoughts?
> > 
> 
> How about this?
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index a01df3e..7ff694e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4743,7 +4743,6 @@ static void sched_core_balance(struct rq *rq)
>  	int cpu = cpu_of(rq);
>  
>  	rcu_read_lock();
> -	raw_spin_unlock_irq(rq_lockp(rq));
>  	for_each_domain(cpu, sd) {
>  		if (!(sd->flags & SD_LOAD_BALANCE))
>  			break;
> @@ -4754,7 +4753,6 @@ static void sched_core_balance(struct rq *rq)
>  		if (steal_cookie_task(cpu, sd))
>  			break;
>  	}
> -	raw_spin_lock_irq(rq_lockp(rq));

try_steal_cookie() does a double_rq_lock(). Would this change not deadlock
with that?

thanks,

 - Joel

Re: [PATCH] sched: Use RCU-sched in core-scheduling balancing logic

[Li, Aubrey]

On 2020/3/25 2:49, Joel Fernandes wrote:
> On Tue, Mar 24, 2020 at 11:01:27AM +0800, Li, Aubrey wrote:
>> On 2020/3/23 23:21, Joel Fernandes wrote:
>>> On Mon, Mar 23, 2020 at 02:58:18PM +0800, Li, Aubrey wrote:
>>>> On 2020/3/14 8:30, Paul E. McKenney wrote:
>>>>> On Fri, Mar 13, 2020 at 07:29:18PM -0400, Joel Fernandes (Google) wrote:
>>>>>> rcu_read_unlock() can incur an infrequent deadlock in
>>>>>> sched_core_balance(). Fix this by using the RCU-sched flavor instead.
>>>>>>
>>>>>> This fixes the following spinlock recursion observed when testing the
>>>>>> core scheduling patches on PREEMPT=y kernel on ChromeOS:
>>>>>>
>>>>>> [   14.998590] watchdog: BUG: soft lockup - CPU#0 stuck for 11s! [kworker/0:10:965]
>>>>>>
>>>>>
>>>>> The original could indeed deadlock, and this would avoid that deadlock.
>>>>> (The commit to solve this deadlock is sadly not yet in mainline.)
>>>>>
>>>>> Acked-by: Paul E. McKenney <paulmck@kernel.org>
>>>>
>>>> I saw this in dmesg with this patch, is it expected?
>>>>
>>>> [  117.000905] =============================
>>>> [  117.000907] WARNING: suspicious RCU usage
>>>> [  117.000911] 5.5.7+ #160 Not tainted
>>>> [  117.000913] -----------------------------
>>>> [  117.000916] kernel/sched/core.c:4747 suspicious rcu_dereference_check() usage!
>>>> [  117.000918] 
>>>>                other info that might help us debug this:
>>>
>>> Sigh, this is because for_each_domain() expects rcu_read_lock(). From an RCU
>>> PoV, the code is correct (warning doesn't cause any issue).
>>>
>>> To silence warning, we could replace the rcu_read_lock_sched() in my patch with:
>>> preempt_disable();
>>> rcu_read_lock();
>>>
>>> and replace the unlock with:
>>>
>>> rcu_read_unlock();
>>> preempt_enable();
>>>
>>> That should both take care of both the warning and the scheduler-related
>>> deadlock. Thoughts?
>>>
>>
>> How about this?
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index a01df3e..7ff694e 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -4743,7 +4743,6 @@ static void sched_core_balance(struct rq *rq)
>>  	int cpu = cpu_of(rq);
>>  
>>  	rcu_read_lock();
>> -	raw_spin_unlock_irq(rq_lockp(rq));
>>  	for_each_domain(cpu, sd) {
>>  		if (!(sd->flags & SD_LOAD_BALANCE))
>>  			break;
>> @@ -4754,7 +4753,6 @@ static void sched_core_balance(struct rq *rq)
>>  		if (steal_cookie_task(cpu, sd))
>>  			break;
>>  	}
>> -	raw_spin_lock_irq(rq_lockp(rq));
> 
> try_steal_cookie() does a double_rq_lock(). Would this change not deadlock
> with that?
> 
Oh yes, missed double_rq_lock() inside, just want to keep local intr disabled
to avoid preemption. will try Paul's patch and report back.

Thanks,
-Aubrey