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

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 1 Jul 2013, James Hogan wrote:
> On 1 July 2013 22:51, Thomas Gleixner <tglx@linutronix.de> 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.

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
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.

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.

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

Thanks,

	tglx