LinuxLists.cc - [RFC][PATCH] irq: remove IRQF

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2009-03-02 at 08:47 -0700, Matthew Wilcox wrote:
> On Mon, Mar 02, 2009 at 03:43:03PM +0100, Ingo Molnar wrote:
> > * Peter Zijlstra <[email protected]> wrote:
> > > Hi Linus,
> > >
> > > Would you be willing to take such a patch?
> > >
> > > Its a rather big one, but if you think its a good idea, I can
> > > generate one against whatever tree is needed.
> > >
> > > Provided I didn't break any !genirq stuff.. then again, that
> > > might be a nice incentive to fix up those last few archs.
> >
> > look good IMO. If there are no objections, and given that it
>
> Perhaps someone could explain why we want to remove it? I just added
> it to AHCI for good reason, so I'd like to know why we think it should
> be taken away.
>
> I added it because I was adding support for per-port interrupts.
> In the interrupt handler, we take the *host* lock (not the port lock).
> With multiple interrupt handlers per host, we would have to disable
> interrupts in the interrupt handler before taking the lock. Which is
> foolish because we've just re-enabled interrupts in the genirq code.
> Specifying IRQF_DISABLED means we just run with interrupts disabled.
>
> You can argue that libata/ahci should be using a per-port lock, and
> I wouldn't disagree. But I'd still like to know why IRQF_DISABLED is
> being removed.

The idea is to mandate IRQF_DISABLED for everybody, and since it doesn't
do any good to have it a flag if you have it always enabled, remove
it ;-)

2009-03-02 16:32:42

by Matthew Wilcox

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, Mar 02, 2009 at 04:59:48PM +0100, Peter Zijlstra wrote:
> The idea is to mandate IRQF_DISABLED for everybody, and since it doesn't
> do any good to have it a flag if you have it always enabled, remove
> it ;-)

I don't have a problem with that.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-03-02 17:13:15

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2 Mar 2009, Peter Zijlstra wrote:
>
> Would you be willing to take such a patch?

Yes - some day.

The "irq's disabled fastpath" thing has been there since pretty much day
one, because some irq handlers always wanted it. Making it the default
(and the only choice) is fine.

HOWEVER.

The fact is, some interrupt handlers are too slow for this. If you are
using a PIO IDE driver, not enabling interrupts will not only make you
lose serial line interrupts like mad (even at slow speeds), but has
historically resulted in even timer interrupts being lost because the IDE
layer is so slow.

The thing is, with PIO, a 512-byte disk read ends up doing 256 16-bit word
reads from the controller, each potentially up to 600ns long (PIO0
timings). That's 150ms - for a single sector!

Now, IDE controllers that we end up using PIO on all tend to be pretty
old, but admittedly even the old ones are faster than the worst-case
timings, so in practice you're not looking quite at that kind of horror
case, but you're still looking at each sector transfer (256 16-bit words)
taking on the order of tens of us.

For example, Mode2 timings are probably still something we should consider
realistic, and that's 240ns per word - a single sector now takes ~60ms to
read off the disk.

And we don't do single sectors. Most transfers will be 8 sectors (4k
contiguos read). So now that 60ms is 480ms per such IDE interrupt. And
even with the _good_ timings, we're certainly looking at a reduction of
that to about half.

Imagine what happens when we have interrupts disabled for half a second at
a time.

In other words, I do not think we are ready for this.

Now: I suspect you don't have a single machine that does PIO by default,
and you probably haven't had any for the last ten years. These things
_are_ old. But they do exist.

Could we make IRQF_DISABLED the default? Hell yes. We probably should. But
we realistically can't remove the non-disabled case, until you also have a
(tested) patch to fix these kinds of issues for the IDE PIO case (and
possibly others - IDE is the one I _know_ needs it).

Could we make just the IDE driver itself enable interrupts? Sure. But that
means that the interrupt controller code needs to know that some drivers
may enable interrupts and they may have recursive interrupts occurring. In
which case you really do want a flag anyway, since the interrupt
controller may well end up deciding that the "we're not going to enable
interrupts" case allows certain optimizations (like just ACK'ing it
immediately and not masking it at the interrupt controller).

So this is not as simple as your patch. Not even _nearly_.

Linus

2009-03-02 17:54:18

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Monday 02 March 2009, Linus Torvalds wrote:
>
> On Mon, 2 Mar 2009, Peter Zijlstra wrote:
> >
> > Would you be willing to take such a patch?
>
> Yes - some day.
>
> The "irq's disabled fastpath" thing has been there since pretty much day
> one, because some irq handlers always wanted it. Making it the default
> (and the only choice) is fine.
>
> HOWEVER.
>
> The fact is, some interrupt handlers are too slow for this. If you are
> using a PIO IDE driver, not enabling interrupts will not only make you
> lose serial line interrupts like mad (even at slow speeds), but has
> historically resulted in even timer interrupts being lost because the IDE
> layer is so slow.
>
> The thing is, with PIO, a 512-byte disk read ends up doing 256 16-bit word
> reads from the controller, each potentially up to 600ns long (PIO0
> timings). That's 150ms - for a single sector!
>
> Now, IDE controllers that we end up using PIO on all tend to be pretty
> old, but admittedly even the old ones are faster than the worst-case
> timings, so in practice you're not looking quite at that kind of horror
> case, but you're still looking at each sector transfer (256 16-bit words)
> taking on the order of tens of us.
>
> For example, Mode2 timings are probably still something we should consider
> realistic, and that's 240ns per word - a single sector now takes ~60ms to
> read off the disk.
>
> And we don't do single sectors. Most transfers will be 8 sectors (4k
> contiguos read). So now that 60ms is 480ms per such IDE interrupt. And
> even with the _good_ timings, we're certainly looking at a reduction of
> that to about half.
>
> Imagine what happens when we have interrupts disabled for half a second at
> a time.
>
> In other words, I do not think we are ready for this.
>
> Now: I suspect you don't have a single machine that does PIO by default,
> and you probably haven't had any for the last ten years. These things
> _are_ old. But they do exist.
>
> Could we make IRQF_DISABLED the default? Hell yes. We probably should. But
> we realistically can't remove the non-disabled case, until you also have a
> (tested) patch to fix these kinds of issues for the IDE PIO case (and
> possibly others - IDE is the one I _know_ needs it).
>
> Could we make just the IDE driver itself enable interrupts? Sure. But that

Actually it has been doing it for years (some host drivers don't do this by
default and still need "hdparm -u" or equivalent but I was planning to change
it for 2.6.30).

> means that the interrupt controller code needs to know that some drivers
> may enable interrupts and they may have recursive interrupts occurring. In
> which case you really do want a flag anyway, since the interrupt
> controller may well end up deciding that the "we're not going to enable
> interrupts" case allows certain optimizations (like just ACK'ing it
> immediately and not masking it at the interrupt controller).
> So this is not as simple as your patch. Not even _nearly_.

IDE IRQ handler (& probably many others) assume that IRQ layer won't be
trying to call it again while IRQ is being serviced -- is this going to
change with the introduction of threaded IRQs?

Thanks,
Bart

2009-03-02 17:56:36

by Andrew Morton

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 02 Mar 2009 13:21:17 +0100 Peter Zijlstra <[email protected]> wrote:

>
> People are playing odd games with IRQF_DISABLED, remove it.
>
> Its not reliable, since shared interrupt lines could disable it for you,
> and its possible and allowed for archs to disable IRQs to limit IRQ nesting.
>
> Therefore, simply mandate that _ALL_ IRQ handlers are run with IRQs disabled.
>
> [ This _should_ not break anything, since we've mandated that IRQ handlers
> _must_ be able to deal with this for a _long_ time ]
>
> IRQ handlers should be fast, no if buts and any other exceptions. We also have
> plenty instrumentation to find any offending IRQ latency sources.

Changelog is a bit cruddy. What are these "odd games" and why are they
so serious as to warrant a fairly drastic-looking patch?

Where are these odd games being played, and what are the implications
to those codesites of having their ball taken away? etc.

wrt the patch itself - it would make life easier if we were to leave
the IRQF_DISABLED definition in place for a while. I'm counting 47 new
additions of references to IRQF_DISABLED in linux-next/-mm. It would
grease the wheels a bit were these things (and out-of-tree drivers) to
not instabreak. One could add a nice runtime warning at request_irq()
time, leave that in place until everything is fixed up.

2009-03-02 17:57:20

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2009-03-02 at 09:11 -0800, Linus Torvalds wrote:

<story about how PIO IDE sucks>

> So this is not as simple as your patch. Not even _nearly_.

Yuckness, yes that does complicate matters significantly.

That leaves us trying to find all request_irq() invocations that do not
have IRQF_DISABLED.. /me puts it on the todo list.

2009-03-02 18:03:35

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2009-03-02 at 18:55 +0100, Bartlomiej Zolnierkiewicz wrote:
> IDE IRQ handler (& probably many others) assume that IRQ layer won't be
> trying to call it again while IRQ is being serviced -- is this going to
> change with the introduction of threaded IRQs?

No, the real hardirq handler needs to disable the irq line before
telling it wants the thread to run. The thread will then have to
re-enable the irq line after its done.

2009-03-02 18:07:50

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2009-03-02 at 18:55 +0100, Bartlomiej Zolnierkiewicz wrote:
> > Could we make just the IDE driver itself enable interrupts? Sure. But that
>
> Actually it has been doing it for years (some host drivers don't do this by
> default and still need "hdparm -u" or equivalent but I was planning to change
> it for 2.6.30).

What does it use to do that? A local_irq_enable() in hardirq context
will make lockdep yell bloody murder :-).

Then there is local_irq_enable_in_hardirq(), which plain simply lies
when lockdep is enabled -- IOW it would generate horrid irq latencies.

Not having to deal with IRQ recursion in lockdep helps.

2009-03-02 18:11:15

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2009-03-02 at 09:55 -0800, Andrew Morton wrote:
> On Mon, 02 Mar 2009 13:21:17 +0100 Peter Zijlstra <[email protected]> wrote:
>
> >
> > People are playing odd games with IRQF_DISABLED, remove it.
> >
> > Its not reliable, since shared interrupt lines could disable it for you,
> > and its possible and allowed for archs to disable IRQs to limit IRQ nesting.
> >
> > Therefore, simply mandate that _ALL_ IRQ handlers are run with IRQs disabled.
> >
> > [ This _should_ not break anything, since we've mandated that IRQ handlers
> > _must_ be able to deal with this for a _long_ time ]
> >
> > IRQ handlers should be fast, no if buts and any other exceptions. We also have
> > plenty instrumentation to find any offending IRQ latency sources.
>
> Changelog is a bit cruddy. What are these "odd games" and why are they
> so serious as to warrant a fairly drastic-looking patch?

See for example the stuff David Brownell was trying to pull off.

I was -- naively it turns out -- hoping it would be a simple matter of
cleaning up, as lockdep has been doing this for a long while now.

> Where are these odd games being played, and what are the implications
> to those codesites of having their ball taken away? etc.

Generation of terrible IRQ latency, or in David's case, more pain for
the abuse of the genirq layer.

> wrt the patch itself - it would make life easier if we were to leave
> the IRQF_DISABLED definition in place for a while. I'm counting 47 new
> additions of references to IRQF_DISABLED in linux-next/-mm. It would
> grease the wheels a bit were these things (and out-of-tree drivers) to
> not instabreak. One could add a nice runtime warning at request_irq()
> time, leave that in place until everything is fixed up.

Sure, can do.

2009-03-02 18:25:36

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Monday 02 March 2009, Peter Zijlstra wrote:
> On Mon, 2009-03-02 at 18:55 +0100, Bartlomiej Zolnierkiewicz wrote:
> > > Could we make just the IDE driver itself enable interrupts? Sure. But that
> >
> > Actually it has been doing it for years (some host drivers don't do this by
> > default and still need "hdparm -u" or equivalent but I was planning to change
> > it for 2.6.30).
>
> What does it use to do that? A local_irq_enable() in hardirq context
> will make lockdep yell bloody murder :-).
>
> Then there is local_irq_enable_in_hardirq(), which plain simply lies
> when lockdep is enabled -- IOW it would generate horrid irq latencies.
>
> Not having to deal with IRQ recursion in lockdep helps.

Lockdep problems are the least of our worries here (please read Linus'
original mail for details). If you're asking why it is not handled in
softirq -- that's solely because of the historical reasons and I have
high hopes w.r.t. threaded IRQs work here [that it will allow us to
address this issue without too much pain].

Thanks,
Bart

2009-03-02 18:30:25

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2 Mar 2009, Bartlomiej Zolnierkiewicz wrote:
> >
> > Could we make just the IDE driver itself enable interrupts? Sure. But that
>
> Actually it has been doing it for years (some host drivers don't do this by
> default and still need "hdparm -u" or equivalent but I was planning to change
> it for 2.6.30).

The IDE layer has the option to enable irq's during the transfer itself,
yes. But it actually works the reverse way from what you think: the irq
layer will enable interrupts, and the IDE layer will then _not_ disable
them during the transfer if you use "hdparm -u".

Look at ide_intr: it generally gets called with interrupts _enabled_
(because it doesn't use IRQF_DISABLED) and then it does:

spin_lock_irqsave(&hwif->lock, flags);
..
spin_unlock(&hwif->lock);
..
if (drive->dev_flags & IDE_DFLAG_UNMASK)
local_irq_enable_in_hardirq();
...
spin_lock_irq(&hwif->lock);
...
spin_unlock_irqrestore(&hwif->lock, flags);

where the magic thing is how it enables irqs again if the "irq unmask"
flag is set.

The point I'm making is that

- as far as the generic irq layer is concerned, IDE might as well have
interrupts enabled all the time (and disabling them is a local issue,
more to do with locking and with timing-induced hardware _bugs_ rather
than anything else)

- .. and more importantly, that is AS IT MUST BE. Because quite frankly,
if the irq handler enables interrupts (like IDE does), the generic IRQ
layer really _must_ know about it, because it may depend on
non-reentrancy of that interrupt.

(Small detail: the current irq layer actually does have that
"IRQ_INPROGRESS" flag to handle re-entrancy issues regardless of anything
else, so I guess we technically are robust in this regard. But that's
partly an SMP issue, and conceptually it's still really really important
information to know whether interrupts can nest. In practice, it does
affect things like stack usage too, for example, so even with
IRQ_INPROGRESS, there really is a difference between IRQF_DISABLED and not
having it on).

Linus

2009-03-02 18:49:16

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Monday 02 March 2009, Linus Torvalds wrote:
>
> On Mon, 2 Mar 2009, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > Could we make just the IDE driver itself enable interrupts? Sure. But that
> >
> > Actually it has been doing it for years (some host drivers don't do this by
> > default and still need "hdparm -u" or equivalent but I was planning to change
> > it for 2.6.30).
>
> The IDE layer has the option to enable irq's during the transfer itself,
> yes. But it actually works the reverse way from what you think: the irq

Hmm, I said nothing about how it is implemented in the IDE code itself. :)

> layer will enable interrupts, and the IDE layer will then _not_ disable
> them during the transfer if you use "hdparm -u".
>
> Look at ide_intr: it generally gets called with interrupts _enabled_
> (because it doesn't use IRQF_DISABLED) and then it does:
>
> spin_lock_irqsave(&hwif->lock, flags);
> ..
> spin_unlock(&hwif->lock);
> ..
> if (drive->dev_flags & IDE_DFLAG_UNMASK)
> local_irq_enable_in_hardirq();
> ...
> spin_lock_irq(&hwif->lock);
> ...
> spin_unlock_irqrestore(&hwif->lock, flags);
>
> where the magic thing is how it enables irqs again if the "irq unmask"
> flag is set.
>
> The point I'm making is that
>
> - as far as the generic irq layer is concerned, IDE might as well have
> interrupts enabled all the time (and disabling them is a local issue,
> more to do with locking and with timing-induced hardware _bugs_ rather
> than anything else)
>
> - .. and more importantly, that is AS IT MUST BE. Because quite frankly,
> if the irq handler enables interrupts (like IDE does), the generic IRQ
> layer really _must_ know about it, because it may depend on
> non-reentrancy of that interrupt.

Fixing this is on long-term TODO (there was just a ton of more high-prio
stuff to take care of first).

Thanks,
Bart

2009-03-02 18:52:33

by Vadim Lobanov

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Monday 02 March 2009 09:11:54 Linus Torvalds wrote:
> The thing is, with PIO, a 512-byte disk read ends up doing 256 16-bit word
> reads from the controller, each potentially up to 600ns long (PIO0
> timings). That's 150ms - for a single sector!

Out of curiosity, the peanut gallery wishes to ask:
Is the above supposed to be 600us (*1000), or 150us (/1000)? Probably the
latter.

> For example, Mode2 timings are probably still something we should consider
> realistic, and that's 240ns per word - a single sector now takes ~60ms to
> read off the disk.

Same factor of 1000 question here.

> And we don't do single sectors. Most transfers will be 8 sectors (4k
> contiguos read). So now that 60ms is 480ms per such IDE interrupt. And
> even with the _good_ timings, we're certainly looking at a reduction of
> that to about half.
>
> Imagine what happens when we have interrupts disabled for half a second at
> a time.

-- Vadim Lobanov

2009-03-02 18:55:55

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2 Mar 2009, Vadim Lobanov wrote:

> On Monday 02 March 2009 09:11:54 Linus Torvalds wrote:
> > The thing is, with PIO, a 512-byte disk read ends up doing 256 16-bit word
> > reads from the controller, each potentially up to 600ns long (PIO0
> > timings). That's 150ms - for a single sector!
>
> Out of curiosity, the peanut gallery wishes to ask:
> Is the above supposed to be 600us (*1000), or 150us (/1000)? Probably the
> latter.

Sorry, I was off by a lot. Yes. /1000. The end result ends up being lots
better (and I should have realized I did my conversion wrong, because the
times ended up being _so_ big), but not better enough - we've had dropped
timer interrupts etc on those kinds of machines unless you use "hdparm -u1".

Although it porbably does mean that the problem tends to be more in the
really bad mode0 case (600ns -> 150us/sector -> milliseconds for
multi-sector transfers).

I forget what our multi-sector limit is, I think it tends to be 16. So
you'll never get _really_ long irq-off times, but "several ms" is still
pretty damn bad.

Linus

2009-03-02 19:07:51

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2 Mar 2009, Bartlomiej Zolnierkiewicz wrote:
>
> Hmm, I said nothing about how it is implemented in the IDE code itself. :)

Bart, you're missing the point. I'm _also_ not at all interested in how
it's implemented in the IDE code.

The whole - and only - point is that there are drivers that are _known_ to
require non-IRQF_DISABLED semantics. IDE is one such one.

> Fixing this is on long-term TODO (there was just a ton of more high-prio
> stuff to take care of first).

Even if you can fix IDE to do everything using softirq's or other tricks
(threads, whatever), nothing really changes. It just means that now there
is one less driver that may need the non-IRQF_DISABLED semantics.

So I'm perfectly happy with

- making IRQF_DISABLED the _default_ (and make the value of the flag be
0, for example, so that people can still _use_ it)

- but when we find a driver that depended on irq's being on, have a new
flag like IRQF_REENABLE, which has the semantics of the current default
of !IRQF_DISABLED

but I don't think it's reasonable to just drop the support. IDE requires
it, and it's been a known requirement for basically forever. Other drivers
with similar issues? Who the heck knows?

I do know that the _generic_ SCSI layer used to have some insane reset
timeouts inside the interrupt handler, and literally *depended* on timer
interrupts happening while inside the SCSI interrupt. I'm hoping that's no
longer the case. But this is the kind of thing that has been going on.

Linus

2009-03-02 19:16:51

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Monday 02 March 2009, Linus Torvalds wrote:
>
> On Mon, 2 Mar 2009, Bartlomiej Zolnierkiewicz wrote:
> >
> > Hmm, I said nothing about how it is implemented in the IDE code itself. :)
>
> Bart, you're missing the point. I'm _also_ not at all interested in how
> it's implemented in the IDE code.
>
> The whole - and only - point is that there are drivers that are _known_ to
> require non-IRQF_DISABLED semantics. IDE is one such one.
>
> > Fixing this is on long-term TODO (there was just a ton of more high-prio
> > stuff to take care of first).
>
> Even if you can fix IDE to do everything using softirq's or other tricks
> (threads, whatever), nothing really changes. It just means that now there
> is one less driver that may need the non-IRQF_DISABLED semantics.

I didn't meant non-IRQF_DISABLED semantics (I see absolutely no point in
changing it) but IDE's internal "disable IRQs just to enable them" issue.

[ Sorry for poorly explaining things. ]

Thanks,
Bart

2009-03-02 21:03:31

by Russell King

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, Mar 02, 2009 at 04:59:48PM +0100, Peter Zijlstra wrote:
> The idea is to mandate IRQF_DISABLED for everybody, and since it doesn't
> do any good to have it a flag if you have it always enabled, remove
> it ;-)

Bad. Very bad.

So, when I use PIO IDE on embedded platforms, I have to have all other
interrupts locked out for things like serial ports, network interfaces
with small packet buffers, etc and I can't specify IRQF_DISABLE on
such network interfaces anymore?

No, this is a step backwards. Please don't go there.

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

2009-03-02 21:18:26

by Ingo Molnar

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

* Russell King <[email protected]> wrote:

> On Mon, Mar 02, 2009 at 04:59:48PM +0100, Peter Zijlstra wrote:
> > The idea is to mandate IRQF_DISABLED for everybody, and since it doesn't
> > do any good to have it a flag if you have it always enabled, remove
> > it ;-)
>
> Bad. Very bad.

btw., your mailer is very broken and generates:

Mail-Followup-To: Peter Zijlstra <[email protected]>,
Matthew Wilcox <[email protected]>, Ingo Molnar <[email protected]>,
Thomas Gleixner <[email protected]>,
Linus Torvalds <[email protected]>,
lkml <[email protected]>,
linux-arch <[email protected]>,
Andrew Morton <[email protected]>

this messes up the To: line in a big way. Please stop it.

Ingo

2009-03-02 21:18:40

by Alan

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

> Now, IDE controllers that we end up using PIO on all tend to be pretty
> old, but admittedly even the old ones are faster than the worst-case
> timings, so in practice you're not looking quite at that kind of horror
> case, but you're still looking at each sector transfer (256 16-bit words)
> taking on the order of tens of us.

The worst case timings are far worse because of IORDY.

> Now: I suspect you don't have a single machine that does PIO by default,
> and you probably haven't had any for the last ten years. These things
> _are_ old. But they do exist.

I bet you do for some stuff. Most CF cards are PIO only. Several
controllers need to do PIO for some corner cases. Almost all pre-AHCI
controllers issue command blocks via PIO.

Worse still of course some controllers have an erratum where touching
some of the other registers during a transfer [which is what you need to
do to see if you are the IRQ source] corrupts the transfer. Not all
devices honour the IRQ masking bit on the device either. That makes the
generalised case of doing both unmasked transfers after an IRQ and the
errata cases of handling IRQs you can't mask and must leave disabled
rather more exciting than is nice.

Alan

2009-03-05 15:41:15

by Mark Lord

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

Linus Torvalds wrote:
>
> Although it porbably does mean that the problem tends to be more in the
> really bad mode0 case (600ns -> 150us/sector -> milliseconds for
> multi-sector transfers).
..

Heh.. it's worse that that: even with commonplace mode4 (120ns) transfers,
each PCI bus transaction from start to finish often takes about 1us,
so for a 16-sector multi-sector read, that means 4ms+ with interrupts off.

> I forget what our multi-sector limit is, I think it tends to be 16. So
> you'll never get _really_ long irq-off times, but "several ms" is still
> pretty damn bad.
..

It's a device limit, usually 8 or 16 sectors for nearly all drives now.

Cheers

2009-03-06 09:00:01

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Mon, 2009-03-02 at 09:11 -0800, Linus Torvalds wrote:
>
> On Mon, 2 Mar 2009, Peter Zijlstra wrote:
> >
> > Would you be willing to take such a patch?
>
> Yes - some day.
>
> The "irq's disabled fastpath" thing has been there since pretty much day
> one, because some irq handlers always wanted it. Making it the default
> (and the only choice) is fine.

.../...

I tend to disagree... (not -that- strongly but I felt like saying it
anyway :-) some archs have a reasonably nice support in the PIC for
interrupt priorities, allowing higher priority interrupts to "preempt"
lower priority ones, which this would effectively render useless.

Also, while yes, I agree, interrupts handlers -should- be short in
practice IDE is far from being the only example where this is not the
case and so we would delay timer interrupts for example for a
significant amount of time (or serial, that's another good example).

Also, we use the priority on some platform to have a high priority used
as a kind of "debugger" NMI .. ie, we don't have a real NMI but it's
better than nothing and here too, this would break it.

I don't see us having such a strong benefit from this... in fact, with
things like -rt, interrupts get moved to threads no ? Thus they
typically run with interrupts enabled... why have a different behaviour
on non-rt ? or I am missing something ? (I'm not terribly familiar with
the -rt stuff here so I probably am missing something).

Cheers,
Ben.

2009-03-06 09:12:47

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Fri, 2009-03-06 at 19:58 +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2009-03-02 at 09:11 -0800, Linus Torvalds wrote:
> >
> > On Mon, 2 Mar 2009, Peter Zijlstra wrote:
> > >
> > > Would you be willing to take such a patch?
> >
> > Yes - some day.
> >
> > The "irq's disabled fastpath" thing has been there since pretty much day
> > one, because some irq handlers always wanted it. Making it the default
> > (and the only choice) is fine.
>
> .../...
>
> I tend to disagree... (not -that- strongly but I felt like saying it
> anyway :-) some archs have a reasonably nice support in the PIC for
> interrupt priorities, allowing higher priority interrupts to "preempt"
> lower priority ones, which this would effectively render useless.
>
> Also, while yes, I agree, interrupts handlers -should- be short in
> practice IDE is far from being the only example where this is not the
> case and so we would delay timer interrupts for example for a
> significant amount of time (or serial, that's another good example).
>
> Also, we use the priority on some platform to have a high priority used
> as a kind of "debugger" NMI .. ie, we don't have a real NMI but it's
> better than nothing and here too, this would break it.

If you have distinct interrupt priorities, you can

1) provide an interrupt stack for each priority
2) mask all lower priorities when handling one

Would that not work?

> I don't see us having such a strong benefit from this... in fact, with
> things like -rt, interrupts get moved to threads no ? Thus they
> typically run with interrupts enabled... why have a different behaviour
> on non-rt ? or I am missing something ? (I'm not terribly familiar with
> the -rt stuff here so I probably am missing something).

Threaded interrupts are quite a different beast. To make best use of
them device drivers will have to be re-written. Hence the opt-in
approach.

The problems with enabling irqs in hardirq handlers are that you get
unlimited irq nesting, which is bad for your stack, furthermore, somehow
people thing it makes things 'faster' because the irq-off latency goes
down.

The latter just isn't true, as you still have preemption disabled, so
everything but irqs still suffers.

The only way to make things low-latency is to pull work out of
non-preemptable context. Using threaded IRQs is one way to do that.

2009-03-06 09:59:43

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

> If you have distinct interrupt priorities, you can
>
> 1) provide an interrupt stack for each priority
> 2) mask all lower priorities when handling one
>
> Would that not work?

The PIC does that already. IE. it will only interrupt again before
->eoi() for an interrupt of a higher priority. But by using
IRQF_DISABLED, you mask interrupts in the core, and thus effectively
completely prevents the whole thing.

> The problems with enabling irqs in hardirq handlers are that you get
> unlimited irq nesting, which is bad for your stack, furthermore, somehow
> people thing it makes things 'faster' because the irq-off latency goes
> down.

No, you don't get unlimited IRQ nesting, at least not on sane archs with
a decent PIC that does things like what I described above :-)

> The latter just isn't true, as you still have preemption disabled, so
> everything but irqs still suffers.
>
> The only way to make things low-latency is to pull work out of
> non-preemptable context. Using threaded IRQs is one way to do that.

Cheers,
Ben.

2009-03-06 10:06:35

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

> No, you don't get unlimited IRQ nesting, at least not on sane archs with
> a decent PIC that does things like what I described above :-)

Note that if unlimited nesting is what you are afraid of, you can do
something along the lines of:

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

With arch_nest_irq() then set to always return 0 if you want, or be
smarter and check how much stack space is left, and return 1 only if
above a reasonable threshold.

Cheers,
Ben.

2009-03-06 10:24:30

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Fri, 2009-03-06 at 20:59 +1100, Benjamin Herrenschmidt wrote:
> > If you have distinct interrupt priorities, you can
> >
> > 1) provide an interrupt stack for each priority
> > 2) mask all lower priorities when handling one
> >
> > Would that not work?
>
> The PIC does that already. IE. it will only interrupt again before
> ->eoi() for an interrupt of a higher priority. But by using
> IRQF_DISABLED, you mask interrupts in the core, and thus effectively
> completely prevents the whole thing.
>
> > The problems with enabling irqs in hardirq handlers are that you get
> > unlimited irq nesting, which is bad for your stack, furthermore, somehow
> > people thing it makes things 'faster' because the irq-off latency goes
> > down.
>
> No, you don't get unlimited IRQ nesting, at least not on sane archs with
> a decent PIC that does things like what I described above :-)

Right, welcome to x86 ;-)

Sounds to me we need to extend genirq a bit... Thomas?

2009-03-06 17:03:11

[permalink] [raw]

Subject: Re: [RFC][PATCH] irq: remove IRQF_DISABLED

On Fri, 2009-03-06 at 11:24 +0100, Peter Zijlstra wrote:
> On Fri, 2009-03-06 at 20:59 +1100, Benjamin Herrenschmidt wrote:
> > > If you have distinct interrupt priorities, you can
> > >
> > > 1) provide an interrupt stack for each priority
> > > 2) mask all lower priorities when handling one
> > >
> > > Would that not work?
> >
> > The PIC does that already. IE. it will only interrupt again before
> > ->eoi() for an interrupt of a higher priority. But by using
> > IRQF_DISABLED, you mask interrupts in the core, and thus effectively
> > completely prevents the whole thing.
> >
> > > The problems with enabling irqs in hardirq handlers are that you get
> > > unlimited irq nesting, which is bad for your stack, furthermore, somehow
> > > people thing it makes things 'faster' because the irq-off latency goes
> > > down.
> >
> > No, you don't get unlimited IRQ nesting, at least not on sane archs with
> > a decent PIC that does things like what I described above :-)
>
> Right, welcome to x86 ;-)

Ok, people put me straight here. Since linux not support interrupt
priorities, wouldn't it simply be a matter of implementing
local_irq_en/dis-able() as masking the lowest level you use to run
normal interrupts on?

That will leave your other interrupt level available as NMI/debug
thingies.

2009-03-06 21:41:26