ftrace: handle NMIs safely

Modifying code is something that needs special care. On SMP boxes, if code that is being modified is also being executed on another CPU, that CPU will have undefined results.

The dynamic ftrace uses kstop_machine to make the system act like a uniprocessor system. But this does not address NMIs, that can still run on other CPUs.

One approach to handle this is to make all code that are used by NMIs not be traced. But NMIs can call notifiers that spread throughout the kernel and this will be very hard to maintain, and the chance of missing a function is very high.

The approach that this patch takes is to have the NMIs modify the code if the modification is taking place. The way this works is that just writing to code executing on another CPU is not harmful if what is written is the same as what exists.

arch/sparc/include/asm/ftrace.h +2 -0

+#endif
+
 #ifdef CONFIG_MCOUNT
 #define MCOUNT_ADDR		((long)(_mcount))
 #define MCOUNT_INSN_SIZE	4 /* sizeof mcount call */

Index: linux-tip.git/arch/x86/include/asm/ftrace.h
===================================================================

arch/x86/include/asm/ftrace.h +0 -0

 #endif /* CONFIG_FUNCTION_TRACER */

Index: linux-tip.git/arch/x86/kernel/ftrace.c
===================================================================

arch/x86/kernel/ftrace.c +2 -0

@@ -56,6 +56,111 @@ unsigned char *ftrace_call_replace(unsig
 	return calc.code;
 }
 
+/*
+ * Modifying code must take extra care. On an SMP machine, if

arch/x86/kernel/ftrace.c +8 -0

+ *    and the new code into the "code" buffer.
+ * 2) Set a flag that says we are modifying code
+ * 3) Wait for any running NMIs to finish.
+ * 4) Write the code
+ * 5) clear the flag.
+ * 6) Wait for any running NMIs to finish.
+ *
+ * If an NMI is executed, the first thing it does is to call

arch/x86/kernel/ftrace.c +58 -0

+static void ftrace_mod_code(void)
+{
+	/*
+	 * Yes, more than one CPU process can be writing to mod_code_status.
+	 *    (and the code itself)
+	 * But if one were to fail, then they all should, and if one were
+	 * to succeed, then they all should.
+	 */
+	mod_code_status = probe_kernel_write(mod_code_ip, mod_code_newcode,
+					     MCOUNT_INSN_SIZE);
+
+}
+
+void ftrace_nmi_enter(void)
+{
+	atomic_inc(&in_nmi);
+	/* Must have in_nmi seen before reading write flag */
+	smp_mb();
+	if (mod_code_write)
+		ftrace_mod_code();
+}
+
+void ftrace_nmi_exit(void)
+{
+	/* Finish all executions before clearing in_nmi */
+	smp_wmb();
+	atomic_dec(&in_nmi);
+}
+
+static void wait_for_nmi(void)
+{
+	while (atomic_read(&in_nmi))
+		cpu_relax();
+}
+
+static int
+do_ftrace_mod_code(unsigned long ip, void *new_code)
+{
+	mod_code_ip = (void *)ip;
+	mod_code_newcode = new_code;
+
+	/* The buffers need to be visible before we let NMIs write them */
+	smp_wmb();
+
+	mod_code_write = 1;
+
+	/* Make sure write bit is visible before we wait on NMIs */
+	smp_mb();
+
+	wait_for_nmi();
+
+	/* Make sure all running NMIs have finished before we write the code */
+	smp_mb();
+
+	ftrace_mod_code();
+
+	/* Make sure the write happens before clearing the bit */
+	smp_wmb();

include/linux/hardirq.h +4 -2

@@ -5,6 +5,7 @@
 #include <linux/smp_lock.h>
 #include <linux/lockdep.h>
 #include <asm/hardirq.h>
+#include <asm/ftrace.h>
 #include <asm/system.h>
 
 /*

@@ -161,7 +162,17 @@ extern void irq_enter(void);
  */
 extern void irq_exit(void);
 
-#define nmi_enter()		do { lockdep_off(); __irq_enter(); } while (0)
-#define nmi_exit()		do { __irq_exit(); lockdep_on(); } while (0)
+#define nmi_enter()				\
+	do {					\
+		ftrace_nmi_enter();		\

arch/x86/kernel/ftrace.c unsigned char *ftrace_call_replace(unsig · line 161

Quoted 7 lines

arch/x86/kernel/ftrace.c +7 -0

+	/* make sure NMIs see the cleared bit */
+	smp_mb();
+
+	wait_for_nmi();
+
+	return mod_code_status;
+}

I guess the weakness here is that the code will only allow a single contiguous hunk of text to be modified. One could envisage situations where two or more separate areas of memory need to be modified atomically/together.

I guess we can cross that bridge when we fall off it.

arch/x86/include/asm/ftrace.h static inline unsigned long ftrace_call_

Quoted 10 lines

arch/x86/include/asm/ftrace.h +8 -0

+#ifndef __ASSEMBLY__
+#define ftrace_nmi_enter()	do { } while (0)
+#define ftrace_nmi_exit()	do { } while (0)
+#endif
...
+#ifndef __ASSEMBLY__
+#define ftrace_nmi_enter()	do { } while (0)
+#define ftrace_nmi_exit()	do { } while (0)
+#endif

These could all be written in C. If there's a reson to write them in cpp then the `#ifndef __ASSEMBLY__' isn't really needed.

Quoted 11 lines

arch/sparc/include/asm/ftrace.h +2 -0

+#endif
+
 #ifdef CONFIG_MCOUNT
 #define MCOUNT_ADDR		((long)(_mcount))
 #define MCOUNT_INSN_SIZE	4 /* sizeof mcount call */

Index: linux-tip.git/arch/x86/include/asm/ftrace.h
===================================================================

arch/x86/include/asm/ftrace.h static inline unsigned long ftrace_call_ · line 26

Quoted 10 lines

arch/x86/include/asm/ftrace.h +7 -0

@@ -17,6 +17,21 @@ static inline unsigned long ftrace_call_
 	 */
 	return addr - 1;
 }
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+extern void ftrace_nmi_enter(void);
+extern void ftrace_nmi_exit(void);
+#else
+#define ftrace_nmi_enter()	do { } while (0)
+#define ftrace_nmi_exit()	do { } while (0)

Either this one misses the `#ifndef __ASSEMBLY__' ...

arch/x86/include/asm/ftrace.h static inline unsigned long ftrace_call_ · line 35

Quoted 9 lines

arch/x86/include/asm/ftrace.h +8 -0

+#endif
+#endif
+
+#else /* CONFIG_FUNCTION_TRACER */
+
+#ifndef __ASSEMBLY__
+#define ftrace_nmi_enter()	do { } while (0)
+#define ftrace_nmi_exit()	do { } while (0)
 #endif

or this one didn't need it.

Quoted 11 lines

arch/x86/include/asm/ftrace.h +0 -0

 #endif /* CONFIG_FUNCTION_TRACER */

Index: linux-tip.git/arch/x86/kernel/ftrace.c
===================================================================

arch/x86/kernel/ftrace.c +0 -0

@@ -56,6 +56,111 @@ unsigned char *ftrace_call_replace(unsig
 	return calc.code;
 }
 

arch/x86/kernel/ftrace.c unsigned char *ftrace_call_replace(unsig · line 70

Quoted 12 lines

arch/x86/kernel/ftrace.c +12 -0

+/*
+ * Modifying code must take extra care. On an SMP machine, if
+ * the code being modified is also being executed on another CPU
+ * that CPU will have undefined results and possibly take a GPF.
+ * We use kstop_machine to stop other CPUS from exectuing code.
+ * But this does not stop NMIs from happening. We still need
+ * to protect against that. We separate out the modification of
+ * the code to take care of this.
+ *
+ * Two buffers are added: An IP buffer and a "code" buffer.
+ *
+ * 1) Put in the instruction pointer into the IP buffer

s/in //

Quoted 14 lines

arch/x86/kernel/ftrace.c +13 -0

+ *    and the new code into the "code" buffer.
+ * 2) Set a flag that says we are modifying code
+ * 3) Wait for any running NMIs to finish.
+ * 4) Write the code
+ * 5) clear the flag.
+ * 6) Wait for any running NMIs to finish.
+ *
+ * If an NMI is executed, the first thing it does is to call
+ * "ftrace_nmi_enter". This will check if the flag is set to write
+ * and if it is, it will write what is in the IP and "code" buffers.
+ *
+ * The trick is, it does not matter if everyone is writing the same
+ * content to the code location. Also, if a CPU is executing code

arch/x86/kernel/ftrace.c unsigned char *ftrace_call_replace(unsig · line 88

Quoted 5 lines

arch/x86/kernel/ftrace.c +5 -0

+ * it is OK to write to that code location if the contents being written
+ * are the same as what exists.
+ */
+
+static atomic_t in_nmi;

Should formally have an ATONIC_INIT. If we're going to formalise the "all zeroes is OK for atomic_t's" rule then we can leave this as-is.

arch/x86/kernel/ftrace.c unsigned char *ftrace_call_replace(unsig · line 92

Quoted 5 lines

arch/x86/kernel/ftrace.c +4 -0

+static int mod_code_status;
+static int mod_code_write;
+static void *mod_code_ip;
+static void *mod_code_newcode;

Some comments decribing the global state would be nice.

Quoted 59 lines

arch/x86/kernel/ftrace.c +58 -0

+static void ftrace_mod_code(void)
+{
+	/*
+	 * Yes, more than one CPU process can be writing to mod_code_status.
+	 *    (and the code itself)
+	 * But if one were to fail, then they all should, and if one were
+	 * to succeed, then they all should.
+	 */
+	mod_code_status = probe_kernel_write(mod_code_ip, mod_code_newcode,
+					     MCOUNT_INSN_SIZE);
+
+}
+
+void ftrace_nmi_enter(void)
+{
+	atomic_inc(&in_nmi);
+	/* Must have in_nmi seen before reading write flag */
+	smp_mb();
+	if (mod_code_write)
+		ftrace_mod_code();
+}
+
+void ftrace_nmi_exit(void)
+{
+	/* Finish all executions before clearing in_nmi */
+	smp_wmb();
+	atomic_dec(&in_nmi);
+}
+
+static void wait_for_nmi(void)
+{
+	while (atomic_read(&in_nmi))
+		cpu_relax();
+}
+
+static int
+do_ftrace_mod_code(unsigned long ip, void *new_code)
+{
+	mod_code_ip = (void *)ip;
+	mod_code_newcode = new_code;
+
+	/* The buffers need to be visible before we let NMIs write them */
+	smp_wmb();
+
+	mod_code_write = 1;
+
+	/* Make sure write bit is visible before we wait on NMIs */
+	smp_mb();
+
+	wait_for_nmi();
+
+	/* Make sure all running NMIs have finished before we write the code */
+	smp_mb();
+
+	ftrace_mod_code();
+
+	/* Make sure the write happens before clearing the bit */
+	smp_wmb();

arch/x86/kernel/ftrace.c unsigned char *ftrace_call_replace(unsig · line 161

Quoted 10 lines

arch/x86/kernel/ftrace.c +10 -0

+
+	mod_code_write = 0;
+
+	/* make sure NMIs see the cleared bit */
+	smp_mb();
+
+	wait_for_nmi();
+
+	return mod_code_status;
+}

I guess the weakness here is that the code will only allow a single contiguous hunk of text to be modified. One could envisage situations where two or more separate areas of memory need to be modified atomically/together.

I guess we can cross that bridge when we fall off it.

Quoted 24 lines

include/linux/hardirq.h +8 -2

@@ -5,6 +5,7 @@
 #include <linux/smp_lock.h>
 #include <linux/lockdep.h>
 #include <asm/hardirq.h>
+#include <asm/ftrace.h>
 #include <asm/system.h>
 
 /*

@@ -161,7 +162,17 @@ extern void irq_enter(void);
  */
 extern void irq_exit(void);
 
-#define nmi_enter()		do { lockdep_off(); __irq_enter(); } while (0)
-#define nmi_exit()		do { __irq_exit(); lockdep_on(); } while (0)
+#define nmi_enter()				\
+	do {					\
+		ftrace_nmi_enter();		\
+		lockdep_off();			\
+		__irq_enter();			\
+	} while (0)
+#define nmi_exit()				\

include/linux/hardirq.h extern void irq_enter(void); · line 177

Quoted 6 lines

include/linux/hardirq.h +5 -0

+	do {					\
+		__irq_exit();			\
+		lockdep_on();			\
+		ftrace_nmi_exit();		\
+	} while (0)
 

<wonders if these needed to be written in cpp>

Quoted 13 lines · Andrew Morton

arch/x86/include/asm/ftrace.h static inline unsigned long ftrace_call_

Quoted 10 lines

arch/x86/include/asm/ftrace.h +8 -0

+#ifndef __ASSEMBLY__
+#define ftrace_nmi_enter()	do { } while (0)
+#define ftrace_nmi_exit()	do { } while (0)
+#endif
...
+#ifndef __ASSEMBLY__
+#define ftrace_nmi_enter()	do { } while (0)
+#define ftrace_nmi_exit()	do { } while (0)
+#endif

These could all be written in C. If there's a reson to write them in cpp then the `#ifndef __ASSEMBLY__' isn't really needed.

I could do the C macro, and you are right, I did not need the __ASSEMBLY__ part. I guess that was me just being over-protective :-/

Which would you prefer? Changing to C or removing the __ASSEMBLY__?

From a general perspective, C is better. Has typechecking, adds a ref

This patch converts the CPP macros of __irq_enter, __irq_exit, nmi_enter, and nmi_exit into static inlines.

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/hardirq.h |   53 ++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

Not all archs implement ftrace, and therefore do not have an asm/ftrace.h. This patch corrects the problem.

The ftrace_nmi_enter/exit now must be defined for all archs that implement dynamic ftrace. Currently, only x86 does.

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/arm/include/asm/ftrace.h     |    5 -----
 arch/powerpc/include/asm/ftrace.h |    5 -----
 arch/sh/include/asm/ftrace.h      |    5 -----
 arch/sparc/include/asm/ftrace.h   |    5 -----
 arch/x86/include/asm/ftrace.h     |   16 ----------------
 include/linux/ftrace.h            |    5 ++++-
 include/linux/hardirq.h           |    2 +-
 7 files changed, 5 insertions(+), 38 deletions(-)

This patch cleans up the NMI safe code for dynamic ftrace as suggested by Andrew Morton.

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/arm/include/asm/ftrace.h     |    4 ++--
 arch/powerpc/include/asm/ftrace.h |    4 ++--
 arch/sh/include/asm/ftrace.h      |    4 ++--
 arch/sparc/include/asm/ftrace.h   |    4 ++--
 arch/x86/include/asm/ftrace.h     |   10 +++++-----
 arch/x86/kernel/ftrace.c          |   16 ++++++++--------
 include/linux/ftrace.h            |    3 +++
 kernel/trace/trace.c              |    9 ++++-----
 8 files changed, 28 insertions(+), 26 deletions(-)

This patch adds dynamic ftrace NMI update statistics to the /debugfs/tracing/dyn_ftrace_total_info stat file.

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/kernel/ftrace.c |   26 ++++++++++++++++++++++++--
 kernel/trace/trace.c     |   31 ++++++++++++++++++++++++-------

arch/x86/kernel/ftrace.c +11 -2

 static void ftrace_mod_code(void)
 {
 	/*

@@ -109,8 +122,10 @@ void ftrace_nmi_enter(void)
 	atomic_inc(&in_nmi);
 	/* Must have in_nmi seen before reading write flag */
 	smp_mb();
-	if (mod_code_write)
+	if (mod_code_write) {
 		ftrace_mod_code();
+		atomic_inc(&nmi_update_count);
+	}
 }
 
 void ftrace_nmi_exit(void)

@@ -122,8 +137,15 @@ void ftrace_nmi_exit(void)
 
 static void wait_for_nmi(void)
 {
-	while (atomic_read(&in_nmi))
+	int waited = 0;
+
+	while (atomic_read(&in_nmi)) {
+		waited = 1;
 		cpu_relax();
+	}
+
+	if (waited)
+		nmi_wait_count++;
 }
 
 static int

Index: linux-tip.git/kernel/trace/trace.c
===================================================================

kernel/trace/trace.c +22 -7

+static DEFINE_MUTEX(dyn_info_mutex);
+
+int __weak ftrace_arch_read_dyn_info(char *buf, int size)
+{
+	return 0;
+}
+
 static ssize_t
-tracing_read_long(struct file *filp, char __user *ubuf,
+tracing_read_dyn_info(struct file *filp, char __user *ubuf,
 		  size_t cnt, loff_t *ppos)
 {
 	unsigned long *p = filp->private_data;
-	char buf[64];
+	char *buf = ftrace_dyn_info_buffer;
 	int r;
 
-	r = sprintf(buf, "%ld\n", *p);
+	mutex_lock(&dyn_info_mutex);
+	r = sprintf(buf, "%ld ", *p);
+
+	r += ftrace_arch_read_dyn_info(buf+r, DYN_INFO_BUF_SIZE-r);
+	buf[r++] = '\n';
+
+	r = simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
+
+	mutex_unlock(&dyn_info_mutex);
 
-	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
+	return r;
 }
 
-static struct file_operations tracing_read_long_fops = {
+static struct file_operations tracing_dyn_info_fops = {
 	.open		= tracing_open_generic,
-	.read		= tracing_read_long,
+	.read		= tracing_read_dyn_info,
 };
 #endif
 

@@ -2961,7 +2978,7 @@ static __init int tracer_init_debugfs(vo
 #ifdef CONFIG_DYNAMIC_FTRACE
 	entry = debugfs_create_file("dyn_ftrace_total_info", 0444, d_tracer,
 				    &ftrace_update_tot_cnt,
-				    &tracing_read_long_fops);
+				    &tracing_dyn_info_fops);
 	if (!entry)
 		pr_warning("Could not create debugfs "
 			   "'dyn_ftrace_total_info' entry\n");

Quoted 3 lines · On Thu, 30 Oct 2008 16:08:33 -0400 Steven Rostedt <[email protected]> wrote:

This patch adds dynamic ftrace NMI update statistics to the /debugfs/tracing/dyn_ftrace_total_info stat file.

Quoted 4 lines

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/kernel/ftrace.c |   26 ++++++++++++++++++++++++--
 kernel/trace/trace.c     |   31 ++++++++++++++++++++++++-------

No header files were modified, but ftrace_arch_read_dyn_info() should have been declared so that the above two compilation units see the declaration.

Quoted 10 lines

Index: linux-tip.git/arch/x86/kernel/ftrace.c
===================================================================

arch/x86/kernel/ftrace.c +0 -0

@@ -91,6 +91,19 @@ static int mod_code_write;
 static void *mod_code_ip;
 static void *mod_code_newcode;
 

arch/x86/kernel/ftrace.c static int mod_code_write; · line 95

Quoted 2 lines

arch/x86/kernel/ftrace.c +2 -0

+static int nmi_wait_count;
+static atomic_t nmi_update_count;

<the ATOMIC_INIT thing>

arch/x86/kernel/ftrace.c static int mod_code_write; · line 105

Quoted 10 lines

arch/x86/kernel/ftrace.c +9 -0

+int ftrace_arch_read_dyn_info(char *buf, int size)
+{
+	int r;
+
+	r = snprintf(buf, size, "%u %u",
+		     nmi_wait_count,
+		     atomic_read(&nmi_update_count));
+	return r;
+}

Yes, nmi_wait_count is an unsigned quantity. Might as well define it thusly?

Quoted 38 lines

arch/x86/kernel/ftrace.c +11 -2

 static void ftrace_mod_code(void)
 {
 	/*

@@ -109,8 +122,10 @@ void ftrace_nmi_enter(void)
 	atomic_inc(&in_nmi);
 	/* Must have in_nmi seen before reading write flag */
 	smp_mb();
-	if (mod_code_write)
+	if (mod_code_write) {
 		ftrace_mod_code();
+		atomic_inc(&nmi_update_count);
+	}
 }
 
 void ftrace_nmi_exit(void)

@@ -122,8 +137,15 @@ void ftrace_nmi_exit(void)
 
 static void wait_for_nmi(void)
 {
-	while (atomic_read(&in_nmi))
+	int waited = 0;
+
+	while (atomic_read(&in_nmi)) {
+		waited = 1;
 		cpu_relax();
+	}
+
+	if (waited)
+		nmi_wait_count++;
 }
 
 static int

Index: linux-tip.git/kernel/trace/trace.c
===================================================================

kernel/trace/trace.c static struct file_operations tracing_ma · line 2841

Quoted 5 lines

kernel/trace/trace.c +2 -0

@@ -2837,22 +2837,39 @@ static struct file_operations tracing_ma
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
+#define DYN_INFO_BUF_SIZE 1023
+static char ftrace_dyn_info_buffer[DYN_INFO_BUF_SIZE+1];

Could be made local to tracing_read_dyn_info().

Could just be

	static char ftrace_dyn_info_buffer[1024];

then use sizeof/ARRAY_SIZE elsewhere. I think this is a bit safer and

more readable - the reader doesn't have to run around the code checking that the correct #define was used in both places.

The robustness of ftrace has been the focus of the code modification in 2.6.28. There is one remaining issue that needed to be addressed. This was the case of NMIs.

Quoted 3 lines · Steven Rostedt

The robustness of ftrace has been the focus of the code modification in 2.6.28. There is one remaining issue that needed to be addressed. This was the case of NMIs.

applied to tip/tracing/nmisafe, thanks Steve!

this looks a lot nicer approach than either putting some sort of lock into NMI context (yuck) or the disabling of NMIs (not really possible in a generic way architecturally).

the impact is quite non-trivial, so i dont think this is v2.6.28 material.

Quoted 7 lines · Ingo Molnar

Quoted 3 lines · Steven Rostedt

The robustness of ftrace has been the focus of the code modification in 2.6.28. There is one remaining issue that needed to be addressed. This was the case of NMIs.

Quoted 8 lines

applied to tip/tracing/nmisafe, thanks Steve!

this looks a lot nicer approach than either putting some sort of lock into NMI context (yuck) or the disabling of NMIs (not really possible in a generic way architecturally).

the impact is quite non-trivial, so i dont think this is v2.6.28 material.

Ingo,

Do you want me to send patches on top of these to address Andrew's comements? Or do you want me to resend these with the updates.

I prefer to send patches on top, that way Andrew can make sure I did his changes correctly ;-)