| Message ID | 20240405223110.1609888-8-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coalesced Interrupt Delivery with posted MSI | expand |
> From: Jacob Pan <jacob.jun.pan@linux.intel.com> > Sent: Saturday, April 6, 2024 6:31 AM > > Prepare for calling external IRQ handlers directly from the posted MSI > demultiplexing loop. Extract the common code with common interrupt to > avoid code duplication. > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > --- > arch/x86/kernel/irq.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c > index f39f6147104c..c54de9378943 100644 > --- a/arch/x86/kernel/irq.c > +++ b/arch/x86/kernel/irq.c > @@ -242,18 +242,10 @@ static __always_inline void handle_irq(struct > irq_desc *desc, > __handle_irq(desc, regs); > } > > -/* > - * common_interrupt() handles all normal device IRQ's (the special SMP > - * cross-CPU interrupts have their own entry points). > - */ > -DEFINE_IDTENTRY_IRQ(common_interrupt) > +static __always_inline void call_irq_handler(int vector, struct pt_regs *regs) > { > - struct pt_regs *old_regs = set_irq_regs(regs); > struct irq_desc *desc; > > - /* entry code tells RCU that we're not quiescent. Check it. */ > - RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up > RCU"); > - > desc = __this_cpu_read(vector_irq[vector]); > if (likely(!IS_ERR_OR_NULL(desc))) { > handle_irq(desc, regs); the hidden lines has one problem: } else { apic_eoi(); if (desc == VECTOR_UNUSED) { ... there will be two EOI's for unused vectors, adding the one in sysvec_posted_msi_notification().
Hi Kevin, On Fri, 12 Apr 2024 09:21:45 +0000, "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > Sent: Saturday, April 6, 2024 6:31 AM > > > > Prepare for calling external IRQ handlers directly from the posted MSI > > demultiplexing loop. Extract the common code with common interrupt to > > avoid code duplication. > > > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > > --- > > arch/x86/kernel/irq.c | 23 ++++++++++++++--------- > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c > > index f39f6147104c..c54de9378943 100644 > > --- a/arch/x86/kernel/irq.c > > +++ b/arch/x86/kernel/irq.c > > @@ -242,18 +242,10 @@ static __always_inline void handle_irq(struct > > irq_desc *desc, > > __handle_irq(desc, regs); > > } > > > > -/* > > - * common_interrupt() handles all normal device IRQ's (the special SMP > > - * cross-CPU interrupts have their own entry points). > > - */ > > -DEFINE_IDTENTRY_IRQ(common_interrupt) > > +static __always_inline void call_irq_handler(int vector, struct > > pt_regs *regs) { > > - struct pt_regs *old_regs = set_irq_regs(regs); > > struct irq_desc *desc; > > > > - /* entry code tells RCU that we're not quiescent. Check it. */ > > - RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up > > RCU"); > > - > > desc = __this_cpu_read(vector_irq[vector]); > > if (likely(!IS_ERR_OR_NULL(desc))) { > > handle_irq(desc, regs); > > the hidden lines has one problem: > > } else { > apic_eoi(); > > if (desc == VECTOR_UNUSED) { > ... > > there will be two EOI's for unused vectors, adding the one > in sysvec_posted_msi_notification(). Indeed this unlikely case could cause lost interrupt. Imagine we have: - IDT vector N (MSI notification), O, and P (other high-priority system vectors). - Device MSI vector A which triggers N. Action APIC IRR APIC ISR --------------------------------------------------------- Device MSI A N APIC accepts N - N New IRQs arrive O,P N handle_irq(A) eoi() due to A's fault - O,P eoi in post_msi - P ---------------------------------------------------------- The second EOI clears ISR for vector O but missed processing it. Intel SDM 11.8.4 for background. "The IRR contains the active interrupt requests that have been accepted, but not yet dispatched to the processor for servicing. When the local APIC accepts an interrupt, it sets the bit in the IRR that corresponds the vector of the accepted interrupt. When the processor core is ready to handle the next interrupt, the local APIC clears the highest priority IRR bit that is set and sets the corresponding ISR bit. The vector for the highest priority bit set in the ISR is then dispatched to the processor core for servicing. While the processor is servicing the highest priority interrupt, the local APIC can send additional fixed interrupts by setting bits in the IRR. When the interrupt service routine issues a write to the EOI register (see Section 11.8.5, Signaling Interrupt Servicing Completion), the local APIC responds by clearing the highest priority ISR bit that is set. It then repeats the process of clearing the highest priority bit in the IRR and setting the corresponding bit in the ISR. The processor core then begins executing the service routing for the highest priority bit set in the ISR " I need to avoid the duplicated EOI in this case and at minimum cost for the hot path. Thanks, Jacob
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c index f39f6147104c..c54de9378943 100644 --- a/arch/x86/kernel/irq.c +++ b/arch/x86/kernel/irq.c @@ -242,18 +242,10 @@ static __always_inline void handle_irq(struct irq_desc *desc, __handle_irq(desc, regs); } -/* - * common_interrupt() handles all normal device IRQ's (the special SMP - * cross-CPU interrupts have their own entry points). - */ -DEFINE_IDTENTRY_IRQ(common_interrupt) +static __always_inline void call_irq_handler(int vector, struct pt_regs *regs) { - struct pt_regs *old_regs = set_irq_regs(regs); struct irq_desc *desc; - /* entry code tells RCU that we're not quiescent. Check it. */ - RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up RCU"); - desc = __this_cpu_read(vector_irq[vector]); if (likely(!IS_ERR_OR_NULL(desc))) { handle_irq(desc, regs); @@ -268,7 +260,20 @@ DEFINE_IDTENTRY_IRQ(common_interrupt) __this_cpu_write(vector_irq[vector], VECTOR_UNUSED); } } +} + +/* + * common_interrupt() handles all normal device IRQ's (the special SMP + * cross-CPU interrupts have their own entry points). + */ +DEFINE_IDTENTRY_IRQ(common_interrupt) +{ + struct pt_regs *old_regs = set_irq_regs(regs); + + /* entry code tells RCU that we're not quiescent. Check it. */ + RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up RCU"); + call_irq_handler(vector, regs); set_irq_regs(old_regs); }
Prepare for calling external IRQ handlers directly from the posted MSI demultiplexing loop. Extract the common code with common interrupt to avoid code duplication. Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> --- arch/x86/kernel/irq.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)