2009-03-02 15:14:37

by Peter Zijlstra

[permalink] [raw]
Subject: [PATCH] genirq: assert that irq handlers are indeed run in hardirq context.

On Fri, 2009-02-27 at 15:18 -0800, David Brownell wrote:
> But these handlers are *NOT* running in hardirq context;

Ah, let us stop this tinkering dead in its tracks

Signed-off-by: Peter Zijlstra <[email protected]>
---
kernel/irq/handle.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index c87d146..b75d73b 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -354,6 +354,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
irqreturn_t ret, retval = IRQ_NONE;
unsigned int status = 0;

+ BUG_ON(!in_irq());
+
if (!(action->flags & IRQF_DISABLED))
local_irq_enable_in_hardirq();



2009-03-02 19:48:55

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH] genirq: assert that irq handlers are indeed run in hardirq context.

On Monday 02 March 2009, Peter Zijlstra wrote:
> > But these handlers are *NOT* running in hardirq context;
>
> Ah, let us stop this tinkering dead in its tracks

How about providing adequate support for threaded IRQs
first, before breaking code that's been working for the
last few years?

This patch fails the "no regressions" test. And with
no reason given -- not even a bad one.

For what it's worth: NAK.

- Dave


2009-03-02 22:02:34

by Peter Zijlstra

[permalink] [raw]
Subject: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

Commit-ID: 3427ce9e6aac783a576c8e2712c323eaddd123a9
Gitweb: http://git.kernel.org/tip/3427ce9e6aac783a576c8e2712c323eaddd123a9
Author: Peter Zijlstra <[email protected]>
AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
Commit: Ingo Molnar <[email protected]>
CommitDate: Mon, 2 Mar 2009 22:59:15 +0100

genirq: assert that irq handlers are indeed running in hardirq context

Make sure the genirq layer handlers are indeed running handlers
in hardirq context. That is the genirq expectation and doing
anything else is broken.

Signed-off-by: Peter Zijlstra <[email protected]>
Cc: Andrew Morton <[email protected]>
LKML-Reference: <1236006812.5330.632.camel@laptop>
Signed-off-by: Ingo Molnar <[email protected]>


---
kernel/irq/handle.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 3aba8d1..716f4f4 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -328,6 +328,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
irqreturn_t ret, retval = IRQ_NONE;
unsigned int status = 0;

+ BUG_ON(!in_irq());
+
if (!(action->flags & IRQF_DISABLED))
local_irq_enable_in_hardirq();

2009-03-02 23:17:19

by Peter Zijlstra

[permalink] [raw]
Subject: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

Commit-ID: 044d408409cc4e1bc75c886e27ca85c270db104c
Gitweb: http://git.kernel.org/tip/044d408409cc4e1bc75c886e27ca85c270db104c
Author: Peter Zijlstra <[email protected]>
AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
Commit: Ingo Molnar <[email protected]>
CommitDate: Tue, 3 Mar 2009 00:05:45 +0100

genirq: assert that irq handlers are indeed running in hardirq context

Make sure the genirq layer handlers are indeed running handlers
in hardirq context. That is the genirq expectation and doing
anything else is broken.

Signed-off-by: Peter Zijlstra <[email protected]>
Cc: Andrew Morton <[email protected]>
LKML-Reference: <1236006812.5330.632.camel@laptop>
Signed-off-by: Ingo Molnar <[email protected]>


---
kernel/irq/handle.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 3aba8d1..a2ee682 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -328,6 +328,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
irqreturn_t ret, retval = IRQ_NONE;
unsigned int status = 0;

+ WARN_ONCE(!in_irq(), "BUG: IRQ handler called from non-hardirq context!");
+
if (!(action->flags & IRQF_DISABLED))
local_irq_enable_in_hardirq();

2009-04-10 07:11:57

by Eric Miao

[permalink] [raw]
Subject: Re: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

On Tue, Mar 3, 2009 at 7:15 AM, Peter Zijlstra <[email protected]> wrote:
> Commit-ID:  044d408409cc4e1bc75c886e27ca85c270db104c
> Gitweb:     http://git.kernel.org/tip/044d408409cc4e1bc75c886e27ca85c270db104c
> Author:     Peter Zijlstra <[email protected]>
> AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
> Commit:     Ingo Molnar <[email protected]>
> CommitDate: Tue, 3 Mar 2009 00:05:45 +0100
>
> genirq: assert that irq handlers are indeed running in hardirq context
>
> Make sure the genirq layer handlers are indeed running handlers
> in hardirq context. That is the genirq expectation and doing
> anything else is broken.
>
> Signed-off-by: Peter Zijlstra <[email protected]>
> Cc: Andrew Morton <[email protected]>
> LKML-Reference: <1236006812.5330.632.camel@laptop>
> Signed-off-by: Ingo Molnar <[email protected]>
>

OK, now this gave me the warning below and it looks resend_irqs()
and resend_tasklet are somehow found guilty and doing wrong, as
the comment of this commit suggested, yet I'm not sure if this makes
sense:


[ 34.728943] ------------[ cut here ]------------
[ 34.733573] WARNING: at
/home/ycmiao/work/linux-2.6/kernel/irq/handle.c:366
handle_IRQ_event+0x48/0x160()
[ 34.743088] BUG: IRQ[82] handler called from non-hardirq
context!Modules linked in:
[ 34.750729] [<c002e958>] (unwind_backtrace+0x0/0xdc) from
[<c00438d0>] (warn_slowpath+0x68/0x8c)
[ 34.759520] [<c00438d0>] (warn_slowpath+0x68/0x8c) from
[<c006d100>] (handle_IRQ_event+0x48/0x160)
[ 34.768461] [<c006d100>] (handle_IRQ_event+0x48/0x160) from
[<c006eb8c>] (handle_edge_irq+0x14c/0x1c4)
[ 34.777747] [<c006eb8c>] (handle_edge_irq+0x14c/0x1c4) from
[<c006e440>] (resend_irqs+0x44/0x78)
[ 34.786517] [<c006e440>] (resend_irqs+0x44/0x78) from [<c0048658>]
(tasklet_action+0x7c/0xdc)
[ 34.795039] [<c0048658>] (tasklet_action+0x7c/0xdc) from
[<c0048b18>] (__do_softirq+0x60/0xe8)
[ 34.803638] [<c0048b18>] (__do_softirq+0x60/0xe8) from [<c0048c88>]
(do_softirq+0x44/0x60)
[ 34.811881] [<c0048c88>] (do_softirq+0x44/0x60) from [<c0048d1c>]
(ksoftirqd+0x78/0x168)
[ 34.819953] [<c0048d1c>] (ksoftirqd+0x78/0x168) from [<c0058d38>]
(kthread+0x54/0x80)
[ 34.827786] [<c0058d38>] (kthread+0x54/0x80) from [<c0046780>]
(do_exit+0x0/0x658)
[ 34.835342] [<c0046780>] (do_exit+0x0/0x658) from [<00000000>] (0x0)
[ 34.841687] ---[ end trace 26b21608484430d3 ]---



>
> ---
>  kernel/irq/handle.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 3aba8d1..a2ee682 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -328,6 +328,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
>        irqreturn_t ret, retval = IRQ_NONE;
>        unsigned int status = 0;
>
> +       WARN_ONCE(!in_irq(), "BUG: IRQ handler called from non-hardirq context!");
> +
>        if (!(action->flags & IRQF_DISABLED))
>                local_irq_enable_in_hardirq();
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



--
Cheers
- eric

2009-04-10 09:58:44

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

On Fri, 10 Apr 2009, Eric Miao wrote:
> On Tue, Mar 3, 2009 at 7:15 AM, Peter Zijlstra <[email protected]> wrote:
> > Commit-ID:  044d408409cc4e1bc75c886e27ca85c270db104c
> > Gitweb:     http://git.kernel.org/tip/044d408409cc4e1bc75c886e27ca85c270db104c
> > Author:     Peter Zijlstra <[email protected]>
> > AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
> > Commit:     Ingo Molnar <[email protected]>
> > CommitDate: Tue, 3 Mar 2009 00:05:45 +0100
> >
> > genirq: assert that irq handlers are indeed running in hardirq context
> >
> > Make sure the genirq layer handlers are indeed running handlers
> > in hardirq context. That is the genirq expectation and doing
> > anything else is broken.
> >
> > Signed-off-by: Peter Zijlstra <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > LKML-Reference: <1236006812.5330.632.camel@laptop>
> > Signed-off-by: Ingo Molnar <[email protected]>
> >
>
> OK, now this gave me the warning below and it looks resend_irqs()
> and resend_tasklet are somehow found guilty and doing wrong, as
> the comment of this commit suggested, yet I'm not sure if this makes
> sense:

No, it doesn't. We didn't think about the resend tasklet. I'm going to
fix it.

Thanks,

tglx