[patch 1/1] genirq: Disable interrupts for force threaded handlers

[patch 1/1] genirq: Disable interrupts for force threaded handlers

[Thomas Gleixner]

With interrupt force threading all device interrupt handlers are invoked
from kernel threads. Contrary to hard interrupt context the invocation only
disables bottom halfs, but not interrupts. This was an oversight back then
because any code like this will have an issue:

thread(irq_A)
  irq_handler(A)
    spin_lock(&foo->lock);

interrupt(irq_B)
  irq_handler(B)
    spin_lock(&foo->lock);

This has been triggered with networking (NAPI vs. hrtimers) and console
drivers where printk() happens from an interrupt which interrupted the
force threaded handler.

Now people noticed and started to change the spin_lock() in the handler to
spin_lock_irqsave() which affects performance or add IRQF_NOTHREAD to the
interrupt request which in turn breaks RT.

Fix the root cause and not the symptom and disable interrupts before
invoking the force threaded handler which preserves the regular semantics
and the usefulness of the interrupt force threading as a general debugging
tool.

For not RT this is not changing much, except that during the execution of
the threaded handler interrupts are delayed until the handler
returns. Vs. scheduling and softirq processing there is no difference.

For RT kernels there is no issue.

Fixes: 8d32a307e4fa ("genirq: Provide forced interrupt threading")
Reported-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: netdev <netdev@vger.kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
CC: Peter Zijlstra <peterz@infradead.org>
Cc: linux-serial@vger.kernel.org
Cc: netdev <netdev@vger.kernel.org>
---
 kernel/irq/manage.c |    4 ++++
 1 file changed, 4 insertions(+)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1142,11 +1142,15 @@ irq_forced_thread_fn(struct irq_desc *de
 	irqreturn_t ret;
 
 	local_bh_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_disable();
 	ret = action->thread_fn(action->irq, action->dev_id);
 	if (ret == IRQ_HANDLED)
 		atomic_inc(&desc->threads_handled);
 
 	irq_finalize_oneshot(desc, action);
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_enable();
 	local_bh_enable();
 	return ret;
 }

Re: [patch 1/1] genirq: Disable interrupts for force threaded handlers

[Sebastian Andrzej Siewior]

On 2021-03-17 15:38:52 [+0100], Thomas Gleixner wrote:
> With interrupt force threading all device interrupt handlers are invoked
> from kernel threads. Contrary to hard interrupt context the invocation only
> disables bottom halfs, but not interrupts. This was an oversight back then
> because any code like this will have an issue:
> 
> thread(irq_A)
>   irq_handler(A)
>     spin_lock(&foo->lock);
> 
> interrupt(irq_B)
>   irq_handler(B)
>     spin_lock(&foo->lock);

It will not because both threads will wake_up(thread). It is an issue if
- if &foo->lock is shared between a hrtimer and threaded-IRQ
- if &foo->lock is shared between a non-threaded and thread-IRQ
- if &foo->lock is shared between a printk() in hardirq context and
  thread-IRQ as I learned today.

> This has been triggered with networking (NAPI vs. hrtimers) and console
> drivers where printk() happens from an interrupt which interrupted the
> force threaded handler.
> 
> Now people noticed and started to change the spin_lock() in the handler to
> spin_lock_irqsave() which affects performance or add IRQF_NOTHREAD to the
> interrupt request which in turn breaks RT.
> 
> Fix the root cause and not the symptom and disable interrupts before
> invoking the force threaded handler which preserves the regular semantics
> and the usefulness of the interrupt force threading as a general debugging
> tool.
> 
> For not RT this is not changing much, except that during the execution of
> the threaded handler interrupts are delayed until the handler
> returns. Vs. scheduling and softirq processing there is no difference.
> 
> For RT kernels there is no issue.

Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> Fixes: 8d32a307e4fa ("genirq: Provide forced interrupt threading")
> Reported-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: netdev <netdev@vger.kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-serial@vger.kernel.org
> Cc: netdev <netdev@vger.kernel.org>
> ---
>  kernel/irq/manage.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1142,11 +1142,15 @@ irq_forced_thread_fn(struct irq_desc *de
>  	irqreturn_t ret;
>  
>  	local_bh_disable();
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +		local_irq_disable();
>  	ret = action->thread_fn(action->irq, action->dev_id);
>  	if (ret == IRQ_HANDLED)
>  		atomic_inc(&desc->threads_handled);
>  
>  	irq_finalize_oneshot(desc, action);
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +		local_irq_enable();
>  	local_bh_enable();
>  	return ret;
>  }

Sebastian

Re: [patch 1/1] genirq: Disable interrupts for force threaded handlers

[Thomas Gleixner]

On Wed, Mar 17 2021 at 15:48, Sebastian Andrzej Siewior wrote:
> On 2021-03-17 15:38:52 [+0100], Thomas Gleixner wrote:
>> thread(irq_A)
>>   irq_handler(A)
>>     spin_lock(&foo->lock);
>> 
>> interrupt(irq_B)
>>   irq_handler(B)
>>     spin_lock(&foo->lock);
>
> It will not because both threads will wake_up(thread). It is an issue if
> - if &foo->lock is shared between a hrtimer and threaded-IRQ
> - if &foo->lock is shared between a non-threaded and thread-IRQ
> - if &foo->lock is shared between a printk() in hardirq context and
>   thread-IRQ as I learned today.

That's the point and it's entirely clear from the above: A is thread
context and B is hard interrupt context and if the lock is shared then
it's busted. Otherwise we would not have this discussion at all.

Re: [patch 1/1] genirq: Disable interrupts for force threaded handlers

[Johan Hovold]

On Wed, Mar 17, 2021 at 03:48:06PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-17 15:38:52 [+0100], Thomas Gleixner wrote:
> > With interrupt force threading all device interrupt handlers are invoked
> > from kernel threads. Contrary to hard interrupt context the invocation only
> > disables bottom halfs, but not interrupts. This was an oversight back then
> > because any code like this will have an issue:
> > 
> > thread(irq_A)
> >   irq_handler(A)
> >     spin_lock(&foo->lock);
> > 
> > interrupt(irq_B)
> >   irq_handler(B)
> >     spin_lock(&foo->lock);
> 
> It will not because both threads will wake_up(thread).

Note that the above says "interrupt(irq_B)" suggesting it's a
non-threaded interrupt unlike irq_A.

> It is an issue if
> - if &foo->lock is shared between a hrtimer and threaded-IRQ
> - if &foo->lock is shared between a non-threaded and thread-IRQ

So this is the above case.

> - if &foo->lock is shared between a printk() in hardirq context and
>   thread-IRQ as I learned today.

But generally it's any lock taken by a threaded handler that can end up
being taken in hard interrupt context.

> > This has been triggered with networking (NAPI vs. hrtimers) and console
> > drivers where printk() happens from an interrupt which interrupted the
> > force threaded handler.
> > 
> > Now people noticed and started to change the spin_lock() in the handler to
> > spin_lock_irqsave() which affects performance or add IRQF_NOTHREAD to the
> > interrupt request which in turn breaks RT.
> >
> > Fix the root cause and not the symptom and disable interrupts before
> > invoking the force threaded handler which preserves the regular semantics
> > and the usefulness of the interrupt force threading as a general debugging
> > tool.
> > 
> > For not RT this is not changing much, except that during the execution of
> > the threaded handler interrupts are delayed until the handler
> > returns. Vs. scheduling and softirq processing there is no difference.
> > 
> > For RT kernels there is no issue.
> 
> Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Johan Hovold <johan@kernel.org>

> > Fixes: 8d32a307e4fa ("genirq: Provide forced interrupt threading")
> > Reported-by: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Cc: netdev <netdev@vger.kernel.org>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > CC: Peter Zijlstra <peterz@infradead.org>
> > Cc: linux-serial@vger.kernel.org
> > Cc: netdev <netdev@vger.kernel.org>
> > ---
> >  kernel/irq/manage.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -1142,11 +1142,15 @@ irq_forced_thread_fn(struct irq_desc *de
> >  	irqreturn_t ret;
> >  
> >  	local_bh_disable();
> > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> > +		local_irq_disable();
> >  	ret = action->thread_fn(action->irq, action->dev_id);
> >  	if (ret == IRQ_HANDLED)
> >  		atomic_inc(&desc->threads_handled);
> >  
> >  	irq_finalize_oneshot(desc, action);
> > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> > +		local_irq_enable();
> >  	local_bh_enable();
> >  	return ret;
> >  }

Johan

Re: [patch 1/1] genirq: Disable interrupts for force threaded handlers

[Sebastian Andrzej Siewior]

On 2021-03-17 17:23:39 [+0100], Johan Hovold wrote:
> > > thread(irq_A)
> > >   irq_handler(A)
> > >     spin_lock(&foo->lock);
> > > 
> > > interrupt(irq_B)
> > >   irq_handler(B)
> > >     spin_lock(&foo->lock);
> > 
> > It will not because both threads will wake_up(thread).
> 
> Note that the above says "interrupt(irq_B)" suggesting it's a
> non-threaded interrupt unlike irq_A.

I missed that bit, thanks.

Sebastian

[tip: irq/urgent] genirq: Disable interrupts for force threaded handlers

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     81e2073c175b887398e5bca6c004efa89983f58d
Gitweb:        https://git.kernel.org/tip/81e2073c175b887398e5bca6c004efa89983f58d
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 17 Mar 2021 15:38:52 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sun, 21 Mar 2021 00:17:52 +01:00

genirq: Disable interrupts for force threaded handlers

With interrupt force threading all device interrupt handlers are invoked
from kernel threads. Contrary to hard interrupt context the invocation only
disables bottom halfs, but not interrupts. This was an oversight back then
because any code like this will have an issue:

thread(irq_A)
  irq_handler(A)
    spin_lock(&foo->lock);

interrupt(irq_B)
  irq_handler(B)
    spin_lock(&foo->lock);

This has been triggered with networking (NAPI vs. hrtimers) and console
drivers where printk() happens from an interrupt which interrupted the
force threaded handler.

Now people noticed and started to change the spin_lock() in the handler to
spin_lock_irqsave() which affects performance or add IRQF_NOTHREAD to the
interrupt request which in turn breaks RT.

Fix the root cause and not the symptom and disable interrupts before
invoking the force threaded handler which preserves the regular semantics
and the usefulness of the interrupt force threading as a general debugging
tool.

For not RT this is not changing much, except that during the execution of
the threaded handler interrupts are delayed until the handler
returns. Vs. scheduling and softirq processing there is no difference.

For RT kernels there is no issue.

Fixes: 8d32a307e4fa ("genirq: Provide forced interrupt threading")
Reported-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Johan Hovold <johan@kernel.org>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Link: https://lore.kernel.org/r/20210317143859.513307808@linutronix.de

---
 kernel/irq/manage.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index dec3f73..21ea370 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1142,11 +1142,15 @@ irq_forced_thread_fn(struct irq_desc *desc, struct irqaction *action)
 	irqreturn_t ret;
 
 	local_bh_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_disable();
 	ret = action->thread_fn(action->irq, action->dev_id);
 	if (ret == IRQ_HANDLED)
 		atomic_inc(&desc->threads_handled);
 
 	irq_finalize_oneshot(desc, action);
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_enable();
 	local_bh_enable();
 	return ret;
 }