diff mbox

[RFC] lockdep: WARN about local_irq_{en,dis}able in NMI context

Message ID 1270552321.1595.42.camel@laptop
State RFC
Delegated to: David Miller
Headers show

Commit Message

Peter Zijlstra April 6, 2010, 11:12 a.m. UTC 
On Tue, 2010-04-06 at 02:50 -0700, David Miller wrote:
> From: Frederic Weisbecker <fweisbec@gmail.com>
> Date: Mon, 5 Apr 2010 21:40:58 +0200
> 
> > It happens without CONFIG_FUNCTION_TRACER as well (but it happens
> > when the function tracer runs). And I hadn't your
> > perf_arch_save_caller_regs() when I triggered this.
> 
> I figured out the problem, it's NMIs.  As soon as I disable all of the
> NMI watchdog code, the problem goes away.
> 
> This is because some parts of the NMI interrupt handling path are not
> marked with "notrace" and the various tracer code paths use
> local_irq_disable() (either directly or indirectly) which doesn't work
> with sparc64's NMI scheme.  These essentially turn NMIs back on in the
> NMI handler before the NMI condition has been cleared, and thus we can
> re-enter with another NMI interrupt.
> 
> We went through this for perf events, and we just made sure that
> local_irq_{enable,disable}() never occurs in any of the code paths in
> perf events that can be reached via the NMI interrupt handler.  (the
> only one we had was sched_clock() and that was easily fixed)

One thing we can do is make the code WARN about this, how about
something like the below

---
Subject: lockdep: WARN about local_irq_{en,dis}able in NMI context

Some architectures implement NMIs by using IRQ priority levels and
mixing local_irq_{en,dis}able() with NMIs will give unexpected results.

Hence disallow this in general and WARN about it.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/lockdep.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller April 6, 2010, 11:13 a.m. UTC | #1 
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 06 Apr 2010 13:12:01 +0200

> One thing we can do is make the code WARN about this, how about
> something like the below

It's going to warn every bootup in cpu_clock() on x86.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Zijlstra April 6, 2010, 11:20 a.m. UTC | #2 
On Tue, 2010-04-06 at 04:13 -0700, David Miller wrote:
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue, 06 Apr 2010 13:12:01 +0200
> 
> > One thing we can do is make the code WARN about this, how about
> > something like the below
> 
> It's going to warn every bootup in cpu_clock() on x86.

*sigh*, yes, we could hack around that I suppose.. it would be nice to
automate this check though, I bet you don't fancy tracking down more
such splats than you have to.

You could of course insert the debug code into your arch routines but
that would limit the coverage checks to whatever you happen to run.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller April 6, 2010, 11:22 a.m. UTC | #3 
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 06 Apr 2010 13:20:19 +0200

> On Tue, 2010-04-06 at 04:13 -0700, David Miller wrote:
>> From: Peter Zijlstra <peterz@infradead.org>
>> Date: Tue, 06 Apr 2010 13:12:01 +0200
>> 
>> > One thing we can do is make the code WARN about this, how about
>> > something like the below
>> 
>> It's going to warn every bootup in cpu_clock() on x86.
> 
> *sigh*, yes, we could hack around that I suppose.. it would be nice to
> automate this check though, I bet you don't fancy tracking down more
> such splats than you have to.
> 
> You could of course insert the debug code into your arch routines but
> that would limit the coverage checks to whatever you happen to run.

Yes, f.e. you could add local_irq_*_nmi() or similar that don't
complain when called inside of an NMI.

I would certainly welcome this debugging facility, for sure!

It would have saved me two days of work this time, in fact.


--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 08e6f76..06ec1c7 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2341,6 +2341,11 @@  EXPORT_SYMBOL(trace_hardirqs_on_caller);
 
 void trace_hardirqs_on(void)
 {
+	/*
+	 * Some architectures can't deal with local_irq_{enable,disable}
+	 * from NMI context (SPARC), enforce this.
+	 */
+	WARN_ON_ONCE(in_nmi());
 	trace_hardirqs_on_caller(CALLER_ADDR0);
 }
 EXPORT_SYMBOL(trace_hardirqs_on);
@@ -2375,6 +2380,11 @@  EXPORT_SYMBOL(trace_hardirqs_off_caller);
 
 void trace_hardirqs_off(void)
 {
+	/*
+	 * Some architectures can't deal with local_irq_{enable,disable}
+	 * from NMI context (SPARC), enforce this.
+	 */
+	WARN_ON_ONCE(in_nmi());
 	trace_hardirqs_off_caller(CALLER_ADDR0);
 }
 EXPORT_SYMBOL(trace_hardirqs_off);