| Message ID | 20200117001706.40620-1-jiaxun.yang@flygoat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v1,1/2] genirq: Check for level based percpu irq | expand |
Jiaxun Yang <jiaxun.yang@flygoat.com> writes: > MIPS processors implemented their IPI IRQ and CPU interrupt line > as level triggered IRQ. However, our current percpu_irq flow is trying > do it in a level triggered manner. So what are you trying to solve? The CPU uses level type and the per cpu irq flow does the same. I can't find the problem with that. > Thus we attempt to determine whether it is or not level triggered type by > checking if both ack and eoi operation not exist. And handle it in > mask/unmask way. What? No. This is fundamentally wrong. percpu interrupts which are used for IPIs/per CPU timers/PERF have the the following semantics: 1) No serialization required because the interrupt cannot happen concurrently on the same CPU 2) The interrupt handler is strictly hard interrupt context. There can't be a threaded handler associated to it. 3) The interrupt hardware interaction is reduced to the absolute minimum depending on the trigger type of the relevant interrupt controller. Now #3 is the interesting one here and there are the following cases: A) Edge type Edge type interrupts are events which are triggered once when the level of the interrupt line changes. Depending on the implementation and configuration of the interrupt controller they react on either one direction of level change (low -> high, high ->low) or on both, They require that the interrupt is acked at the interrupt controller _before_ the handler runs. That's required because otherwise the device could issue a new interrupt after the initial one has been handled and before the ack was issued. That ack would then clear the new interrupt which would get lost and potentially starve the device forever. See handle_edge_irq() Edge type interrupts are also not suitable for interrupt sharing. Note, that MSI interrupts are edge type because the interrupt message is only sent once when the event happens. B) Level type Level type interrupts react on a static level state. If the interrupt line has the implementation/configuration defined level then the interrupt is delivered to the CPU up to the point where the interrupt line returns to the inactive level. For regular operation they usually require that the interrupt is masked before the handler runs and unmasked after the handler completes. This is required to prevent interrupt storms in case the handler reenables interrupts at the CPU level which is not relevant for Linux unless the interrupt handler is threaded. For threaded handlers the line must be masked until the thread completes. They are lossless because if the interrupt is reactivated after handling the initial one before the unmask of the line happens it is immediately delivered to the CPU again after the unmask takes place. Level interrupts can be shared lossless. (Note that interrupt sharing is a design fail from a performance perspective because the handling requires that all devices which share the same line need to be polled whether they actually issued an interrupt). Some level type controllers require an MASK/ACK sequence to work correctly. That's often the case when the controller supports both level and edge mode. C) EOI type EOI type interrupts are from their concept level interrupts. Contrary to regular level interrupts they do not require the mask and unmask operation to prevent an interrupt storm. The interrupt controller guarantees not to deliver the still active interrupt up to the point where the software issues EOI. The EOI happens after the actual device handler has run. That's a very efficient solution which avoids slow mask/unmask operations and at the same time guarantees that no interrupts are lost. So lets look how this relates to strict per cpu interrupts, i.e. interrupts which can only be delivered to one particular CPU. As documented above the strict percpuness removes the requirement for locking as the CPU itself guarantees non-reentrancy for the same interrupt as long as interrupts are disabled at the CPU level while the interrupt is serviced. Aside of that this also affects the required operations for interrupt handling. None of the true per cpu interrupts requires any form of mask/unmask operation in the handler. They only require that the relevant ACK/EOI operations take place. So for the 3 types above this results in the following: A) Edge type Issue ACK before invoking the handler B) Level type If the controller does not require ACK, then no action is required. If the controller requires ACK, the ACK has to be issued before invoking the handler. C) EOI type Issue EOI after invoking the handler As the MIPS GIC is a strict level type controller which does not require ACK or EOI operations, the irq_ack/irq_eoi callbacks are completely bogus. There is also no need to change anything in the core code percpu handler implementations. All you need to do is to remove the bogus irq_ack/irq_eoi function pointers from the MIPS GIC irq_chip and be done with it. Thanks, tglx
17.01.2020, 09:29, "Thomas Gleixner" <tglx@linutronix.de>: > Jiaxun Yang <jiaxun.yang@flygoat.com> writes: >> MIPS processors implemented their IPI IRQ and CPU interrupt line >> as level triggered IRQ. However, our current percpu_irq flow is trying >> do it in a level triggered manner. > Hi Thomas, Thanks for your kind explanation. That appears to be my misunderstanding of the trigger type. Paul, I have confirmed it seems fine to handle percpu IRQ without mask it on both Ingenic and Loongson processors. How about other MIPS Cores? Could you please help check that? Thanks. -- Jiaxun Yang
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index b3fa2d87d2f3..4fafe572d022 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -893,6 +893,7 @@ void handle_edge_eoi_irq(struct irq_desc *desc) void handle_percpu_irq(struct irq_desc *desc) { struct irq_chip *chip = irq_desc_get_chip(desc); + bool is_level = !chip->irq_ack && !chip->irq_eoi; /* * PER CPU interrupts are not serialized. Do not touch @@ -900,12 +901,16 @@ void handle_percpu_irq(struct irq_desc *desc) */ __kstat_incr_irqs_this_cpu(desc); - if (chip->irq_ack) + if (is_level) + chip->irq_mask(&desc->irq_data); + else if (chip->irq_ack) chip->irq_ack(&desc->irq_data); handle_irq_event_percpu(desc); - if (chip->irq_eoi) + if (is_level) + chip->irq_unmask(&desc->irq_data); + else if (chip->irq_eoi) chip->irq_eoi(&desc->irq_data); } @@ -925,6 +930,7 @@ void handle_percpu_devid_irq(struct irq_desc *desc) struct irq_chip *chip = irq_desc_get_chip(desc); struct irqaction *action = desc->action; unsigned int irq = irq_desc_get_irq(desc); + bool is_level = !chip->irq_ack && !chip->irq_eoi; irqreturn_t res; /* @@ -933,7 +939,9 @@ void handle_percpu_devid_irq(struct irq_desc *desc) */ __kstat_incr_irqs_this_cpu(desc); - if (chip->irq_ack) + if (is_level) + chip->irq_mask(&desc->irq_data); + else if (chip->irq_ack) chip->irq_ack(&desc->irq_data); if (likely(action)) { @@ -951,7 +959,9 @@ void handle_percpu_devid_irq(struct irq_desc *desc) enabled ? " and unmasked" : "", irq, cpu); } - if (chip->irq_eoi) + if (is_level) + chip->irq_unmask(&desc->irq_data); + else if (chip->irq_eoi) chip->irq_eoi(&desc->irq_data); }
MIPS processors implemented their IPI IRQ and CPU interrupt line as level triggered IRQ. However, our current percpu_irq flow is trying do it in a level triggered manner. Thus we attempt to determine whether it is or not level triggered type by checking if both ack and eoi operation not exist. And handle it in mask/unmask way. Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> --- kernel/irq/chip.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)