Interesting csd deadlock on ARC

Interesting csd deadlock on ARC

[Vineet Gupta]

Hi Peter,

I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
turned out to be lot more interesting than I'd hoped for. This is stock v4.4

Trouble starts with an IPI to self which doesn't get delivered as the inter-core
interrupt providing h/w is not capable of IPI to self (which I found as part of
debugging this). Subsequent IPIs from other cores to this core get elided as well
due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()

There are ways to use a different h/w mechanism to solve the trigger issue and I'd
hoped to just implement arch_irq_work_raise(). But the trouble is the call stack
for this issue: IPI to self is triggered from

sys_sched_setscheduler
   __balance_callback
       pull_rt_task
         irq_work_queue_on  <-- called with @cpu == self

Looking into irq_work.c, irq_work_queue() is what is semantically needed,
specifically arch_irq_work_raise() will not be called, which means I need
arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
expected from arch code....

Just wanted to understand before writing patches...

Test case triggering is harmless looking LTP: trace_sched -c 1
It is kind of scheduler fizzer as it triggers a whole bunch of sched activity.

Thx,
-Vineet

Interesting csd deadlock on ARC

[Vineet Gupta]

Hi Peter,

I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
turned out to be lot more interesting than I'd hoped for. This is stock v4.4

Trouble starts with an IPI to self which doesn't get delivered as the inter-core
interrupt providing h/w is not capable of IPI to self (which I found as part of
debugging this). Subsequent IPIs from other cores to this core get elided as well
due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()

There are ways to use a different h/w mechanism to solve the trigger issue and I'd
hoped to just implement arch_irq_work_raise(). But the trouble is the call stack
for this issue: IPI to self is triggered from

sys_sched_setscheduler
   __balance_callback
       pull_rt_task
         irq_work_queue_on  <-- called with @cpu == self

Looking into irq_work.c, irq_work_queue() is what is semantically needed,
specifically arch_irq_work_raise() will not be called, which means I need
arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
expected from arch code....

Just wanted to understand before writing patches...

Test case triggering is harmless looking LTP: trace_sched -c 1
It is kind of scheduler fizzer as it triggers a whole bunch of sched activity.

Thx,
-Vineet

Re: Interesting csd deadlock on ARC

[Vineet Gupta]

On Friday 19 February 2016 12:17 PM, Vineet Gupta wrote:
> Hi Peter,
> 
> I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
> turned out to be lot more interesting than I'd hoped for. This is stock v4.4
> 
> Trouble starts with an IPI to self which doesn't get delivered as the inter-core
> interrupt providing h/w is not capable of IPI to self (which I found as part of
> debugging this). Subsequent IPIs from other cores to this core get elided as well
> due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()
> 
> There are ways to use a different h/w mechanism to solve the trigger issue and I'd
> hoped to just implement arch_irq_work_raise(). But the trouble is the call stack
> for this issue: IPI to self is triggered from
> 
> sys_sched_setscheduler
>    __balance_callback
>        pull_rt_task
>          irq_work_queue_on  <-- called with @cpu == self
> 
> Looking into irq_work.c, irq_work_queue() is what is semantically needed,
> specifically arch_irq_work_raise() will not be called, which means I need
> arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
> expected from arch code....

What I actually meant was is it OK for irq_work_queue_on() to be called locally
(is this a sched bug/optimization(. Further if it is OK to be called, does it need
to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
arch_send_call_function_single_ipi() is expected to handle sending IPI to self !

> 
> Just wanted to understand before writing patches...
> 
> Test case triggering is harmless looking LTP: trace_sched -c 1
> It is kind of scheduler fizzer as it triggers a whole bunch of sched activity.
> 
> Thx,
> -Vineet
> 

Interesting csd deadlock on ARC

[Vineet Gupta]

On Friday 19 February 2016 12:17 PM, Vineet Gupta wrote:
> Hi Peter,
> 
> I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
> turned out to be lot more interesting than I'd hoped for. This is stock v4.4
> 
> Trouble starts with an IPI to self which doesn't get delivered as the inter-core
> interrupt providing h/w is not capable of IPI to self (which I found as part of
> debugging this). Subsequent IPIs from other cores to this core get elided as well
> due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()
> 
> There are ways to use a different h/w mechanism to solve the trigger issue and I'd
> hoped to just implement arch_irq_work_raise(). But the trouble is the call stack
> for this issue: IPI to self is triggered from
> 
> sys_sched_setscheduler
>    __balance_callback
>        pull_rt_task
>          irq_work_queue_on  <-- called with @cpu == self
> 
> Looking into irq_work.c, irq_work_queue() is what is semantically needed,
> specifically arch_irq_work_raise() will not be called, which means I need
> arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
> expected from arch code....

What I actually meant was is it OK for irq_work_queue_on() to be called locally
(is this a sched bug/optimization(. Further if it is OK to be called, does it need
to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
arch_send_call_function_single_ipi() is expected to handle sending IPI to self !

> 
> Just wanted to understand before writing patches...
> 
> Test case triggering is harmless looking LTP: trace_sched -c 1
> It is kind of scheduler fizzer as it triggers a whole bunch of sched activity.
> 
> Thx,
> -Vineet
> 

Re: Interesting csd deadlock on ARC

[Peter Zijlstra]

On Tue, Feb 23, 2016 at 10:51:42AM +0530, Vineet Gupta wrote:
> On Friday 19 February 2016 12:17 PM, Vineet Gupta wrote:
> > Hi Peter,
> > 
> > I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
> > turned out to be lot more interesting than I'd hoped for. This is stock v4.4
> > 
> > Trouble starts with an IPI to self which doesn't get delivered as the inter-core
> > interrupt providing h/w is not capable of IPI to self (which I found as part of
> > debugging this). Subsequent IPIs from other cores to this core get elided as well
> > due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()
> > 
> > There are ways to use a different h/w mechanism to solve the trigger issue and I'd
> > hoped to just implement arch_irq_work_raise(). 

Yes, there are other architectures that use other means for self-IPI,
IIRC PowerPC has to program their timer in the past to generate a local
interrupt.

> > But the trouble is the call stack
> > for this issue: IPI to self is triggered from
> > 
> > sys_sched_setscheduler
> >    __balance_callback
> >        pull_rt_task
> >          irq_work_queue_on  <-- called with @cpu == self
> > 
> > Looking into irq_work.c, irq_work_queue() is what is semantically needed,
> > specifically arch_irq_work_raise() will not be called, which means I need
> > arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
> > expected from arch code....
> 
> What I actually meant was is it OK for irq_work_queue_on() to be called locally
> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !

Right, so I'm not actually sure we started out with this requirement.
But you're not the first to run into this, see:

  lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg@mail.gmail.com

Initially I think irq_work_queue_on() was only used remotely, but I
think it makes sense to allow the current cpu, esp. since people seem to
be using it like that.

Now the distinct difference between arch_irq_work_raise() and
arch_send_call_function_single_ipi() is that arch_irq_work_raise()
should be NMI-safe.

So on x86 it has to be extra careful about the lapic state, whereas the
regular IPI code doesn't.

I seem to have forgotten the status of NMIs on ARC, but this is
something to make a note of.

Re: Interesting csd deadlock on ARC

[Peter Zijlstra]

On Tue, Feb 23, 2016 at 10:51:42AM +0530, Vineet Gupta wrote:
> On Friday 19 February 2016 12:17 PM, Vineet Gupta wrote:
> > Hi Peter,
> > 
> > I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
> > turned out to be lot more interesting than I'd hoped for. This is stock v4.4
> > 
> > Trouble starts with an IPI to self which doesn't get delivered as the inter-core
> > interrupt providing h/w is not capable of IPI to self (which I found as part of
> > debugging this). Subsequent IPIs from other cores to this core get elided as well
> > due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()
> > 
> > There are ways to use a different h/w mechanism to solve the trigger issue and I'd
> > hoped to just implement arch_irq_work_raise(). 

Yes, there are other architectures that use other means for self-IPI,
IIRC PowerPC has to program their timer in the past to generate a local
interrupt.

> > But the trouble is the call stack
> > for this issue: IPI to self is triggered from
> > 
> > sys_sched_setscheduler
> >    __balance_callback
> >        pull_rt_task
> >          irq_work_queue_on  <-- called with @cpu == self
> > 
> > Looking into irq_work.c, irq_work_queue() is what is semantically needed,
> > specifically arch_irq_work_raise() will not be called, which means I need
> > arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
> > expected from arch code....
> 
> What I actually meant was is it OK for irq_work_queue_on() to be called locally
> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !

Right, so I'm not actually sure we started out with this requirement.
But you're not the first to run into this, see:

  lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg@mail.gmail.com

Initially I think irq_work_queue_on() was only used remotely, but I
think it makes sense to allow the current cpu, esp. since people seem to
be using it like that.

Now the distinct difference between arch_irq_work_raise() and
arch_send_call_function_single_ipi() is that arch_irq_work_raise()
should be NMI-safe.

So on x86 it has to be extra careful about the lapic state, whereas the
regular IPI code doesn't.

I seem to have forgotten the status of NMIs on ARC, but this is
something to make a note of.

Interesting csd deadlock on ARC

[Peter Zijlstra]

On Tue, Feb 23, 2016@10:51:42AM +0530, Vineet Gupta wrote:
> On Friday 19 February 2016 12:17 PM, Vineet Gupta wrote:
> > Hi Peter,
> > 
> > I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
> > turned out to be lot more interesting than I'd hoped for. This is stock v4.4
> > 
> > Trouble starts with an IPI to self which doesn't get delivered as the inter-core
> > interrupt providing h/w is not capable of IPI to self (which I found as part of
> > debugging this). Subsequent IPIs from other cores to this core get elided as well
> > due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()
> > 
> > There are ways to use a different h/w mechanism to solve the trigger issue and I'd
> > hoped to just implement arch_irq_work_raise(). 

Yes, there are other architectures that use other means for self-IPI,
IIRC PowerPC has to program their timer in the past to generate a local
interrupt.

> > But the trouble is the call stack
> > for this issue: IPI to self is triggered from
> > 
> > sys_sched_setscheduler
> >    __balance_callback
> >        pull_rt_task
> >          irq_work_queue_on  <-- called with @cpu == self
> > 
> > Looking into irq_work.c, irq_work_queue() is what is semantically needed,
> > specifically arch_irq_work_raise() will not be called, which means I need
> > arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
> > expected from arch code....
> 
> What I actually meant was is it OK for irq_work_queue_on() to be called locally
> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !

Right, so I'm not actually sure we started out with this requirement.
But you're not the first to run into this, see:

  lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg at mail.gmail.com

Initially I think irq_work_queue_on() was only used remotely, but I
think it makes sense to allow the current cpu, esp. since people seem to
be using it like that.

Now the distinct difference between arch_irq_work_raise() and
arch_send_call_function_single_ipi() is that arch_irq_work_raise()
should be NMI-safe.

So on x86 it has to be extra careful about the lapic state, whereas the
regular IPI code doesn't.

I seem to have forgotten the status of NMIs on ARC, but this is
something to make a note of.

Re: Interesting csd deadlock on ARC

[Vineet Gupta]

>> What I actually meant was is it OK for irq_work_queue_on() to be called locally
>> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
>> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
>> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
> 
> Right, so I'm not actually sure we started out with this requirement.
> But you're not the first to run into this, see:
> 
>   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg@mail.gmail.com

Thx for the link, very helpful. I've posted fix for ARC which uses software
interrupt and is thus UP/SMP safe.

> Initially I think irq_work_queue_on() was only used remotely, but I
> think it makes sense to allow the current cpu, esp. since people seem to
> be using it like that.
> 
> Now the distinct difference between arch_irq_work_raise() and
> arch_send_call_function_single_ipi() is that arch_irq_work_raise()
> should be NMI-safe.

Ok - so when I implement interrupt priorities (aka NMI for ARC), this needs to be
highest.

> 
> So on x86 it has to be extra careful about the lapic state, whereas the
> regular IPI code doesn't.
> 
> I seem to have forgotten the status of NMIs on ARC, but this is
> something to make a note of.

Not had a chance to go back to it since we last discussed.
I've just been swamped with bug fixing like this one :-(

Thx,
-Vineet

Interesting csd deadlock on ARC

[Vineet Gupta]

>> What I actually meant was is it OK for irq_work_queue_on() to be called locally
>> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
>> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
>> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
> 
> Right, so I'm not actually sure we started out with this requirement.
> But you're not the first to run into this, see:
> 
>   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg at mail.gmail.com

Thx for the link, very helpful. I've posted fix for ARC which uses software
interrupt and is thus UP/SMP safe.

> Initially I think irq_work_queue_on() was only used remotely, but I
> think it makes sense to allow the current cpu, esp. since people seem to
> be using it like that.
> 
> Now the distinct difference between arch_irq_work_raise() and
> arch_send_call_function_single_ipi() is that arch_irq_work_raise()
> should be NMI-safe.

Ok - so when I implement interrupt priorities (aka NMI for ARC), this needs to be
highest.

> 
> So on x86 it has to be extra careful about the lapic state, whereas the
> regular IPI code doesn't.
> 
> I seem to have forgotten the status of NMIs on ARC, but this is
> something to make a note of.

Not had a chance to go back to it since we last discussed.
I've just been swamped with bug fixing like this one :-(

Thx,
-Vineet

Re: Interesting csd deadlock on ARC

[Peter Zijlstra]

On Tue, Feb 23, 2016 at 03:51:23PM +0530, Vineet Gupta wrote:
> > 
> > Now the distinct difference between arch_irq_work_raise() and
> > arch_send_call_function_single_ipi() is that arch_irq_work_raise()
> > should be NMI-safe.
> 
> Ok - so when I implement interrupt priorities (aka NMI for ARC), this needs to be
> highest.

So on x86 the issue is that the NMI can interrupt someone else writing
to the lapic. So there's a bit of extra care to be taken.

If your platform doesn't suffer such issues, then that should be fine.

The only requirement for irq_work is that it runs after the NMI
completes and runs from regular IRQ context. There are no strict
interrupt priority requirements, only that it happens.

> > I seem to have forgotten the status of NMIs on ARC, but this is
> > something to make a note of.
> 
> Not had a chance to go back to it since we last discussed.
> I've just been swamped with bug fixing like this one :-(

Yeah, I'm familiar with the problem ...

Interesting csd deadlock on ARC

[Peter Zijlstra]

On Tue, Feb 23, 2016@03:51:23PM +0530, Vineet Gupta wrote:
> > 
> > Now the distinct difference between arch_irq_work_raise() and
> > arch_send_call_function_single_ipi() is that arch_irq_work_raise()
> > should be NMI-safe.
> 
> Ok - so when I implement interrupt priorities (aka NMI for ARC), this needs to be
> highest.

So on x86 the issue is that the NMI can interrupt someone else writing
to the lapic. So there's a bit of extra care to be taken.

If your platform doesn't suffer such issues, then that should be fine.

The only requirement for irq_work is that it runs after the NMI
completes and runs from regular IRQ context. There are no strict
interrupt priority requirements, only that it happens.

> > I seem to have forgotten the status of NMIs on ARC, but this is
> > something to make a note of.
> 
> Not had a chance to go back to it since we last discussed.
> I've just been swamped with bug fixing like this one :-(

Yeah, I'm familiar with the problem ...

RE: Interesting csd deadlock on ARC

[Noam Camus]

>From: Peter Zijlstra [mailto:peterz@infradead.org] 
>Sent: Tuesday, February 23, 2016 12:40 PM

>The only requirement for irq_work is that it runs after the NMI completes and runs from regular IRQ context. >There are no strict interrupt priority requirements, only that it happens.

We here already use self IPI and irq_work on ARC (with no NMI concerns).
Please see patch at:
https://github.com/EZchip/linux/commit/e42556738e610295f917c79dae166373cd11de88

Noam

RE: Interesting csd deadlock on ARC

[Noam Camus]

>From: Peter Zijlstra [mailto:peterz@infradead.org] 
>Sent: Tuesday, February 23, 2016 12:40 PM

>The only requirement for irq_work is that it runs after the NMI completes and runs from regular IRQ context. >There are no strict interrupt priority requirements, only that it happens.

We here already use self IPI and irq_work on ARC (with no NMI concerns).
Please see patch at:
https://github.com/EZchip/linux/commit/e42556738e610295f917c79dae166373cd11de88

Noam

Interesting csd deadlock on ARC

[Noam Camus]

>From: Peter Zijlstra [mailto:peterz at infradead.org] 
>Sent: Tuesday, February 23, 2016 12:40 PM

>The only requirement for irq_work is that it runs after the NMI completes and runs from regular IRQ context. >There are no strict interrupt priority requirements, only that it happens.

We here already use self IPI and irq_work on ARC (with no NMI concerns).
Please see patch at:
https://github.com/EZchip/linux/commit/e42556738e610295f917c79dae166373cd11de88

Noam

Re: Interesting csd deadlock on ARC

[Vineet Gupta]

On Tuesday 23 February 2016 04:28 PM, Noam Camus wrote:
>> From: Peter Zijlstra [mailto:peterz@infradead.org] 
>> Sent: Tuesday, February 23, 2016 12:40 PM
> 
>> The only requirement for irq_work is that it runs after the NMI completes and runs from regular IRQ context. >There are no strict interrupt priority requirements, only that it happens.
> 
> We here already use self IPI and irq_work on ARC (with no NMI concerns).
> Please see patch at:
> https://github.com/EZchip/linux/commit/e42556738e610295f917c79dae166373cd11de88

So this relies on 2nd level intc providing the IPI.
For UP we still need the software triggered interrupt provided by core intc.

Interesting csd deadlock on ARC

[Vineet Gupta]

On Tuesday 23 February 2016 04:28 PM, Noam Camus wrote:
>> From: Peter Zijlstra [mailto:peterz at infradead.org] 
>> Sent: Tuesday, February 23, 2016 12:40 PM
> 
>> The only requirement for irq_work is that it runs after the NMI completes and runs from regular IRQ context. >There are no strict interrupt priority requirements, only that it happens.
> 
> We here already use self IPI and irq_work on ARC (with no NMI concerns).
> Please see patch at:
> https://github.com/EZchip/linux/commit/e42556738e610295f917c79dae166373cd11de88

So this relies on 2nd level intc providing the IPI.
For UP we still need the software triggered interrupt provided by core intc.

Re: Interesting csd deadlock on ARC

[Vineet Gupta]

On Tuesday 23 February 2016 03:28 PM, Peter Zijlstra wrote:
> On Tue, Feb 23, 2016 at 10:51:42AM +0530, Vineet Gupta wrote:
>> On Friday 19 February 2016 12:17 PM, Vineet Gupta wrote:
>>> Hi Peter,
>>>
>>> I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
>>> turned out to be lot more interesting than I'd hoped for. This is stock v4.4
>>>
>>> Trouble starts with an IPI to self which doesn't get delivered as the inter-core
>>> interrupt providing h/w is not capable of IPI to self (which I found as part of
>>> debugging this). Subsequent IPIs from other cores to this core get elided as well
>>> due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()
>>>
>>> There are ways to use a different h/w mechanism to solve the trigger issue and I'd
>>> hoped to just implement arch_irq_work_raise(). 
> 
> Yes, there are other architectures that use other means for self-IPI,
> IIRC PowerPC has to program their timer in the past to generate a local
> interrupt.
> 
>>> But the trouble is the call stack
>>> for this issue: IPI to self is triggered from
>>>
>>> sys_sched_setscheduler
>>>    __balance_callback
>>>        pull_rt_task
>>>          irq_work_queue_on  <-- called with @cpu == self
>>>
>>> Looking into irq_work.c, irq_work_queue() is what is semantically needed,
>>> specifically arch_irq_work_raise() will not be called, which means I need
>>> arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
>>> expected from arch code....
>>
>> What I actually meant was is it OK for irq_work_queue_on() to be called locally
>> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
>> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
>> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
> 
> Right, so I'm not actually sure we started out with this requirement.
> But you're not the first to run into this, see:
> 
>   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg@mail.gmail.com
> 
> Initially I think irq_work_queue_on() was only used remotely, but I
> think it makes sense to allow the current cpu, esp. since people seem to
> be using it like that.

So it seems Russell's questions in the thread above stands still. IMO we need to
massage irq_work_queue_on() to handle the case of called for local cpu. This will
automatically take care of CONFIG_SMP kernel running on UP hardware.

> 
> Now the distinct difference between arch_irq_work_raise() and
> arch_send_call_function_single_ipi() is that arch_irq_work_raise()
> should be NMI-safe.
> 
> So on x86 it has to be extra careful about the lapic state, whereas the
> regular IPI code doesn't.
> 
> I seem to have forgotten the status of NMIs on ARC, but this is
> something to make a note of.
> 

Interesting csd deadlock on ARC

[Vineet Gupta]

On Tuesday 23 February 2016 03:28 PM, Peter Zijlstra wrote:
> On Tue, Feb 23, 2016@10:51:42AM +0530, Vineet Gupta wrote:
>> On Friday 19 February 2016 12:17 PM, Vineet Gupta wrote:
>>> Hi Peter,
>>>
>>> I've been debugging a csd_lock_wait() deadlock on SMP+PREEMPT ARC HS38x2 and it
>>> turned out to be lot more interesting than I'd hoped for. This is stock v4.4
>>>
>>> Trouble starts with an IPI to self which doesn't get delivered as the inter-core
>>> interrupt providing h/w is not capable of IPI to self (which I found as part of
>>> debugging this). Subsequent IPIs from other cores to this core get elided as well
>>> due to the IPI coalescing optimization in arch/arc/kernel/smp.c: ipi_send_msg_one()
>>>
>>> There are ways to use a different h/w mechanism to solve the trigger issue and I'd
>>> hoped to just implement arch_irq_work_raise(). 
> 
> Yes, there are other architectures that use other means for self-IPI,
> IIRC PowerPC has to program their timer in the past to generate a local
> interrupt.
> 
>>> But the trouble is the call stack
>>> for this issue: IPI to self is triggered from
>>>
>>> sys_sched_setscheduler
>>>    __balance_callback
>>>        pull_rt_task
>>>          irq_work_queue_on  <-- called with @cpu == self
>>>
>>> Looking into irq_work.c, irq_work_queue() is what is semantically needed,
>>> specifically arch_irq_work_raise() will not be called, which means I need
>>> arch_send_call_function_single_ipi() to be able to IPI to self cpu also. Is that
>>> expected from arch code....
>>
>> What I actually meant was is it OK for irq_work_queue_on() to be called locally
>> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
>> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
>> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
> 
> Right, so I'm not actually sure we started out with this requirement.
> But you're not the first to run into this, see:
> 
>   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg at mail.gmail.com
> 
> Initially I think irq_work_queue_on() was only used remotely, but I
> think it makes sense to allow the current cpu, esp. since people seem to
> be using it like that.

So it seems Russell's questions in the thread above stands still. IMO we need to
massage irq_work_queue_on() to handle the case of called for local cpu. This will
automatically take care of CONFIG_SMP kernel running on UP hardware.

> 
> Now the distinct difference between arch_irq_work_raise() and
> arch_send_call_function_single_ipi() is that arch_irq_work_raise()
> should be NMI-safe.
> 
> So on x86 it has to be extra careful about the lapic state, whereas the
> regular IPI code doesn't.
> 
> I seem to have forgotten the status of NMIs on ARC, but this is
> something to make a note of.
> 

Re: Interesting csd deadlock on ARC

[Peter Zijlstra]

On Wed, Feb 24, 2016 at 10:21:25AM +0530, Vineet Gupta wrote:
> >> What I actually meant was is it OK for irq_work_queue_on() to be called locally
> >> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
> >> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
> >> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
> > 
> > Right, so I'm not actually sure we started out with this requirement.
> > But you're not the first to run into this, see:
> > 
> >   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg@mail.gmail.com
> > 
> > Initially I think irq_work_queue_on() was only used remotely, but I
> > think it makes sense to allow the current cpu, esp. since people seem to
> > be using it like that.
> 
> So it seems Russell's questions in the thread above stands still. IMO we need to
> massage irq_work_queue_on() to handle the case of called for local cpu. This will
> automatically take care of CONFIG_SMP kernel running on UP hardware.

Hmm, I missed that there was still an open question.

Afaict the only thing that needs doing to the generic code is drop the
CONFIG_SMP guard, no?

Re: Interesting csd deadlock on ARC

[Peter Zijlstra]

On Wed, Feb 24, 2016 at 10:21:25AM +0530, Vineet Gupta wrote:
> >> What I actually meant was is it OK for irq_work_queue_on() to be called locally
> >> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
> >> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
> >> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
> > 
> > Right, so I'm not actually sure we started out with this requirement.
> > But you're not the first to run into this, see:
> > 
> >   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg@mail.gmail.com
> > 
> > Initially I think irq_work_queue_on() was only used remotely, but I
> > think it makes sense to allow the current cpu, esp. since people seem to
> > be using it like that.
> 
> So it seems Russell's questions in the thread above stands still. IMO we need to
> massage irq_work_queue_on() to handle the case of called for local cpu. This will
> automatically take care of CONFIG_SMP kernel running on UP hardware.

Hmm, I missed that there was still an open question.

Afaict the only thing that needs doing to the generic code is drop the
CONFIG_SMP guard, no?

Interesting csd deadlock on ARC

[Peter Zijlstra]

On Wed, Feb 24, 2016@10:21:25AM +0530, Vineet Gupta wrote:
> >> What I actually meant was is it OK for irq_work_queue_on() to be called locally
> >> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
> >> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
> >> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
> > 
> > Right, so I'm not actually sure we started out with this requirement.
> > But you're not the first to run into this, see:
> > 
> >   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg at mail.gmail.com
> > 
> > Initially I think irq_work_queue_on() was only used remotely, but I
> > think it makes sense to allow the current cpu, esp. since people seem to
> > be using it like that.
> 
> So it seems Russell's questions in the thread above stands still. IMO we need to
> massage irq_work_queue_on() to handle the case of called for local cpu. This will
> automatically take care of CONFIG_SMP kernel running on UP hardware.

Hmm, I missed that there was still an open question.

Afaict the only thing that needs doing to the generic code is drop the
CONFIG_SMP guard, no?

Re: Interesting csd deadlock on ARC

[Vineet Gupta]

On Thursday 25 February 2016 07:36 PM, Peter Zijlstra wrote:
> On Wed, Feb 24, 2016 at 10:21:25AM +0530, Vineet Gupta wrote:
>>>> What I actually meant was is it OK for irq_work_queue_on() to be called locally
>>>> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
>>>> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
>>>> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
>>>
>>> Right, so I'm not actually sure we started out with this requirement.
>>> But you're not the first to run into this, see:
>>>
>>>   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg@mail.gmail.com
>>>
>>> Initially I think irq_work_queue_on() was only used remotely, but I
>>> think it makes sense to allow the current cpu, esp. since people seem to
>>> be using it like that.
>>
>> So it seems Russell's questions in the thread above stands still. IMO we need to
>> massage irq_work_queue_on() to handle the case of called for local cpu. This will
>> automatically take care of CONFIG_SMP kernel running on UP hardware.
> 
> Hmm, I missed that there was still an open question.
> 
> Afaict the only thing that needs doing to the generic code is drop the
> CONFIG_SMP guard, no?

But then ARM CONFIG_SMP on UP hardware will still crap out because there is no way
to send IPI to self. Same as the bug in above discussion. I'm surprised they way
ARM guys worked around it.

But yeah what you propose needs to be done and additionally make
irq_work_queue_on() behave like irq_work_queue() if @cpu == smp_processor_id(). So
the IPI will not be involved at all. But that means arch has to implement
arch_irq_work_raise() or maybe not - per comment there "Lame architectures will
get the timer tick callback" !

This would be a cleaner solution IMHO !

Interesting csd deadlock on ARC

[Vineet Gupta]

On Thursday 25 February 2016 07:36 PM, Peter Zijlstra wrote:
> On Wed, Feb 24, 2016@10:21:25AM +0530, Vineet Gupta wrote:
>>>> What I actually meant was is it OK for irq_work_queue_on() to be called locally
>>>> (is this a sched bug/optimization(. Further if it is OK to be called, does it need
>>>> to do behave more like irq_work_queue() i.e. call arch_irq_work_raise() or
>>>> arch_send_call_function_single_ipi() is expected to handle sending IPI to self !
>>>
>>> Right, so I'm not actually sure we started out with this requirement.
>>> But you're not the first to run into this, see:
>>>
>>>   lkml.kernel.org/r/CAJZ5v0gLankSuziQq25qTCyNqeOX43yD9jnJu_XXwbdyajfmKg at mail.gmail.com
>>>
>>> Initially I think irq_work_queue_on() was only used remotely, but I
>>> think it makes sense to allow the current cpu, esp. since people seem to
>>> be using it like that.
>>
>> So it seems Russell's questions in the thread above stands still. IMO we need to
>> massage irq_work_queue_on() to handle the case of called for local cpu. This will
>> automatically take care of CONFIG_SMP kernel running on UP hardware.
> 
> Hmm, I missed that there was still an open question.
> 
> Afaict the only thing that needs doing to the generic code is drop the
> CONFIG_SMP guard, no?

But then ARM CONFIG_SMP on UP hardware will still crap out because there is no way
to send IPI to self. Same as the bug in above discussion. I'm surprised they way
ARM guys worked around it.

But yeah what you propose needs to be done and additionally make
irq_work_queue_on() behave like irq_work_queue() if @cpu == smp_processor_id(). So
the IPI will not be involved at all. But that means arch has to implement
arch_irq_work_raise() or maybe not - per comment there "Lame architectures will
get the timer tick callback" !

This would be a cleaner solution IMHO !

Re: Interesting csd deadlock on ARC

[Russell King - ARM Linux]

On Thu, Feb 25, 2016 at 07:53:39PM +0530, Vineet Gupta wrote:
> But then ARM CONFIG_SMP on UP hardware will still crap out because there
> is no way to send IPI to self. Same as the bug in above discussion. I'm
> surprised they way ARM guys worked around it.

We haven't worked around it - the code which provoked the oops that was
seen (in the cpufreq code) was changed not to call it, which has the
effect of making the problem "go away", at least for now.

We still have the problem that if it does get called on UP, it'll blow
up - and I don't see any point in complicating the code for something
that never happens right now.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Interesting csd deadlock on ARC

[Russell King - ARM Linux]

On Thu, Feb 25, 2016@07:53:39PM +0530, Vineet Gupta wrote:
> But then ARM CONFIG_SMP on UP hardware will still crap out because there
> is no way to send IPI to self. Same as the bug in above discussion. I'm
> surprised they way ARM guys worked around it.

We haven't worked around it - the code which provoked the oops that was
seen (in the cpufreq code) was changed not to call it, which has the
effect of making the problem "go away", at least for now.

We still have the problem that if it does get called on UP, it'll blow
up - and I don't see any point in complicating the code for something
that never happens right now.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Interesting csd deadlock on ARC

[Vineet Gupta]

On Thursday 25 February 2016 08:00 PM, Russell King - ARM Linux wrote:
> On Thu, Feb 25, 2016 at 07:53:39PM +0530, Vineet Gupta wrote:
>> But then ARM CONFIG_SMP on UP hardware will still crap out because there
>> is no way to send IPI to self. Same as the bug in above discussion. I'm
>> surprised they way ARM guys worked around it.
> 
> We haven't worked around it - the code which provoked the oops that was
> seen (in the cpufreq code) was changed not to call it, which has the
> effect of making the problem "go away", at least for now.
> 
> We still have the problem that if it does get called on UP, it'll blow
> up - and I don't see any point in complicating the code for something
> that never happens right now.

Right so my statement "workaround" was technically incorrect. But like you say,
it's a ticking bomb which will certainly go off on your SMP on UP systems the
moment someone adds irq_work_queue_on() in some obscure corner of generic code.

And I think this merits fixing in generic code !

-Vineet

Interesting csd deadlock on ARC

[Vineet Gupta]

On Thursday 25 February 2016 08:00 PM, Russell King - ARM Linux wrote:
> On Thu, Feb 25, 2016@07:53:39PM +0530, Vineet Gupta wrote:
>> But then ARM CONFIG_SMP on UP hardware will still crap out because there
>> is no way to send IPI to self. Same as the bug in above discussion. I'm
>> surprised they way ARM guys worked around it.
> 
> We haven't worked around it - the code which provoked the oops that was
> seen (in the cpufreq code) was changed not to call it, which has the
> effect of making the problem "go away", at least for now.
> 
> We still have the problem that if it does get called on UP, it'll blow
> up - and I don't see any point in complicating the code for something
> that never happens right now.

Right so my statement "workaround" was technically incorrect. But like you say,
it's a ticking bomb which will certainly go off on your SMP on UP systems the
moment someone adds irq_work_queue_on() in some obscure corner of generic code.

And I think this merits fixing in generic code !

-Vineet