[patch 0/3] IRQ disable vs NMI

[patch 0/3] IRQ disable vs NMI

[Peter Zijlstra]

This patch set tries to solve the local_irq_disable() vs NMI problem
that SPARC has by providing new arch hooks and instrumenting the
existing interface to WARN on conflicting usage.

[patch 0/3] IRQ disable vs NMI

[Peter Zijlstra]

This patch set tries to solve the local_irq_disable() vs NMI problem
that SPARC has by providing new arch hooks and instrumenting the
existing interface to WARN on conflicting usage.

[patch 1/3] kernel: local_irq_{save,restore}_nmi()

[Peter Zijlstra]

[-- Attachment #1: nmi-irq.patch --]
[-- Type: text/plain, Size: 5135 bytes --]

Provide local_irq_{save,restore}_nmi() which will allow us to help
architectures that implement NMIs using IRQ priorities like SPARC64
does.

Sparc uses IRQ prio 15 for NMIs and implements local_irq_disable() as
disable <= 14. However if you do that while inside an NMI you re-
enable the NMI priority again, causing all kinds of fun.

A more solid implementation would first check the disable level and
never lower it, however that is more costly and would slow down the
rest of the kernel for no particular reason.

Therefore introduce local_irq_save_nmi() which can implement this
slower but more solid scheme and dis-allow local_irq_save() from NMI
context.

Suggested-by: David Miller <davem@davemloft.net>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/irqflags.h     |   51 ++++++++++++++++++++++++++++++++++++++++---
 kernel/lockdep.c             |    7 +++++
 kernel/trace/trace_irqsoff.c |    8 ++++++
 3 files changed, 63 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/irqflags.h
===================================================================
--- linux-2.6.orig/include/linux/irqflags.h
+++ linux-2.6/include/linux/irqflags.h
@@ -18,6 +18,7 @@
   extern void trace_softirqs_off(unsigned long ip);
   extern void trace_hardirqs_on(void);
   extern void trace_hardirqs_off(void);
+  extern void trace_hardirqs_off_no_nmi(void);
 # define trace_hardirq_context(p)	((p)->hardirq_context)
 # define trace_softirq_context(p)	((p)->softirq_context)
 # define trace_hardirqs_enabled(p)	((p)->hardirqs_enabled)
@@ -30,6 +31,7 @@
 #else
 # define trace_hardirqs_on()		do { } while (0)
 # define trace_hardirqs_off()		do { } while (0)
+# define trace_hardirqs_off_no_nmi()	do { } while (0)
 # define trace_softirqs_on(ip)		do { } while (0)
 # define trace_softirqs_off(ip)		do { } while (0)
 # define trace_hardirq_context(p)	0
@@ -59,15 +61,15 @@
 #define local_irq_enable() \
 	do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0)
 #define local_irq_disable() \
-	do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
+	do { raw_local_irq_disable(); trace_hardirqs_off_no_nmi(); } while (0)
+
 #define local_irq_save(flags)				\
 	do {						\
 		typecheck(unsigned long, flags);	\
 		raw_local_irq_save(flags);		\
-		trace_hardirqs_off();			\
+		trace_hardirqs_off_no_nmi();		\
 	} while (0)
 
-
 #define local_irq_restore(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
@@ -79,6 +81,30 @@
 			raw_local_irq_restore(flags);	\
 		}					\
 	} while (0)
+
+#ifndef local_irq_save_nmi
+# define local_irq_save_nmi(flags) 			\
+	 do {						\
+		 typecheck(unsigned long, flags);	\
+		 raw_local_irq_save(flags);		\
+		 trace_hardirqs_off();			\
+	 } while (0)
+#endif
+
+#ifndef local_irq_restore_nmi
+#define local_irq_restore_nmi(flags)			\
+	do {						\
+		typecheck(unsigned long, flags);	\
+		if (raw_irqs_disabled_flags(flags)) {	\
+			raw_local_irq_restore(flags);	\
+			trace_hardirqs_off();		\
+		} else {				\
+			trace_hardirqs_on();		\
+			raw_local_irq_restore(flags);	\
+		}					\
+	} while (0)
+#endif
+
 #else /* !CONFIG_TRACE_IRQFLAGS_SUPPORT */
 /*
  * The local_irq_*() APIs are equal to the raw_local_irq*()
@@ -86,16 +112,35 @@
  */
 # define raw_local_irq_disable()	local_irq_disable()
 # define raw_local_irq_enable()		local_irq_enable()
+
 # define raw_local_irq_save(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
 		local_irq_save(flags);			\
 	} while (0)
+
 # define raw_local_irq_restore(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
 		local_irq_restore(flags);		\
 	} while (0)
+
+#ifndef local_irq_save_nmi
+# define local_irq_save_nmi(flags)			\
+	 do {						\
+		 typecheck(unsigned long, flags);	\
+		 local_irq_save(flags);			\
+	 } while (0)
+#endif
+
+#ifndef local_irq_restore_nmi
+# define local_irq_restore_nmi(flags)			\
+	 do {						\
+		 typecheck(unsigned long, flags);	\
+		 local_irq_restore(flags);		\
+	 } while (0)
+#endif
+
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
Index: linux-2.6/kernel/lockdep.c
===================================================================
--- linux-2.6.orig/kernel/lockdep.c
+++ linux-2.6/kernel/lockdep.c
@@ -2369,6 +2369,13 @@ void trace_hardirqs_off(void)
 }
 EXPORT_SYMBOL(trace_hardirqs_off);
 
+void trace_hardirqs_off_no_nmi(void)
+{
+	WARN_ON_ONCE(in_nmi());
+	trace_hardirqs_off_caller(CALLER_ADDR0);
+}
+EXPORT_SYMBOL(trace_hardirqs_off_no_nmi);
+
 /*
  * Softirqs will be enabled:
  */
Index: linux-2.6/kernel/trace/trace_irqsoff.c
===================================================================
--- linux-2.6.orig/kernel/trace/trace_irqsoff.c
+++ linux-2.6/kernel/trace/trace_irqsoff.c
@@ -316,6 +316,14 @@ void trace_hardirqs_off(void)
 }
 EXPORT_SYMBOL(trace_hardirqs_off);
 
+void trace_hardirqs_off_no_nmi(void)
+{
+	WARN_ON_ONCE(in_nmi());
+	if (!preempt_trace() && irq_trace())
+		start_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
+}
+EXPORT_SYMBOL(trace_hardirqs_off_no_nmi);
+
 void trace_hardirqs_on_caller(unsigned long caller_addr)
 {
 	if (!preempt_trace() && irq_trace())

[patch 1/3] kernel: local_irq_{save,restore}_nmi()

[Peter Zijlstra]

[-- Attachment #1: nmi-irq.patch --]
[-- Type: text/plain, Size: 5137 bytes --]

Provide local_irq_{save,restore}_nmi() which will allow us to help
architectures that implement NMIs using IRQ priorities like SPARC64
does.

Sparc uses IRQ prio 15 for NMIs and implements local_irq_disable() as
disable <= 14. However if you do that while inside an NMI you re-
enable the NMI priority again, causing all kinds of fun.

A more solid implementation would first check the disable level and
never lower it, however that is more costly and would slow down the
rest of the kernel for no particular reason.

Therefore introduce local_irq_save_nmi() which can implement this
slower but more solid scheme and dis-allow local_irq_save() from NMI
context.

Suggested-by: David Miller <davem@davemloft.net>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/irqflags.h     |   51 ++++++++++++++++++++++++++++++++++++++++---
 kernel/lockdep.c             |    7 +++++
 kernel/trace/trace_irqsoff.c |    8 ++++++
 3 files changed, 63 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/irqflags.h
===================================================================
--- linux-2.6.orig/include/linux/irqflags.h
+++ linux-2.6/include/linux/irqflags.h
@@ -18,6 +18,7 @@
   extern void trace_softirqs_off(unsigned long ip);
   extern void trace_hardirqs_on(void);
   extern void trace_hardirqs_off(void);
+  extern void trace_hardirqs_off_no_nmi(void);
 # define trace_hardirq_context(p)	((p)->hardirq_context)
 # define trace_softirq_context(p)	((p)->softirq_context)
 # define trace_hardirqs_enabled(p)	((p)->hardirqs_enabled)
@@ -30,6 +31,7 @@
 #else
 # define trace_hardirqs_on()		do { } while (0)
 # define trace_hardirqs_off()		do { } while (0)
+# define trace_hardirqs_off_no_nmi()	do { } while (0)
 # define trace_softirqs_on(ip)		do { } while (0)
 # define trace_softirqs_off(ip)		do { } while (0)
 # define trace_hardirq_context(p)	0
@@ -59,15 +61,15 @@
 #define local_irq_enable() \
 	do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0)
 #define local_irq_disable() \
-	do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
+	do { raw_local_irq_disable(); trace_hardirqs_off_no_nmi(); } while (0)
+
 #define local_irq_save(flags)				\
 	do {						\
 		typecheck(unsigned long, flags);	\
 		raw_local_irq_save(flags);		\
-		trace_hardirqs_off();			\
+		trace_hardirqs_off_no_nmi();		\
 	} while (0)
 
-
 #define local_irq_restore(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
@@ -79,6 +81,30 @@
 			raw_local_irq_restore(flags);	\
 		}					\
 	} while (0)
+
+#ifndef local_irq_save_nmi
+# define local_irq_save_nmi(flags) 			\
+	 do {						\
+		 typecheck(unsigned long, flags);	\
+		 raw_local_irq_save(flags);		\
+		 trace_hardirqs_off();			\
+	 } while (0)
+#endif
+
+#ifndef local_irq_restore_nmi
+#define local_irq_restore_nmi(flags)			\
+	do {						\
+		typecheck(unsigned long, flags);	\
+		if (raw_irqs_disabled_flags(flags)) {	\
+			raw_local_irq_restore(flags);	\
+			trace_hardirqs_off();		\
+		} else {				\
+			trace_hardirqs_on();		\
+			raw_local_irq_restore(flags);	\
+		}					\
+	} while (0)
+#endif
+
 #else /* !CONFIG_TRACE_IRQFLAGS_SUPPORT */
 /*
  * The local_irq_*() APIs are equal to the raw_local_irq*()
@@ -86,16 +112,35 @@
  */
 # define raw_local_irq_disable()	local_irq_disable()
 # define raw_local_irq_enable()		local_irq_enable()
+
 # define raw_local_irq_save(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
 		local_irq_save(flags);			\
 	} while (0)
+
 # define raw_local_irq_restore(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
 		local_irq_restore(flags);		\
 	} while (0)
+
+#ifndef local_irq_save_nmi
+# define local_irq_save_nmi(flags)			\
+	 do {						\
+		 typecheck(unsigned long, flags);	\
+		 local_irq_save(flags);			\
+	 } while (0)
+#endif
+
+#ifndef local_irq_restore_nmi
+# define local_irq_restore_nmi(flags)			\
+	 do {						\
+		 typecheck(unsigned long, flags);	\
+		 local_irq_restore(flags);		\
+	 } while (0)
+#endif
+
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
Index: linux-2.6/kernel/lockdep.c
===================================================================
--- linux-2.6.orig/kernel/lockdep.c
+++ linux-2.6/kernel/lockdep.c
@@ -2369,6 +2369,13 @@ void trace_hardirqs_off(void)
 }
 EXPORT_SYMBOL(trace_hardirqs_off);
 
+void trace_hardirqs_off_no_nmi(void)
+{
+	WARN_ON_ONCE(in_nmi());
+	trace_hardirqs_off_caller(CALLER_ADDR0);
+}
+EXPORT_SYMBOL(trace_hardirqs_off_no_nmi);
+
 /*
  * Softirqs will be enabled:
  */
Index: linux-2.6/kernel/trace/trace_irqsoff.c
===================================================================
--- linux-2.6.orig/kernel/trace/trace_irqsoff.c
+++ linux-2.6/kernel/trace/trace_irqsoff.c
@@ -316,6 +316,14 @@ void trace_hardirqs_off(void)
 }
 EXPORT_SYMBOL(trace_hardirqs_off);
 
+void trace_hardirqs_off_no_nmi(void)
+{
+	WARN_ON_ONCE(in_nmi());
+	if (!preempt_trace() && irq_trace())
+		start_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
+}
+EXPORT_SYMBOL(trace_hardirqs_off_no_nmi);
+
 void trace_hardirqs_on_caller(unsigned long caller_addr)
 {
 	if (!preempt_trace() && irq_trace())

[patch 2/3] perf: Use local_irq_save_nmi()

[Peter Zijlstra]

[-- Attachment #1: perf-fix-perf_output_lock.patch --]
[-- Type: text/plain, Size: 2527 bytes --]

Patch 8bb39f9 (perf: Fix 'perf sched record' deadlock) introduced a
local_irq_save() in NMI context, convert that to local_irq_save_nmi()
and move the IRQ disable into perf_output_lock/unlock().

The former is needed because we now disallow local_irq_disable() from
NMI context due to some arch limitations.

The second is because its really about IRQ lock inversion with that
funny output lock, and perf_event_task_output() is only one site that
could trigger it.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
---
 include/linux/perf_event.h |    1 +
 kernel/perf_event.c        |   17 ++++++-----------
 2 files changed, 7 insertions(+), 11 deletions(-)

Index: linux-2.6/include/linux/perf_event.h
===================================================================
--- linux-2.6.orig/include/linux/perf_event.h
+++ linux-2.6/include/linux/perf_event.h
@@ -758,6 +758,7 @@ struct perf_output_handle {
 	struct perf_mmap_data		*data;
 	unsigned long			head;
 	unsigned long			offset;
+	unsigned long			flags;
 	int				nmi;
 	int				sample;
 	int				locked;
Index: linux-2.6/kernel/perf_event.c
===================================================================
--- linux-2.6.orig/kernel/perf_event.c
+++ linux-2.6/kernel/perf_event.c
@@ -2848,6 +2848,10 @@ static void perf_output_lock(struct perf
 	struct perf_mmap_data *data = handle->data;
 	int cur, cpu = get_cpu();
 
+	/*
+	 * Since this is a lock we need to be IRQ-safe
+	 */
+	local_irq_save_nmi(handle->flags);
 	handle->locked = 0;
 
 	for (;;) {
@@ -2906,6 +2910,7 @@ again:
 	if (atomic_xchg(&data->wakeup, 0))
 		perf_output_wakeup(handle);
 out:
+	local_irq_restore_nmi(handle->flags);
 	put_cpu();
 }
 
@@ -3385,19 +3390,10 @@ static void perf_event_task_output(struc
 	unsigned long flags;
 	int size, ret;
 
-	/*
-	 * If this CPU attempts to acquire an rq lock held by a CPU spinning
-	 * in perf_output_lock() from interrupt context, it's game over.
-	 */
-	local_irq_save(flags);
-
 	size  = task_event->event_id.header.size;
 	ret = perf_output_begin(&handle, event, size, 0, 0);
-
-	if (ret) {
-		local_irq_restore(flags);
+	if (ret)
 		return;
-	}
 
 	task_event->event_id.pid = perf_event_pid(event, task);
 	task_event->event_id.ppid = perf_event_pid(event, current);
@@ -3408,7 +3404,6 @@ static void perf_event_task_output(struc
 	perf_output_put(&handle, task_event->event_id);
 
 	perf_output_end(&handle);
-	local_irq_restore(flags);
 }
 
 static int perf_event_task_match(struct perf_event *event)

[patch 2/3] perf: Use local_irq_save_nmi()

[Peter Zijlstra]

[-- Attachment #1: perf-fix-perf_output_lock.patch --]
[-- Type: text/plain, Size: 2529 bytes --]

Patch 8bb39f9 (perf: Fix 'perf sched record' deadlock) introduced a
local_irq_save() in NMI context, convert that to local_irq_save_nmi()
and move the IRQ disable into perf_output_lock/unlock().

The former is needed because we now disallow local_irq_disable() from
NMI context due to some arch limitations.

The second is because its really about IRQ lock inversion with that
funny output lock, and perf_event_task_output() is only one site that
could trigger it.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
---
 include/linux/perf_event.h |    1 +
 kernel/perf_event.c        |   17 ++++++-----------
 2 files changed, 7 insertions(+), 11 deletions(-)

Index: linux-2.6/include/linux/perf_event.h
===================================================================
--- linux-2.6.orig/include/linux/perf_event.h
+++ linux-2.6/include/linux/perf_event.h
@@ -758,6 +758,7 @@ struct perf_output_handle {
 	struct perf_mmap_data		*data;
 	unsigned long			head;
 	unsigned long			offset;
+	unsigned long			flags;
 	int				nmi;
 	int				sample;
 	int				locked;
Index: linux-2.6/kernel/perf_event.c
===================================================================
--- linux-2.6.orig/kernel/perf_event.c
+++ linux-2.6/kernel/perf_event.c
@@ -2848,6 +2848,10 @@ static void perf_output_lock(struct perf
 	struct perf_mmap_data *data = handle->data;
 	int cur, cpu = get_cpu();
 
+	/*
+	 * Since this is a lock we need to be IRQ-safe
+	 */
+	local_irq_save_nmi(handle->flags);
 	handle->locked = 0;
 
 	for (;;) {
@@ -2906,6 +2910,7 @@ again:
 	if (atomic_xchg(&data->wakeup, 0))
 		perf_output_wakeup(handle);
 out:
+	local_irq_restore_nmi(handle->flags);
 	put_cpu();
 }
 
@@ -3385,19 +3390,10 @@ static void perf_event_task_output(struc
 	unsigned long flags;
 	int size, ret;
 
-	/*
-	 * If this CPU attempts to acquire an rq lock held by a CPU spinning
-	 * in perf_output_lock() from interrupt context, it's game over.
-	 */
-	local_irq_save(flags);
-
 	size  = task_event->event_id.header.size;
 	ret = perf_output_begin(&handle, event, size, 0, 0);
-
-	if (ret) {
-		local_irq_restore(flags);
+	if (ret)
 		return;
-	}
 
 	task_event->event_id.pid = perf_event_pid(event, task);
 	task_event->event_id.ppid = perf_event_pid(event, current);
@@ -3408,7 +3404,6 @@ static void perf_event_task_output(struc
 	perf_output_put(&handle, task_event->event_id);
 
 	perf_output_end(&handle);
-	local_irq_restore(flags);
 }
 
 static int perf_event_task_match(struct perf_event *event)

[patch 3/3] sched: Use local_irq_save_nmi() in cpu_clock()

[Peter Zijlstra]

[-- Attachment #1: sched-clock-nmi.patch --]
[-- Type: text/plain, Size: 704 bytes --]

Since we can call cpu_clock() from NMI context fix up the IRQ
disabling to conform to the new rules.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/sched_clock.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/kernel/sched_clock.c
===================================================================
--- linux-2.6.orig/kernel/sched_clock.c
+++ linux-2.6/kernel/sched_clock.c
@@ -241,9 +241,9 @@ unsigned long long cpu_clock(int cpu)
 	unsigned long long clock;
 	unsigned long flags;
 
-	local_irq_save(flags);
+	local_irq_save_nmi(flags);
 	clock = sched_clock_cpu(cpu);
-	local_irq_restore(flags);
+	local_irq_restore_nmi(flags);
 
 	return clock;
 }

[patch 3/3] sched: Use local_irq_save_nmi() in cpu_clock()

[Peter Zijlstra]

[-- Attachment #1: sched-clock-nmi.patch --]
[-- Type: text/plain, Size: 706 bytes --]

Since we can call cpu_clock() from NMI context fix up the IRQ
disabling to conform to the new rules.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/sched_clock.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/kernel/sched_clock.c
===================================================================
--- linux-2.6.orig/kernel/sched_clock.c
+++ linux-2.6/kernel/sched_clock.c
@@ -241,9 +241,9 @@ unsigned long long cpu_clock(int cpu)
 	unsigned long long clock;
 	unsigned long flags;
 
-	local_irq_save(flags);
+	local_irq_save_nmi(flags);
 	clock = sched_clock_cpu(cpu);
-	local_irq_restore(flags);
+	local_irq_restore_nmi(flags);
 
 	return clock;
 }

Re: [patch 0/3] IRQ disable vs NMI

[David Miller]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue, 06 Apr 2010 15:28:07 +0200

> This patch set tries to solve the local_irq_disable() vs NMI problem
> that SPARC has by providing new arch hooks and instrumenting the
> existing interface to WARN on conflicting usage.

Thanks Peter, looks good at first sight.

I'll toss together the sparc bits and give this a go.

Re: [patch 0/3] IRQ disable vs NMI

[David Miller]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue, 06 Apr 2010 15:28:07 +0200

> This patch set tries to solve the local_irq_disable() vs NMI problem
> that SPARC has by providing new arch hooks and instrumenting the
> existing interface to WARN on conflicting usage.

Thanks Peter, looks good at first sight.

I'll toss together the sparc bits and give this a go.

Re: [patch 0/3] IRQ disable vs NMI

[David Miller]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue, 06 Apr 2010 15:28:07 +0200

> This patch set tries to solve the local_irq_disable() vs NMI problem
> that SPARC has by providing new arch hooks and instrumenting the
> existing interface to WARN on conflicting usage.

Ok, two patches coming.

One which adds the sparc64 irqflags.h methods.

And one which annotates the ftrace functions, as needed.

With this I have the function tracer working.

Thanks!

Re: [patch 0/3] IRQ disable vs NMI

[David Miller]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Tue, 06 Apr 2010 15:28:07 +0200

> This patch set tries to solve the local_irq_disable() vs NMI problem
> that SPARC has by providing new arch hooks and instrumenting the
> existing interface to WARN on conflicting usage.

Ok, two patches coming.

One which adds the sparc64 irqflags.h methods.

And one which annotates the ftrace functions, as needed.

With this I have the function tracer working.

Thanks!

Re: [patch 1/3] kernel: local_irq_{save,restore}_nmi()

[Steven Rostedt]

On Tue, 2010-04-06 at 15:28 +0200, Peter Zijlstra wrote:

> Index: linux-2.6/kernel/trace/trace_irqsoff.c
> ===================================================================
> --- linux-2.6.orig/kernel/trace/trace_irqsoff.c
> +++ linux-2.6/kernel/trace/trace_irqsoff.c
> @@ -316,6 +316,14 @@ void trace_hardirqs_off(void)
>  }
>  EXPORT_SYMBOL(trace_hardirqs_off);
>  
> +void trace_hardirqs_off_no_nmi(void)
> +{
> +	WARN_ON_ONCE(in_nmi());

Should we do this for all archs? I can imagine a lot of warning reports
coming in the near future. And they will be passing it towards me.

-- Steve



> +	if (!preempt_trace() && irq_trace())
> +		start_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
> +}
> +EXPORT_SYMBOL(trace_hardirqs_off_no_nmi);
> +
>  void trace_hardirqs_on_caller(unsigned long caller_addr)
>  {
>  	if (!preempt_trace() && irq_trace())
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 1/3] kernel: local_irq_{save,restore}_nmi()

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Tue, 06 Apr 2010 21:13:10 -0400

> On Tue, 2010-04-06 at 15:28 +0200, Peter Zijlstra wrote:
> 
>> Index: linux-2.6/kernel/trace/trace_irqsoff.c
>> ===================================================================
>> --- linux-2.6.orig/kernel/trace/trace_irqsoff.c
>> +++ linux-2.6/kernel/trace/trace_irqsoff.c
>> @@ -316,6 +316,14 @@ void trace_hardirqs_off(void)
>>  }
>>  EXPORT_SYMBOL(trace_hardirqs_off);
>>  
>> +void trace_hardirqs_off_no_nmi(void)
>> +{
>> +	WARN_ON_ONCE(in_nmi());
> 
> Should we do this for all archs? I can imagine a lot of warning reports
> coming in the near future. And they will be passing it towards me.

That's the whole point, so that the problem is more easily noticed and
it gets fixed long before I end up accidently testing the code on my
machines :-)

To be honest, the fix is so trivial, you just need to add '_nmi' to
the local_irq_{save,restore}() calls that warn like this.

I'm even willing to have you forward all of those reports to me and
I'll be responsible for fixing them.

How's that? :-)

Re: [patch 1/3] kernel: local_irq_{save,restore}_nmi()

[Steven Rostedt]

On Tue, 2010-04-06 at 18:19 -0700, David Miller wrote:

> That's the whole point, so that the problem is more easily noticed and
> it gets fixed long before I end up accidently testing the code on my
> machines :-)
> 
> To be honest, the fix is so trivial, you just need to add '_nmi' to
> the local_irq_{save,restore}() calls that warn like this.
> 
> I'm even willing to have you forward all of those reports to me and
> I'll be responsible for fixing them.
> 
> How's that? :-)

Sure.

/me sets up his procmailrc to search for the WARN_ON line in
trace_irqsoff.c and have it forward to davem.

-- Steve

Re: [patch 1/3] kernel: local_irq_{save,restore}_nmi()

[Steven Rostedt]

On Tue, 2010-04-06 at 18:19 -0700, David Miller wrote:

> That's the whole point, so that the problem is more easily noticed and
> it gets fixed long before I end up accidently testing the code on my
> machines :-)
> 
> To be honest, the fix is so trivial, you just need to add '_nmi' to
> the local_irq_{save,restore}() calls that warn like this.
> 
> I'm even willing to have you forward all of those reports to me and
> I'll be responsible for fixing them.
> 
> How's that? :-)

Sure.

/me sets up his procmailrc to search for the WARN_ON line in
trace_irqsoff.c and have it forward to davem.

-- Steve

Re: [patch 3/3] sched: Use local_irq_save_nmi() in cpu_clock()

[Frederic Weisbecker]

On Tue, Apr 06, 2010 at 03:28:10PM +0200, Peter Zijlstra wrote:
> Since we can call cpu_clock() from NMI context fix up the IRQ
> disabling to conform to the new rules.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  kernel/sched_clock.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/kernel/sched_clock.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched_clock.c
> +++ linux-2.6/kernel/sched_clock.c
> @@ -241,9 +241,9 @@ unsigned long long cpu_clock(int cpu)
>  	unsigned long long clock;
>  	unsigned long flags;
>  
> -	local_irq_save(flags);
> +	local_irq_save_nmi(flags);
>  	clock = sched_clock_cpu(cpu);
> -	local_irq_restore(flags);
> +	local_irq_restore_nmi(flags);
>  
>  	return clock;
>  }


That seem to add a small overhead in various places.
Do we want to make local_irq_save_nmi == local_irq_save
for archs that have native nmi?

Or cpu_clock_nmi()?

Re: [patch 3/3] sched: Use local_irq_save_nmi() in cpu_clock()

[Peter Zijlstra]

On Wed, 2010-04-07 at 13:27 +0200, Frederic Weisbecker wrote:
> On Tue, Apr 06, 2010 at 03:28:10PM +0200, Peter Zijlstra wrote:
> > Since we can call cpu_clock() from NMI context fix up the IRQ
> > disabling to conform to the new rules.
> > 
> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > ---
> >  kernel/sched_clock.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Index: linux-2.6/kernel/sched_clock.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/sched_clock.c
> > +++ linux-2.6/kernel/sched_clock.c
> > @@ -241,9 +241,9 @@ unsigned long long cpu_clock(int cpu)
> >  	unsigned long long clock;
> >  	unsigned long flags;
> >  
> > -	local_irq_save(flags);
> > +	local_irq_save_nmi(flags);
> >  	clock = sched_clock_cpu(cpu);
> > -	local_irq_restore(flags);
> > +	local_irq_restore_nmi(flags);
> >  
> >  	return clock;
> >  }
> 
> 
> That seem to add a small overhead in various places.
> Do we want to make local_irq_save_nmi == local_irq_save
> for archs that have native nmi?

That is already so, see 1/3.

Re: [patch 3/3] sched: Use local_irq_save_nmi() in cpu_clock()

[Peter Zijlstra]

On Wed, 2010-04-07 at 13:27 +0200, Frederic Weisbecker wrote:
> On Tue, Apr 06, 2010 at 03:28:10PM +0200, Peter Zijlstra wrote:
> > Since we can call cpu_clock() from NMI context fix up the IRQ
> > disabling to conform to the new rules.
> > 
> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > ---
> >  kernel/sched_clock.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Index: linux-2.6/kernel/sched_clock.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/sched_clock.c
> > +++ linux-2.6/kernel/sched_clock.c
> > @@ -241,9 +241,9 @@ unsigned long long cpu_clock(int cpu)
> >  	unsigned long long clock;
> >  	unsigned long flags;
> >  
> > -	local_irq_save(flags);
> > +	local_irq_save_nmi(flags);
> >  	clock = sched_clock_cpu(cpu);
> > -	local_irq_restore(flags);
> > +	local_irq_restore_nmi(flags);
> >  
> >  	return clock;
> >  }
> 
> 
> That seem to add a small overhead in various places.
> Do we want to make local_irq_save_nmi == local_irq_save
> for archs that have native nmi?

That is already so, see 1/3.

Re: [patch 3/3] sched: Use local_irq_save_nmi() in cpu_clock()

[Frederic Weisbecker]

On Wed, Apr 07, 2010 at 01:31:34PM +0200, Peter Zijlstra wrote:
> On Wed, 2010-04-07 at 13:27 +0200, Frederic Weisbecker wrote:
> > On Tue, Apr 06, 2010 at 03:28:10PM +0200, Peter Zijlstra wrote:
> > > Since we can call cpu_clock() from NMI context fix up the IRQ
> > > disabling to conform to the new rules.
> > > 
> > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > ---
> > >  kernel/sched_clock.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > Index: linux-2.6/kernel/sched_clock.c
> > > ===================================================================
> > > --- linux-2.6.orig/kernel/sched_clock.c
> > > +++ linux-2.6/kernel/sched_clock.c
> > > @@ -241,9 +241,9 @@ unsigned long long cpu_clock(int cpu)
> > >  	unsigned long long clock;
> > >  	unsigned long flags;
> > >  
> > > -	local_irq_save(flags);
> > > +	local_irq_save_nmi(flags);
> > >  	clock = sched_clock_cpu(cpu);
> > > -	local_irq_restore(flags);
> > > +	local_irq_restore_nmi(flags);
> > >  
> > >  	return clock;
> > >  }
> > 
> > 
> > That seem to add a small overhead in various places.
> > Do we want to make local_irq_save_nmi == local_irq_save
> > for archs that have native nmi?
> 
> That is already so, see 1/3.


Ah you're right.