[PATCH 0/4] interrupt tracing fixes

[PATCH 0/4] interrupt tracing fixes

[Nicholas Piggin]

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(-)

-- 
2.15.0

[PATCH 1/4] powerpc: define __ARCH_IRQ_EXIT_IRQS_DISABLED

[Nicholas Piggin]

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

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/hardirq.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/include/asm/hardirq.h b/arch/powerpc/include/asm/hardirq.h
index 456f9e7b8d83..5986d473722b 100644
--- a/arch/powerpc/include/asm/hardirq.h
+++ b/arch/powerpc/include/asm/hardirq.h
@@ -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))
-- 
2.15.0

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

[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 <npiggin@gmail.com>
---
 arch/powerpc/kernel/entry_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 3320bcac7192..36878b6ee8b8 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -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
 
 	/*
-- 
2.15.0

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

[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 <npiggin@gmail.com>
---
 drivers/cpuidle/cpuidle-powernv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index e06605b21841..1a8234e706bc 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -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;
 }
 
-- 
2.15.0

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

[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 <npiggin@gmail.com>
---
 drivers/cpuidle/cpuidle-pseries.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index a187a39fb866..0f2b697cbb27 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -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;
-- 
2.15.0

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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> 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 <npiggin@gmail.com>
> ---
>  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: stable@vger.kernel.org # 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

> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 3320bcac7192..36878b6ee8b8 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -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
>  
>  	/*
> -- 
> 2.15.0

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

[Nicholas Piggin]

On Mon, 04 Dec 2017 16:09:57 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > 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 <npiggin@gmail.com>
> > ---
> >  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: stable@vger.kernel.org # 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

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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> On Mon, 04 Dec 2017 16:09:57 +1100
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> 
>> > 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 <npiggin@gmail.com>
>> > ---
>> >  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: stable@vger.kernel.org # 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.

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

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

[Benjamin Herrenschmidt]

On Mon, 2017-12-04 at 16:09 +1100, Michael Ellerman wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > 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 <npiggin@gmail.com>
> > ---
> >  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: stable@vger.kernel.org # 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.

> cheers
> 
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 3320bcac7192..36878b6ee8b8 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -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
> >  
> >  	/*
> > -- 
> > 2.15.0

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

[Michael Ellerman]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Mon, 2017-12-04 at 16:09 +1100, Michael Ellerman wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> 
>> > 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 <npiggin@gmail.com>
>> > ---
>> >  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: stable@vger.kernel.org # 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.

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

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

[Michael Ellerman]

On Thu, 2017-11-16 at 16:00:50 UTC, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>

Applied to powerpc next, thanks.

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

cheers

Re: [1/4] powerpc: define __ARCH_IRQ_EXIT_IRQS_DISABLED

[Michael Ellerman]

On Thu, 2017-11-16 at 16:00:49 UTC, Nicholas Piggin wrote:
> powerpc calls irq_exit() with local irqs disabled, therefore it
> can define __ARCH_IRQ_EXIT_IRQS_DISABLED.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

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

cheers

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

[Michael Ellerman]

On Thu, 2017-11-16 at 16:00:51 UTC, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>

Applied to powerpc next, thanks.

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

cheers

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

[Michael Ellerman]

On Thu, 2017-11-16 at 16:00:52 UTC, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>

Applied to powerpc next, thanks.

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

cheers