Question about IRQ_PENDING/IRQ_REPLAY

Question about IRQ_PENDING/IRQ_REPLAY

[Benjamin Herrenschmidt]

Hi Linus !

I've some questions regarding the behaviour of arch/i386/kernel/irq.c
regarding IRQ_PENDING and IRQ_REPLAY.

Especially, my question is about the code in enable_irq() which checks
for IRQ_PENDING, and then 
"replays" the interrupt by asking the APIC to issue it again.

I don't have a simple way on PPC to cause the interrupt to happen again,
as you can imagine this is rather controller-specific. However, looking
at the code closely, I couldn't figure out a case where having
IRQ_PENDING in enable_irq() makes sense.

How can IRQ_PENDING happen to be set on an IRQ_DISABLED interrupt, and
why would that matter (why should we take this interrupt) ?

AFAIK, IRQ_PENDING can only be set as a result of a call to do_IRQ().
Since we loop when calling the actual handler, I fail to see how we could
"miss" an interrupt. If the interrupt is actually disabled, we should not
get it at all, and if we did, I don't see why it would matter to resend
it when it gets enabled since disabled interrupts are supposed to be
ignored (well, they are by most PICs). Obviously, this matters only for
an edge interrupt as level ones will stay asserted until the device is happy.

I'd be glad if you could take the time to enlighten me about this as I'm
trying to make the PPC code as close as the i386, according to your
comment stating that it would be generic in 2.5, and I don't like having
code I don't fully understand ;)

Regards,
Ben.

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Linus Torvalds]

On Sat, 3 Mar 2001, Benjamin Herrenschmidt wrote:
> 
> Especially, my question is about the code in enable_irq() which checks
> for IRQ_PENDING, and then 
> "replays" the interrupt by asking the APIC to issue it again.
> 
> I don't have a simple way on PPC to cause the interrupt to happen again,
> as you can imagine this is rather controller-specific. However, looking
> at the code closely, I couldn't figure out a case where having
> IRQ_PENDING in enable_irq() makes sense.

It only makes sense for broken irq controllers. And most aren't. You can
likely ignore it - and note how even on an x86, most of the irq controller
code _does_ ignore it. 

> How can IRQ_PENDING happen to be set on an IRQ_DISABLED interrupt, and
> why would that matter (why should we take this interrupt) ?

We have to take the interrupt - we can't just drop it on the floor. And
some stupid interrupts controllers _will_ ignore it.

In particular, if an edge-triggered interrupt comes in on an x86 IO-APIC
while that interrupt is disabled, enabling the interrupt will have caused
that irq to get dropped. And if it gets dropped, it will never ever happen
again: the interrupt line is now active, and there will never be another
edge again.

> I'd be glad if you could take the time to enlighten me about this as I'm
> trying to make the PPC code as close as the i386, according to your
> comment stating that it would be generic in 2.5, and I don't like having
> code I don't fully understand ;)

You likely don't have this problem at all. Most sane interrupt controllers
are level-triggered, and won't show the problem. And others (like the
i8259) will see a disabled->enabled transition as an edge if the interrupt
is active (ie they have the edge-detection logic _after_ the disable
logic), and again won't have this problem.

		Linus

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Cort Dougan]

} > I don't have a simple way on PPC to cause the interrupt to happen again,
} > as you can imagine this is rather controller-specific. However, looking
} > at the code closely, I couldn't figure out a case where having
} > IRQ_PENDING in enable_irq() makes sense.
} 
} It only makes sense for broken irq controllers. And most aren't. You can
} likely ignore it - and note how even on an x86, most of the irq controller
} code _does_ ignore it. 

We do have broken interrupt controllers in this respect.  We already have a
way of handling it.  Ben, take a look at set_lost().

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Benjamin Herrenschmidt]

>We do have broken interrupt controllers in this respect.  We already have a
>way of handling it.  Ben, take a look at set_lost().

Heh, I know, thanks ;)

However, our current scheme implies a hack to __sti() that I'd like to get
rid of since it adds an overhead allover the place that could probably be
localized if we managed to force an interrupt (using the DEC for example,
or using a mac-specific device as this controller only exist on macs anyway).

Also, we currently don't use the same mecanism as i386, and since Linus
expressed his desire to have irq.c become generic, I'm trying to make sure
I fully understand it before merging in PPC the bits that I didn't merge
them yet.

Ben.

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Cort Dougan]

} Also, we currently don't use the same mecanism as i386, and since Linus
} expressed his desire to have irq.c become generic, I'm trying to make sure
} I fully understand it before merging in PPC the bits that I didn't merge
} them yet.

More generic in terms of using irq_desc[] and some similar structures I can
see.  Making do_IRQ() and enable/disable use the same names and structures
as x86 isn't sensible.  They're different ports, with different design
philosophies.

I don't believe that the plan is a common irq.c - lets stay away from that.

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Benjamin Herrenschmidt]

>More generic in terms of using irq_desc[] and some similar structures I can
>see.  Making do_IRQ() and enable/disable use the same names and structures
>as x86 isn't sensible.  They're different ports, with different design
>philosophies.
>
>I don't believe that the plan is a common irq.c - lets stay away from that.

Why ? Except for a few things like irq probing, our irq.c is already very
similar
to i386 (well, mostly because I merged most of it some time ago) and I
don't see
why it would be a bad thing. The design of irq.c makes it perfectly
adapted to our
needs, there's nothing really x86-specific in it, it handles things we
need to be
handled correctly, does nothing more than what we need (well it does, but
those parts,
mostly the irq locking, got already removed), etc...

I did that merge in the first place because I wanted the depth support in
enable/disable_irq, and more fine-grained spinlocking. I really see
nothing wrong
in the way irq.c works, I really think that except the small added bit we have
in our do_IRQ() to call ppc_md.get_irq(), it's perfectly adapted to our needs.

Remember that it allowed to remove the (mostly useless) post_irq() thing
we had ?
It also allow proper implement of irq distribution even with controllers
that could
trigger the same IRQ on several CPUs, re-entrancy in the handler if we do
early-eoi
without masking an edge interrupt is also handled properly, enable/
disable from
within the handler too, all sorts of things our previous code didn't do right.

The only thing I added to the core irq.c code is that IRQ_PERCPU flag
that prevents
IRQ_INPROGRESS to be set. It's a bit hackish but allows our IPIs to use a
single
desc for all CPUs without beeing mutually exclusive. 

Ben.

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Benjamin Herrenschmidt]

>handled correctly, does nothing more than what we need (well it does, but
>those parts,
>mostly the irq locking, got already removed), etc...

Sorry, I meant mostly the irq probing

Ben.

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Linus Torvalds]

In article <20010304230707.L2565@ftsoj.fsmlabs.com>,
Cort Dougan  <cort@fsmlabs.com> wrote:
>
>More generic in terms of using irq_desc[] and some similar structures I can
>see.  Making do_IRQ() and enable/disable use the same names and structures
>as x86 isn't sensible.  They're different ports, with different design
>philosophies.
>
>I don't believe that the plan is a common irq.c - lets stay away from that.

Most of arch/i386/kernel/irq.c should really be fairly generic, and the
fact is that a lot of the issues are a lot more subtle than most people
really end up realizing.  I got really tired of seeing the same old SMP
problems that had long since been fixed on x86 show up on other
architectures. 

So the plan is to have at least a framework for allowing other
architectures to use a common irq.c if they want to. Probably not force
it down peoples throats, because this is an area where the differences
can be _so_ large that it might not be worth it for everybody. But I
seriously doubt that PPC is all that different.

And I seriously doubt that PPC SMP irq handling has gotten _nearly_ the
amount of testing and hard work that the x86 counterpart has. Things
like support for CPU affinity, per-irq spinlocks, etc etc.

Now, I'm not saying that irq.c would necessarily work as-is. It probably
doesn't support all the things that other architectures might need (but
with three completely different irq controllers on just standard PCs
alone, I bet it supports most of it), and I know ia64 wants to extend it
to be more spread out over different CPU's, but most of the high-level
stuff probably _can_ and should be fairly common.

		Linus

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Cort Dougan]

We have about 12 interrupt controllers we end up using on PPC.  I'm
suspicious of any effort to base Linux/PPC generic interrupt control code
paths on a software architecture that's been tested with 3.  More to the
point, we get ASIC's that roll in a standard interrupt controller and add
some "improvements" at the same time.

As for SMP, I'm sure x86 has seen a lot more testing.  I'm not going to
sacrifice time-tested stability so we can look just like x86 and get clean
SMP locking.  We've lost stability already because of some PPC folks'
excitement at getting us to behave like x86 in irq.c.

As for a generic irq.c, as a guiding light, I'm all for it.  It'll
certainly help work with RTLinux.  It'll also help new architectures by
giving them a snap-together port construction kit.  I'm still not going to
sacrifice stability in the short-term for this nice feature in the
long-run.  I'm pretty sure we agree on this.

} Most of arch/i386/kernel/irq.c should really be fairly generic, and the
} fact is that a lot of the issues are a lot more subtle than most people
} really end up realizing.  I got really tired of seeing the same old SMP
} problems that had long since been fixed on x86 show up on other
} architectures. 
} 
} So the plan is to have at least a framework for allowing other
} architectures to use a common irq.c if they want to. Probably not force
} it down peoples throats, because this is an area where the differences
} can be _so_ large that it might not be worth it for everybody. But I
} seriously doubt that PPC is all that different.
} 
} And I seriously doubt that PPC SMP irq handling has gotten _nearly_ the
} amount of testing and hard work that the x86 counterpart has. Things
} like support for CPU affinity, per-irq spinlocks, etc etc.
} 
} Now, I'm not saying that irq.c would necessarily work as-is. It probably
} doesn't support all the things that other architectures might need (but
} with three completely different irq controllers on just standard PCs
} alone, I bet it supports most of it), and I know ia64 wants to extend it
} to be more spread out over different CPU's, but most of the high-level
} stuff probably _can_ and should be fairly common.

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Benjamin Herrenschmidt]

>We have about 12 interrupt controllers we end up using on PPC.  I'm
>suspicious of any effort to base Linux/PPC generic interrupt control code
>paths on a software architecture that's been tested with 3.  More to the
>point, we get ASIC's that roll in a standard interrupt controller and add
>some "improvements" at the same time.

Well, I personally don't see what would be a problem... Of course, the 
current i386 irq.c cannot be re-used completel "as is". The bit of code
that gets the actual irq number has to be arch specific. But most of the
locking issues are completely platform neutral.

I personally see that code as a good framework that provides many features
that may or may not be neccessary depending on the level of brokenness of
a given interrupt controller.

>As for SMP, I'm sure x86 has seen a lot more testing.  I'm not going to
>sacrifice time-tested stability so we can look just like x86 and get clean
>SMP locking.  We've lost stability already because of some PPC folks'
>excitement at getting us to behave like x86 in irq.c.

We lost stability ? Hrm... If we had ever a problem with SMP, it was in the
openpic code, and apparently, due to a HW bug. I don't think the new irq.c
code in itself caused us to lose stability. I actually do think it improved
the locking, and so, stability.

>As for a generic irq.c, as a guiding light, I'm all for it.  It'll
>certainly help work with RTLinux.  It'll also help new architectures by
>giving them a snap-together port construction kit.  I'm still not going to
>sacrifice stability in the short-term for this nice feature in the
>long-run.  I'm pretty sure we agree on this.

Well, we have been running this new irq.c which I partially based on
i386 for some monthes now, and had enough time to iron out most problems.
Again, all the stability problems we had so far were related to the openpic
implementation, I don't remember seeing one stability problem reported so
far that was related to irq.c. And I've been running a couple of dual
G4s without much trouble for some time now. 
We do (did ?) have a problem with irq distribution on SMP with openpic. I'm
not sure we yet know exactly why, according to both you and IBM people, we
are running over an HW bug of the openpic core. I see nothing in irq.c
that can cause this.

On the other hand, the new irq.c brings the irq depth handling, the ability
to call enable/disable from within the handler (I've been wanting that for
some time for the PMU driver), proper spinlock'ing, etc...
And last, but not least, consistent semantics of enable/disable irq
exposed to drivers (especially things like disable_irq() actually waiting
for that irq to be completed on any other CPU).

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Benjamin Herrenschmidt]

>And I seriously doubt that PPC SMP irq handling has gotten _nearly_ the
>amount of testing and hard work that the x86 counterpart has. Things
>like support for CPU affinity, per-irq spinlocks, etc etc.

Some of those are the reason I moved part of the x86 irq.c code to PPC
indeed.

>Now, I'm not saying that irq.c would necessarily work as-is. It probably
>doesn't support all the things that other architectures might need (but
>with three completely different irq controllers on just standard PCs
>alone, I bet it supports most of it), and I know ia64 wants to extend it
>to be more spread out over different CPU's, but most of the high-level
>stuff probably _can_ and should be fairly common.

And I think they are. One thing is that if made "common", do_IRQ have to
be split into an arch-specific function that retrives the irq_number (and
does the ack on some controller), and the actual "dispatch" function that
does all the flags game and calls the handler.

I've slightly extended it using the IRQ_PERCPU flag to prevent IRQ_INPROGRESS
from ever beeing set (a bit hackish but I wanted that for IPIs since they
use ordinary irq_desc structures for us in most cases).

Ben.

Re: Question about IRQ_PENDING/IRQ_REPLAY

[Benjamin Herrenschmidt]

>In particular, if an edge-triggered interrupt comes in on an x86 IO-APIC
>while that interrupt is disabled, enabling the interrupt will have caused
>that irq to get dropped. And if it gets dropped, it will never ever happen
>again: the interrupt line is now active, and there will never be another
>edge again.

Ok, I see. We have a different issue with the old Apple IRQ controller that
can lose interrupts if they are active when re-enabled. We currently rely
on a hack to work aroud this that may re-send interrupts, but that involves
hacking into __sti() to check for lost interrupts, which is bad.

Basically, even a level interrupt, if active while re-enabled, will not be
sent by the pic to the CPU, and so further interrupts will be blocked too.
We have some code in enable_irq() that can detect this case, but re-triggering
the interrupt is not really simple and requires the __sti() hack for now. 

I beleive we may have a way to re-trigger the interrupt without having to
hack __sti() by using a fake timer interrupt. I'll look into this, but in
that case, the code can be mostly self-contained in enable_irq, we will
probably not need to play with IRQ_PENDING & IRQ_REPLAY flag at all.

>> I'd be glad if you could take the time to enlighten me about this as I'm
>> trying to make the PPC code as close as the i386, according to your
>> comment stating that it would be generic in 2.5, and I don't like having
>> code I don't fully understand ;)
>
>You likely don't have this problem at all. Most sane interrupt controllers
>are level-triggered, and won't show the problem. And others (like the
>i8259) will see a disabled->enabled transition as an edge if the interrupt
>is active (ie they have the edge-detection logic _after_ the disable
>logic), and again won't have this problem.

Well, Apple now uses OpenPICs, but all slightly older macs had a home-made
Apple controller that had the above issue :( In fact, it can happpen with
both and and level interrupts for us.

Regards,
Ben.