Re: [PATCH 4/5] metag: kick: prevent nested kick handlers

[James Hogan]

Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

>On Mon, 1 Jul 2013, James Hogan wrote:
>> On 1 July 2013 22:51, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> > On Mon, 1 Jul 2013, James Hogan wrote:
>> >
>> >> The main kick trigger handler iterates a list of kick handlers and
>calls
>> >> each one. This is done with the kick_handlers_lock spin lock held,
>but
>> >> this causes a problem on SMP where IPIs are implemented with
>kicks. A
>> >> reschedule IPI calls scheduler_ipi() which uses irq_enter() and
>> >> irq_exit(). This results in the scheduler being invoked with
>> >> kick_handlers_lock held which can result in a nested kick trigger
>> >> attempting to acquire the lock, resulting in deadlock.
>> >>
>> >> irq_enter() and irq_exit() can nest, so call them from the main
>kick
>> >> interrupt handler so that softirqs are only handled after
>> >> kick_handlers_lock is released.
>> >
>> > This changelog is confusing. What I decode from the patch is, that
>you
>> > are adding a missing irq_enter/exit pair to the kick_handler, right
>?
>> 
>> Yes. Previously the outermost pair of irq_enter/exit was inside the
>> spin lock critical section (inside scheduler_ipi). so soft-irqs
>> (apparently including the scheduler) would run from irq_exit with the
>> spinlock still held. Now it waits until the new outermost irq_exit(),
>> after the spin lock is released.
>> 
>> I should probably have increased the number of lines of diff context.
>> http://lxr.linux.no/#linux+v3.10/arch/metag/kernel/kick.c#L66
>
>No, you should have written the changelog less confusing. I can see
>the context by just looking at the current and the patched code.

Yes, i agree, sorry it wasn't clear.

>The explanation you provided right now that the outermost pair of
>irq_enter/exit was inside the spin locked section matches the patch
>and makes sense.
>
>So the real issue is, that all of your various interrupts come from

Only kicks, which are the hardware IPI mechanism (in practice only used for SMP IPIs and for communication with non-linux hw threads). Timer/peripheral interrupts come via different handlers.

>the same place, i.e. kick_handler. The kick_handler locks a global
>lock and calls the handlers which have registered. These handlers
>(partially calling generic code) might invoke irq_enter/exit pairs,
>which leads to the underlying problem.
>
>If irq_exit() results in a nest count of hard interrupts = 0, then it
>invokes eventually pending soft interrupts. The softirq callbacks can
>issue an IPI of some sorts and because this IPI will end up in the
>kick handler, the systems livelocks on the already held global lock
>which protects the kick_handler list. This can happen from any invoked
>handler, because the kick_handler is missing an irq_enter/pair
>preventing a kick_handler invocation recursion.

Yes, that's my understanding of the problem.

>
>The obvious fix for now is to add an irq_enter/exit() pair around the
>spin locked section in the kick handler, so irq_exit() invocations of
>subhandlers wont lead to softirq execution.
>
>The more elegant fix would be to replace that global lock which is not
>really scalable with an RCU protected list of subscribed handlers.

Yes, I considered switching it to an RCU list (thanks for confirming that), but this patch looks sensible to me even with rcu, so I figured that can be a separate patch.

>The most elegant fix would be to have separate entry points for
>various exceptions, but that seems to be an architectural issue.

thanks for taking the time to review and for the other acks. i'll rewrite the commit message of this patch and resend.

cheers
James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/