[patch 0/2] Run interrupt handlers always with interrupts disabled

[patch 0/2] Run interrupt handlers always with interrupts disabled

[Thomas Gleixner]

The following patch series removes the IRQF_DISABLED functionality
from the core interrupt code and runs all interrupt handlers with
interrupts disabled.

IRQF_DISABLED is kept as a define and scheduled for feature removal.

I booted and stressed that patches w/o any obvious fallout on more
than 20 different systems in my arsenal of test machines which
includes various embedded non x86 systems.

To debug eventual latency issues we (admittedly I talked acme into
looking at that) want to extend perf with a top like tool to monitor
the maximum runtime of interrupt handlers with the already existing
tracepoints.

Thanks,

	tglx

[patch 1/2] genirq: Run irq handlers with interrupts disabled

[Thomas Gleixner]

[-- Attachment #1: genirq-run-handlers-with-irqs-disabled.patch --]
[-- Type: text/plain, Size: 1309 bytes --]

Running interrupt handlers with interrupts enabled can cause stack
overflows. That has been observed with multiqueue NICs delivering all
their interrupts to a single core. We might band aid that somehow by
checking the interrupt stacks, but the real safe fix is to run the irq
handlers with interrupts disabled.

Drivers for whacky hardware still can reenable them in the handler
itself, if the need arises. (They do already due to lockdep)

The risk of doing this is rather low:

 - lockdep already enforces this
 - CONFIG_NOHZ has shaken out the drivers which relied on jiffies updates
 - time keeping is not longer sensitive to the timer interrupt being delayed

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/handle.c |    3 ---
 1 file changed, 3 deletions(-)

Index: linux-2.6/kernel/irq/handle.c
===================================================================
--- linux-2.6.orig/kernel/irq/handle.c
+++ linux-2.6/kernel/irq/handle.c
@@ -370,9 +370,6 @@ irqreturn_t handle_IRQ_event(unsigned in
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
 
-	if (!(action->flags & IRQF_DISABLED))
-		local_irq_enable_in_hardirq();
-
 	do {
 		trace_irq_handler_entry(irq, action);
 		ret = action->handler(irq, action->dev_id);

[patch 2/2] genirq: Remove IRQF_DISABLED from core code

[Thomas Gleixner]

[-- Attachment #1: genirq-remove-irqf-disabled-from-core-code.patch --]
[-- Type: text/plain, Size: 3207 bytes --]

Remove all code which is related to IRQF_DISABLED from the core kernel
code. IRQF_DISABLED still exists as a flag, but becomes a NOOP and
will be removed after a grace period. That way we can easily revert to
the previous behaviour by just restoring the core code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 Documentation/feature-removal-schedule.txt |    7 +++++++
 include/linux/interrupt.h                  |    3 ++-
 kernel/irq/manage.c                        |   20 --------------------
 3 files changed, 9 insertions(+), 21 deletions(-)

Index: linux-2.6/Documentation/feature-removal-schedule.txt
===================================================================
--- linux-2.6.orig/Documentation/feature-removal-schedule.txt
+++ linux-2.6/Documentation/feature-removal-schedule.txt
@@ -589,3 +589,10 @@ Why:	Useful in 2003, implementation is a
 	Generally invoked by accident today.
 	Seen as doing more harm than good.
 Who:	Len Brown <len.brown@intel.com>
+
+----------------------------
+
+What:	IRQF_DISABLED
+When:	2.6.36
+Why:	The flag is a NOOP as we run interrupt handlers with interrupts disabled
+Who:	Thomas Gleixner <tglx@linutronix.de>
Index: linux-2.6/include/linux/interrupt.h
===================================================================
--- linux-2.6.orig/include/linux/interrupt.h
+++ linux-2.6/include/linux/interrupt.h
@@ -39,7 +39,8 @@
  * These flags used only by the kernel as part of the
  * irq handling routines.
  *
- * IRQF_DISABLED - keep irqs disabled when calling the action handler
+ * IRQF_DISABLED - keep irqs disabled when calling the action handler.
+ *                 DEPRECATED. This flag is a NOOP and scheduled to be removed
  * IRQF_SAMPLE_RANDOM - irq is used to feed the random generator
  * IRQF_SHARED - allow sharing the irq among several devices
  * IRQF_PROBE_SHARED - set by callers when they expect sharing mismatches to occur
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/kernel/irq/manage.c
@@ -995,7 +995,6 @@ EXPORT_SYMBOL(free_irq);
  *	Flags:
  *
  *	IRQF_SHARED		Interrupt is shared
- *	IRQF_DISABLED	Disable local interrupts while processing
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *	IRQF_TRIGGER_*		Specify active edge(s) or level
  *
@@ -1009,25 +1008,6 @@ int request_threaded_irq(unsigned int ir
 	int retval;
 
 	/*
-	 * handle_IRQ_event() always ignores IRQF_DISABLED except for
-	 * the _first_ irqaction (sigh).  That can cause oopsing, but
-	 * the behavior is classified as "will not fix" so we need to
-	 * start nudging drivers away from using that idiom.
-	 */
-	if ((irqflags & (IRQF_SHARED|IRQF_DISABLED)) ==
-					(IRQF_SHARED|IRQF_DISABLED)) {
-		pr_warning(
-		  "IRQ %d/%s: IRQF_DISABLED is not guaranteed on shared IRQs\n",
-			irq, devname);
-	}
-
-#ifdef CONFIG_LOCKDEP
-	/*
-	 * Lockdep wants atomic interrupt handlers:
-	 */
-	irqflags |= IRQF_DISABLED;
-#endif
-	/*
 	 * Sanity-check: shared interrupts must pass in a real dev-ID,
 	 * otherwise we'll have trouble later trying to figure out
 	 * which interrupt is which (messes up the interrupt freeing

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Fri, 26 Mar 2010 00:06:44 -0000

> The following patch series removes the IRQF_DISABLED functionality
> from the core interrupt code and runs all interrupt handlers with
> interrupts disabled.
> 
> IRQF_DISABLED is kept as a define and scheduled for feature removal.
> 
> I booted and stressed that patches w/o any obvious fallout on more
> than 20 different systems in my arsenal of test machines which
> includes various embedded non x86 systems.
> 
> To debug eventual latency issues we (admittedly I talked acme into
> looking at that) want to extend perf with a top like tool to monitor
> the maximum runtime of interrupt handlers with the already existing
> tracepoints.

Acked-by: David S. Miller <davem@davemloft.net>

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Andi Kleen]

> Drivers for whacky hardware still can reenable them in the handler
> itself, if the need arises. (They do already due to lockdep)

Can you please explain that lockdep reference?
I don't think lockdep really forces on interrupts, does it?

BTW the one problem I have with this patchkit is that it's clearly
no stable candidate and I was hoping for a stable fix too.
Any chance to at least approve my original patch for .32/.33 only?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [patch 2/2] genirq: Remove IRQF_DISABLED from core code

[Andi Kleen]

On Fri, Mar 26, 2010 at 12:06:55AM -0000, Thomas Gleixner wrote:
> Remove all code which is related to IRQF_DISABLED from the core kernel
> code. IRQF_DISABLED still exists as a flag, but becomes a NOOP and
> will be removed after a grace period. That way we can easily revert to
> the previous behaviour by just restoring the core code.

Perhaps I'm dense but it's not fully clear to me why is suddenly safe to use 
the behaviour of this flags on shared interrupts when it wasn't before?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Russell King]

On Fri, Mar 26, 2010 at 12:06:44AM -0000, Thomas Gleixner wrote:
> The following patch series removes the IRQF_DISABLED functionality
> from the core interrupt code and runs all interrupt handlers with
> interrupts disabled.

As was covered in previous discussions, what about drivers such as
SMC91x which take a long time to retrieve packets from the hardware?
Always running handlers with IRQs disabled will kill things such as
serial on these platforms.

So based on your description, I have no option but to NAK this.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Ingo Molnar]

* Russell King <rmk@arm.linux.org.uk> wrote:

> On Fri, Mar 26, 2010 at 12:06:44AM -0000, Thomas Gleixner wrote:
> > The following patch series removes the IRQF_DISABLED functionality
> > from the core interrupt code and runs all interrupt handlers with
> > interrupts disabled.
> 
> As was covered in previous discussions, what about drivers such as SMC91x 
> which take a long time to retrieve packets from the hardware? Always running 
> handlers with IRQs disabled will kill things such as serial on these 
> platforms.

As long as it's rare (which it is) i dont see a problem: you can enable 
interrupts in the handler by using local_irq_enable(), like the IDE PIO 
drivers do. That way it's documented a bit better as well, because it shows 
the precise source of the latency, with a big comment explaining it, etc.

Thanks,

	Ingo

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Ingo Molnar]

* Russell King <rmk@arm.linux.org.uk> wrote:

> On Fri, Mar 26, 2010 at 12:06:44AM -0000, Thomas Gleixner wrote:
> > The following patch series removes the IRQF_DISABLED functionality
> > from the core interrupt code and runs all interrupt handlers with
> > interrupts disabled.
> 
> As was covered in previous discussions, what about drivers such as SMC91x 
> which take a long time to retrieve packets from the hardware? Always running 
> handlers with IRQs disabled will kill things such as serial on these 
> platforms.

As long as it's rare (which it is) i dont see a problem: you can enable 
interrupts in the handler by using local_irq_enable(), like the IDE PIO 
drivers do. That way it's documented a bit better as well, because it shows 
the precise source of the latency, with a big comment explaining it, etc.

Thanks,

	Ingo

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Peter Zijlstra]

On Fri, 2010-03-26 at 10:20 +0100, Ingo Molnar wrote:
> * Russell King <rmk@arm.linux.org.uk> wrote:
> 
> > On Fri, Mar 26, 2010 at 12:06:44AM -0000, Thomas Gleixner wrote:
> > > The following patch series removes the IRQF_DISABLED functionality
> > > from the core interrupt code and runs all interrupt handlers with
> > > interrupts disabled.
> > 
> > As was covered in previous discussions, what about drivers such as SMC91x 
> > which take a long time to retrieve packets from the hardware? Always running 
> > handlers with IRQs disabled will kill things such as serial on these 
> > platforms.
> 
> As long as it's rare (which it is) i dont see a problem: you can enable 
> interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> drivers do. That way it's documented a bit better as well, because it shows 
> the precise source of the latency, with a big comment explaining it, etc.

Or alternatively, use threaded interrupts for such slow hardware.

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Alan Cox]

> As long as it's rare (which it is) i dont see a problem: you can enable 
> interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> drivers do. That way it's documented a bit better as well, because it shows 
> the precise source of the latency, with a big comment explaining it, etc.

I don't think it's as rare as you think particularly in embedded, and the
moment you start explicitly using local_irq_enable() you've simply moved
the underlying problem back and made it far harder to grep for.

Alan

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Peter Zijlstra]

On Fri, 2010-03-26 at 09:59 +0000, Alan Cox wrote:
> > As long as it's rare (which it is) i dont see a problem: you can enable 
> > interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> > drivers do. That way it's documented a bit better as well, because it shows 
> > the precise source of the latency, with a big comment explaining it, etc.
> 
> I don't think it's as rare as you think particularly in embedded, and the
> moment you start explicitly using local_irq_enable() you've simply moved
> the underlying problem back and made it far harder to grep for.

We've got local_irq_enable_in_hardirq() which should be used and can
easily be grep'ed for.

But yes, I would much prefer to simply convert these known slow handlers
to threaded interrupts.

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Andi Kleen]

On Fri, Mar 26, 2010 at 11:08:53AM +0100, Peter Zijlstra wrote:
> On Fri, 2010-03-26 at 09:59 +0000, Alan Cox wrote:
> > > As long as it's rare (which it is) i dont see a problem: you can enable 
> > > interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> > > drivers do. That way it's documented a bit better as well, because it shows 
> > > the precise source of the latency, with a big comment explaining it, etc.
> > 
> > I don't think it's as rare as you think particularly in embedded, and the
> > moment you start explicitly using local_irq_enable() you've simply moved
> > the underlying problem back and made it far harder to grep for.
> 
> We've got local_irq_enable_in_hardirq() which should be used and can
> easily be grep'ed for.
> 
> But yes, I would much prefer to simply convert these known slow handlers
> to threaded interrupts.

That could be potentially hundreds in old obscure drivers all over the tree.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, 2010-03-26 at 09:59 +0000, Alan Cox wrote:
> > > As long as it's rare (which it is) i dont see a problem: you can enable 
> > > interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> > > drivers do. That way it's documented a bit better as well, because it shows 
> > > the precise source of the latency, with a big comment explaining it, etc.
> > 
> > I don't think it's as rare as you think particularly in embedded, and the 
> > moment you start explicitly using local_irq_enable() you've simply moved 
> > the underlying problem back and made it far harder to grep for.
> 
> We've got local_irq_enable_in_hardirq() which should be used and can easily 
> be grep'ed for.
> 
> But yes, I would much prefer to simply convert these known slow handlers to 
> threaded interrupts.

Yeah, agreed. So there's multiple solutions:

 - On old hw with a driver that nobody is willing to convert to threaded IRQs: 
   use the existing local_irq_enable_in_hardirq() API. This preserves the 
   status quo.

 - On new hw with new drivers where there's such a level of IRQ parallelism 
   that enabling IRQs in hardirqs is not an option, use threaded IRQ handlers.

Thanks,

	Ingo

Re: [patch 2/2] genirq: Remove IRQF_DISABLED from core code

[Thomas Gleixner]

On Fri, 26 Mar 2010, Andi Kleen wrote:

> On Fri, Mar 26, 2010 at 12:06:55AM -0000, Thomas Gleixner wrote:
> > Remove all code which is related to IRQF_DISABLED from the core kernel
> > code. IRQF_DISABLED still exists as a flag, but becomes a NOOP and
> > will be removed after a grace period. That way we can easily revert to
> > the previous behaviour by just restoring the core code.
> 
> Perhaps I'm dense but it's not fully clear to me why is suddenly safe to use 
> the behaviour of this flags on shared interrupts when it wasn't before?

The shared handlers cannot guarantee to run one with irqs enabled and
the other with irqs disabled. That's all. There is absolutely no
reason why we would need interrupts enabled to process the shared
handler chain.

Thanks,

	tglx

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Nicolas Pitre]

On Fri, 26 Mar 2010, Peter Zijlstra wrote:

> On Fri, 2010-03-26 at 09:59 +0000, Alan Cox wrote:
> > > As long as it's rare (which it is) i dont see a problem: you can enable 
> > > interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> > > drivers do. That way it's documented a bit better as well, because it shows 
> > > the precise source of the latency, with a big comment explaining it, etc.
> > 
> > I don't think it's as rare as you think particularly in embedded, and the
> > moment you start explicitly using local_irq_enable() you've simply moved
> > the underlying problem back and made it far harder to grep for.
> 
> We've got local_irq_enable_in_hardirq() which should be used and can
> easily be grep'ed for.
> 
> But yes, I would much prefer to simply convert these known slow handlers
> to threaded interrupts.

Can't do that.  The smc91x has a very small internal buffer which has to 
be emptied using PIO.  Threaded interrupts simply have too high 
latencies for overruns not to occur.  That's why the RX path is entirely 
done in hardirq context while the TX path is done in softirq context.


Nicolas

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Jamie Lokier]

Peter Zijlstra wrote:
> On Fri, 2010-03-26 at 10:20 +0100, Ingo Molnar wrote:
> > * Russell King <rmk@arm.linux.org.uk> wrote:
> > 
> > > On Fri, Mar 26, 2010 at 12:06:44AM -0000, Thomas Gleixner wrote:
> > > > The following patch series removes the IRQF_DISABLED functionality
> > > > from the core interrupt code and runs all interrupt handlers with
> > > > interrupts disabled.
> > > 
> > > As was covered in previous discussions, what about drivers such as SMC91x 
> > > which take a long time to retrieve packets from the hardware? Always running 
> > > handlers with IRQs disabled will kill things such as serial on these 
> > > platforms.
> > 
> > As long as it's rare (which it is) i dont see a problem: you can enable 
> > interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> > drivers do. That way it's documented a bit better as well, because it shows 
> > the precise source of the latency, with a big comment explaining it, etc.
> 
> Or alternatively, use threaded interrupts for such slow hardware.

What is the latency of threaded interrupts these days, compared with
non-threaded interrupts?

Slow hardware is quite sensitive to increases in latency.  Obviously
not a problem for the sources of latency: it's a problem for the irq
which is _sensitive_ to latency caused by the other one.  That is
typically a serial port or something.

But the benefit of kernel-settable interrupt priorities (i.e. due to
the threads) may be worth it even for serial ports.  I would love to
see some measurements comparing with and without.

-- Jamie

Re: [patch 0/2] Run interrupt handlers always with interrupts disabled

[Jamie Lokier]

Nicolas Pitre wrote:
> On Fri, 26 Mar 2010, Peter Zijlstra wrote:
> 
> > On Fri, 2010-03-26 at 09:59 +0000, Alan Cox wrote:
> > > > As long as it's rare (which it is) i dont see a problem: you can enable 
> > > > interrupts in the handler by using local_irq_enable(), like the IDE PIO 
> > > > drivers do. That way it's documented a bit better as well, because it shows 
> > > > the precise source of the latency, with a big comment explaining it, etc.
> > > 
> > > I don't think it's as rare as you think particularly in embedded, and the
> > > moment you start explicitly using local_irq_enable() you've simply moved
> > > the underlying problem back and made it far harder to grep for.
> > 
> > We've got local_irq_enable_in_hardirq() which should be used and can
> > easily be grep'ed for.
> > 
> > But yes, I would much prefer to simply convert these known slow handlers
> > to threaded interrupts.
> 
> Can't do that.  The smc91x has a very small internal buffer which has to 
> be emptied using PIO.  Threaded interrupts simply have too high 
> latencies for overruns not to occur.  That's why the RX path is entirely 
> done in hardirq context while the TX path is done in softirq context.

Although I wouldn't be surprised to find threaded interrupts are too
slow on certain hardware, is that _fundamental_ to threaded
interrupts, or is it just that our implementation doesn't have the
funky hot path straight direct from hardirq -> running high priority
RT irq thread when it exceeds previously running priority?

In other words, can we swizzle threaded irqs into something more
resembling software-implemented hard irq priorities, while cunningly
updating the kernel state just enough to look like it's a thread?

-- Jamie

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Thomas Gleixner]

On Fri, 26 Mar 2010, Andi Kleen wrote:

> > Drivers for whacky hardware still can reenable them in the handler
> > itself, if the need arises. (They do already due to lockdep)
> 
> Can you please explain that lockdep reference?
> I don't think lockdep really forces on interrupts, does it?

Lockdep forces interrupts off. It yells at anyone enabling irqs in the
handler. The ones which do that have been annotated with
local_irq_enable_in_hardirq(), which is a nop for lockdep.

> BTW the one problem I have with this patchkit is that it's clearly
> no stable candidate and I was hoping for a stable fix too.
> Any chance to at least approve my original patch for .32/.33 only?

Why not simply force IRQF_DISABLED for all MSI interrupts. That still
allows nesting for non MSI ones, but it limits the chance of throwing
up reasonably well. That's a two liner.

Can you please test whether it resolves the issue at hand ?

Thanks,

	tglx
---
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eb6078c..1d55e92 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -735,6 +735,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		if (new->flags & IRQF_ONESHOT)
 			desc->status |= IRQ_ONESHOT;
 
+		if (desc->msi_desc)
+			new->flags |= IRQF_DISABLED;
+
 		if (!(desc->status & IRQ_NOAUTOEN)) {
 			desc->depth = 0;
 			desc->status &= ~IRQ_DISABLED;

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Andi Kleen]

> Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> allows nesting for non MSI ones, but it limits the chance of throwing
> up reasonably well. That's a two liner.
> 
> Can you please test whether it resolves the issue at hand ?

Sorry for the late answer. Got confirmation that this patch
fixes the test case. Thanks.

-Andi

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Thomas Gleixner]

On Tue, 30 Mar 2010, Andi Kleen wrote:

> > Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> > allows nesting for non MSI ones, but it limits the chance of throwing
> > up reasonably well. That's a two liner.
> > 
> > Can you please test whether it resolves the issue at hand ?
> 
> Sorry for the late answer. Got confirmation that this patch
> fixes the test case. Thanks.

Ok, I'll push it linus wards and cc stable. I think thats the least
intrusive safe bet we can have right now.

Thanks,

	tglx

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Pavel Machek]

On Wed 2010-03-31 13:16:37, Thomas Gleixner wrote:
> On Tue, 30 Mar 2010, Andi Kleen wrote:
> 
> > > Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> > > allows nesting for non MSI ones, but it limits the chance of throwing
> > > up reasonably well. That's a two liner.
> > > 
> > > Can you please test whether it resolves the issue at hand ?
> > 
> > Sorry for the late answer. Got confirmation that this patch
> > fixes the test case. Thanks.
> 
> Ok, I'll push it linus wards and cc stable. I think thats the least
> intrusive safe bet we can have right now.

stable? I'd say thats way too intrusive for -stable...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Thomas Gleixner]

On Fri, 2 Apr 2010, Pavel Machek wrote:

> On Wed 2010-03-31 13:16:37, Thomas Gleixner wrote:
> > On Tue, 30 Mar 2010, Andi Kleen wrote:
> > 
> > > > Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> > > > allows nesting for non MSI ones, but it limits the chance of throwing
> > > > up reasonably well. That's a two liner.
> > > > 
> > > > Can you please test whether it resolves the issue at hand ?
> > > 
> > > Sorry for the late answer. Got confirmation that this patch
> > > fixes the test case. Thanks.
> > 
> > Ok, I'll push it linus wards and cc stable. I think thats the least
> > intrusive safe bet we can have right now.
> 
> stable? I'd say thats way too intrusive for -stable...

So we better let the possible stack overruns unaddressed ?

Thanks,

	tglx

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Pavel Machek]

On Fri 2010-04-02 22:42:51, Thomas Gleixner wrote:
> On Fri, 2 Apr 2010, Pavel Machek wrote:
> 
> > On Wed 2010-03-31 13:16:37, Thomas Gleixner wrote:
> > > On Tue, 30 Mar 2010, Andi Kleen wrote:
> > > 
> > > > > Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> > > > > allows nesting for non MSI ones, but it limits the chance of throwing
> > > > > up reasonably well. That's a two liner.
> > > > > 
> > > > > Can you please test whether it resolves the issue at hand ?
> > > > 
> > > > Sorry for the late answer. Got confirmation that this patch
> > > > fixes the test case. Thanks.
> > > 
> > > Ok, I'll push it linus wards and cc stable. I think thats the least
> > > intrusive safe bet we can have right now.
> > 
> > stable? I'd say thats way too intrusive for -stable...
> 
> So we better let the possible stack overruns unaddressed ?

-stable should have no regressions, first and foremost. And this is
pretty certain to introduce some, at least on low-powered system with
serial ports.

So yes, it is probably better to let the possible stack overruns
unaddressed. We have lived with them for 15 years or so...

(Alternatively, just make the irq stacks bigger? Or just take Andi's
patch, which solves the overruns, and only introduces latency
regressions when it would otherwise crash?)

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Peter Zijlstra]

On Fri, 2010-04-02 at 23:09 +0200, Pavel Machek wrote:
> On Fri 2010-04-02 22:42:51, Thomas Gleixner wrote:
> > On Fri, 2 Apr 2010, Pavel Machek wrote:
> > 
> > > On Wed 2010-03-31 13:16:37, Thomas Gleixner wrote:
> > > > On Tue, 30 Mar 2010, Andi Kleen wrote:
> > > > 
> > > > > > Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> > > > > > allows nesting for non MSI ones, but it limits the chance of throwing
> > > > > > up reasonably well. That's a two liner.
> > > > > > 
> > > > > > Can you please test whether it resolves the issue at hand ?
> > > > > 
> > > > > Sorry for the late answer. Got confirmation that this patch
> > > > > fixes the test case. Thanks.
> > > > 
> > > > Ok, I'll push it linus wards and cc stable. I think thats the least
> > > > intrusive safe bet we can have right now.
> > > 
> > > stable? I'd say thats way too intrusive for -stable...
> > 
> > So we better let the possible stack overruns unaddressed ?
> 
> -stable should have no regressions, first and foremost. And this is
> pretty certain to introduce some, at least on low-powered system with
> serial ports.
> 
> So yes, it is probably better to let the possible stack overruns
> unaddressed. We have lived with them for 15 years or so...
> 
> (Alternatively, just make the irq stacks bigger? Or just take Andi's
> patch, which solves the overruns, and only introduces latency
> regressions when it would otherwise crash?)

You've got serial ports with MSI interrupts?

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Thomas Gleixner]

Pavel,

On Fri, 2 Apr 2010, Pavel Machek wrote:

> On Fri 2010-04-02 22:42:51, Thomas Gleixner wrote:
> > On Fri, 2 Apr 2010, Pavel Machek wrote:
> > 
> > > On Wed 2010-03-31 13:16:37, Thomas Gleixner wrote:
> > > > On Tue, 30 Mar 2010, Andi Kleen wrote:
> > > > 
> > > > > > Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> > > > > > allows nesting for non MSI ones, but it limits the chance of throwing
> > > > > > up reasonably well. That's a two liner.
> > > > > > 
> > > > > > Can you please test whether it resolves the issue at hand ?
> > > > > 
> > > > > Sorry for the late answer. Got confirmation that this patch
> > > > > fixes the test case. Thanks.
> > > > 
> > > > Ok, I'll push it linus wards and cc stable. I think thats the least
> > > > intrusive safe bet we can have right now.
> > > 
> > > stable? I'd say thats way too intrusive for -stable...
> > 
> > So we better let the possible stack overruns unaddressed ?
> 
> -stable should have no regressions, first and foremost. And this is
> pretty certain to introduce some, at least on low-powered system with
> serial ports.

I think you misunderstood what I'm going to push. The patch merily
forces IRQF_DISABLED for MSI(X) based interrupts. So that does not
affect low powered systems in any way.

It only affects high end systems where Dave Miller already said he did
the IRQF_DISABLED magic already in some NIC drivers just to prevent
that.

So I think your fear of regressions for low-powered systems is
completely unsubstantiated.

Thanks,

	tglx

 

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Pavel Machek]

On Sat 2010-04-03 00:51:20, Thomas Gleixner wrote:
> Pavel,
> 
> On Fri, 2 Apr 2010, Pavel Machek wrote:
> 
> > On Fri 2010-04-02 22:42:51, Thomas Gleixner wrote:
> > > On Fri, 2 Apr 2010, Pavel Machek wrote:
> > > 
> > > > On Wed 2010-03-31 13:16:37, Thomas Gleixner wrote:
> > > > > On Tue, 30 Mar 2010, Andi Kleen wrote:
> > > > > 
> > > > > > > Why not simply force IRQF_DISABLED for all MSI interrupts. That still
> > > > > > > allows nesting for non MSI ones, but it limits the chance of throwing
> > > > > > > up reasonably well. That's a two liner.
> > > > > > > 
> > > > > > > Can you please test whether it resolves the issue at hand ?
> > > > > > 
> > > > > > Sorry for the late answer. Got confirmation that this patch
> > > > > > fixes the test case. Thanks.
> > > > > 
> > > > > Ok, I'll push it linus wards and cc stable. I think thats the least
> > > > > intrusive safe bet we can have right now.
> > > > 
> > > > stable? I'd say thats way too intrusive for -stable...
> > > 
> > > So we better let the possible stack overruns unaddressed ?
> > 
> > -stable should have no regressions, first and foremost. And this is
> > pretty certain to introduce some, at least on low-powered system with
> > serial ports.
> 
> I think you misunderstood what I'm going to push. The patch merily
> forces IRQF_DISABLED for MSI(X) based interrupts. So that does not
> affect low powered systems in any way.
> 
> It only affects high end systems where Dave Miller already said he did
> the IRQF_DISABLED magic already in some NIC drivers just to prevent
> that.

Oops, yes, I did; lost in all the mails.

> So I think your fear of regressions for low-powered systems is
> completely unsubstantiated.

Yep. Sorry for the noise.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[tip:irq/core] genirq: Run irq handlers with interrupts disabled

[tip-bot for Ingo Molnar]

Commit-ID:  e58aa3d2d0cc01ad8d6f7f640a0670433f794922
Gitweb:     http://git.kernel.org/tip/e58aa3d2d0cc01ad8d6f7f640a0670433f794922
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Fri, 26 Mar 2010 00:06:51 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 13 Apr 2010 16:36:40 +0200

genirq: Run irq handlers with interrupts disabled

Running interrupt handlers with interrupts enabled can cause stack
overflows. That has been observed with multiqueue NICs delivering all
their interrupts to a single core. We might band aid that somehow by
checking the interrupt stacks, but the real safe fix is to run the irq
handlers with interrupts disabled.

Drivers for whacky hardware still can reenable them in the handler
itself, if the need arises. (They do already due to lockdep)

The risk of doing this is rather low:

 - lockdep already enforces this
 - CONFIG_NOHZ has shaken out the drivers which relied on jiffies updates
 - time keeping is not longer sensitive to the timer interrupt being delayed

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Miller <davem@davemloft.net>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Linus Torvalds <torvalds@osdl.org>
LKML-Reference: <20100326000405.758579387@linutronix.de>

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

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 76d5a67..27e5c69 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -370,9 +370,6 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
 
-	if (!(action->flags & IRQF_DISABLED))
-		local_irq_enable_in_hardirq();
-
 	do {
 		trace_irq_handler_entry(irq, action);
 		ret = action->handler(irq, action->dev_id);

[tip:irq/core] genirq: Remove IRQF_DISABLED from core code

[tip-bot for Thomas Gleixner]

Commit-ID:  6932bf37bed45ce8ed531928b1b0f98162fe6df6
Gitweb:     http://git.kernel.org/tip/6932bf37bed45ce8ed531928b1b0f98162fe6df6
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Fri, 26 Mar 2010 00:06:55 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 13 Apr 2010 16:36:40 +0200

genirq: Remove IRQF_DISABLED from core code

Remove all code which is related to IRQF_DISABLED from the core kernel
code. IRQF_DISABLED still exists as a flag, but becomes a NOOP and
will be removed after a grace period. That way we can easily revert to
the previous behaviour by just restoring the core code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Miller <davem@davemloft.net>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Linus Torvalds <torvalds@osdl.org>
LKML-Reference: <20100326000405.991244690@linutronix.de>
---
 Documentation/feature-removal-schedule.txt |    7 ++++++
 include/linux/interrupt.h                  |    3 +-
 kernel/irq/manage.c                        |   30 ----------------------------
 3 files changed, 9 insertions(+), 31 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index ed511af..9c31c2e 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -589,3 +589,10 @@ Why:	Useful in 2003, implementation is a hack.
 	Generally invoked by accident today.
 	Seen as doing more harm than good.
 Who:	Len Brown <len.brown@intel.com>
+
+----------------------------
+
+What:	IRQF_DISABLED
+When:	2.6.36
+Why:	The flag is a NOOP as we run interrupt handlers with interrupts disabled
+Who:	Thomas Gleixner <tglx@linutronix.de>
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index d7e7a76..e6d2f44 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -39,7 +39,8 @@
  * These flags used only by the kernel as part of the
  * irq handling routines.
  *
- * IRQF_DISABLED - keep irqs disabled when calling the action handler
+ * IRQF_DISABLED - keep irqs disabled when calling the action handler.
+ *                 DEPRECATED. This flag is a NOOP and scheduled to be removed
  * IRQF_SAMPLE_RANDOM - irq is used to feed the random generator
  * IRQF_SHARED - allow sharing the irq among several devices
  * IRQF_PROBE_SHARED - set by callers when they expect sharing mismatches to occur
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 84f3227..444d5a8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -757,16 +757,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		if (new->flags & IRQF_ONESHOT)
 			desc->status |= IRQ_ONESHOT;
 
-		/*
-		 * Force MSI interrupts to run with interrupts
-		 * disabled. The multi vector cards can cause stack
-		 * overflows due to nested interrupts when enough of
-		 * them are directed to a core and fire at the same
-		 * time.
-		 */
-		if (desc->msi_desc)
-			new->flags |= IRQF_DISABLED;
-
 		if (!(desc->status & IRQ_NOAUTOEN)) {
 			desc->depth = 0;
 			desc->status &= ~IRQ_DISABLED;
@@ -1027,7 +1017,6 @@ EXPORT_SYMBOL(free_irq);
  *	Flags:
  *
  *	IRQF_SHARED		Interrupt is shared
- *	IRQF_DISABLED	Disable local interrupts while processing
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *	IRQF_TRIGGER_*		Specify active edge(s) or level
  *
@@ -1041,25 +1030,6 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	int retval;
 
 	/*
-	 * handle_IRQ_event() always ignores IRQF_DISABLED except for
-	 * the _first_ irqaction (sigh).  That can cause oopsing, but
-	 * the behavior is classified as "will not fix" so we need to
-	 * start nudging drivers away from using that idiom.
-	 */
-	if ((irqflags & (IRQF_SHARED|IRQF_DISABLED)) ==
-					(IRQF_SHARED|IRQF_DISABLED)) {
-		pr_warning(
-		  "IRQ %d/%s: IRQF_DISABLED is not guaranteed on shared IRQs\n",
-			irq, devname);
-	}
-
-#ifdef CONFIG_LOCKDEP
-	/*
-	 * Lockdep wants atomic interrupt handlers:
-	 */
-	irqflags |= IRQF_DISABLED;
-#endif
-	/*
 	 * Sanity-check: shared interrupts must pass in a real dev-ID,
 	 * otherwise we'll have trouble later trying to figure out
 	 * which interrupt is which (messes up the interrupt freeing

Re: [tip:irq/core] genirq: Run irq handlers with interrupts disabled

[Peter Zijlstra]

On Tue, 2010-04-13 at 19:33 +0000, tip-bot for Ingo Molnar wrote:
> Commit-ID:  e58aa3d2d0cc01ad8d6f7f640a0670433f794922
> Gitweb:     http://git.kernel.org/tip/e58aa3d2d0cc01ad8d6f7f640a0670433f794922
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Fri, 26 Mar 2010 00:06:51 +0000
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Tue, 13 Apr 2010 16:36:40 +0200
> 
> genirq: Run irq handlers with interrupts disabled
> 
> Running interrupt handlers with interrupts enabled can cause stack
> overflows. That has been observed with multiqueue NICs delivering all
> their interrupts to a single core. We might band aid that somehow by
> checking the interrupt stacks, but the real safe fix is to run the irq
> handlers with interrupts disabled.
> 
> Drivers for whacky hardware still can reenable them in the handler
> itself, if the need arises. (They do already due to lockdep)
> 
> The risk of doing this is rather low:
> 
>  - lockdep already enforces this
>  - CONFIG_NOHZ has shaken out the drivers which relied on jiffies updates
>  - time keeping is not longer sensitive to the timer interrupt being delayed

Clearly I don't mind this, but shouldn't we at least first convert the
known problematic drivers to an alternative strategy?

IDE-PIO, that one ancient NIC etc..

> ---
>  kernel/irq/handle.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 76d5a67..27e5c69 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -370,9 +370,6 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
>  	irqreturn_t ret, retval = IRQ_NONE;
>  	unsigned int status = 0;
>  
> -	if (!(action->flags & IRQF_DISABLED))
> -		local_irq_enable_in_hardirq();
> -
>  	do {
>  		trace_irq_handler_entry(irq, action);
>  		ret = action->handler(irq, action->dev_id);

Re: [patch 1/2] genirq: Run irq handlers with interrupts disabled

[Venkatesh Pallipadi]

Just noticed a minor side effect of this patch on /proc/stat.

As int handlers run with ints disabled, timer interrupt based
samples will no longer account for any irq time. As a result, irq
time in /proc/stat, top, etc will be mostly zero (unless some handler
is enabling ints).

This is not a problem on s390, powerpc, ia64 when VIRT_CPU_ACCOUNTING is
enabled.

May be we should gracefully call it out as zero, with patch like below?

Signed-off-by: Venkatesh Pallipadi <venki@google.com>
---
 Documentation/filesystems/proc.txt |    2 +-
 fs/proc/stat.c                     |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 6507d2a..8f5254a 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -1123,7 +1123,7 @@ second).  The meanings of the columns are as follows, from left to right:
 - system: processes executing in kernel mode
 - idle: twiddling thumbs
 - iowait: waiting for I/O to complete
-- irq: servicing interrupts
+- irq: servicing interrupts (valid only with VIRT_CPU_ACCOUNTING, 0 otherwise)
 - softirq: servicing softirqs
 - steal: involuntary wait
 - guest: running a normal guest
diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index bf31b03..d92bea8 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -46,7 +46,9 @@ static int show_stat(struct seq_file *p, void *v)
 		idle = cputime64_add(idle, kstat_cpu(i).cpustat.idle);
 		idle = cputime64_add(idle, arch_idle_time(i));
 		iowait = cputime64_add(iowait, kstat_cpu(i).cpustat.iowait);
+#ifdef CONFIG_VIRT_CPU_ACCOUNTING
 		irq = cputime64_add(irq, kstat_cpu(i).cpustat.irq);
+#endif
 		softirq = cputime64_add(softirq, kstat_cpu(i).cpustat.softirq);
 		steal = cputime64_add(steal, kstat_cpu(i).cpustat.steal);
 		guest = cputime64_add(guest, kstat_cpu(i).cpustat.guest);
@@ -87,7 +89,9 @@ static int show_stat(struct seq_file *p, void *v)
 		idle = kstat_cpu(i).cpustat.idle;
 		idle = cputime64_add(idle, arch_idle_time(i));
 		iowait = kstat_cpu(i).cpustat.iowait;
+#ifdef CONFIG_VIRT_CPU_ACCOUNTING
 		irq = kstat_cpu(i).cpustat.irq;
+#endif
 		softirq = kstat_cpu(i).cpustat.softirq;
 		steal = kstat_cpu(i).cpustat.steal;
 		guest = kstat_cpu(i).cpustat.guest;
-- 
1.7.0.1