* [PATCH] x86/ioapic: Do not unmask io_apic when interrupt is in progress @ 2018-03-02 13:04 Sebastian Andrzej Siewior 2018-07-16 16:41 ` Thomas Gleixner 0 siblings, 1 reply; 5+ messages in thread From: Sebastian Andrzej Siewior @ 2018-03-02 13:04 UTC (permalink / raw) To: linux-kernel; +Cc: x86 From: Ingo Molnar <mingo@elte.hu> Date: Fri, 3 Jul 2009 08:29:27 -0500 With threaded interrupts we might see an interrupt in progress on migration. Do not unmask it when this is the case. Signed-off-by: Ingo Molnar <mingo@elte.hu> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- arch/x86/kernel/apic/io_apic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -1690,7 +1690,8 @@ static bool io_apic_level_ack_pending(st static inline bool ioapic_irqd_mask(struct irq_data *data) { /* If we are moving the irq we need to mask it */ - if (unlikely(irqd_is_setaffinity_pending(data))) { + if (unlikely(irqd_is_setaffinity_pending(data) && + !irqd_irq_inprogress(data))) { mask_ioapic_irq(data); return true; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/ioapic: Do not unmask io_apic when interrupt is in progress 2018-03-02 13:04 [PATCH] x86/ioapic: Do not unmask io_apic when interrupt is in progress Sebastian Andrzej Siewior @ 2018-07-16 16:41 ` Thomas Gleixner 2018-07-16 20:53 ` Andy Shevchenko 0 siblings, 1 reply; 5+ messages in thread From: Thomas Gleixner @ 2018-07-16 16:41 UTC (permalink / raw) To: Sebastian Andrzej Siewior; +Cc: LKML, x86, Peter Zijlstra On Fri, 2 Mar 2018, Sebastian Andrzej Siewior wrote: > From: Ingo Molnar <mingo@elte.hu> > Date: Fri, 3 Jul 2009 08:29:27 -0500 > > With threaded interrupts we might see an interrupt in progress on > migration. Do not unmask it when this is the case. It took me quite a while to wrap my head around this one. > --- a/arch/x86/kernel/apic/io_apic.c > +++ b/arch/x86/kernel/apic/io_apic.c > @@ -1690,7 +1690,8 @@ static bool io_apic_level_ack_pending(st > static inline bool ioapic_irqd_mask(struct irq_data *data) > { > /* If we are moving the irq we need to mask it */ > - if (unlikely(irqd_is_setaffinity_pending(data))) { > + if (unlikely(irqd_is_setaffinity_pending(data) && > + !irqd_irq_inprogress(data))) { I really don't see how that is preventing anything. The point is that chip->eoi_irq() is always called _after_ the INPROGRESS flag has been cleared. And if that's not the case then the interrupt would be never moved. But, that patch is almost 10 years old and I didn't find the exact context. So it might have been valid back then, but I doubt it. Though there is an issue with threaded interrupts which are marked ONESHOT and using the fasteoi handler. if (IS_ONESHOT()) mask_irq(); .... .... cond_unmask_eoi_irq() chip->irq_eoi(); So if setaffinity is pending then the interrupt will be moved and then unmasked, which is wrong as it should be kept masked up to the point where the threaded handler finished. It's not a real problem, the interrupt will just be able to fire before the threaded handler has finished, though the irq masked state will be wrong for a bit. The patch below should cure the issue. It also renames the horribly misnomed functions so it becomes clear what they are supposed to do. Thanks, tglx 8<------------------- diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c index 3982f79d2377..23667451e44e 100644 --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -1721,19 +1721,20 @@ static bool io_apic_level_ack_pending(struct mp_chip_data *data) return false; } -static inline bool ioapic_irqd_mask(struct irq_data *data) +static inline bool ioapic_prepare_move(struct irq_data *data) { /* If we are moving the irq we need to mask it */ if (unlikely(irqd_is_setaffinity_pending(data))) { - mask_ioapic_irq(data); + if (!irqd_irq_masked(data)) + mask_ioapic_irq(data); return true; } return false; } -static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked) +static inline void ioapic_finish_move(struct irq_data *data, bool moveit) { - if (unlikely(masked)) { + if (unlikely(moveit)) { /* Only migrate the irq if the ack has been received. * * On rare occasions the broadcast level triggered ack gets @@ -1762,15 +1763,17 @@ static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked) */ if (!io_apic_level_ack_pending(data->chip_data)) irq_move_masked_irq(data); - unmask_ioapic_irq(data); + /* If the irq is masked in the core, leave it */ + if (!irqd_irq_masked(data)) + unmask_ioapic_irq(data); } } #else -static inline bool ioapic_irqd_mask(struct irq_data *data) +static inline bool ioapic_move_prepare(struct irq_data *data) { return false; } -static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked) +static inline void ioapic_move_irq(struct irq_data *data, bool masked) { } #endif @@ -1779,11 +1782,11 @@ static void ioapic_ack_level(struct irq_data *irq_data) { struct irq_cfg *cfg = irqd_cfg(irq_data); unsigned long v; - bool masked; + bool moveit; int i; irq_complete_move(cfg); - masked = ioapic_irqd_mask(irq_data); + moveit = ioapic_prepare_move(irq_data); /* * It appears there is an erratum which affects at least version 0x11 @@ -1838,7 +1841,7 @@ static void ioapic_ack_level(struct irq_data *irq_data) eoi_ioapic_pin(cfg->vector, irq_data->chip_data); } - ioapic_irqd_unmask(irq_data, masked); + ioapic_finish_move(irq_data, moveit); } static void ioapic_ir_ack_level(struct irq_data *irq_data) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/ioapic: Do not unmask io_apic when interrupt is in progress 2018-07-16 16:41 ` Thomas Gleixner @ 2018-07-16 20:53 ` Andy Shevchenko 2018-07-16 21:32 ` Thomas Gleixner 0 siblings, 1 reply; 5+ messages in thread From: Andy Shevchenko @ 2018-07-16 20:53 UTC (permalink / raw) To: Thomas Gleixner Cc: Sebastian Andrzej Siewior, LKML, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Peter Zijlstra On Mon, Jul 16, 2018 at 7:41 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > +static inline bool ioapic_prepare_move(struct irq_data *data) > +static inline void ioapic_finish_move(struct irq_data *data, bool moveit) > +static inline bool ioapic_move_prepare(struct irq_data *data) > +static inline void ioapic_move_irq(struct irq_data *data, bool masked) Verb or noun? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/ioapic: Do not unmask io_apic when interrupt is in progress 2018-07-16 20:53 ` Andy Shevchenko @ 2018-07-16 21:32 ` Thomas Gleixner 2018-07-17 16:25 ` [PATCH] x86/ioapic: Don't let setaffinity unmask threaded EOI interrupt too early Sebastian Andrzej Siewior 0 siblings, 1 reply; 5+ messages in thread From: Thomas Gleixner @ 2018-07-16 21:32 UTC (permalink / raw) To: Andy Shevchenko Cc: Sebastian Andrzej Siewior, LKML, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Peter Zijlstra On Mon, 16 Jul 2018, Andy Shevchenko wrote: > On Mon, Jul 16, 2018 at 7:41 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > > +static inline bool ioapic_prepare_move(struct irq_data *data) > > > +static inline void ioapic_finish_move(struct irq_data *data, bool moveit) > > > +static inline bool ioapic_move_prepare(struct irq_data *data) > > > +static inline void ioapic_move_irq(struct irq_data *data, bool masked) > > Verb or noun? Bah. prepare/finish_move() for both places .... ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] x86/ioapic: Don't let setaffinity unmask threaded EOI interrupt too early 2018-07-16 21:32 ` Thomas Gleixner @ 2018-07-17 16:25 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 5+ messages in thread From: Sebastian Andrzej Siewior @ 2018-07-17 16:25 UTC (permalink / raw) To: Thomas Gleixner Cc: Andy Shevchenko, LKML, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Peter Zijlstra From: Thomas Gleixner <tglx@linutronix.de> There is an issue with threaded interrupts which are marked ONESHOT and using the fasteoi handler. if (IS_ONESHOT()) mask_irq(); .... .... cond_unmask_eoi_irq() chip->irq_eoi(); So if setaffinity is pending then the interrupt will be moved and then unmasked, which is wrong as it should be kept masked up to the point where the threaded handler finished. It's not a real problem, the interrupt will just be able to fire before the threaded handler has finished, though the irq masked state will be wrong for a bit. The patch below should cure the issue. It also renames the horribly misnomed functions so it becomes clear what they are supposed to do. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> [bigeasy: add the body of the patch, use the same functions in both ifdef paths (spotted by Andy Shevchenko)] Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- arch/x86/kernel/apic/io_apic.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c index 3982f79d2377..adfaf5549fcc 100644 --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -1721,19 +1721,20 @@ static bool io_apic_level_ack_pending(struct mp_chip_data *data) return false; } -static inline bool ioapic_irqd_mask(struct irq_data *data) +static inline bool ioapic_prepare_move(struct irq_data *data) { /* If we are moving the irq we need to mask it */ if (unlikely(irqd_is_setaffinity_pending(data))) { - mask_ioapic_irq(data); + if (!irqd_irq_masked(data)) + mask_ioapic_irq(data); return true; } return false; } -static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked) +static inline void ioapic_finish_move(struct irq_data *data, bool moveit) { - if (unlikely(masked)) { + if (unlikely(moveit)) { /* Only migrate the irq if the ack has been received. * * On rare occasions the broadcast level triggered ack gets @@ -1762,15 +1763,17 @@ static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked) */ if (!io_apic_level_ack_pending(data->chip_data)) irq_move_masked_irq(data); - unmask_ioapic_irq(data); + /* If the irq is masked in the core, leave it */ + if (!irqd_irq_masked(data)) + unmask_ioapic_irq(data); } } #else -static inline bool ioapic_irqd_mask(struct irq_data *data) +static inline bool ioapic_prepare_move(struct irq_data *data) { return false; } -static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked) +static inline void ioapic_finish_move(struct irq_data *data, bool moveit) { } #endif @@ -1779,11 +1782,11 @@ static void ioapic_ack_level(struct irq_data *irq_data) { struct irq_cfg *cfg = irqd_cfg(irq_data); unsigned long v; - bool masked; + bool moveit; int i; irq_complete_move(cfg); - masked = ioapic_irqd_mask(irq_data); + moveit = ioapic_prepare_move(irq_data); /* * It appears there is an erratum which affects at least version 0x11 @@ -1838,7 +1841,7 @@ static void ioapic_ack_level(struct irq_data *irq_data) eoi_ioapic_pin(cfg->vector, irq_data->chip_data); } - ioapic_irqd_unmask(irq_data, masked); + ioapic_finish_move(irq_data, moveit); } static void ioapic_ir_ack_level(struct irq_data *irq_data) -- 2.18.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-17 16:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-02 13:04 [PATCH] x86/ioapic: Do not unmask io_apic when interrupt is in progress Sebastian Andrzej Siewior 2018-07-16 16:41 ` Thomas Gleixner 2018-07-16 20:53 ` Andy Shevchenko 2018-07-16 21:32 ` Thomas Gleixner 2018-07-17 16:25 ` [PATCH] x86/ioapic: Don't let setaffinity unmask threaded EOI interrupt too early Sebastian Andrzej Siewior
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).