Skip to contentSkip to conversation

powerpc/64: irq_work avoid immediate interrupt when raised with hard irqs enabled

PATCH In review Nicholas Piggin 2 participants 3 messages Apr 5, 2018 – Apr 9, 2018 PowerPC

lore
Share & tools
lore

Nicholas Piggin

author reviewer
#0 lore raw

PATCH 1/1 powerpc/64: irq_work avoid immediate interrupt when raised with hard irqs enabled

irq_work_raise should not schedule the hardware decrementer interrupt unless it is called from NMI context. Doing so often just results in an immediate masked decrementer interrupt:

   <...>-550    90d...    4us : update_curr_rt <-dequeue_task_rt
   <...>-550    90d...    5us : dbs_update_util_handler <-update_curr_rt
   <...>-550    90d...    6us : arch_irq_work_raise <-irq_work_queue
   <...>-550    90d...    7us : soft_nmi_interrupt <-soft_nmi_common
   <...>-550    90d...    7us : printk_nmi_enter <-soft_nmi_interrupt
   <...>-550    90d.Z.    8us : rcu_nmi_enter <-soft_nmi_interrupt
   <...>-550    90d.Z.    9us : rcu_nmi_exit <-soft_nmi_interrupt
   <...>-550    90d...    9us : printk_nmi_exit <-soft_nmi_interrupt
   <...>-550    90d...   10us : cpuacct_charge <-update_curr_rt

Set the decrementer pending in the irq_happened mask directly, rather than having the masked decrementer handler do it.

---
 arch/powerpc/kernel/time.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
arch/powerpc/kernel/time.c +33 -2
index a32823dcd9a4..9d1cc183c974 100644
@@ -510,6 +510,35 @@ static inline void clear_irq_work_pending(void)
 		"i" (offsetof(struct paca_struct, irq_work_pending)));
 }
 
+void arch_irq_work_raise(void)
+{
+	WARN_ON(!irqs_disabled());
+
+	preempt_disable();
+	set_irq_work_pending_flag();
+	/*
+	 * Regular iterrupts will check pending irq_happened as they return,
+	 * or process context when it next enables interrupts, so the
+	 * decrementer can be scheduled there.
+	 *
+	 * NMI interrupts do not, so setting the decrementer hardware
+	 * interrupt to fire ensures the work runs upon RI (if it's to a
+	 * MSR[EE]=1 context). We do not want to do this in other contexts
+	 * because if interrupts are hard enabled, the decrementer will
+	 * fire immediately here and just go to the masked handler to be
+	 * recorded in irq_happened.
+	 *
+	 * BookE does not support this yet, it must audit all NMI
+	 * interrupt handlers call nmi_enter().
+	 */
+	if (IS_ENABLED(CONFIG_BOOKE) || in_nmi()) {
+		set_dec(1);
+	} else {
+		local_paca->irq_happened |= PACA_IRQ_DEC;
+	}
+	preempt_enable();
+}
+
 #else /* 32-bit */
 
 DEFINE_PER_CPU(u8, irq_work_pending);
@@ -518,16 +547,18 @@ DEFINE_PER_CPU(u8, irq_work_pending);
 #define test_irq_work_pending()		__this_cpu_read(irq_work_pending)
 #define clear_irq_work_pending()	__this_cpu_write(irq_work_pending, 0)
 
-#endif /* 32 vs 64 bit */
-
 void arch_irq_work_raise(void)
 {
+	WARN_ON(!irqs_disabled());
+
 	preempt_disable();
 	set_irq_work_pending_flag();
 	set_dec(1);
 	preempt_enable();
 }
 
+#endif /* 32 vs 64 bit */
+
 #else  /* CONFIG_IRQ_WORK */
 
 #define test_irq_work_pending()	0
Signature · 1 line
2.16.3
#1 lore raw
Quoted 14 lines · Nicholas Piggin

irq_work_raise should not schedule the hardware decrementer interrupt unless it is called from NMI context. Doing so often just results in an immediate masked decrementer interrupt:

   <...>-550    90d...    4us : update_curr_rt <-dequeue_task_rt
   <...>-550    90d...    5us : dbs_update_util_handler <-update_curr_rt
   <...>-550    90d...    6us : arch_irq_work_raise <-irq_work_queue
   <...>-550    90d...    7us : soft_nmi_interrupt <-soft_nmi_common
   <...>-550    90d...    7us : printk_nmi_enter <-soft_nmi_interrupt
   <...>-550    90d.Z.    8us : rcu_nmi_enter <-soft_nmi_interrupt
   <...>-550    90d.Z.    9us : rcu_nmi_exit <-soft_nmi_interrupt
   <...>-550    90d...    9us : printk_nmi_exit <-soft_nmi_interrupt
   <...>-550    90d...   10us : cpuacct_charge <-update_curr_rt
Quoted 2 lines

Set the decrementer pending in the irq_happened mask directly, rather than having the masked decrementer handler do it.

Setting the paca field needs hard irqs off... also preempt_disable doesn't look necessary if IRQs are off.

Quoted 66 lines
---
 arch/powerpc/kernel/time.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
arch/powerpc/kernel/time.c +33 -2
index a32823dcd9a4..9d1cc183c974 100644
@@ -510,6 +510,35 @@ static inline void clear_irq_work_pending(void)
 		"i" (offsetof(struct paca_struct, irq_work_pending)));
 }
 
+void arch_irq_work_raise(void)
+{
+	WARN_ON(!irqs_disabled());
+
+	preempt_disable();
+	set_irq_work_pending_flag();
+	/*
+	 * Regular iterrupts will check pending irq_happened as they return,
+	 * or process context when it next enables interrupts, so the
+	 * decrementer can be scheduled there.
+	 *
+	 * NMI interrupts do not, so setting the decrementer hardware
+	 * interrupt to fire ensures the work runs upon RI (if it's to a
+	 * MSR[EE]=1 context). We do not want to do this in other contexts
+	 * because if interrupts are hard enabled, the decrementer will
+	 * fire immediately here and just go to the masked handler to be
+	 * recorded in irq_happened.
+	 *
+	 * BookE does not support this yet, it must audit all NMI
+	 * interrupt handlers call nmi_enter().
+	 */
+	if (IS_ENABLED(CONFIG_BOOKE) || in_nmi()) {
+		set_dec(1);
+	} else {
+		local_paca->irq_happened |= PACA_IRQ_DEC;
+	}
+	preempt_enable();
+}
+
 #else /* 32-bit */
 
 DEFINE_PER_CPU(u8, irq_work_pending);
@@ -518,16 +547,18 @@ DEFINE_PER_CPU(u8, irq_work_pending);
 #define test_irq_work_pending()		__this_cpu_read(irq_work_pending)
 #define clear_irq_work_pending()	__this_cpu_write(irq_work_pending, 0)
 
-#endif /* 32 vs 64 bit */
-
 void arch_irq_work_raise(void)
 {
+	WARN_ON(!irqs_disabled());
+
 	preempt_disable();
 	set_irq_work_pending_flag();
 	set_dec(1);
 	preempt_enable();
 }
 
+#endif /* 32 vs 64 bit */
+
 #else  /* CONFIG_IRQ_WORK */
 
 #define test_irq_work_pending()	0
author reviewer
#2 lore raw
Quoted 15 lines · On Mon, 09 Apr 2018 18:46:29 +1000 Benjamin Herrenschmidt…
Quoted 14 lines · Nicholas Piggin

irq_work_raise should not schedule the hardware decrementer interrupt unless it is called from NMI context. Doing so often just results in an immediate masked decrementer interrupt:

   <...>-550    90d...    4us : update_curr_rt <-dequeue_task_rt
   <...>-550    90d...    5us : dbs_update_util_handler <-update_curr_rt
   <...>-550    90d...    6us : arch_irq_work_raise <-irq_work_queue
   <...>-550    90d...    7us : soft_nmi_interrupt <-soft_nmi_common
   <...>-550    90d...    7us : printk_nmi_enter <-soft_nmi_interrupt
   <...>-550    90d.Z.    8us : rcu_nmi_enter <-soft_nmi_interrupt
   <...>-550    90d.Z.    9us : rcu_nmi_exit <-soft_nmi_interrupt
   <...>-550    90d...    9us : printk_nmi_exit <-soft_nmi_interrupt
   <...>-550    90d...   10us : cpuacct_charge <-update_curr_rt
Quoted 4 lines
Quoted 2 lines

Set the decrementer pending in the irq_happened mask directly, rather than having the masked decrementer handler do it.

Setting the paca field needs hard irqs off...

Doh! Good catch, I should have noticed that :)

Quoted 2 lines

also preempt_disable
doesn't look necessary if IRQs are off.

True, just copied from existing code.

Thanks,
Nick