Skip to contentSkip to conversation

ftrace updates for tip, and ground work for other architectures

PATCH In review Steven Rostedt 2 participants 7 messages Nov 15, 2008 – Nov 16, 2008

Subsystems (3)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover ftrace updates for tip, and ground work for other architectures

Ingo,

Patch 2 is a change in dynamic ftrace interface between the generic code and the arch dependent code. The original code did not allow for the flexibility to handle some architecture limitations. I ran into this when trying to port to PPC32 and realized that the modules used trampolines to make the jumps to mcount.

I have successfully ported PPC64 and PPC32 and will send those patches out later. But this patch series sets up the ground work for other architectures to port dynamic ftrace.

The main changes to the code is:

1) I removed the ftarce_modify_code and replaced it with two functions:
ftrace_make_nop() and ftrace_make_call. The former converts the code into a nop. The later converts a nop into a call.

2) instead of passing just the address and expected and replace in I pass in the dynamic ftrace record itself. This allows the architecture code save data per record if needed. To do this, there is now a dyn_arch_ftrace structure in the dyn_ftrace record.

3) On intialization, ftrace_make_nop gets passed in the module that created it or NULL if it is core kernel code. This allows the archicecture code do special things for modules, and also use the module sturcture to store data for later use with the conversions.

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 (5):
ftrace: replace raw_local_irq_save with local_irq_save
ftrace: pass module struct to arch dynamic ftrace functions
ftrace: allow NULL pointers in mcount_loc
ftrace: fix dyn ftrace filter
ftrace: make filtered functions effective on setting

----

 arch/x86/include/asm/ftrace.h |    8 ++
 arch/x86/kernel/ftrace.c      |   29 +++++++-
 include/linux/ftrace.h        |   53 +++++++++++----
 kernel/module.c               |    2 +-
 kernel/trace/ftrace.c         |  147 ++++++++++++++++++-----------------------
 kernel/trace/trace.c          |    4 +-
 kernel/trace/trace_selftest.c |    4 +-
 7 files changed, 145 insertions(+), 102 deletions(-)
author maintainer
#1 lore raw

PATCH 1/5 ftrace: replace raw_local_irq_save with local_irq_save

Impact: let logdev work when function tracing is enabled

The raw_local_irq_saves used in ftrace is causing problems with lockdep. They are not needed, and the normal local_irq_save is fine.

---
 kernel/trace/trace.c          |    4 ++--
 kernel/trace/trace_selftest.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
kernel/trace/trace.c +2 -2
index 4a90462..dff4bee 100644
@@ -1051,7 +1051,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip)
 	 * Need to use raw, since this must be called before the
 	 * recursive protection is performed.
 	 */
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	data = tr->data[cpu];
 	disabled = atomic_inc_return(&data->disabled);
@@ -1062,7 +1062,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip)
 	}
 
 	atomic_dec(&data->disabled);
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 }
 
 #ifdef CONFIG_FUNCTION_RET_TRACER
kernel/trace/trace_selftest.c +2 -2
index 24e6e07..5cb64ea 100644
@@ -52,7 +52,7 @@ static int trace_test_buffer(struct trace_array *tr, unsigned long *count)
 	int cpu, ret = 0;
 
 	/* Don't allow flipping of max traces now */
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	__raw_spin_lock(&ftrace_max_lock);
 
 	cnt = ring_buffer_entries(tr->buffer);
@@ -63,7 +63,7 @@ static int trace_test_buffer(struct trace_array *tr, unsigned long *count)
 			break;
 	}
 	__raw_spin_unlock(&ftrace_max_lock);
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 
 	if (count)
 		*count = cnt;
Signature · 3 lines
1.5.6.5

-- 
author maintainer
#2 lore raw

PATCH 2/5 ftrace: pass module struct to arch dynamic ftrace functions

In reply to Steven Rostedt

Impact: allow archs more flexibility on dynamic ftrace implementations

Dynamic ftrace has largly been developed on x86. Since x86 does not have the same limitations as other architectures, the ftrace interaction between the generic code and the architecture specific code was not flexible enough to handle some of the issues that other architectures have.

Most notably, module trampolines. Due to the limited branch distance that archs make in calling kernel core code from modules, the module load code must create a trampoline to jump to what will make the larger jump into core kernel code.

The problem arises when this happens to a call to mcount. Ftrace checks all code before modifying it and makes sure the current code is what it expects. Right now, there is not enough information to handle modifying module trampolines.

This patch changes the API between generic dynamic ftrace code and the arch dependent code. There is now two functions for modifying code:

ftrace_make_nop(mod, rec, addr) - convert the code at rec->ip into
a nop, where the original text is calling addr. (mod is the module struct if called by module init)

ftrace_make_caller(rec, addr) - convert the code rec->ip that should
be a nop into a caller to addr.

The record "rec" now has a new field called "arch" where the architecture can add any special attributes to each call site record.

---
 arch/x86/include/asm/ftrace.h |    8 +++++
 arch/x86/kernel/ftrace.c      |   29 +++++++++++++++++--
 include/linux/ftrace.h        |   53 +++++++++++++++++++++++++++--------
 kernel/module.c               |    2 +-
 kernel/trace/ftrace.c         |   62 ++++++++++++++++-------------------------
 5 files changed, 100 insertions(+), 54 deletions(-)
arch/x86/include/asm/ftrace.h +8 -0
index 9b6a1fa..2bb43b4 100644
@@ -17,6 +17,14 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 	 */
 	return addr - 1;
 }
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+
+struct dyn_arch_ftrace {
+	/* No extra data needed for x86 */
+};
+
+#endif /*  CONFIG_DYNAMIC_FTRACE */
 #endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_TRACER */
 
arch/x86/kernel/ftrace.c +26 -3
index fe83273..762222a 100644
@@ -166,7 +166,7 @@ static int ftrace_calc_offset(long ip, long addr)
 	return (int)(addr - ip);
 }
 
-unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
+static unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
 {
 	static union ftrace_code_union calc;
 
@@ -311,12 +311,12 @@ do_ftrace_mod_code(unsigned long ip, void *new_code)
 
 static unsigned char ftrace_nop[MCOUNT_INSN_SIZE];
 
-unsigned char *ftrace_nop_replace(void)
+static unsigned char *ftrace_nop_replace(void)
 {
 	return ftrace_nop;
 }
 
-int
+static int
 ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 		   unsigned char *new_code)
 {
@@ -349,6 +349,29 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 	return 0;
 }
 
+int ftrace_make_nop(struct module *mod,
+		    struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned char *new, *old;
+	unsigned long ip = rec->ip;
+
+	old = ftrace_call_replace(ip, addr);
+	new = ftrace_nop_replace();
+
+	return ftrace_modify_code(rec->ip, old, new);
+}
+
+int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned char *new, *old;
+	unsigned long ip = rec->ip;
+
+	old = ftrace_nop_replace();
+	new = ftrace_call_replace(ip, addr);
+
+	return ftrace_modify_code(rec->ip, old, new);
+}
+
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {
 	unsigned long ip = (unsigned long)(&ftrace_call);
include/linux/ftrace.h +41 -12

100 lines not on this page · raw message

kernel/module.c +1 -1
index 48c323c..bf94e1e 100644
@@ -2202,7 +2202,7 @@ static noinline struct module *load_module(void __user *umod,
 	/* sechdrs[0].sh_size is always zero */
 	mseg = section_objs(hdr, sechdrs, secstrings, "__mcount_loc",
 			    sizeof(*mseg), &num_mcount);
-	ftrace_init_module(mseg, mseg + num_mcount);
+	ftrace_init_module(mod, mseg, mseg + num_mcount);
 
 	err = module_finalize(hdr, sechdrs, mod);
 	if (err < 0)
kernel/trace/ftrace.c +24 -38

173 lines not on this page · raw message

Signature · 3 lines
1.5.6.5

-- 
author maintainer
#3 lore raw

PATCH 3/5 ftrace: allow NULL pointers in mcount_loc

In reply to Steven Rostedt

Impact: let archs insert NULL pointers in mcount_loc section

Due to the way different architecture linkers combine the data sections of the mcount_loc (the section that lists all the locations that call mcount), there may be zeros added in that section. This is usually due to strange alignments that the linker performs, that pads in zeros.

This patch makes the conversion code to nops skip any pointer in the mcount_loc section that is NULL.

---
 kernel/trace/ftrace.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
kernel/trace/ftrace.c +8 -0
index 6adad23..2384a9c 100644
@@ -1305,6 +1305,14 @@ static int ftrace_convert_nops(struct module *mod,
 	p = start;
 	while (p < end) {
 		addr = ftrace_call_adjust(*p++);
+		/*
+		 * Some architecture linkers will pad between
+		 * the different mcount_loc sections of different
+		 * object files to satisfy alignments.
+		 * Skip any NULL pointers.
+		 */
+		if (!addr)
+			continue;
 		ftrace_record_ip(addr);
 	}
 
Signature · 3 lines
1.5.6.5

-- 
author maintainer
#4 lore raw

PATCH 4/5 ftrace: fix dyn ftrace filter

In reply to Steven Rostedt

Impact: correct implementation of dyn ftrace filter

The old decisions made by the filter algorithm was complex and incorrect. This lead to inconsistent enabling or disabling of functions when the filter was used.

This patch simplifies that code and in doing so, corrects the usage of the filters.

---
 kernel/trace/ftrace.c |   83 +++++++++++++++++++++---------------------------
 1 files changed, 36 insertions(+), 47 deletions(-)
kernel/trace/ftrace.c +36 -47
index 2384a9c..3ef0ee6 100644
@@ -395,72 +395,62 @@ __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
 
 	ip = rec->ip;
 
-	if (ftrace_filtered && enable) {
+	/*
+	 * If this record is not to be traced and
+	 * it is not enabled then do nothing.
+	 *
+	 * If this record is not to be traced and
+	 * it is enabled then disabled it.
+	 *
+	 */
+	if (rec->flags & FTRACE_FL_NOTRACE) {
+		if (rec->flags & FTRACE_FL_ENABLED)
+			rec->flags &= ~FTRACE_FL_ENABLED;
+		else
+			return 0;
+
+	} else if (ftrace_filtered && enable) {
 		/*
-		 * If filtering is on:
-		 *
-		 * If this record is set to be filtered and
-		 * is enabled then do nothing.
-		 *
-		 * If this record is set to be filtered and
-		 * it is not enabled, enable it.
-		 *
-		 * If this record is not set to be filtered
-		 * and it is not enabled do nothing.
-		 *
-		 * If this record is set not to trace then
-		 * do nothing.
-		 *
-		 * If this record is set not to trace and
-		 * it is enabled then disable it.
-		 *
-		 * If this record is not set to be filtered and
-		 * it is enabled, disable it.
+		 * Filtering is on:
 		 */
 
-		fl = rec->flags & (FTRACE_FL_FILTER | FTRACE_FL_NOTRACE |
-				   FTRACE_FL_ENABLED);
+		fl = rec->flags & (FTRACE_FL_FILTER | FTRACE_FL_ENABLED);
 
-		if ((fl ==  (FTRACE_FL_FILTER | FTRACE_FL_ENABLED)) ||
-		    (fl ==  (FTRACE_FL_FILTER | FTRACE_FL_NOTRACE)) ||
-		    !fl || (fl == FTRACE_FL_NOTRACE))
+		/* Record is filtered and enabled, do nothing */
+		if (fl == (FTRACE_FL_FILTER | FTRACE_FL_ENABLED))
 			return 0;
 
-		/*
-		 * If it is enabled disable it,
-		 * otherwise enable it!
-		 */
-		if (fl & FTRACE_FL_ENABLED) {
-			enable = 0;
+		/* Record is not filtered and is not enabled do nothing */
+		if (!fl)
+			return 0;
+
+		/* Record is not filtered but enabled, disable it */
+		if (fl == FTRACE_FL_ENABLED)
 			rec->flags &= ~FTRACE_FL_ENABLED;
-		} else {
-			enable = 1;
+		else
+		/* Otherwise record is filtered but not enabled, enable it */
 			rec->flags |= FTRACE_FL_ENABLED;
-		}
 	} else {
+		/* Disable or not filtered */
 
 		if (enable) {
-			/*
-			 * If this record is set not to trace and is
-			 * not enabled, do nothing.
-			 */
-			fl = rec->flags & (FTRACE_FL_NOTRACE | FTRACE_FL_ENABLED);
-			if (fl == FTRACE_FL_NOTRACE)
-				return 0;
-		}
-
-		if (enable) {
+			/* if record is enabled, do nothing */
 			if (rec->flags & FTRACE_FL_ENABLED)
 				return 0;
+
 			rec->flags |= FTRACE_FL_ENABLED;
+
 		} else {
+
+			/* if record is not enabled do nothing */
 			if (!(rec->flags & FTRACE_FL_ENABLED))
 				return 0;
+
 			rec->flags &= ~FTRACE_FL_ENABLED;
 		}
 	}
 
-	if (enable)
+	if (rec->flags & FTRACE_FL_ENABLED)
 		return ftrace_make_call(rec, FTRACE_ADDR);
 	else
 		return ftrace_make_nop(NULL, rec, FTRACE_ADDR);
@@ -555,8 +545,7 @@ static void ftrace_startup(void)
 
 	mutex_lock(&ftrace_start_lock);
 	ftrace_start_up++;
-	if (ftrace_start_up == 1)
-		command |= FTRACE_ENABLE_CALLS;
+	command |= FTRACE_ENABLE_CALLS;
 
 	if (saved_ftrace_func != ftrace_trace_function) {
 		saved_ftrace_func = ftrace_trace_function;
Signature · 3 lines
1.5.6.5

-- 
author maintainer
#5 lore raw

PATCH 5/5 ftrace: make filtered functions effective on setting

In reply to Steven Rostedt

Impact: set filtered functions at time the filter is set

It can be confusing when the set_filter_functions is set (or cleared) and the functions being recorded by the dynamic tracer does not match.

This patch causes the code to be updated if the function tracer is enabled and the filter is changed.

---
 kernel/trace/ftrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/trace/ftrace.c +1 -1
index 3ef0ee6..80b1de3 100644
@@ -1195,7 +1195,7 @@ ftrace_regex_release(struct inode *inode, struct file *file, int enable)
 
 	mutex_lock(&ftrace_sysctl_lock);
 	mutex_lock(&ftrace_start_lock);
-	if (iter->filtered && ftrace_start_up && ftrace_enabled)
+	if (ftrace_start_up && ftrace_enabled)
 		ftrace_run_update_code(FTRACE_ENABLE_CALLS);
 	mutex_unlock(&ftrace_start_lock);
 	mutex_unlock(&ftrace_sysctl_lock);
Signature · 3 lines
1.5.6.5

-- 
#6 lore raw
In reply to Steven Rostedt
Quoted 7 lines · Steven Rostedt

The following patches are in:

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

branch: tip/devel

Quoted 6 lines

Steven Rostedt (5):
ftrace: replace raw_local_irq_save with local_irq_save
ftrace: pass module struct to arch dynamic ftrace functions
ftrace: allow NULL pointers in mcount_loc
ftrace: fix dyn ftrace filter
ftrace: make filtered functions effective on setting

I have pulled these:

ee02a2e: ftrace: make filtered functions effective on setting 982c350: ftrace: fix dyn ftrace filter
20e5227: ftrace: allow NULL pointers in mcount_loc 31e8890: ftrace: pass module struct to arch dynamic ftrace functions
d51ad7a: ftrace: replace raw_local_irq_save with local_irq_save 918c115: ftrace: do not process freed records
b17e8a3: ftrace: disable ftrace on anomalies in trace start and stop
f3c7ac4: ftrace: remove condition from ftrace_record_ip

thanks Steve,

Ingo