[PATCH 0/4] ftrace: updates for tip

[PATCH 0/4] ftrace: updates for tip

[Steven Rostedt]

Ingo,

The following patches are in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: tip/devel


Steven Rostedt (4):
      ftrace, x86: rename in_nmi variable
      nmi: add generic nmi tracking state
      ftrace: change function graph tracer to use new in_nmi
      ring-buffer: use generic version of in_nmi

----
 arch/x86/Kconfig           |    2 +-
 arch/x86/kernel/ftrace.c   |   39 ++++++++++-----------------------------
 include/linux/ftrace_irq.h |    8 --------
 include/linux/hardirq.h    |   15 +++++++++++++++
 kernel/trace/ring_buffer.c |   43 +++++++++++++------------------------------
 5 files changed, 39 insertions(+), 68 deletions(-)
-- 

[PATCH 1/4] ftrace, x86: rename in_nmi variable

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-x86-rename-in_nmi-variable.patch --]
[-- Type: text/plain, Size: 2698 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up

The in_nmi variable in x86 arch ftrace.c is a misnomer.
Andrew Morton pointed out that the in_nmi variable is incremented
by all CPUS. It can be set when another CPU is running an NMI.

Since this is actually intentional, the fix is to rename it to
what it really is: "nmi_running"

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/x86/kernel/ftrace.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 4c68358..e3fad2e 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -82,7 +82,7 @@ static unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
  * are the same as what exists.
  */
 
-static atomic_t in_nmi = ATOMIC_INIT(0);
+static atomic_t nmi_running = ATOMIC_INIT(0);
 static int mod_code_status;		/* holds return value of text write */
 static int mod_code_write;		/* set when NMI should do the write */
 static void *mod_code_ip;		/* holds the IP to write to */
@@ -115,8 +115,8 @@ static void ftrace_mod_code(void)
 
 void arch_ftrace_nmi_enter(void)
 {
-	atomic_inc(&in_nmi);
-	/* Must have in_nmi seen before reading write flag */
+	atomic_inc(&nmi_running);
+	/* Must have nmi_running seen before reading write flag */
 	smp_mb();
 	if (mod_code_write) {
 		ftrace_mod_code();
@@ -126,19 +126,19 @@ void arch_ftrace_nmi_enter(void)
 
 void arch_ftrace_nmi_exit(void)
 {
-	/* Finish all executions before clearing in_nmi */
+	/* Finish all executions before clearing nmi_running */
 	smp_wmb();
-	atomic_dec(&in_nmi);
+	atomic_dec(&nmi_running);
 }
 
 static void wait_for_nmi(void)
 {
-	if (!atomic_read(&in_nmi))
+	if (!atomic_read(&nmi_running))
 		return;
 
 	do {
 		cpu_relax();
-	} while(atomic_read(&in_nmi));
+	} while (atomic_read(&nmi_running));
 
 	nmi_wait_count++;
 }
@@ -374,16 +374,16 @@ int ftrace_disable_ftrace_graph_caller(void)
  * this page for dynamic ftrace. They have been
  * simplified to ignore all traces in NMI context.
  */
-static atomic_t in_nmi;
+static atomic_t nmi_running;
 
 void arch_ftrace_nmi_enter(void)
 {
-	atomic_inc(&in_nmi);
+	atomic_inc(&nmi_running);
 }
 
 void arch_ftrace_nmi_exit(void)
 {
-	atomic_dec(&in_nmi);
+	atomic_dec(&nmi_running);
 }
 
 #endif /* !CONFIG_DYNAMIC_FTRACE */
@@ -475,7 +475,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 				&return_to_handler;
 
 	/* Nmi's are currently unsupported */
-	if (unlikely(atomic_read(&in_nmi)))
+	if (unlikely(atomic_read(&nmi_running)))
 		return;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
-- 
1.5.6.5

-- 

[PATCH 2/4] nmi: add generic nmi tracking state

[Steven Rostedt]

[-- Attachment #1: 0002-nmi-add-generic-nmi-tracking-state.patch --]
[-- Type: text/plain, Size: 1843 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

This code adds an in_nmi() macro that uses the current tasks preempt count
to track when it is in NMI context. Other parts of the kernel can
use this to determine if the context is in NMI context or not.

This code was inspired by the -rt patch in_nmi version that was
written by Peter Zijlstra.

Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 include/linux/hardirq.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index f832883..f3cf86e 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -61,6 +61,12 @@
 #error PREEMPT_ACTIVE is too low!
 #endif
 
+#define NMI_OFFSET	(PREEMPT_ACTIVE << 1)
+
+#if NMI_OFFSET >= 0x80000000
+#error PREEMPT_ACTIVE too high!
+#endif
+
 #define hardirq_count()	(preempt_count() & HARDIRQ_MASK)
 #define softirq_count()	(preempt_count() & SOFTIRQ_MASK)
 #define irq_count()	(preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK))
@@ -73,6 +79,11 @@
 #define in_softirq()		(softirq_count())
 #define in_interrupt()		(irq_count())
 
+/*
+ * Are we in NMI context?
+ */
+#define in_nmi()	(preempt_count() & NMI_OFFSET)
+
 #if defined(CONFIG_PREEMPT)
 # define PREEMPT_INATOMIC_BASE kernel_locked()
 # define PREEMPT_CHECK_OFFSET 1
@@ -167,6 +178,8 @@ extern void irq_exit(void);
 #define nmi_enter()				\
 	do {					\
 		ftrace_nmi_enter();		\
+		BUG_ON(in_nmi());		\
+		add_preempt_count(NMI_OFFSET);	\
 		lockdep_off();			\
 		rcu_nmi_enter();		\
 		__irq_enter();			\
@@ -177,6 +190,8 @@ extern void irq_exit(void);
 		__irq_exit();			\
 		rcu_nmi_exit();			\
 		lockdep_on();			\
+		BUG_ON(!in_nmi());		\
+		sub_preempt_count(NMI_OFFSET);	\
 		ftrace_nmi_exit();		\
 	} while (0)
 
-- 
1.5.6.5

-- 

[PATCH 3/4] ftrace: change function graph tracer to use new in_nmi

[Steven Rostedt]

[-- Attachment #1: 0003-ftrace-change-function-graph-tracer-to-use-new-in_n.patch --]
[-- Type: text/plain, Size: 2321 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The function graph tracer piggy backed onto the dynamic ftracer
to use the in_nmi custom code for dynamic tracing. The problem
was (as Andrew Morton pointed out) it really only wanted to bail
out if the context of the current CPU was in NMI context. But the
dynamic ftrace in_nmi custom code was true if _any_ CPU happened
to be in NMI context.

Now that we have a generic in_nmi interface, this patch changes
the function graph code to use it instead of the dynamic ftarce
custom code.

Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/x86/Kconfig         |    2 +-
 arch/x86/kernel/ftrace.c |   21 +--------------------
 2 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3da7121..0f47315 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -34,7 +34,7 @@ config X86
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_TRACE_MCOUNT_TEST
-	select HAVE_FTRACE_NMI_ENTER if DYNAMIC_FTRACE || FUNCTION_GRAPH_TRACER
+	select HAVE_FTRACE_NMI_ENTER if DYNAMIC_FTRACE
 	select HAVE_KVM
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_TRACEHOOK
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index e3fad2e..918073c 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -367,25 +367,6 @@ int ftrace_disable_ftrace_graph_caller(void)
 	return ftrace_mod_jmp(ip, old_offset, new_offset);
 }
 
-#else /* CONFIG_DYNAMIC_FTRACE */
-
-/*
- * These functions are picked from those used on
- * this page for dynamic ftrace. They have been
- * simplified to ignore all traces in NMI context.
- */
-static atomic_t nmi_running;
-
-void arch_ftrace_nmi_enter(void)
-{
-	atomic_inc(&nmi_running);
-}
-
-void arch_ftrace_nmi_exit(void)
-{
-	atomic_dec(&nmi_running);
-}
-
 #endif /* !CONFIG_DYNAMIC_FTRACE */
 
 /* Add a function return address to the trace stack on thread info.*/
@@ -475,7 +456,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 				&return_to_handler;
 
 	/* Nmi's are currently unsupported */
-	if (unlikely(atomic_read(&nmi_running)))
+	if (unlikely(in_nmi()))
 		return;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
-- 
1.5.6.5

-- 

[PATCH 4/4] ring-buffer: use generic version of in_nmi

[Steven Rostedt]

[-- Attachment #1: 0004-ring-buffer-use-generic-version-of-in_nmi.patch --]
[-- Type: text/plain, Size: 4035 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up

Now that a generic in_nmi is available, this patch removes the
special code in the ring_buffer and implements the in_nmi generic
version instead.

With this change, I was also able to rename the "arch_ftrace_nmi_enter"
back to "ftrace_nmi_enter" and remove the code from the ring buffer.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/x86/kernel/ftrace.c   |    4 ++--
 include/linux/ftrace_irq.h |    8 --------
 kernel/trace/ring_buffer.c |   43 +++++++++++++------------------------------
 3 files changed, 15 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 918073c..d74d75e 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -113,7 +113,7 @@ static void ftrace_mod_code(void)
 					     MCOUNT_INSN_SIZE);
 }
 
-void arch_ftrace_nmi_enter(void)
+void ftrace_nmi_enter(void)
 {
 	atomic_inc(&nmi_running);
 	/* Must have nmi_running seen before reading write flag */
@@ -124,7 +124,7 @@ void arch_ftrace_nmi_enter(void)
 	}
 }
 
-void arch_ftrace_nmi_exit(void)
+void ftrace_nmi_exit(void)
 {
 	/* Finish all executions before clearing nmi_running */
 	smp_wmb();
diff --git a/include/linux/ftrace_irq.h b/include/linux/ftrace_irq.h
index 29de677..dca7bf8 100644
--- a/include/linux/ftrace_irq.h
+++ b/include/linux/ftrace_irq.h
@@ -3,14 +3,6 @@
 
 
 #ifdef CONFIG_FTRACE_NMI_ENTER
-extern void arch_ftrace_nmi_enter(void);
-extern void arch_ftrace_nmi_exit(void);
-#else
-static inline void arch_ftrace_nmi_enter(void) { }
-static inline void arch_ftrace_nmi_exit(void) { }
-#endif
-
-#ifdef CONFIG_RING_BUFFER
 extern void ftrace_nmi_enter(void);
 extern void ftrace_nmi_exit(void);
 #else
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index a60a6a8..5ee3444 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -8,6 +8,7 @@
 #include <linux/spinlock.h>
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
+#include <linux/hardirq.h>
 #include <linux/module.h>
 #include <linux/percpu.h>
 #include <linux/mutex.h>
@@ -20,35 +21,6 @@
 #include "trace.h"
 
 /*
- * Since the write to the buffer is still not fully lockless,
- * we must be careful with NMIs. The locks in the writers
- * are taken when a write crosses to a new page. The locks
- * protect against races with the readers (this will soon
- * be fixed with a lockless solution).
- *
- * Because we can not protect against NMIs, and we want to
- * keep traces reentrant, we need to manage what happens
- * when we are in an NMI.
- */
-static DEFINE_PER_CPU(int, rb_in_nmi);
-
-void ftrace_nmi_enter(void)
-{
-	__get_cpu_var(rb_in_nmi)++;
-	/* call arch specific handler too */
-	arch_ftrace_nmi_enter();
-}
-
-void ftrace_nmi_exit(void)
-{
-	arch_ftrace_nmi_exit();
-	__get_cpu_var(rb_in_nmi)--;
-	/* NMIs are not recursive */
-	WARN_ON_ONCE(__get_cpu_var(rb_in_nmi));
-}
-
-
-/*
  * A fast way to enable or disable all ring buffers is to
  * call tracing_on or tracing_off. Turning off the ring buffers
  * prevents all ring buffers from being recorded to.
@@ -1027,12 +999,23 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 
 		local_irq_save(flags);
 		/*
+		 * Since the write to the buffer is still not
+		 * fully lockless, we must be careful with NMIs.
+		 * The locks in the writers are taken when a write
+		 * crosses to a new page. The locks protect against
+		 * races with the readers (this will soon be fixed
+		 * with a lockless solution).
+		 *
+		 * Because we can not protect against NMIs, and we
+		 * want to keep traces reentrant, we need to manage
+		 * what happens when we are in an NMI.
+		 *
 		 * NMIs can happen after we take the lock.
 		 * If we are in an NMI, only take the lock
 		 * if it is not already taken. Otherwise
 		 * simply fail.
 		 */
-		if (unlikely(__get_cpu_var(rb_in_nmi))) {
+		if (unlikely(in_nmi())) {
 			if (!__raw_spin_trylock(&cpu_buffer->lock))
 				goto out_unlock;
 		} else
-- 
1.5.6.5

-- 

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Andrew Morton]

On Fri, 06 Feb 2009 01:53:54 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> This code adds an in_nmi() macro that uses the current tasks preempt count
> to track when it is in NMI context. Other parts of the kernel can
> use this to determine if the context is in NMI context or not.
> 
> This code was inspired by the -rt patch in_nmi version that was
> written by Peter Zijlstra.
> 
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  include/linux/hardirq.h |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index f832883..f3cf86e 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -61,6 +61,12 @@
>  #error PREEMPT_ACTIVE is too low!
>  #endif
>  
> +#define NMI_OFFSET	(PREEMPT_ACTIVE << 1)
> +
> +#if NMI_OFFSET >= 0x80000000
> +#error PREEMPT_ACTIVE too high!
> +#endif
> +
>  #define hardirq_count()	(preempt_count() & HARDIRQ_MASK)
>  #define softirq_count()	(preempt_count() & SOFTIRQ_MASK)
>  #define irq_count()	(preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK))
> @@ -73,6 +79,11 @@
>  #define in_softirq()		(softirq_count())
>  #define in_interrupt()		(irq_count())
>  
> +/*
> + * Are we in NMI context?
> + */
> +#define in_nmi()	(preempt_count() & NMI_OFFSET)
> +
>  #if defined(CONFIG_PREEMPT)
>  # define PREEMPT_INATOMIC_BASE kernel_locked()
>  # define PREEMPT_CHECK_OFFSET 1
> @@ -167,6 +178,8 @@ extern void irq_exit(void);
>  #define nmi_enter()				\
>  	do {					\
>  		ftrace_nmi_enter();		\
> +		BUG_ON(in_nmi());		\
> +		add_preempt_count(NMI_OFFSET);	\
>  		lockdep_off();			\
>  		rcu_nmi_enter();		\
>  		__irq_enter();			\
> @@ -177,6 +190,8 @@ extern void irq_exit(void);
>  		__irq_exit();			\
>  		rcu_nmi_exit();			\
>  		lockdep_on();			\
> +		BUG_ON(!in_nmi());		\
> +		sub_preempt_count(NMI_OFFSET);	\
>  		ftrace_nmi_exit();		\
>  	} while (0)
>  

Well that was tidy.

We're sure that no present or future architecture will for some weird
reason nest NMIs?

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Peter Zijlstra]

On Fri, 2009-02-06 at 01:53 -0500, Steven Rostedt wrote:

> This code adds an in_nmi() macro that uses the current tasks preempt count
> to track when it is in NMI context. Other parts of the kernel can
> use this to determine if the context is in NMI context or not.
> 
> This code was inspired by the -rt patch in_nmi version that was
> written by Peter Zijlstra.

Which in turn I borrowed from Mathieu.

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Steven Rostedt]

On Thu, 5 Feb 2009, Andrew Morton wrote:
> >  
> > +/*
> > + * Are we in NMI context?
> > + */
> > +#define in_nmi()	(preempt_count() & NMI_OFFSET)
> > +
> >  #if defined(CONFIG_PREEMPT)
> >  # define PREEMPT_INATOMIC_BASE kernel_locked()
> >  # define PREEMPT_CHECK_OFFSET 1
> > @@ -167,6 +178,8 @@ extern void irq_exit(void);
> >  #define nmi_enter()				\
> >  	do {					\
> >  		ftrace_nmi_enter();		\
> > +		BUG_ON(in_nmi());		\
> > +		add_preempt_count(NMI_OFFSET);	\
> >  		lockdep_off();			\
> >  		rcu_nmi_enter();		\
> >  		__irq_enter();			\
> > @@ -177,6 +190,8 @@ extern void irq_exit(void);
> >  		__irq_exit();			\
> >  		rcu_nmi_exit();			\
> >  		lockdep_on();			\
> > +		BUG_ON(!in_nmi());		\
> > +		sub_preempt_count(NMI_OFFSET);	\
> >  		ftrace_nmi_exit();		\
> >  	} while (0)
> >  
> 
> Well that was tidy.
> 
> We're sure that no present or future architecture will for some weird
> reason nest NMIs?

That would be fun to implement. Not the in_nmi code, but the handling of 
nested NMIs. How would you be able to save the state when the NMI occurred 
without being preempted by another NMI?

I think the arch that has nested NMIs will have many more issues to solve 
in the kernel than this one.

-- Steve

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, 2009-02-06 at 01:53 -0500, Steven Rostedt wrote:
> 
> > This code adds an in_nmi() macro that uses the current tasks preempt count
> > to track when it is in NMI context. Other parts of the kernel can
> > use this to determine if the context is in NMI context or not.
> > 
> > This code was inspired by the -rt patch in_nmi version that was
> > written by Peter Zijlstra.
> 
> Which in turn I borrowed from Mathieu.

Steve, could you please fix the attribution?

	Ingo

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Steven Rostedt]

On Fri, 6 Feb 2009, Ingo Molnar wrote:

> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Fri, 2009-02-06 at 01:53 -0500, Steven Rostedt wrote:
> > 
> > > This code adds an in_nmi() macro that uses the current tasks preempt count
> > > to track when it is in NMI context. Other parts of the kernel can
> > > use this to determine if the context is in NMI context or not.
> > > 
> > > This code was inspired by the -rt patch in_nmi version that was
> > > written by Peter Zijlstra.
> > 
> > Which in turn I borrowed from Mathieu.
> 
> Steve, could you please fix the attribution?

Is it OK to rebase the branch to do so?

-- Steve

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 6 Feb 2009, Ingo Molnar wrote:
> 
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Fri, 2009-02-06 at 01:53 -0500, Steven Rostedt wrote:
> > > 
> > > > This code adds an in_nmi() macro that uses the current tasks preempt count
> > > > to track when it is in NMI context. Other parts of the kernel can
> > > > use this to determine if the context is in NMI context or not.
> > > > 
> > > > This code was inspired by the -rt patch in_nmi version that was
> > > > written by Peter Zijlstra.
> > > 
> > > Which in turn I borrowed from Mathieu.
> > 
> > Steve, could you please fix the attribution?
> 
> Is it OK to rebase the branch to do so?

Sure, that's necessary.

And note that unless you base your tree against tip:tracing/ftrace i cannot 
do a straight pull anyway. (your trees are usually based against tip:master 
- which brings in all other branches)

	Ingo

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Steven Rostedt]

On Fri, 6 Feb 2009, Ingo Molnar wrote:
> > Is it OK to rebase the branch to do so?
> 
> Sure, that's necessary.

OK, will do.

> 
> And note that unless you base your tree against tip:tracing/ftrace i cannot 
> do a straight pull anyway. (your trees are usually based against tip:master 
> - which brings in all other branches)

Which is better for you? Should I base it off of tracing/ftrace instead? I 
just been using master to make sure I did not break anything else in your 
repo ;-)

I can set up a branch called tip/ftrace/devel that will be based off of 
tracing/ftrace, and keep tip/devel off of tip/master. This will let
you know which repo my post is based off of. I'll also add it to the 
subject.

-- Steve

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Fri, 6 Feb 2009, Ingo Molnar wrote:
> > > Is it OK to rebase the branch to do so?
> > 
> > Sure, that's necessary.
> 
> OK, will do.
> 
> > 
> > And note that unless you base your tree against tip:tracing/ftrace i cannot 
> > do a straight pull anyway. (your trees are usually based against tip:master 
> > - which brings in all other branches)
> 
> Which is better for you? Should I base it off of tracing/ftrace instead? I 
> just been using master to make sure I did not break anything else in your 
> repo ;-)

I'll resolve conflicts, no need to worry about that - so it would be nice if 
you could base it on tracing/ftrace (or tracing/core). You can test-merge 
your branch against tip/master and see whether there's any conflict.

> I can set up a branch called tip/ftrace/devel that will be based off of 
> tracing/ftrace, and keep tip/devel off of tip/master. This will let you 
> know which repo my post is based off of. I'll also add it to the subject.

ok, that will work - thanks!

	Ingo

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Frederic Weisbecker]

On Fri, Feb 06, 2009 at 03:54:31PM +0100, Ingo Molnar wrote:
> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 6 Feb 2009, Ingo Molnar wrote:
> > 
> > > 
> > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > On Fri, 2009-02-06 at 01:53 -0500, Steven Rostedt wrote:
> > > > 
> > > > > This code adds an in_nmi() macro that uses the current tasks preempt count
> > > > > to track when it is in NMI context. Other parts of the kernel can
> > > > > use this to determine if the context is in NMI context or not.
> > > > > 
> > > > > This code was inspired by the -rt patch in_nmi version that was
> > > > > written by Peter Zijlstra.
> > > > 
> > > > Which in turn I borrowed from Mathieu.
> > > 
> > > Steve, could you please fix the attribution?
> > 
> > Is it OK to rebase the branch to do so?
> 
> Sure, that's necessary.
> 
> And note that unless you base your tree against tip:tracing/ftrace i cannot 
> do a straight pull anyway. (your trees are usually based against tip:master 
> - which brings in all other branches)


Oh really? I always base my tracing patches against tip/master, assuming
tracing/ftrace is about always quickly merged into master.
But the opposite is not necessarily true, I guess you don't merge master
into tracing/ftrace so quickly to not break the history right? And I guess
it's better to catch bugs if each individual topics is not too quickly synced
against tip/master.

 
> 	Ingo

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Feb 06, 2009 at 03:54:31PM +0100, Ingo Molnar wrote:
> > 
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Fri, 6 Feb 2009, Ingo Molnar wrote:
> > > 
> > > > 
> > > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > > 
> > > > > On Fri, 2009-02-06 at 01:53 -0500, Steven Rostedt wrote:
> > > > > 
> > > > > > This code adds an in_nmi() macro that uses the current tasks preempt count
> > > > > > to track when it is in NMI context. Other parts of the kernel can
> > > > > > use this to determine if the context is in NMI context or not.
> > > > > > 
> > > > > > This code was inspired by the -rt patch in_nmi version that was
> > > > > > written by Peter Zijlstra.
> > > > > 
> > > > > Which in turn I borrowed from Mathieu.
> > > > 
> > > > Steve, could you please fix the attribution?
> > > 
> > > Is it OK to rebase the branch to do so?
> > 
> > Sure, that's necessary.
> > 
> > And note that unless you base your tree against tip:tracing/ftrace i cannot 
> > do a straight pull anyway. (your trees are usually based against tip:master 
> > - which brings in all other branches)
> 
> 
> Oh really? I always base my tracing patches against tip/master, assuming 
> tracing/ftrace is about always quickly merged into master. But the 
> opposite is not necessarily true, I guess you don't merge master into 
> tracing/ftrace so quickly to not break the history right? And I guess it's 
> better to catch bugs if each individual topics is not too quickly synced 
> against tip/master.

email submissions are perfectly OK against tip:master - please keep doing it 
that way. It is Git pull requests (which Steve is sending) that should be 
against pure topics.

	Ingo

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Andrew Morton]

On Fri, 6 Feb 2009 08:25:52 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Thu, 5 Feb 2009, Andrew Morton wrote:
> > >  
> > > +/*
> > > + * Are we in NMI context?
> > > + */
> > > +#define in_nmi()	(preempt_count() & NMI_OFFSET)
> > > +
> > >  #if defined(CONFIG_PREEMPT)
> > >  # define PREEMPT_INATOMIC_BASE kernel_locked()
> > >  # define PREEMPT_CHECK_OFFSET 1
> > > @@ -167,6 +178,8 @@ extern void irq_exit(void);
> > >  #define nmi_enter()				\
> > >  	do {					\
> > >  		ftrace_nmi_enter();		\
> > > +		BUG_ON(in_nmi());		\
> > > +		add_preempt_count(NMI_OFFSET);	\
> > >  		lockdep_off();			\
> > >  		rcu_nmi_enter();		\
> > >  		__irq_enter();			\
> > > @@ -177,6 +190,8 @@ extern void irq_exit(void);
> > >  		__irq_exit();			\
> > >  		rcu_nmi_exit();			\
> > >  		lockdep_on();			\
> > > +		BUG_ON(!in_nmi());		\
> > > +		sub_preempt_count(NMI_OFFSET);	\
> > >  		ftrace_nmi_exit();		\
> > >  	} while (0)
> > >  
> > 
> > Well that was tidy.
> > 
> > We're sure that no present or future architecture will for some weird
> > reason nest NMIs?
> 
> That would be fun to implement. Not the in_nmi code, but the handling of 
> nested NMIs. How would you be able to save the state when the NMI occurred 
> without being preempted by another NMI?

Like with normal interrupts?

As long as the number of sources is finite, nested NMIs could work OK.

> I think the arch that has nested NMIs will have many more issues to solve 
> in the kernel than this one.

I have a vague memory that x86 can do this.

<googles a bit>

What's all this about?
https://www.x86-64.org/pipermail/discuss/2005-October/007010.html
http://kerneltrap.org/index.php?q=mailarchive/linux-kernel/2008/2/12/830704/thread

I expect that even if it is possible, we can live without it.

And if I'm wrong, it'll be easy to accommodate by adding a new counter
into the task_struct or thread_struct.

Does your above implementation make in_interrupt() return true if
in_nmi()?  I think it doesn't, but should?

Re: [PATCH 2/4] nmi: add generic nmi tracking state

[Steven Rostedt]

On Fri, 6 Feb 2009, Andrew Morton wrote:
> > 
> > That would be fun to implement. Not the in_nmi code, but the handling of 
> > nested NMIs. How would you be able to save the state when the NMI occurred 
> > without being preempted by another NMI?
> 
> Like with normal interrupts?

Normal interrupts can enable interrupts again, while in the handler, as 
well as disable them.

> 
> As long as the number of sources is finite, nested NMIs could work OK.

I guess you would need a mechanism to enable and disable NMIs.

> 
> > I think the arch that has nested NMIs will have many more issues to solve 
> > in the kernel than this one.
> 
> I have a vague memory that x86 can do this.
> 
> <googles a bit>
> 
> What's all this about?
> https://www.x86-64.org/pipermail/discuss/2005-October/007010.html

Yuck, masking Non Maskable Interrupts?

> http://kerneltrap.org/index.php?q=mailarchive/linux-kernel/2008/2/12/830704/thread

It looks like it calls nmi_exit, so the code would dec it.

> 
> I expect that even if it is possible, we can live without it.
> 
> And if I'm wrong, it'll be easy to accommodate by adding a new counter
> into the task_struct or thread_struct.

Yeah, the bug on would trigger as soon as we do that, and we could
easily update the code when that time comes.

> 
> Does your above implementation make in_interrupt() return true if
> in_nmi()?  I think it doesn't, but should?

The "in_nmi()" is set when we do nmi_enter, and nmi_enter also calls 
irq_enter which makes in_interrupt() true. I thought adding the in_nmi 
condition to in_interrupt would be redundant.

-- Steve