[PATCH] genirq: don't sync irq thread if current happen to be the very irq thread

[PATCH] genirq: don't sync irq thread if current happen to be the very irq thread

[Yong Zhang]

On Thu, May 10, 2012 at 03:17:17PM +0200, Christophe Huriaux wrote:
> 2012/5/9 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> > If you enable CONFIG_KALLSYMS you get a more usable backtrace.
> > Alternatively you can use
> >
> > ? ? ? ?$CROSS_COMPILE-addr2line -e vmlinux 0xc000e90c
> >
> > to get the file and line that resulted in the code at that address.
> >
> 
>   Thanks, I was wondering which config option would enable that. The
> complete backtrace is much more usable :

Actually I don't think this is a -rt issue, you could also trigger this
warning with vanilla if you boot your kernel with 'threadirqs'.

Could you pleaes try the follow patch?

Thanks,
Yong

---
From: Yong Zhang <yong.zhang@windriver.com>
Date: Sun, 20 May 2012 12:56:46 +0800
Subject: [PATCH] genirq: don't sync irq thread if current happen to be the very irq thread

Christophe reported against -rt:
BUG: scheduling while atomic: irq/37-s3c-mci/253/0x00000102
Modules linked in:
[<c000e9fc>] (unwind_backtrace+0x0/0x12c) from [<c029b82c>] (__schedule+0x58/0x2c0)
[<c029b82c>] (__schedule+0x58/0x2c0) from [<c029bc10>] (schedule+0x8c/0xb0)
[<c029bc10>] (schedule+0x8c/0xb0) from [<c0055614>] (synchronize_irq+0xbc/0xd8)
[<c0055614>] (synchronize_irq+0xbc/0xd8) from [<c01db6b0>] (pio_tasklet+0x34/0x11c)
[<c01db6b0>] (pio_tasklet+0x34/0x11c) from [<c0024914>] (__tasklet_action+0x68/0x80)
[<c0024914>] (__tasklet_action+0x68/0x80) from [<c0024ca4>] (__do_softirq+0x88/0x130)
[<c0024ca4>] (__do_softirq+0x88/0x130) from [<c0024ef0>] (do_softirq+0x48/0x54)
[<c0024ef0>] (do_softirq+0x48/0x54) from [<c0025048>] (local_bh_enable+0x8c/0xc0)
[<c0025048>] (local_bh_enable+0x8c/0xc0) from [<c0054678>] (irq_forced_thread_fn+0x4c/0x54)
[<c0054678>] (irq_forced_thread_fn+0x4c/0x54) from [<c0054454>] (irq_thread+0xa0/0x1c0)
[<c0054454>] (irq_thread+0xa0/0x1c0) from [<c0038628>] (kthread+0x84/0x8c)
[<c0038628>] (kthread+0x84/0x8c) from [<c000a100>] (kernel_thread_exit+0x0/0x8)

Whe looking at this issue, I find that there is a typical deadlock
scenario with forced treaded irq,

irq_forced_thread_fn()
  local_bh_enable();
    do_softirq();
      disable_irq();
        synchronize_irq();
          wait_event();
          /*DEAD*/

Cure it by unsync if current happen to be the very irq thread.

Reported-by: Christophe Huriaux <c.huriaux@gmail.com>
Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/manage.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 89a3ea8..d5b96e7 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -41,6 +41,7 @@ early_param("threadirqs", setup_forced_irqthreads);
 void synchronize_irq(unsigned int irq)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
+	struct irqaction *action = desc->action;
 	bool inprogress;
 
 	if (!desc)
@@ -67,7 +68,15 @@ void synchronize_irq(unsigned int irq)
 	/*
 	 * We made sure that no hardirq handler is running. Now verify
 	 * that no threaded handlers are active.
+	 * But for theaded irq, we don't sync if current happens to be
+	 * the irq thread; otherwise we could deadlock.
 	 */
+	while (action) {
+		if (action->thread && action->thread == current)
+			return;
+		action = action->next;
+	}
+
 	wait_event(desc->wait_for_threads, !atomic_read(&desc->threads_active));
 }
 EXPORT_SYMBOL(synchronize_irq);
-- 
1.7.1

[PATCH v2] genirq: don't sync irq thread if current happen to be the very irq thread

[Yong Zhang]

On Sun, May 20, 2012 at 01:27:31PM +0800, Yong Zhang wrote:
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -41,6 +41,7 @@ early_param("threadirqs", setup_forced_irqthreads);
>  void synchronize_irq(unsigned int irq)
>  {
>  	struct irq_desc *desc = irq_to_desc(irq);
> +	struct irqaction *action = desc->action;

Bad time for dereferencing *action.

---
From: Yong Zhang <yong.zhang@windriver.com>
Date: Sun, 20 May 2012 12:56:46 +0800
Subject: [PATCH v2] genirq: don't sync irq thread if current happen to be the very irq thread

Christophe reported against -rt http://marc.info/?l=linux-rt-users&m=133665600214984&w=2
BUG: scheduling while atomic: irq/37-s3c-mci/253/0x00000102
Modules linked in:
[<c000e9fc>] (unwind_backtrace+0x0/0x12c) from [<c029b82c>] (__schedule+0x58/0x2c0)
[<c029b82c>] (__schedule+0x58/0x2c0) from [<c029bc10>] (schedule+0x8c/0xb0)
[<c029bc10>] (schedule+0x8c/0xb0) from [<c0055614>] (synchronize_irq+0xbc/0xd8)
[<c0055614>] (synchronize_irq+0xbc/0xd8) from [<c01db6b0>] (pio_tasklet+0x34/0x11c)
[<c01db6b0>] (pio_tasklet+0x34/0x11c) from [<c0024914>] (__tasklet_action+0x68/0x80)
[<c0024914>] (__tasklet_action+0x68/0x80) from [<c0024ca4>] (__do_softirq+0x88/0x130)
[<c0024ca4>] (__do_softirq+0x88/0x130) from [<c0024ef0>] (do_softirq+0x48/0x54)
[<c0024ef0>] (do_softirq+0x48/0x54) from [<c0025048>] (local_bh_enable+0x8c/0xc0)
[<c0025048>] (local_bh_enable+0x8c/0xc0) from [<c0054678>] (irq_forced_thread_fn+0x4c/0x54)
[<c0054678>] (irq_forced_thread_fn+0x4c/0x54) from [<c0054454>] (irq_thread+0xa0/0x1c0)
[<c0054454>] (irq_thread+0xa0/0x1c0) from [<c0038628>] (kthread+0x84/0x8c)
[<c0038628>] (kthread+0x84/0x8c) from [<c000a100>] (kernel_thread_exit+0x0/0x8)

Whe looking at this issue, I find that there is a typical deadlock
scenario with forced treaded irq,

irq_forced_thread_fn()
  local_bh_enable();
    do_softirq();
      disable_irq();
        synchronize_irq();
          wait_event();
          /*DEAD*/

Cure it by unsync if current happen to be the very irq thread.

Reported-by: Christophe Huriaux <c.huriaux@gmail.com>
Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/manage.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 89a3ea8..7a48f74 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -41,6 +41,7 @@ early_param("threadirqs", setup_forced_irqthreads);
 void synchronize_irq(unsigned int irq)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
+	struct irqaction *action;
 	bool inprogress;
 
 	if (!desc)
@@ -67,7 +68,16 @@ void synchronize_irq(unsigned int irq)
 	/*
 	 * We made sure that no hardirq handler is running. Now verify
 	 * that no threaded handlers are active.
+	 * But for theaded irq, we don't sync if current happens to be
+	 * the irq thread; otherwise we could deadlock.
 	 */
+	action = desc->action;
+	while (action) {
+		if (action->thread && action->thread == current)
+			return;
+		action = action->next;
+	}
+
 	wait_event(desc->wait_for_threads, !atomic_read(&desc->threads_active));
 }
 EXPORT_SYMBOL(synchronize_irq);
-- 
1.7.1

Re: [PATCH v2] genirq: don't sync irq thread if current happen to be the very irq thread

[Thomas Gleixner]

On Sun, 20 May 2012, Yong Zhang wrote:
> On Sun, May 20, 2012 at 01:27:31PM +0800, Yong Zhang wrote:
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -41,6 +41,7 @@ early_param("threadirqs", setup_forced_irqthreads);
> >  void synchronize_irq(unsigned int irq)
> >  {
> >  	struct irq_desc *desc = irq_to_desc(irq);
> > +	struct irqaction *action = desc->action;
> 
> Bad time for dereferencing *action.

You meant dereferencing *desc :)
 
>  	/*
>  	 * We made sure that no hardirq handler is running. Now verify
>  	 * that no threaded handlers are active.
> +	 * But for theaded irq, we don't sync if current happens to be
> +	 * the irq thread; otherwise we could deadlock.
>  	 */
> +	action = desc->action;

And dereferencing action w/o being protected by desc->lock is buggy.

+	while (action) {
> +		if (action->thread && action->thread == current)
> +			return;
> +		action = action->next;
> +	}
> +

Aside of that I really do not like that change. It'll hide real
deadlocks when disable_irq() is called from the interrupt handler.

Also this will not cure all problems of that MMC driver on RT or with
forced threaded interrupts.

Assume that tasklet code runs from the softirq thread so it will
schedule when desc->threads_active > 0. This will trigger a
"scheduling while atomic" warning.

The irq_enable/disable dance in that driver is amazing. I have no time
at the moment to grok the logic behind this, but it bet this can be
done way simpler and less horrible.

Thanks,

	tglx

Re: [PATCH v2] genirq: don't sync irq thread if current happen to be the very irq thread

[Yong Zhang]

On Tue, May 22, 2012 at 03:50:38PM +0200, Thomas Gleixner wrote:
> On Sun, 20 May 2012, Yong Zhang wrote:
> > On Sun, May 20, 2012 at 01:27:31PM +0800, Yong Zhang wrote:
> > > --- a/kernel/irq/manage.c
> > > +++ b/kernel/irq/manage.c
> > > @@ -41,6 +41,7 @@ early_param("threadirqs", setup_forced_irqthreads);
> > >  void synchronize_irq(unsigned int irq)
> > >  {
> > >  	struct irq_desc *desc = irq_to_desc(irq);
> > > +	struct irqaction *action = desc->action;
> > 
> > Bad time for dereferencing *action.
> 
> You meant dereferencing *desc :)

Ah, yes :)

>  
> >  	/*
> >  	 * We made sure that no hardirq handler is running. Now verify
> >  	 * that no threaded handlers are active.
> > +	 * But for theaded irq, we don't sync if current happens to be
> > +	 * the irq thread; otherwise we could deadlock.
> >  	 */
> > +	action = desc->action;
> 
> And dereferencing action w/o being protected by desc->lock is buggy.
> 
> +	while (action) {
> > +		if (action->thread && action->thread == current)
> > +			return;
> > +		action = action->next;
> > +	}
> > +
> 
> Aside of that I really do not like that change. It'll hide real
> deadlocks when disable_irq() is called from the interrupt handler.
> 
> Also this will not cure all problems of that MMC driver on RT or with
> forced threaded interrupts.
> 
> Assume that tasklet code runs from the softirq thread so it will
> schedule when desc->threads_active > 0. This will trigger a
> "scheduling while atomic" warning.

Yes.

> 
> The irq_enable/disable dance in that driver is amazing. I have no time
> at the moment to grok the logic behind this, but it bet this can be
> done way simpler and less horrible.

I'll reconsider this issue and try to find the simpler way.

Thanks,
Yong