2017-03-08 15:23:48

by Laurent Dufour

[permalink] [raw]
Subject: RFC: SysRq nice-all-RT-tasks is broken

Hi,

It appears that triggering the SysRq nice-all-RT-tasks from the console
while a real task is active is leading to panic the system like this :

sysrq: SysRq : Nice All RT Tasks
------------[ cut here ]------------
kernel BUG at /build/linux-twbIHf/linux-4.10.0/kernel/sched/core.c:4089!
Oops: Exception in kernel mode, sig: 5 [#1]
SMP NR_CPUS=2048
NUMA
pSeries
Modules linked in: rpcsec_gss_krb5 auth_rpcgss nfsv4 nfs lockd grace
fscache pseries_rng vmx_crypto binfmt_misc dm_multipath scsi_dh_rdac
scsi_dh_emc scsi_dh_alua sunrpc ip_tables x_tables autofs4 btrfs xor
raid6_pq ses enclosure scsi_transport_sas crc32c_vpmsum mlx5_core be2net
ipr devlink
CPU: 100 PID: 0 Comm: swapper/100 Not tainted 4.10.0-8-generic #10-Ubuntu
task: c0000003b6179c00 task.stack: c0000003b620c000
NIP: c0000000001044f0 LR: c00000000010e524 CTR: c0000000006d0ec0
REGS: c00000077fc978d0 TRAP: 0700 Not tainted (4.10.0-8-generic)
MSR: 8000000000029033 <SF,EE,ME,IR,DR,RI,LE>
CR: 28002228 XER: 20000001
CFAR: c00000000010e520 SOFTE: 0
GPR00: c00000000010e524 c00000077fc97b50 c00000000143c900 c00000038785fa00
GPR04: c00000077fc97c00 0000000000000000 0000000000000000 ffffffffffffffff
GPR08: 0000000000000000 0000000000010000 0000000000000000 0000000000000406
GPR12: 0000000028002228 c000000007b58400 c0000003b620ff90 000000001ede2d80
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000001 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 c00000077a0f8b50 0000000000000063 0000000000000000
GPR28: c00000038785fa00 c00000077fc97c00 0000000000000000 c00000038785fa00
NIP [c0000000001044f0] __sched_setscheduler+0x90/0xd30
LR [c00000000010e524] normalize_rt_tasks+0x184/0x1c0
Call Trace:
[c00000077fc97be0] [c00000000010e524] normalize_rt_tasks+0x184/0x1c0
[c00000077fc97c60] [c0000000006d0ee0] sysrq_handle_unrt+0x20/0x40
[c00000077fc97c80] [c0000000006d1c98] __handle_sysrq+0xe8/0x280
[c00000077fc97d20] [c0000000006eab38] hvc_poll+0x1c8/0x360
[c00000077fc97dc0] [c0000000006ebd74] hvc_handle_interrupt+0x24/0x50
[c00000077fc97de0] [c0000000001453d0] __handle_irq_event_percpu+0x90/0x2c0
[c00000077fc97ea0] [c000000000145638] handle_irq_event_percpu+0x38/0x90
[c00000077fc97ee0] [c0000000001456f4] handle_irq_event+0x64/0xb0
[c00000077fc97f10] [c00000000014add8] handle_fasteoi_irq+0xe8/0x290
[c00000077fc97f40] [c000000000143ebc] generic_handle_irq+0x4c/0x80
[c00000077fc97f60] [c0000000000167b0] __do_irq+0x80/0x1d0
[c00000077fc97f90] [c000000000029414] call_do_irq+0x14/0x24
[c0000003b620fa40] [c000000000016994] do_IRQ+0x94/0x140
[c0000003b620fa80] [c000000000008be4] hardware_interrupt_common+0x114/0x120
--- interrupt: 501 at plpar_hcall_norets+0x1c/0x28
LR = check_and_cede_processor+0x34/0x50
[c0000003b620fd70] [c0000003b620fe10] 0xc0000003b620fe10 (unreliable)
[c0000003b620fdd0] [c00000000095db50] shared_cede_loop+0x50/0x150
[c0000003b620fe00] [c00000000095accc] cpuidle_enter_state+0x16c/0x430
[c0000003b620fe60] [c00000000012c8bc] call_cpuidle+0x4c/0x90
[c0000003b620fe80] [c00000000012cce0] do_idle+0x2c0/0x330
[c0000003b620ff00] [c00000000012cfa8] cpu_startup_entry+0x38/0x50
[c0000003b620ff30] [c000000000044180] start_secondary+0x330/0x370
[c0000003b620ff90] [c00000000000aa6c] start_secondary_prolog+0x10/0x14
Instruction dump:
7c7f1b78 7cb62b78 7cdb3378 2b8a0006 7d5507b4 419e0010 83440014 235a0063
7f5a07b4 78290464 8129000c 552902ee <0b090000> 2f950000 419c04b8 2f950005
---[ end trace 891618fbca78ebe7 ]---

I got it on Power and on X86_64, but I guess it should happen in all
architectures.
Here are the steps to recreate it :
1. Create a RT task : sudo chrt -f 50 /bin/sleep 999999
2. On the console trigger the 'nice-all-RT-tasks' SYS-RQ.

The panic is triggered by the BUG_ON(in_interrupt()) introduced by this
commit:

66e5393a78b3 ("[PATCH] BUG() if setscheduler is called from interrupt
context")

Since SysRq is run from the interrupt context, the panic is expected.

Looking at the code, I'm wondering if the BUG_ON() is still required in
__sched_setscheduler(). But I'm not so confident, so requesting your
advise here.

Thanks,
Laurent.


2017-03-08 16:51:22

by Steven Rostedt

[permalink] [raw]
Subject: Re: RFC: SysRq nice-all-RT-tasks is broken

On Wed, 8 Mar 2017 16:23:35 +0100
Laurent Dufour <[email protected]> wrote:

> I got it on Power and on X86_64, but I guess it should happen in all
> architectures.
> Here are the steps to recreate it :
> 1. Create a RT task : sudo chrt -f 50 /bin/sleep 999999
> 2. On the console trigger the 'nice-all-RT-tasks' SYS-RQ.
>
> The panic is triggered by the BUG_ON(in_interrupt()) introduced by this
> commit:
>
> 66e5393a78b3 ("[PATCH] BUG() if setscheduler is called from interrupt
> context")
>
> Since SysRq is run from the interrupt context, the panic is expected.
>
> Looking at the code, I'm wondering if the BUG_ON() is still required in
> __sched_setscheduler(). But I'm not so confident, so requesting your
> advise here.
>

Hmm, that commit was added in 2.6.18, and you're right, a lot has
changed since then. Have you tried removing it and running it under
lockdep, and see if it triggers any warnings?

-- Steve

2017-03-08 16:59:07

by Steven Rostedt

[permalink] [raw]
Subject: Re: RFC: SysRq nice-all-RT-tasks is broken

On Wed, 8 Mar 2017 11:51:14 -0500
Steven Rostedt <[email protected]> wrote:


> Hmm, that commit was added in 2.6.18, and you're right, a lot has
> changed since then. Have you tried removing it and running it under
> lockdep, and see if it triggers any warnings?

I did a little digging, and it appears that its the rt mutex wait lock
that the comment was referring to. Today that spin lock is irq safe. I
believe its safe to remove the BUG_ON(). Want me to send a patch?

-- Steve

2017-03-08 17:40:20

by Steven Rostedt

[permalink] [raw]
Subject: Re: RFC: SysRq nice-all-RT-tasks is broken


[
Added Peter

Update: Laurent noticed that sysrq 'n' (nice-all-RT-tasks) calls
__sched_setscheduler() form interrupt context. At the start of that
function, there's a BUG_ON(in_interrupt()). The reason for that was
due to the rt mutex pi code calling wait_lock. Which was not irq
safe. Now it is, but that's not good enough.
]

On Wed, 8 Mar 2017 18:03:55 +0100
Laurent Dufour <[email protected]> wrote:

> On 08/03/2017 17:57, Steven Rostedt wrote:
> > On Wed, 8 Mar 2017 11:51:14 -0500
> > Steven Rostedt <[email protected]> wrote:
> >
> >
> >> Hmm, that commit was added in 2.6.18, and you're right, a lot has
> >> changed since then. Have you tried removing it and running it under
> >> lockdep, and see if it triggers any warnings?
> >
> > I did a little digging, and it appears that its the rt mutex wait lock
> > that the comment was referring to. Today that spin lock is irq safe. I
> > believe its safe to remove the BUG_ON(). Want me to send a patch?
>
> Sure, go ahead ;)
>

Actually, it's still not safe :-/

I just noticed this in the call path:

raw_spin_unlock_irq(&task->pi_lock);

As well as other raw_spin_unlock_irq()s.

Which would enable interrupts regardless of the previous state.

One solution is to change all those to irqsave() but that seems to be a
big step for something that is rarely done (how many years has it been
since 2.6.18?).

I wonder if we should just have a special flag sent by that sysrq
trigger. Since it is causing all tasks to go "nice" there's no need to
do the pi chain walk in __sched_setscheduler().

-- Steve

2017-03-08 17:56:25

by Steven Rostedt

[permalink] [raw]
Subject: Re: RFC: SysRq nice-all-RT-tasks is broken

On Wed, 8 Mar 2017 12:40:12 -0500
Steven Rostedt <[email protected]> wrote:

> I wonder if we should just have a special flag sent by that sysrq
> trigger. Since it is causing all tasks to go "nice" there's no need to
> do the pi chain walk in __sched_setscheduler().

Hah, there already is a flag!

Laurent, can you test this patch:

-- Steve

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3b31fc0..7292fa9 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4129,8 +4129,8 @@ static int __sched_setscheduler(struct task_struct *p,
int queue_flags = DEQUEUE_SAVE | DEQUEUE_MOVE;
struct rq *rq;

- /* May grab non-irq protected spin_locks: */
- BUG_ON(in_interrupt());
+ /* The pi code expects interrupts enabled */
+ BUG_ON(pi && in_interrupt());
recheck:
/* Double check policy once rq lock held: */
if (policy < 0) {

2017-03-08 20:59:29

by Laurent Dufour

[permalink] [raw]
Subject: Re: RFC: SysRq nice-all-RT-tasks is broken

On 08/03/2017 17:57, Steven Rostedt wrote:
> On Wed, 8 Mar 2017 11:51:14 -0500
> Steven Rostedt <[email protected]> wrote:
>
>
>> Hmm, that commit was added in 2.6.18, and you're right, a lot has
>> changed since then. Have you tried removing it and running it under
>> lockdep, and see if it triggers any warnings?
>
> I did a little digging, and it appears that its the rt mutex wait lock
> that the comment was referring to. Today that spin lock is irq safe. I
> believe its safe to remove the BUG_ON(). Want me to send a patch?

Sure, go ahead ;)

Thanks,
Laurent.

2017-03-09 11:23:36

by Laurent Dufour

[permalink] [raw]
Subject: Re: RFC: SysRq nice-all-RT-tasks is broken

On 08/03/2017 18:46, Steven Rostedt wrote:
> On Wed, 8 Mar 2017 12:40:12 -0500
> Steven Rostedt <[email protected]> wrote:
>
>> I wonder if we should just have a special flag sent by that sysrq
>> trigger. Since it is causing all tasks to go "nice" there's no need to
>> do the pi chain walk in __sched_setscheduler().
>
> Hah, there already is a flag!
>
> Laurent, can you test this patch:

Tested on ppc64, no more panic \o/

FWIW,
Tested-by: Laurent Dufour <[email protected]>

Thanks,
Laurent.

>
> -- Steve
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3b31fc0..7292fa9 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4129,8 +4129,8 @@ static int __sched_setscheduler(struct task_struct *p,
> int queue_flags = DEQUEUE_SAVE | DEQUEUE_MOVE;
> struct rq *rq;
>
> - /* May grab non-irq protected spin_locks: */
> - BUG_ON(in_interrupt());
> + /* The pi code expects interrupts enabled */
> + BUG_ON(pi && in_interrupt());
> recheck:
> /* Double check policy once rq lock held: */
> if (policy < 0) {
>