interrupt tracing fixes

PATCH Applied Nicholas Piggin 4 participants 14 messages Nov 16, 2017 – Jan 22, 2018

Subsystems (2)

PowerPC CPU idle time management framework

lore

Share & tools

lore

Nicholas Piggin

author reviewer

2017-11-16 16:01 UTC #0 lore raw

cover interrupt tracing fixes

Here are a few loosely related fixes for interrupt tracing code and irq state handling which eliminates local_irq_enable() when already enabled, and local_irq_disable() when already disabled, and also fixes an NMI re-entrancy bug in irq tracing that has been crashing in the field when PMU interrupts (non-maskable) and irq tracing runs together it's causing things to get into "impossible" states.

I have only tested 64s, and don't know if patch 1 and 2 are right on 64e or 32 so if anyone could take a look or test, that would be good.

Thanks,
Nick

Nicholas Piggin (4):
powerpc: define __ARCH_IRQ_EXIT_IRQS_DISABLED
powerpc/64: do not trace irqs-off at interrupt return to soft-disabled
context
cpuidle/powernv: avoid double irq enable coming out of idle cpuidle/powernv: avoid double irq enable coming out of idle

 arch/powerpc/include/asm/hardirq.h |  1 +
 arch/powerpc/kernel/entry_64.S     | 10 +++++++---
 drivers/cpuidle/cpuidle-powernv.c  |  2 ++
 drivers/cpuidle/cpuidle-pseries.c  |  6 ++++--
 4 files changed, 14 insertions(+), 5 deletions(-)

Signature · 1 line

2.15.0

Nicholas Piggin

author reviewer

2017-11-16 16:01 UTC #1 lore raw

PATCH 1/4 powerpc: define __ARCH_IRQ_EXIT_IRQS_DISABLED

powerpc calls irq_exit() with local irqs disabled, therefore it can define __ARCH_IRQ_EXIT_IRQS_DISABLED.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/include/asm/hardirq.h | 1 +
 1 file changed, 1 insertion(+)

arch/powerpc/include/asm/hardirq.h +1 -0

index 456f9e7b8d83..5986d473722b 100644
@@ -29,6 +29,7 @@ DECLARE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat);
 #define local_softirq_pending()	__this_cpu_read(irq_stat.__softirq_pending)
 
 #define __ARCH_SET_SOFTIRQ_PENDING
+#define __ARCH_IRQ_EXIT_IRQS_DISABLED
 
 #define set_softirq_pending(x) __this_cpu_write(irq_stat.__softirq_pending, (x))
 #define or_softirq_pending(x) __this_cpu_or(irq_stat.__softirq_pending, (x))

Signature · 1 line

2.15.0

Michael Ellerman

Applied

2018-01-22 03:34 UTC #2 lore raw

Re: [1/4] powerpc: define __ARCH_IRQ_EXIT_IRQS_DISABLED

Quoted 4 lines · Nicholas Piggin

powerpc calls irq_exit() with local irqs disabled, therefore it can define __ARCH_IRQ_EXIT_IRQS_DISABLED.
Signed-off-by Nicholas Piggin <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/c16bee4bded5449ec3b3ec73579ba2

cheers

Nicholas Piggin

author reviewer

2017-11-16 16:01 UTC #3 lore raw

PATCH 2/4 powerpc/64: do not trace irqs-off at interrupt return to soft-disabled context

In reply to Nicholas Piggin

When an interrupt is returning to a soft-disabled context (which can happen for non-maskable interrupts or synchronous interrupts), it goes through the motions of soft-disabling again, including calling TRACE_DISABLE_INTS (i.e., trace_hardirqs_off()).

This is not necessary, because we must already be soft-disabled in the interrupt context, it also may be causing crashes in the irq tracing code to re-enter as an nmi. Replace it with a warning to ensure that soft-interrupts are still disabled.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/kernel/entry_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

arch/powerpc/kernel/entry_64.S +7 -3

index 3320bcac7192..36878b6ee8b8 100644
@@ -911,9 +911,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	beq	1f
 	rlwinm	r7,r7,0,~PACA_IRQ_HARD_DIS
 	stb	r7,PACAIRQHAPPENED(r13)
-1:	li	r0,0
-	stb	r0,PACASOFTIRQEN(r13);
-	TRACE_DISABLE_INTS
+1:
+#if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_BUG)
+	/* The interrupt should not have soft enabled. */
+	lbz	r7,PACASOFTIRQEN(r13)
+1:	tdnei	r7,0
+	EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,BUGFLAG_WARNING
+#endif
 	b	.Ldo_restore
 
 	/*

Signature · 1 line

2.15.0

Michael Ellerman

reviewer

2017-12-04 05:09 UTC #4 lore raw

Re: [PATCH 2/4] powerpc/64: do not trace irqs-off at interrupt return to soft-disabled context

Quoted 10 lines · Nicholas Piggin

When an interrupt is returning to a soft-disabled context (which can happen for non-maskable interrupts or synchronous interrupts), it goes through the motions of soft-disabling again, including calling TRACE_DISABLE_INTS (i.e., trace_hardirqs_off()).
This is not necessary, because we must already be soft-disabled in the interrupt context, it also may be causing crashes in the irq tracing code to re-enter as an nmi. Replace it with a warning to ensure that soft-interrupts are still disabled.

Quoted 4 lines

Signed-off-by Nicholas Piggin <[email protected]>
---
 arch/powerpc/kernel/entry_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

So this patch is the core of the bug fix I gather.

Git blames says:

Fixes 7c0482e3d055 ("powerpc/irq: Fix another case of lazy IRQ state getting out of sync")

Cc [email protected] # v3.4+

But I'm wondering how this has been broken that long without us noticing? You hit it doing some sort of perf stress test I think - so is it just that we've never pushed hard enough? Or did something change to expose this? Or we're just not sure?

cheers

Quoted 23 lines

arch/powerpc/kernel/entry_64.S +7 -3

index 3320bcac7192..36878b6ee8b8 100644
@@ -911,9 +911,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	beq	1f
 	rlwinm	r7,r7,0,~PACA_IRQ_HARD_DIS
 	stb	r7,PACAIRQHAPPENED(r13)
-1:	li	r0,0
-	stb	r0,PACASOFTIRQEN(r13);
-	TRACE_DISABLE_INTS
+1:
+#if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_BUG)
+	/* The interrupt should not have soft enabled. */
+	lbz	r7,PACASOFTIRQEN(r13)
+1:	tdnei	r7,0
+	EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,BUGFLAG_WARNING
+#endif
 	b	.Ldo_restore
 
 	/*

Signature · 1 line

2.15.0

Nicholas Piggin

author reviewer

2017-12-04 06:07 UTC #5 lore raw

Re: [PATCH 2/4] powerpc/64: do not trace irqs-off at interrupt return to soft-disabled context

Quoted 21 lines · On Mon, 04 Dec 2017 16:09:57 +1100 Michael Ellerman <[email protected]> wrote:

Quoted 10 lines · Nicholas Piggin
When an interrupt is returning to a soft-disabled context (which can happen for non-maskable interrupts or synchronous interrupts), it goes through the motions of soft-disabling again, including calling TRACE_DISABLE_INTS (i.e., trace_hardirqs_off()).
This is not necessary, because we must already be soft-disabled in the interrupt context, it also may be causing crashes in the irq tracing code to re-enter as an nmi. Replace it with a warning to ensure that soft-interrupts are still disabled.
Quoted 4 lines
Signed-off-by Nicholas Piggin <[email protected]>
---
 arch/powerpc/kernel/entry_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)  
So this patch is the core of the bug fix I gather.
Git blames says:

Quoted 7 lines

Fixes 7c0482e3d055 ("powerpc/irq: Fix another case of lazy IRQ state getting out of sync")
Cc [email protected] # v3.4+
But I'm wondering how this has been broken that long without us noticing? You hit it doing some sort of perf stress test I think - so is it just that we've never pushed hard enough? Or did something change to expose this? Or we're just not sure?

I'm not really sure. A customer hit it, during either a stress test or long running workload with lockdep irq tracing and perf running at the same time. I don't have a lot more details but we might be able to get some offline if necessary.

Thanks,
Nick

Michael Ellerman

reviewer

2017-12-04 12:55 UTC #6 lore raw

Re: [PATCH 2/4] powerpc/64: do not trace irqs-off at interrupt return to soft-disabled context

Quoted 32 lines · Nicholas Piggin

Quoted 21 lines · On Mon, 04 Dec 2017 16:09:57 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 10 lines · Nicholas Piggin
When an interrupt is returning to a soft-disabled context (which can happen for non-maskable interrupts or synchronous interrupts), it goes through the motions of soft-disabling again, including calling TRACE_DISABLE_INTS (i.e., trace_hardirqs_off()).
This is not necessary, because we must already be soft-disabled in the interrupt context, it also may be causing crashes in the irq tracing code to re-enter as an nmi. Replace it with a warning to ensure that soft-interrupts are still disabled.
Quoted 4 lines
Signed-off-by Nicholas Piggin <[email protected]>
---
 arch/powerpc/kernel/entry_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)  
So this patch is the core of the bug fix I gather.
Git blames says:
Quoted 7 lines
Fixes 7c0482e3d055 ("powerpc/irq: Fix another case of lazy IRQ state getting out of sync")
Cc [email protected] # v3.4+
But I'm wondering how this has been broken that long without us noticing? You hit it doing some sort of perf stress test I think - so is it just that we've never pushed hard enough? Or did something change to expose this? Or we're just not sure?

Quoted 4 lines

I'm not really sure. A customer hit it, during either a stress test or long running workload with lockdep irq tracing and perf running at the same time. I don't have a lot more details but we might be able to get some offline if necessary.

No worries.

I'll put it in and add it to my maybe-for-stable list. We can let it soak a bit and then send it to stable if we think it's necessary and has no issues.

cheers

Benjamin Herrenschmidt

2017-12-04 16:12 UTC #7 lore raw

Re: [PATCH 2/4] powerpc/64: do not trace irqs-off at interrupt return to soft-disabled context

In reply to Michael Ellerman

Quoted 21 lines · Michael Ellerman

Quoted 10 lines · Nicholas Piggin
When an interrupt is returning to a soft-disabled context (which can happen for non-maskable interrupts or synchronous interrupts), it goes through the motions of soft-disabling again, including calling TRACE_DISABLE_INTS (i.e., trace_hardirqs_off()).
This is not necessary, because we must already be soft-disabled in the interrupt context, it also may be causing crashes in the irq tracing code to re-enter as an nmi. Replace it with a warning to ensure that soft-interrupts are still disabled.
Quoted 4 lines
Signed-off-by Nicholas Piggin <[email protected]>
---
 arch/powerpc/kernel/entry_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
So this patch is the core of the bug fix I gather.
Git blames says:

Quoted 7 lines

Fixes 7c0482e3d055 ("powerpc/irq: Fix another case of lazy IRQ state getting out of sync")
Cc [email protected] # v3.4+
But I'm wondering how this has been broken that long without us noticing? You hit it doing some sort of perf stress test I think - so is it just that we've never pushed hard enough? Or did something change to expose this? Or we're just not sure?

We have some traps that do local_irq_enable ... you may want to double check instruction emu, page faults, alignment etc... I wouldn't be surprised if we have case where an interrupt "returns" soft enabled.

Quoted 25 lines

cheers

Quoted 23 lines

arch/powerpc/kernel/entry_64.S +7 -3

index 3320bcac7192..36878b6ee8b8 100644
@@ -911,9 +911,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	beq	1f
 	rlwinm	r7,r7,0,~PACA_IRQ_HARD_DIS
 	stb	r7,PACAIRQHAPPENED(r13)
-1:	li	r0,0
-	stb	r0,PACASOFTIRQEN(r13);
-	TRACE_DISABLE_INTS
+1:
+#if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_BUG)
+	/* The interrupt should not have soft enabled. */
+	lbz	r7,PACASOFTIRQEN(r13)
+1:	tdnei	r7,0
+	EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,BUGFLAG_WARNING
+#endif
 	b	.Ldo_restore
 
 	/*

Signature · 1 line

2.15.0

Michael Ellerman

reviewer

2017-12-05 05:52 UTC #8 lore raw

Re: [PATCH 2/4] powerpc/64: do not trace irqs-off at interrupt return to soft-disabled context

Quoted 25 lines · Benjamin Herrenschmidt

Quoted 24 lines · Michael Ellerman
Quoted 10 lines · Nicholas Piggin
When an interrupt is returning to a soft-disabled context (which can happen for non-maskable interrupts or synchronous interrupts), it goes through the motions of soft-disabling again, including calling TRACE_DISABLE_INTS (i.e., trace_hardirqs_off()).
This is not necessary, because we must already be soft-disabled in the interrupt context, it also may be causing crashes in the irq tracing code to re-enter as an nmi. Replace it with a warning to ensure that soft-interrupts are still disabled.
Quoted 4 lines
Signed-off-by Nicholas Piggin <[email protected]>
---
 arch/powerpc/kernel/entry_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
So this patch is the core of the bug fix I gather.
Git blames says:
Fixes 7c0482e3d055 ("powerpc/irq: Fix another case of lazy IRQ state getting out of sync")
Cc [email protected] # v3.4+

Quoted 8 lines

Quoted 4 lines
But I'm wondering how this has been broken that long without us noticing? You hit it doing some sort of perf stress test I think - so is it just that we've never pushed hard enough? Or did something change to expose this? Or we're just not sure?
We have some traps that do local_irq_enable ... you may want to double check instruction emu, page faults, alignment etc... I wouldn't be surprised if we have case where an interrupt "returns" soft enabled.

AFAICT those all check that they're coming from a soft-enabled context:

	if (!arch_irq_disabled_regs(regs))
		local_irq_enable();

And the code Nick is patching is the case where we're returning to a soft-disabled context. So I think the patch is good.

cheers

Michael Ellerman

Applied

2017-12-12 11:39 UTC #9 lore raw

Re: [2/4] powerpc/64: do not trace irqs-off at interrupt return to soft-disabled context

In reply to Nicholas Piggin

Quoted 11 lines · Nicholas Piggin

When an interrupt is returning to a soft-disabled context (which can happen for non-maskable interrupts or synchronous interrupts), it goes through the motions of soft-disabling again, including calling TRACE_DISABLE_INTS (i.e., trace_hardirqs_off()).
This is not necessary, because we must already be soft-disabled in the interrupt context, it also may be causing crashes in the irq tracing code to re-enter as an nmi. Replace it with a warning to ensure that soft-interrupts are still disabled.
Signed-off-by Nicholas Piggin <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/acb1feab320e38588fccc568e37677

cheers

Nicholas Piggin

author reviewer

2017-11-16 16:01 UTC #10 lore raw

PATCH 3/4 cpuidle/powernv: avoid double irq enable coming out of idle

In reply to Nicholas Piggin

Since e1689795a7 ("cpuidle: Add common time keeping and irq enabling"), cpuidle drivers are expected to return from ->enter with irqs disabled.

Update the cpuidle-powernv snooze loop to disable irqs before returning.

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/cpuidle/cpuidle-powernv.c +2 -0

index e06605b21841..1a8234e706bc 100644
@@ -76,6 +76,8 @@ static int snooze_loop(struct cpuidle_device *dev,
 	ppc64_runlatch_on();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
 
+	local_irq_disable();
+
 	return index;
 }

Signature · 1 line

2.15.0

Michael Ellerman

Applied

2018-01-22 03:34 UTC #11 lore raw

Re: [3/4] cpuidle/powernv: avoid double irq enable coming out of idle

Quoted 6 lines · Nicholas Piggin

Since e1689795a7 ("cpuidle: Add common time keeping and irq enabling"), cpuidle drivers are expected to return from ->enter with irqs disabled.
Update the cpuidle-powernv snooze loop to disable irqs before returning.
Signed-off-by Nicholas Piggin <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/f1343d044629f11e7d63ef1a07edb1

cheers

Nicholas Piggin

author reviewer

2017-11-16 16:01 UTC #12 lore raw

PATCH 4/4 cpuidle/powernv: avoid double irq enable coming out of idle

In reply to Nicholas Piggin

Since e1689795a7 ("cpuidle: Add common time keeping and irq enabling"), cpuidle drivers are expected to return from ->enter with irqs disabled.

Update the cpuidle-powernv snooze and cede loops to disable irqs before returning.

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-pseries.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-pseries.c +4 -2

index a187a39fb866..0f2b697cbb27 100644
@@ -51,8 +51,6 @@ static inline void idle_loop_epilog(unsigned long in_purr)
 	get_lppaca()->wait_state_cycles = cpu_to_be64(wait_cycles);
 	get_lppaca()->idle = 0;
 
-	if (irqs_disabled())
-		local_irq_enable();
 	ppc64_runlatch_on();
 }

@@ -87,6 +85,8 @@ static int snooze_loop(struct cpuidle_device *dev,
 	HMT_medium();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
 
+	local_irq_disable();
+
 	idle_loop_epilog(in_purr);
 
 	return index;

@@ -121,6 +121,7 @@ static int dedicated_cede_loop(struct cpuidle_device *dev,
 	HMT_medium();
 	check_and_cede_processor();
 
+	local_irq_disable();
 	get_lppaca()->donate_dedicated_cpu = 0;
 
 	idle_loop_epilog(in_purr);

@@ -145,6 +146,7 @@ static int shared_cede_loop(struct cpuidle_device *dev,
 	 */
 	check_and_cede_processor();
 
+	local_irq_disable();
 	idle_loop_epilog(in_purr);
 
 	return index;

Signature · 1 line

2.15.0

Michael Ellerman

Applied

2018-01-22 03:34 UTC #13 lore raw

Re: [4/4] cpuidle/powernv: avoid double irq enable coming out of idle

Quoted 7 lines · Nicholas Piggin

Since e1689795a7 ("cpuidle: Add common time keeping and irq enabling"), cpuidle drivers are expected to return from ->enter with irqs disabled.
Update the cpuidle-powernv snooze and cede loops to disable irqs before returning.
Signed-off-by Nicholas Piggin <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/ced54c08d8e4060d59c10629ea5a4c

cheers