Re: [patch 2/3] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

Re: [patch 2/3] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Andrew Morton]

On Sat, 13 Mar 2010 01:59:50 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote:

> 
> I'm simply refusing to apply patches which just shuffle code around to
> warn about obscure use cases instead of tackling the real problem of
> long running irq handlers which prevent us to get rid of IRQF_DISABLED
> and just run all interrupt handlers with irqs disabled.

I'm having trouble agreeing with that aim, really.

We can tweak and tune until we're blue in the face, but the system's
IRQ latency will always be worse if handlers always run with interrupts
disabled.

Plus this approach introduces dependencies between unrelated drivers
(and between _each_ driver and the overall system) which simply don't
exist if those drivers are being nicer to the rest of the system.

Re: [patch 2/3] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Thomas Gleixner]

Back to LKML and Cc'ed a few more folks

On Fri, 12 Mar 2010, Andrew Morton wrote:

> On Sat, 13 Mar 2010 00:32:28 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Thu, 11 Mar 2010, akpm@linux-foundation.org wrote:
> > 
> > > From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > 
> > > For shared irqs IRQF_DISABLED is only guaranteed for the first handler. 
> > > So only warn starting at the second registration.
> > > 
> > > The warning is moved to __setup_irq having the additional benefit of
> > > catching actions registered using setup_irq not only register_irq.
> > > 
> > > This doesn't fix the cases where setup order is wrong but it should report
> > > the broken cases more reliably.
> > 
> > Please drop this. It's just useless shuffling off code for no real
> > benefit.
> > 
> 
> I think that warning on the second-registered IRQ was probably a
> mistake.  Because registering just the first IRQF_DISABLED handler only
> works by luck, and is vlunerable to someone else adding a handler later.
> 
> But extending the warning so that it also covers setup_irq() seems
> sensible enough.

Well, this is a crappy concept anyway and it would be way more
worthwhile to figure out why we need IRQF_DISABLED at all.

An irq handler should just run always with irqs disabled. The flag is
just there to deal with crappy drivers which take hundreds of
microseconds or more in their interrupt service routines. 

hint: IDE, USB ...

I'm simply refusing to apply patches which just shuffle code around to
warn about obscure use cases instead of tackling the real problem of
long running irq handlers which prevent us to get rid of IRQF_DISABLED
and just run all interrupt handlers with irqs disabled. Getting rid of
that would also simplify stuff which needs to be aware of reentrancy
now.

Thanks,

	tglx

Re: [patch 2/3] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Jamie Lokier]

Andrew Morton wrote:
> We can tweak and tune until we're blue in the face, but the system's
> IRQ latency will always be worse if handlers always run with interrupts
> disabled.

Are you sure?

If good, fast irq handler A runs with interrupts enabled,
and good, fast irq handler B runs (interrupting A),
A's latency goes _up_ not down.

If B happens first, then you get B's latency going up, and A's latency going down.

It's not really clear what happens to average latency.  But if you
know that some handlers require lower latency than others (e.g. serial
ports with small FIFOs), it makes sense to prioritise them.

-- Jamie

Re: [patch 2/3] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Andrew Morton]

On Mon, 15 Mar 2010 02:52:49 +0000 Jamie Lokier <jamie@shareable.org> wrote:

> Andrew Morton wrote:
> > We can tweak and tune until we're blue in the face, but the system's
> > IRQ latency will always be worse if handlers always run with interrupts
> > disabled.
> 
> Are you sure?
> 
> If good, fast irq handler A runs with interrupts enabled,
> and good, fast irq handler B runs (interrupting A),
> A's latency goes _up_ not down.
> 
> If B happens first, then you get B's latency going up, and A's latency going down.

If the hardware doesn't prioritise these interrupts then the programmer
can do so crudely, by running the critical one with interrupts disabled.

Re: [patch 2/3] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Uwe Kleine-König]

Hello,

On Sat, Mar 13, 2010 at 01:59:50AM +0100, Thomas Gleixner wrote:
> 
> Back to LKML and Cc'ed a few more folks
> 
> On Fri, 12 Mar 2010, Andrew Morton wrote:
> 
> > On Sat, 13 Mar 2010 00:32:28 +0100 (CET)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > On Thu, 11 Mar 2010, akpm@linux-foundation.org wrote:
> > > 
> > > > From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > > 
> > > > For shared irqs IRQF_DISABLED is only guaranteed for the first handler. 
> > > > So only warn starting at the second registration.
> > > > 
> > > > The warning is moved to __setup_irq having the additional benefit of
> > > > catching actions registered using setup_irq not only register_irq.
> > > > 
> > > > This doesn't fix the cases where setup order is wrong but it should report
> > > > the broken cases more reliably.
> > > 
> > > Please drop this. It's just useless shuffling off code for no real
> > > benefit.
> > > 
> > 
> > I think that warning on the second-registered IRQ was probably a
> > mistake.  Because registering just the first IRQF_DISABLED handler only
> > works by luck, and is vlunerable to someone else adding a handler later.
> > 
> > But extending the warning so that it also covers setup_irq() seems
> > sensible enough.
> 
> Well, this is a crappy concept anyway and it would be way more
> worthwhile to figure out why we need IRQF_DISABLED at all.
> 
> An irq handler should just run always with irqs disabled. The flag is
> just there to deal with crappy drivers which take hundreds of
> microseconds or more in their interrupt service routines. 
> 
> hint: IDE, USB ...
> 
> I'm simply refusing to apply patches which just shuffle code around to
> warn about obscure use cases instead of tackling the real problem of
> long running irq handlers which prevent us to get rid of IRQF_DISABLED
> and just run all interrupt handlers with irqs disabled. Getting rid of
> that would also simplify stuff which needs to be aware of reentrancy
> now.
The intention for my patch is a problem that have occured at least twice
in the past on the linux-arm-kernel mailing list.  On at91 the timer irq
is shared with some peripherals and we got reports that on these
machines the timer core BUGged because irq were not disabled.  Up to now
the reporters didn't help to debug that to the end.  So I'd consider it
nice to make it easy for the next person running into that problem now
that we know that there is a problem in that area.

I still think the patch is worthwhile as I think we're not getting rid
of IRQF_DISABLED in the next few release cycles, are we?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |