Messages in this thread | | | Subject | Re: [RFC][PATCH] Cascaded interrupts: a simple solution | From | Benjamin Herrenschmidt <> | Date | Thu, 11 May 2006 21:47:47 +1000 |
| |
> We have solved this problem in the genirq patchset, but in a different > way. No matter how much i'd like to see a simple solution for a hard > problem, we believe the SA_CASCADEIRQ method is insufficient, for a > number of reasons. > > The main goal of genirq: to merge the ARM architecture to the generic > IRQ layer and thus make the generic IRQ layer truly generic. Secondary > goal: to not disturb any of the current genirq architectures (i.e. stay > fully compatible). The ARM IRQ layer is exotic in some respects, but it > is certainly the most advanced IRQ layer in terms of PIC-topology > handling, given the rich variety of ARM hardware in existence.
Well... I yet have to look at the current ARM stuff from Russell, but I've looked at Thomas "port" and there's a lot of stuff in there that I dislike. I yet have to be convinced that whatever it provides can't be done with the current code. Now of course, I'll be happy to discuss every single technical detail until we come to an agreement :) But I'm afraid this won't make it for 2.6.18. If it does, fine. If it doesn't, I'd like my approach to be merged for that kernel version as I really want that in for handling some of the upcoming cell stuff.
I plan to spend a few hours as soon as I find the time to explain more in depth the technical reasons why I don't like some aspects of Thomas work, but I want first to take the time to better review the current ARM implementation and I was sort-of waiting for him to go public so we have an open discussion.
> Just some stats to back this up: arch/arm*/ has 118 separate PIC > implementations, and amongst them there are more than 40 that need > demultiplex handlers. As a comparison, arch/[ppc/powerpc] sports 27 PIC > implementations, amongst which there are 5 that need demultiplex > handlers. arch/i386 has 10 PIC implementations and none of them need > demultiplex handlers [there are minor forms of cascading in x86 > hardware, but none need true irq-vector demultiplexing].
Heh, we don't need to do a pissing contest here :)
> Most architectures modeled their IRQ layer after i386 IRQ layer, and > this resulted in the first phase of generic IRQ layer being quite > similar to the i386 layer. But the largest and most versatile Linux IRQ > subsystem was left out of the original genirq design (done by yours > truly), and many good bits (amongst them the right way to handle > cascading/chaining) i missed.
Heh, I know and that's why I CC'ed you :)
>From what I've seen of Thomas initial work, if you remove all of the renaming of things and splitting into irq_chip vs. irq_type, it essentially boils down to having a way to re-implement the do_IRQ logic (and it's flag/locking manipulations) based on the IRQ type. However, from what I've seen, compared to the current code, it doens't provide much that the current code can't do with maybe few bits & pieces changed, adds overhead (another layer of indirect funciton calls is expensive) and forces us in a model that don't match most of the modern/advanced irq controllers nicely without bloat (basically having spearate irq_type for them).
I'll get in more details later (it's a bit late here and I'm tired :)
> Current status of genirq: it has been part of the -rt tree for more than > a year, and an earlier version has been sent to lkml once already - > Thomas has submitted an improved version of it to the ARM lists roughly > a week ago and it is currently being tested on many ARM boards, with > good results. > > (I suspect you are aware of our genirq efforts, hence did you Cc: Thomas > and me? If you are aware of it, please address the differences between > the two approaches and outline why you chose a different solution - > thanks!)
I will. Not tonight though.
> Most PIC implementations do not need to worry about PIC cascading, and > neither the SA_CASCADEIRQ nor the genirq approach impacts them. So my > analysis of your patch only involves true cascaded PICs and the ways to > support them cleanly. > > For cascaded PICs, the main problem with the SA_CASCADEIRQ approach is > that it assumes that cascading/demultiplexing can be handled via a > "return irq" method, in an iterative way.
Yup.
> But this does not match the demultiplexing model that happens on the > majority of ARM boards for example: there a bitmask is read from the > secondary interrupt controller, and that bitmask might have multiple > bits set.
Which is also what happens on some PowerMacs
> By returning only one bit [which iteration model your > interface forces], the other bits can be lost. On some hardware those > missed bits might be regenerated, but there is PIC hardware where that > information is permanently lost and we end up losing interrupts.
It's fairly simple to handle that in the cascaded controllre driver by or'ing read bits into a "pending" mask and clearing them as they get returned to the upper layer. I don't see the need of adding layers of indirections to handle that simple trick. Looping is the right way to go as my patches imlement to then get one of them at a time. Now if you are worried that we might end up artificially prioritizing the first ones that way, them we can simply also cache the last bit "scanned" in the pending mask and start from there on the next iteration. Again, very simple code that doesn't imho requires turning the whole model upside down.
> Such mask-based demultiplexing PICs are not limited to ARM, they occur > in the PPC world too, for example in arch/ppc/syslib/m82xx_pci.c, > pq2pci_irq_demux():
.../...
> If PCI_INT_MASK_REG is a read-once register, then the SA_CASCADEIRQ > method could result in lost interrupts. (i'm not totally sure, but i > think in this specific case that register is read-once and it also > involves an auto-ack. In any case, there definitely is ARM hardware > where this equivalent register is read-once.)
If implemented stupidly it surely would :) But as I wrote above, it can easily be implemented in a way that won't result in lost interrupts.
> Even if PCI_INT_MASK_REG could be read in a non-destructive way, > multiple bits would need multiple iterations and multiple (unnecessary) > passes over the whole bitmask - and they would thus also need > unnecessary IO cycles to re-fetch the mask itself.
True, my approach would cause re-fetches from the mask though that could be avoided with careful coding as well (only re-fetching when the current "pointer" into the mask wraps around). I yet have to be convince though that your model, while maybe slightly simplifying the implementation of the cascaded controller "fetch" routine, justifies the overhead & bloat of the generic code.
An example is controllers like OpenPIC/MPIC that handle masking and priority stacking transparently currently require very little code in the begin and end handlers of irq_desc. With Thomas model, begin() and end() are gone in irq_chip. Thus, I would end up causing a full mask & umask (significant overhead) if using the standard handlers for level and edge irq_types. Which basically means that I would have to implement different irq_types to avoid that.
In a similar vein, I'm losing startup/shutdown from the controller which I'm using to internally setup the HyperTransport APICs. I could do it elsewhere but it's not as nice imho... or use custom irq_type's. Same for XICS.
Finally, set_affinity() is also missing from irq_chip.
So you basically moved all of the previous interface (begin/end/startup/shutdown/etc...) to the IRQ controller to the new "irq_type" layer which then "emulates" the functionality of a smart irq controller on top of an "irq_chip" which represents a totally dumb controller than can only mask/unmask.
I don't think it's the right approach.
> (Depending on how many different interrupt sources a secondary PIC > connects, and how frequently those are risen, this might or might not be > a real performance problem. But in any case, the "return irq" method is > certainly an ugly and unnatural model for such PICs and there is no > clean way to handle this type of cascading via the 'return irq' method.) > > So the solution we took in genirq was to delegate the act of > demultiplexing into the _demultiplexing handler_, by adopting the ARM > IRQ layer's approach of calling desc_handle_irq(irq, desc, regs). For > example:
The "handler" is the irq_type structure. Looking at all irq_type implementations in thomas patch, it's actually the only thing that changes from a type to another.... The rest all goes to 'default'. I think there is some over-engineering there.
If you guys really want to keep the concept of having a "handler" for different implementation of controllers/irqs, them it could have simply been part of the irq_desc.
I yet also have to figure out if your approach allows the cascade to be shared with a normal IRQ. Mine does. (Yours might, I yt have to figure out what happens if you end up trying to define more than one type per irq, I suppose it's not possible but I don't have the code at hand right now)
> desc_handle_irq() does the locking and the calling of the highlevel irq > handler of the secondary PIC. [which then processes the device IRQ > handler actions and does any pre/post ACKing logic] There is no impact > to non-cascading PIC designs in this model either.
Except for one more level of function pointer indirection which can be fairly expensive.
> Another, conceptual level problem is that (ab-)using the irq handler > methods to return an actual interrupt number is a layering violation. > There is not much in common between an IRQ demultiplexer function and an > interrupt handler function. The act of cascading two PICs _inevitably_ > means that there is a 1:N relationship between the two PICs, while an > IRQ handler is normally a 1:1 relationship between a hardware device and > a PIC. (there are exceptions like shared interrupts, but the norm we are > designing for is a 1:1 relationship) There are many other fundamental > differences too. Thus in our genirq work we have separated these two > concepts.
This is not completely true. It's actually fairly common that a device itself is a demuxer. It's interrupt handler then reads a status register to check which of the many local sources triggered. I don't think there is any fundamental difference between a cascaded controller and a device.
> In terms of patch merging (unless there are some arguments i've > overlooked), due to the reasons above i'm against merging SA_CASCADEIRQ, > even if it's relatively simple. It does not solve the demultiplexing > problem for most of the ARM handlers (nor for the PPC example i cited), > hence a separate variant has to be implemented anyway which results in > unnecessary code and concept duplication.
I think it does solve the problem due to my above explanations :)
> The current PPC/powerpc approach of calling __do_IRQ() might be hacky, > but it's functional, so neither is there any instant urgency AFAICS. If > our more complete genirq approach is rejected for whatever reason then > the SA_CASCADEIRQ patch can still be revisited as a secondary choice. > > Nor can i see any big cleanup effect in terms of per-arch PIC code, the > patch actually adds a bit of code:
Not really, it will go away once I remove the special code to handle cascades at the PIC level that we currently have and that will be made obsolete by this mecanism.
In addtition, there are other issues imho with the implementation I've seen in Thomas patch. Since I finally explained most of my concerns above instead of in a couple of days, I'll finish now which whatever remains in my mind of reading it yesterday :) That is essentially that i find the approach of splitting the level and edge handlers less robust.
The current handler has the IN_PROGRESS/PENDING mecanism. The patch keeps that only for edge interrupts and not for level interrupts. That might seem fine on the paper, but as soon as an irq controller starts misbehaving for some reason, the security it provides to avoid re-entrency in a pending interrupt will be lost for level interrupts.
I think I had a couple of other concerns but I don't have them on the top of my mind right now. Let's see how we go with that now tough. I've addressed my main issues I think.
Ben.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |