Skip to contentSkip to conversation

tracing: fixes

GIT PULL In review Steven Rostedt 5 participants 17 messages May 26, 2011 – May 27, 2011

Subsystems (11)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

Ingo,

This patch set contains all fixes. Some of the fixes are to get rid of warnings, but I think getting rid of warnings is a good thing during a merge window.

What's different about this and my last pull request:

1) I removed the two patches that were not bug fixes and put them in my queue for 2.6.41/3.1/2.8.1

2) I added two patches, one that was a bug fix that caused a triple fault when PROVE_RCU was set, and the other that caused problems with truncating 64bit words in the trace events.

3) I pulled out Masami's kprobe stack trace fix because it failed on other archs. My last push request, I did compile test other archs, but I only used the default config. This time, I enabled tracing as well (doing it both with and without tracing enabled) and when tracing was enabled, I detected the problem with the lack of save_stack_trace_regs() in other archs.

I tested this on x86_64 and x86_32 as well as compiled tested for the following archs, with and without tracing enabled:

alpha, arm, blackfin, ia64, parsic, ppc, ppc64, sparc

The following had failures for other reason (failed without this patch set):

cris, cris32, m68k, mips64, mips32, parsic64, sh, xtensa, uml.

Please pull the latest tip/perf/urgent tree, which can be found at:

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

Jiri Olsa (1):
jump_label: Check entries limit in __jump_label_update

Rabin Vincent (1):
ftrace/recordmcount: Avoid STT_FUNC symbols as base on ARM

Rakib Mullick (1):
x86/ftrace: Fix compiler warning in ftrace.c

Steven Rostedt (7):
maccess,probe_kernel: Make write/read src const void * scripts/tags.sh: Fix ctags for DEFINE_EVENT() scripts/tags.sh: Add magic for trace-events for etags too
ftrace: Have ftrace_startup() return failure code
tracing: Have event with function tracer check error return
ftrace: Set ops->flag to enabled even on static function tracing
ftrace: Add internal recursive checks

liubo (2):
tracing: Add __print_symbolic_u64 to avoid warnings on 32bit machine
tracing: Update btrfs's tracepoints to use u64 interface

----

 arch/blackfin/mm/maccess.c   |    4 ++--
 arch/s390/mm/maccess.c       |    4 ++--
 arch/x86/kernel/ftrace.c     |   12 ++++++------
 include/linux/ftrace_event.h |   12 ++++++++++++
 include/linux/sched.h        |    2 +-
 include/linux/uaccess.h      |    8 ++++----
 include/trace/events/btrfs.h |    4 ++--
 include/trace/ftrace.h       |   13 +++++++++++++
 kernel/jump_label.c          |   18 +++++++++++++-----
 kernel/trace/ftrace.c        |   31 ++++++++++++++++++++++++-------
 kernel/trace/ring_buffer.c   |   10 +++++-----
 kernel/trace/trace.h         |   15 +++++++++++++++
 kernel/trace/trace_events.c  |    7 ++++++-
 kernel/trace/trace_output.c  |   27 +++++++++++++++++++++++++++
 mm/maccess.c                 |    8 ++++----
 scripts/recordmcount.h       |    8 ++++++++
 scripts/tags.sh              |    6 ++++--
 17 files changed, 148 insertions(+), 41 deletions(-)
author maintainer
#1 lore raw

PATCH 1/12 maccess,probe_kernel: Make write/read src const void *

From: Steven Rostedt <[email protected]>

The functions probe_kernel_write() and probe_kernel_read() do not modify the src pointer. Allow const pointers to be passed in without the need of a typecast.

---
 arch/blackfin/mm/maccess.c |    4 ++--
 arch/s390/mm/maccess.c     |    4 ++--
 include/linux/uaccess.h    |    8 ++++----
 mm/maccess.c               |    8 ++++----
 4 files changed, 12 insertions(+), 12 deletions(-)
arch/blackfin/mm/maccess.c +2 -2
index b71cebc..e253211 100644
@@ -16,7 +16,7 @@ static int validate_memory_access_address(unsigned long addr, int size)
 	return bfin_mem_access_type(addr, size);
 }
 
-long probe_kernel_read(void *dst, void *src, size_t size)
+long probe_kernel_read(void *dst, const void *src, size_t size)
 {
 	unsigned long lsrc = (unsigned long)src;
 	int mem_type;
@@ -55,7 +55,7 @@ long probe_kernel_read(void *dst, void *src, size_t size)
 	return -EFAULT;
 }
 
-long probe_kernel_write(void *dst, void *src, size_t size)
+long probe_kernel_write(void *dst, const void *src, size_t size)
 {
 	unsigned long ldst = (unsigned long)dst;
 	int mem_type;
arch/s390/mm/maccess.c +2 -2
index 71a4b0d..51e5cd9 100644
@@ -19,7 +19,7 @@
  * using the stura instruction.
  * Returns the number of bytes copied or -EFAULT.
  */
-static long probe_kernel_write_odd(void *dst, void *src, size_t size)
+static long probe_kernel_write_odd(void *dst, const void *src, size_t size)
 {
 	unsigned long count, aligned;
 	int offset, mask;
@@ -45,7 +45,7 @@ static long probe_kernel_write_odd(void *dst, void *src, size_t size)
 	return rc ? rc : count;
 }
 
-long probe_kernel_write(void *dst, void *src, size_t size)
+long probe_kernel_write(void *dst, const void *src, size_t size)
 {
 	long copied = 0;
 
include/linux/uaccess.h +4 -4
index d512d98..5ca0951 100644
@@ -93,8 +93,8 @@ static inline unsigned long __copy_from_user_nocache(void *to,
  * Safely read from address @src to the buffer at @dst.  If a kernel fault
  * happens, handle that and return -EFAULT.
  */
-extern long probe_kernel_read(void *dst, void *src, size_t size);
-extern long __probe_kernel_read(void *dst, void *src, size_t size);
+extern long probe_kernel_read(void *dst, const void *src, size_t size);
+extern long __probe_kernel_read(void *dst, const void *src, size_t size);
 
 /*
  * probe_kernel_write(): safely attempt to write to a location
@@ -105,7 +105,7 @@ extern long __probe_kernel_read(void *dst, void *src, size_t size);
  * Safely write to address @dst from the buffer at @src.  If a kernel fault
  * happens, handle that and return -EFAULT.
  */
-extern long notrace probe_kernel_write(void *dst, void *src, size_t size);
-extern long notrace __probe_kernel_write(void *dst, void *src, size_t size);
+extern long notrace probe_kernel_write(void *dst, const void *src, size_t size);
+extern long notrace __probe_kernel_write(void *dst, const void *src, size_t size);
 
 #endif		/* __LINUX_UACCESS_H__ */
mm/maccess.c +4 -4
index e2b6f56..4cee182 100644
@@ -15,10 +15,10 @@
  * happens, handle that and return -EFAULT.
  */
 
-long __weak probe_kernel_read(void *dst, void *src, size_t size)
+long __weak probe_kernel_read(void *dst, const void *src, size_t size)
     __attribute__((alias("__probe_kernel_read")));
 
-long __probe_kernel_read(void *dst, void *src, size_t size)
+long __probe_kernel_read(void *dst, const void *src, size_t size)
 {
 	long ret;
 	mm_segment_t old_fs = get_fs();
@@ -43,10 +43,10 @@ EXPORT_SYMBOL_GPL(probe_kernel_read);
  * Safely write to address @dst from the buffer at @src.  If a kernel fault
  * happens, handle that and return -EFAULT.
  */
-long __weak probe_kernel_write(void *dst, void *src, size_t size)
+long __weak probe_kernel_write(void *dst, const void *src, size_t size)
     __attribute__((alias("__probe_kernel_write")));
 
-long __probe_kernel_write(void *dst, void *src, size_t size)
+long __probe_kernel_write(void *dst, const void *src, size_t size)
 {
 	long ret;
 	mm_segment_t old_fs = get_fs();
Signature · 1 line
1.7.4.4
author maintainer
#2 lore raw

PATCH 2/12 x86/ftrace: Fix compiler warning in ftrace.c

In reply to Steven Rostedt

From: Rakib Mullick <[email protected]>

Due to commit dc326fca2b64 (x86, cpu: Clean up and unify the NOP selection infrastructure), we get the following warning:

arch/x86/kernel/ftrace.c: In function ‘ftrace_make_nop’:
arch/x86/kernel/ftrace.c:308:6: warning: assignment discards qualifiers from pointer target type arch/x86/kernel/ftrace.c: In function ‘ftrace_make_call’:
arch/x86/kernel/ftrace.c:318:6: warning: assignment discards qualifiers from pointer target type

ftrace_nop_replace() now returns const unsigned char *, so change its associated function/variable to its compatible type to keep compiler clam.

[ updated for change of const void *src in probe_kernel_write() ]

---
 arch/x86/kernel/ftrace.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
arch/x86/kernel/ftrace.c +6 -6
index 0ba15a6..c9a281f 100644
@@ -123,7 +123,7 @@ static unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
 static atomic_t nmi_running = ATOMIC_INIT(0);
 static int mod_code_status;		/* holds return value of text write */
 static void *mod_code_ip;		/* holds the IP to write to */
-static void *mod_code_newcode;		/* holds the text to write to the IP */
+static const void *mod_code_newcode;	/* holds the text to write to the IP */
 
 static unsigned nmi_wait_count;
 static atomic_t nmi_update_count = ATOMIC_INIT(0);
@@ -225,7 +225,7 @@ within(unsigned long addr, unsigned long start, unsigned long end)
 }
 
 static int
-do_ftrace_mod_code(unsigned long ip, void *new_code)
+do_ftrace_mod_code(unsigned long ip, const void *new_code)
 {
 	/*
 	 * On x86_64, kernel text mappings are mapped read-only with
@@ -266,8 +266,8 @@ static const unsigned char *ftrace_nop_replace(void)
 }
 
 static int
-ftrace_modify_code(unsigned long ip, unsigned char *old_code,
-		   unsigned char *new_code)
+ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
+		   unsigned const char *new_code)
 {
 	unsigned char replaced[MCOUNT_INSN_SIZE];
 
@@ -301,7 +301,7 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {
-	unsigned char *new, *old;
+	unsigned const char *new, *old;
 	unsigned long ip = rec->ip;
 
 	old = ftrace_call_replace(ip, addr);
@@ -312,7 +312,7 @@ int ftrace_make_nop(struct module *mod,
 
 int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
-	unsigned char *new, *old;
+	unsigned const char *new, *old;
 	unsigned long ip = rec->ip;
 
 	old = ftrace_nop_replace();
Signature · 1 line
1.7.4.4
author maintainer
#3 lore raw

PATCH 3/12 scripts/tags.sh: Fix ctags for DEFINE_EVENT()

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

The regex to handle DEFINE_EVENT() should not be the same as the TRACE_EVENT() as the first parameter in DEFINE_EVENT is the template name, not the event name. We need the second parameter as that is what the trace_... will use.

---
 scripts/tags.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
scripts/tags.sh +1 -1
index bd6185d..33b53ca 100755
@@ -132,7 +132,7 @@ exuberant()
 	--regex-asm='/^ENTRY\(([^)]*)\).*/\1/'                  \
 	--regex-c='/^SYSCALL_DEFINE[[:digit:]]?\(([^,)]*).*/sys_\1/' \
 	--regex-c++='/^TRACE_EVENT\(([^,)]*).*/trace_\1/'		\
-	--regex-c++='/^DEFINE_EVENT\(([^,)]*).*/trace_\1/'
+	--regex-c++='/^DEFINE_EVENT\([^,)]*, *([^,)]*).*/trace_\1/'
 
 	all_kconfigs | xargs $1 -a                              \
 	--langdef=kconfig --language-force=kconfig              \
Signature · 1 line
1.7.4.4
author maintainer
#4 lore raw

PATCH 4/12 scripts/tags.sh: Add magic for trace-events for etags too

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Seems that Peter Zijlstra treats us emacs users as second class citizens and the commit:

commit 15664125f7cadcb6d725cb2d9b90f9715397848d
Author: Peter Zijlstra <[email protected]>
scripts/tags.sh: Add magic for trace-events

only updated ctags (for vim) and did not do the work to let us lowly emacs users benefit from such a change.

---
 scripts/tags.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
scripts/tags.sh +3 -1
index 33b53ca..75c5d24 100755
@@ -152,7 +152,9 @@ emacs()
 {
 	all_sources | xargs $1 -a                               \
 	--regex='/^ENTRY(\([^)]*\)).*/\1/'                      \
-	--regex='/^SYSCALL_DEFINE[0-9]?(\([^,)]*\).*/sys_\1/'
+	--regex='/^SYSCALL_DEFINE[0-9]?(\([^,)]*\).*/sys_\1/'   \
+	--regex='/^TRACE_EVENT(\([^,)]*\).*/trace_\1/'		\
+	--regex='/^DEFINE_EVENT([^,)]*, *\([^,)]*\).*/trace_\1/'
 
 	all_kconfigs | xargs $1 -a                              \
 	--regex='/^[ \t]*\(\(menu\)*config\)[ \t]+\([a-zA-Z0-9_]+\)/\3/'
Signature · 1 line
1.7.4.4
maintainer
#5 lore raw

Re: [PATCH 04/12] scripts/tags.sh: Add magic for trace-events for etags too

Quoted 2 lines · Steven Rostedt

Seems that Peter Zijlstra treats us emacs users as second class citizens and the commit:

Yeah, I actually use Linux as my OS ;-)

/me runs

anyway, yeah, I completely missed that there were two series of regexes in there.

author maintainer
#6 lore raw

PATCH 5/12 ftrace/recordmcount: Avoid STT_FUNC symbols as base on ARM

In reply to Steven Rostedt

From: Rabin Vincent <[email protected]>

While find_secsym_ndx often finds the unamed local STT_SECTION, if a section has only one function in it, the ARM toolchain generates the STT_FUNC symbol before the STT_SECTION, and recordmcount finds this instead.

This is problematic on ARM because in ARM ELFs, "if a [STT_FUNC] symbol addresses a Thumb instruction, its value is the address of the instruction with bit zero set (in a relocatable object, the section offset with bit zero set)". This leads to incorrect mcount addresses being recorded.

Fix this by not using STT_FUNC symbols as the base on ARM.

---
 scripts/recordmcount.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
scripts/recordmcount.h +8 -0
index 4be6036..f40a6af6 100644
@@ -43,6 +43,7 @@
 #undef ELF_R_INFO
 #undef Elf_r_info
 #undef ELF_ST_BIND
+#undef ELF_ST_TYPE
 #undef fn_ELF_R_SYM
 #undef fn_ELF_R_INFO
 #undef uint_t
@@ -76,6 +77,7 @@
 # define ELF_R_INFO		ELF64_R_INFO
 # define Elf_r_info		Elf64_r_info
 # define ELF_ST_BIND		ELF64_ST_BIND
+# define ELF_ST_TYPE		ELF64_ST_TYPE
 # define fn_ELF_R_SYM		fn_ELF64_R_SYM
 # define fn_ELF_R_INFO		fn_ELF64_R_INFO
 # define uint_t			uint64_t
@@ -108,6 +110,7 @@
 # define ELF_R_INFO		ELF32_R_INFO
 # define Elf_r_info		Elf32_r_info
 # define ELF_ST_BIND		ELF32_ST_BIND
+# define ELF_ST_TYPE		ELF32_ST_TYPE
 # define fn_ELF_R_SYM		fn_ELF32_R_SYM
 # define fn_ELF_R_INFO		fn_ELF32_R_INFO
 # define uint_t			uint32_t
@@ -427,6 +430,11 @@ static unsigned find_secsym_ndx(unsigned const txtndx,
 		if (txtndx == w2(symp->st_shndx)
 			/* avoid STB_WEAK */
 		    && (STB_LOCAL == st_bind || STB_GLOBAL == st_bind)) {
+			/* function symbols on ARM have quirks, avoid them */
+			if (w2(ehdr->e_machine) == EM_ARM
+			    && ELF_ST_TYPE(symp->st_info) == STT_FUNC)
+				continue;
+
 			*recvalp = _w(symp->st_value);
 			return symp - sym0;
 		}
Signature · 1 line
1.7.4.4
author maintainer
#7 lore raw

PATCH 6/12 jump_label: Check entries limit in __jump_label_update

In reply to Steven Rostedt

From: Jiri Olsa <[email protected]>

When iterating the jump_label entries array (core or modules), the __jump_label_update function peeks over the last entry.

The reason is that the end of the for loop depends on the key value of the processed entry. Thus when going through the last array entry, we will touch the memory behind the array limit.

This bug probably will never be triggered, since most likely the memory behind the jump_label entries will be accesable and the entry->key will be different than the expected value.

---
 kernel/jump_label.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)
kernel/jump_label.c +13 -5
index 74d1c09..fa27e75 100644
@@ -105,9 +105,12 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
 }
 
 static void __jump_label_update(struct jump_label_key *key,
-		struct jump_entry *entry, int enable)
+				struct jump_entry *entry,
+				struct jump_entry *stop, int enable)
 {
-	for (; entry->key == (jump_label_t)(unsigned long)key; entry++) {
+	for (; (entry < stop) &&
+	      (entry->key == (jump_label_t)(unsigned long)key);
+	      entry++) {
 		/*
 		 * entry->code set to 0 invalidates module init text sections
 		 * kernel_text_address() verifies we are not in core kernel
@@ -181,7 +184,11 @@ static void __jump_label_mod_update(struct jump_label_key *key, int enable)
 	struct jump_label_mod *mod = key->next;
 
 	while (mod) {
-		__jump_label_update(key, mod->entries, enable);
+		struct module *m = mod->mod;
+
+		__jump_label_update(key, mod->entries,
+				    m->jump_entries + m->num_jump_entries,
+				    enable);
 		mod = mod->next;
 	}
 }
@@ -245,7 +252,8 @@ static int jump_label_add_module(struct module *mod)
 		key->next = jlm;
 
 		if (jump_label_enabled(key))
-			__jump_label_update(key, iter, JUMP_LABEL_ENABLE);
+			__jump_label_update(key, iter, iter_stop,
+					    JUMP_LABEL_ENABLE);
 	}
 
 	return 0;
@@ -371,7 +379,7 @@ static void jump_label_update(struct jump_label_key *key, int enable)
 
 	/* if there are no users, entry can be NULL */
 	if (entry)
-		__jump_label_update(key, entry, enable);
+		__jump_label_update(key, entry, __stop___jump_table, enable);
 
 #ifdef CONFIG_MODULES
 	__jump_label_mod_update(key, enable);
Signature · 1 line
1.7.4.4
author maintainer
#8 lore raw

PATCH 7/12 ftrace: Have ftrace_startup() return failure code

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

The register_ftrace_function() returns an error code on failure except if the call to ftrace_startup() fails. Add a error return to ftrace_startup() if it fails to start, allowing register_ftrace_funtion() to return a proper error value.

---
 kernel/trace/ftrace.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)
kernel/trace/ftrace.c +8 -6
index d017c2c..bebbc95 100644
@@ -1638,12 +1638,12 @@ static void ftrace_startup_enable(int command)
 	ftrace_run_update_code(command);
 }
 
-static void ftrace_startup(struct ftrace_ops *ops, int command)
+static int ftrace_startup(struct ftrace_ops *ops, int command)
 {
 	bool hash_enable = true;
 
 	if (unlikely(ftrace_disabled))
-		return;
+		return -ENODEV;
 
 	ftrace_start_up++;
 	command |= FTRACE_ENABLE_CALLS;
@@ -1662,6 +1662,8 @@ static void ftrace_startup(struct ftrace_ops *ops, int command)
 		ftrace_hash_rec_enable(ops, 1);
 
 	ftrace_startup_enable(command);
+
+	return 0;
 }
 
 static void ftrace_shutdown(struct ftrace_ops *ops, int command)
@@ -2501,7 +2503,7 @@ static void __enable_ftrace_function_probe(void)
 
 	ret = __register_ftrace_function(&trace_probe_ops);
 	if (!ret)
-		ftrace_startup(&trace_probe_ops, 0);
+		ret = ftrace_startup(&trace_probe_ops, 0);
 
 	ftrace_probe_registered = 1;
 }
@@ -3466,7 +3468,7 @@ device_initcall(ftrace_nodyn_init);
 static inline int ftrace_init_dyn_debugfs(struct dentry *d_tracer) { return 0; }
 static inline void ftrace_startup_enable(int command) { }
 /* Keep as macros so we do not need to define the commands */
-# define ftrace_startup(ops, command)	do { } while (0)
+# define ftrace_startup(ops, command)	({0;})
 # define ftrace_shutdown(ops, command)	do { } while (0)
 # define ftrace_startup_sysctl()	do { } while (0)
 # define ftrace_shutdown_sysctl()	do { } while (0)
@@ -3799,7 +3801,7 @@ int register_ftrace_function(struct ftrace_ops *ops)
 
 	ret = __register_ftrace_function(ops);
 	if (!ret)
-		ftrace_startup(ops, 0);
+		ret = ftrace_startup(ops, 0);
 
 
  out_unlock:
@@ -4045,7 +4047,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 	ftrace_graph_return = retfunc;
 	ftrace_graph_entry = entryfunc;
 
-	ftrace_startup(&global_ops, FTRACE_START_FUNC_RET);
+	ret = ftrace_startup(&global_ops, FTRACE_START_FUNC_RET);
 
 out:
 	mutex_unlock(&ftrace_lock);
Signature · 1 line
1.7.4.4
author maintainer
#9 lore raw

PATCH 8/12 tracing: Have event with function tracer check error return

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

The self tests for event tracer does not check if the function tracing was successfully activated. It needs to before it continues the tests, otherwise the wrong errors may be reported.

---
 kernel/trace/trace_events.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
kernel/trace/trace_events.c +6 -1
index 2fe1103..686ec39 100644
@@ -1657,7 +1657,12 @@ static struct ftrace_ops trace_ops __initdata  =
 
 static __init void event_trace_self_test_with_function(void)
 {
-	register_ftrace_function(&trace_ops);
+	int ret;
+	ret = register_ftrace_function(&trace_ops);
+	if (WARN_ON(ret < 0)) {
+		pr_info("Failed to enable function tracer for event tests\n");
+		return;
+	}
 	pr_info("Running tests again, along with the function tracer\n");
 	event_trace_self_tests();
 	unregister_ftrace_function(&trace_ops);
Signature · 1 line
1.7.4.4
author maintainer
#10 lore raw

PATCH 9/12 ftrace: Set ops->flag to enabled even on static function tracing

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

When dynamic ftrace is not configured, the ops->flags still needs to have its FTRACE_OPS_FL_ENABLED bit set in ftrace_startup().

---
 kernel/trace/ftrace.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
kernel/trace/ftrace.c +5 -1
index bebbc95..25949b3 100644
@@ -3468,7 +3468,11 @@ device_initcall(ftrace_nodyn_init);
 static inline int ftrace_init_dyn_debugfs(struct dentry *d_tracer) { return 0; }
 static inline void ftrace_startup_enable(int command) { }
 /* Keep as macros so we do not need to define the commands */
-# define ftrace_startup(ops, command)	({0;})
+# define ftrace_startup(ops, command)			\
+	({						\
+		(ops)->flags |= FTRACE_OPS_FL_ENABLED;	\
+		0;					\
+	})
 # define ftrace_shutdown(ops, command)	do { } while (0)
 # define ftrace_startup_sysctl()	do { } while (0)
 # define ftrace_shutdown_sysctl()	do { } while (0)
Signature · 1 line
1.7.4.4
author maintainer
#11 lore raw

PATCH 10/12 tracing: Add __print_symbolic_u64 to avoid warnings on 32bit machine

In reply to Steven Rostedt

From: liubo <[email protected]>

Filesystem, like Btrfs, has some "ULL" macros, and when these macros are passed to tracepoints'__print_symbolic(), there will be 64->32 truncate WARNINGS during compiling on 32bit box.

---
 include/linux/ftrace_event.h |   12 ++++++++++++
 include/trace/ftrace.h       |   13 +++++++++++++
 kernel/trace/trace_output.c  |   27 +++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 0 deletions(-)
include/linux/ftrace_event.h +12 -0
index b5a550a..59d3ef1 100644
@@ -16,6 +16,11 @@ struct trace_print_flags {
 	const char		*name;
 };
 
+struct trace_print_flags_u64 {
+	unsigned long long	mask;
+	const char		*name;
+};
+
 const char *ftrace_print_flags_seq(struct trace_seq *p, const char *delim,
 				   unsigned long flags,
 				   const struct trace_print_flags *flag_array);
@@ -23,6 +28,13 @@ const char *ftrace_print_flags_seq(struct trace_seq *p, const char *delim,
 const char *ftrace_print_symbols_seq(struct trace_seq *p, unsigned long val,
 				     const struct trace_print_flags *symbol_array);
 
+#if BITS_PER_LONG == 32
+const char *ftrace_print_symbols_seq_u64(struct trace_seq *p,
+					 unsigned long long val,
+					 const struct trace_print_flags_u64
+								 *symbol_array);
+#endif
+
 const char *ftrace_print_hex_seq(struct trace_seq *p,
 				 const unsigned char *buf, int len);
 
include/trace/ftrace.h +13 -0
index 3e68366..533c49f 100644
@@ -205,6 +205,19 @@
 		ftrace_print_symbols_seq(p, value, symbols);		\
 	})
 
+#undef __print_symbolic_u64
+#if BITS_PER_LONG == 32
+#define __print_symbolic_u64(value, symbol_array...)			\
+	({								\
+		static const struct trace_print_flags_u64 symbols[] =	\
+			{ symbol_array, { -1, NULL } };			\
+		ftrace_print_symbols_seq_u64(p, value, symbols);	\
+	})
+#else
+#define __print_symbolic_u64(value, symbol_array...)			\
+			__print_symbolic(value, symbol_array)
+#endif
+
 #undef __print_hex
 #define __print_hex(buf, buf_len) ftrace_print_hex_seq(p, buf, buf_len)
 
kernel/trace/trace_output.c +27 -0
index cf535cc..e37de49 100644
@@ -353,6 +353,33 @@ ftrace_print_symbols_seq(struct trace_seq *p, unsigned long val,
 }
 EXPORT_SYMBOL(ftrace_print_symbols_seq);
 
+#if BITS_PER_LONG == 32
+const char *
+ftrace_print_symbols_seq_u64(struct trace_seq *p, unsigned long long val,
+			 const struct trace_print_flags_u64 *symbol_array)
+{
+	int i;
+	const char *ret = p->buffer + p->len;
+
+	for (i = 0;  symbol_array[i].name; i++) {
+
+		if (val != symbol_array[i].mask)
+			continue;
+
+		trace_seq_puts(p, symbol_array[i].name);
+		break;
+	}
+
+	if (!p->len)
+		trace_seq_printf(p, "0x%llx", val);
+
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+EXPORT_SYMBOL(ftrace_print_symbols_seq_u64);
+#endif
+
 const char *
 ftrace_print_hex_seq(struct trace_seq *p, const unsigned char *buf, int buf_len)
 {
Signature · 1 line
1.7.4.4
author maintainer
#12 lore raw

PATCH 11/12 tracing: Update btrfss tracepoints to use u64 interface

In reply to Steven Rostedt

From: liubo <[email protected]>

To avoid 64->32 truncating WARNING, update btrfs's tracepoints.

---
 include/trace/events/btrfs.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
include/trace/events/btrfs.h +2 -2
index f445cff..4114129 100644
@@ -28,7 +28,7 @@ struct extent_buffer;
 		{ BTRFS_SHARED_DATA_REF_KEY, 	"SHARED_DATA_REF" })
 
 #define __show_root_type(obj)						\
-	__print_symbolic(obj,						\
+	__print_symbolic_u64(obj,					\
 		{ BTRFS_ROOT_TREE_OBJECTID, 	"ROOT_TREE"	},	\
 		{ BTRFS_EXTENT_TREE_OBJECTID, 	"EXTENT_TREE"	},	\
 		{ BTRFS_CHUNK_TREE_OBJECTID, 	"CHUNK_TREE"	},	\
@@ -125,7 +125,7 @@ DEFINE_EVENT(btrfs__inode, btrfs_inode_evict,
 );
 
 #define __show_map_type(type)						\
-	__print_symbolic(type,						\
+	__print_symbolic_u64(type,					\
 		{ EXTENT_MAP_LAST_BYTE, "LAST_BYTE" 	},		\
 		{ EXTENT_MAP_HOLE, 	"HOLE" 		},		\
 		{ EXTENT_MAP_INLINE, 	"INLINE" 	},		\
Signature · 1 line
1.7.4.4
author maintainer
#13 lore raw

PATCH 12/12 ftrace: Add internal recursive checks

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Witold reported a reboot caused by the selftests of the dynamic function tracer. He sent me a config and I used ktest to do a config_bisect on it (as my config did not cause the crash). It pointed out that the problem config was CONFIG_PROVE_RCU.

What happened was that if multiple callbacks are attached to the function tracer, we iterate a list of callbacks. Because the list is managed by synchronize_sched() and preempt_disable, the access to the pointers uses rcu_dereference_raw().

When PROVE_RCU is enabled, the rcu_dereference_raw() calls some debugging functions, which happen to be traced. The tracing of the debug function would then call rcu_dereference_raw() which would then call the debug function and then... well you get the idea.

I first wrote two different patches to solve this bug.

1) add a __rcu_dereference_raw() that would not do any checks.
2) add notrace to the offending debug functions.

Both of these patches worked.

Talking with Paul McKenney on IRC, he suggested to add recursion detection instead. This seemed to be a better solution, so I decided to implement it. As the task_struct already has a trace_recursion to detect recursion in the ring buffer, and that has a very small number it allows, I decided to use that same variable to add flags that can detect the recursion inside the infrastructure of the function tracer.

I plan to change it so that the task struct bit can be checked in mcount, but as that requires changes to all archs, I will hold that off to the next merge window.

---
 include/linux/sched.h      |    2 +-
 kernel/trace/ftrace.c      |   13 ++++++++++++-
 kernel/trace/ring_buffer.c |   10 +++++-----
 kernel/trace/trace.h       |   15 +++++++++++++++
 4 files changed, 33 insertions(+), 7 deletions(-)
include/linux/sched.h +1 -1
index d8b2d0b..7b78d9c 100644
@@ -1513,7 +1513,7 @@ struct task_struct {
 #ifdef CONFIG_TRACING
 	/* state flags for use by tracers */
 	unsigned long trace;
-	/* bitmask of trace recursion */
+	/* bitmask and counter of trace recursion */
 	unsigned long trace_recursion;
 #endif /* CONFIG_TRACING */
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR /* memcg uses this to do batch job */
kernel/trace/ftrace.c +12 -1
index 25949b3..1ee417f 100644
@@ -109,12 +109,18 @@ ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip);
 static void ftrace_global_list_func(unsigned long ip,
 				    unsigned long parent_ip)
 {
-	struct ftrace_ops *op = rcu_dereference_raw(ftrace_global_list); /*see above*/
+	struct ftrace_ops *op;
+
+	if (unlikely(trace_recursion_test(TRACE_GLOBAL_BIT)))
+		return;
 
+	trace_recursion_set(TRACE_GLOBAL_BIT);
+	op = rcu_dereference_raw(ftrace_global_list); /*see above*/
 	while (op != &ftrace_list_end) {
 		op->func(ip, parent_ip);
 		op = rcu_dereference_raw(op->next); /*see above*/
 	};
+	trace_recursion_clear(TRACE_GLOBAL_BIT);
 }
 
 static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
@@ -3490,6 +3496,10 @@ ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
 {
 	struct ftrace_ops *op;
 
+	if (unlikely(trace_recursion_test(TRACE_INTERNAL_BIT)))
+		return;
+
+	trace_recursion_set(TRACE_INTERNAL_BIT);
 	/*
 	 * Some of the ops may be dynamically allocated,
 	 * they must be freed after a synchronize_sched().
@@ -3502,6 +3512,7 @@ ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
 		op = rcu_dereference_raw(op->next);
 	};
 	preempt_enable_notrace();
+	trace_recursion_clear(TRACE_INTERNAL_BIT);
 }
 
 static void clear_ftrace_swapper(void)
kernel/trace/ring_buffer.c +5 -5
index 0ef7b4b..b0c7aa4 100644
@@ -2216,7 +2216,7 @@ static noinline void trace_recursive_fail(void)
 
 	printk_once(KERN_WARNING "Tracing recursion: depth[%ld]:"
 		    "HC[%lu]:SC[%lu]:NMI[%lu]\n",
-		    current->trace_recursion,
+		    trace_recursion_buffer(),
 		    hardirq_count() >> HARDIRQ_SHIFT,
 		    softirq_count() >> SOFTIRQ_SHIFT,
 		    in_nmi());
@@ -2226,9 +2226,9 @@ static noinline void trace_recursive_fail(void)
 
 static inline int trace_recursive_lock(void)
 {
-	current->trace_recursion++;
+	trace_recursion_inc();
 
-	if (likely(current->trace_recursion < TRACE_RECURSIVE_DEPTH))
+	if (likely(trace_recursion_buffer() < TRACE_RECURSIVE_DEPTH))
 		return 0;
 
 	trace_recursive_fail();
@@ -2238,9 +2238,9 @@ static inline int trace_recursive_lock(void)
 
 static inline void trace_recursive_unlock(void)
 {
-	WARN_ON_ONCE(!current->trace_recursion);
+	WARN_ON_ONCE(!trace_recursion_buffer());
 
-	current->trace_recursion--;
+	trace_recursion_dec();
 }
 
 #else
kernel/trace/trace.h +15 -0
index 6b69c4b..229f859 100644
@@ -784,4 +784,19 @@ extern const char *__stop___trace_bprintk_fmt[];
 	FTRACE_ENTRY(call, struct_name, id, PARAMS(tstruct), PARAMS(print))
 #include "trace_entries.h"
 
+/* Only current can touch trace_recursion */
+#define trace_recursion_inc() do { (current)->trace_recursion++; } while (0)
+#define trace_recursion_dec() do { (current)->trace_recursion--; } while (0)
+
+/* Ring buffer has the 10 LSB bits to count */
+#define trace_recursion_buffer() ((current)->trace_recursion & 0x3ff)
+
+/* for function tracing recursion */
+#define TRACE_INTERNAL_BIT		(1<<11)
+#define TRACE_GLOBAL_BIT		(1<<12)
+
+#define trace_recursion_set(bit)	do { (current)->trace_recursion |= (bit); } while (0)
+#define trace_recursion_clear(bit)	do { (current)->trace_recursion &= ~(bit); } while (0)
+#define trace_recursion_test(bit)	((current)->trace_recursion & (bit))
+
 #endif /* _LINUX_KERNEL_TRACE_H */
Signature · 1 line
1.7.4.4
#14 lore raw

Re: [PATCH 12/12] ftrace: Add internal recursive checks

Quoted 6 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Witold reported a reboot caused by the selftests of the dynamic function tracer. He sent me a config and I used ktest to do a config_bisect on it (as my config did not cause the crash). It pointed out that the problem config was CONFIG_PROVE_RCU.

Just disabling PROVE_RCU in my config
make crash disapear, so it is good diagnosis. Good work.

Quoted 25 lines

What happened was that if multiple callbacks are attached to the function tracer, we iterate a list of callbacks. Because the list is managed by synchronize_sched() and preempt_disable, the access to the pointers uses rcu_dereference_raw().

When PROVE_RCU is enabled, the rcu_dereference_raw() calls some debugging functions, which happen to be traced. The tracing of the debug function would then call rcu_dereference_raw() which would then call the debug function and then... well you get the idea.

I first wrote two different patches to solve this bug.

1) add a __rcu_dereference_raw() that would not do any checks.
2) add notrace to the offending debug functions.

Both of these patches worked.

Talking with Paul McKenney on IRC, he suggested to add recursion detection instead. This seemed to be a better solution, so I decided to implement it. As the task_struct already has a trace_recursion to detect recursion in the ring buffer, and that has a very small number it allows, I decided to use that same variable to add flags that can detect the recursion inside the infrastructure of the function tracer.

Quoted 3 lines

I plan to change it so that the task struct bit can be checked in mcount, but as that requires changes to all archs, I will hold that off to the next merge window.

I'm testing this patch now, and do not see any problem right now.

Regards,
Witek

Signature · 1 line
Witold Baryluk
Reviewed-by on v1 12/12
#15 lore raw

Re: [PATCH 12/12] ftrace: Add internal recursive checks

In reply to Steven Rostedt
Quoted 31 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Witold reported a reboot caused by the selftests of the dynamic function tracer. He sent me a config and I used ktest to do a config_bisect on it (as my config did not cause the crash). It pointed out that the problem config was CONFIG_PROVE_RCU.

What happened was that if multiple callbacks are attached to the function tracer, we iterate a list of callbacks. Because the list is managed by synchronize_sched() and preempt_disable, the access to the pointers uses rcu_dereference_raw().

When PROVE_RCU is enabled, the rcu_dereference_raw() calls some debugging functions, which happen to be traced. The tracing of the debug function would then call rcu_dereference_raw() which would then call the debug function and then... well you get the idea.

I first wrote two different patches to solve this bug.

1) add a __rcu_dereference_raw() that would not do any checks.
2) add notrace to the offending debug functions.

Both of these patches worked.

Talking with Paul McKenney on IRC, he suggested to add recursion detection instead. This seemed to be a better solution, so I decided to implement it. As the task_struct already has a trace_recursion to detect recursion in the ring buffer, and that has a very small number it allows, I decided to use that same variable to add flags that can detect the recursion inside the infrastructure of the function tracer.

Quoted 3 lines

I plan to change it so that the task struct bit can be checked in mcount, but as that requires changes to all archs, I will hold that off to the next merge window.

Looks good to me.

Quoted 136 lines
---
 include/linux/sched.h      |    2 +-
 kernel/trace/ftrace.c      |   13 ++++++++++++-
 kernel/trace/ring_buffer.c |   10 +++++-----
 kernel/trace/trace.h       |   15 +++++++++++++++
 4 files changed, 33 insertions(+), 7 deletions(-)
include/linux/sched.h +1 -1
index d8b2d0b..7b78d9c 100644
@@ -1513,7 +1513,7 @@ struct task_struct {
 #ifdef CONFIG_TRACING
 	/* state flags for use by tracers */
 	unsigned long trace;
-	/* bitmask of trace recursion */
+	/* bitmask and counter of trace recursion */
 	unsigned long trace_recursion;
 #endif /* CONFIG_TRACING */
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR /* memcg uses this to do batch job */
kernel/trace/ftrace.c +12 -1
index 25949b3..1ee417f 100644
@@ -109,12 +109,18 @@ ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip);
 static void ftrace_global_list_func(unsigned long ip,
 				    unsigned long parent_ip)
 {
-	struct ftrace_ops *op = rcu_dereference_raw(ftrace_global_list); /*see above*/
+	struct ftrace_ops *op;
+
+	if (unlikely(trace_recursion_test(TRACE_GLOBAL_BIT)))
+		return;
 
+	trace_recursion_set(TRACE_GLOBAL_BIT);
+	op = rcu_dereference_raw(ftrace_global_list); /*see above*/
 	while (op != &ftrace_list_end) {
 		op->func(ip, parent_ip);
 		op = rcu_dereference_raw(op->next); /*see above*/
 	};
+	trace_recursion_clear(TRACE_GLOBAL_BIT);
 }
 
 static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
@@ -3490,6 +3496,10 @@ ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
 {
 	struct ftrace_ops *op;
 
+	if (unlikely(trace_recursion_test(TRACE_INTERNAL_BIT)))
+		return;
+
+	trace_recursion_set(TRACE_INTERNAL_BIT);
 	/*
 	 * Some of the ops may be dynamically allocated,
 	 * they must be freed after a synchronize_sched().
@@ -3502,6 +3512,7 @@ ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
 		op = rcu_dereference_raw(op->next);
 	};
 	preempt_enable_notrace();
+	trace_recursion_clear(TRACE_INTERNAL_BIT);
 }
 
 static void clear_ftrace_swapper(void)
kernel/trace/ring_buffer.c +5 -5
index 0ef7b4b..b0c7aa4 100644
@@ -2216,7 +2216,7 @@ static noinline void trace_recursive_fail(void)
 
 	printk_once(KERN_WARNING "Tracing recursion: depth[%ld]:"
 		    "HC[%lu]:SC[%lu]:NMI[%lu]\n",
-		    current->trace_recursion,
+		    trace_recursion_buffer(),
 		    hardirq_count() >> HARDIRQ_SHIFT,
 		    softirq_count() >> SOFTIRQ_SHIFT,
 		    in_nmi());
@@ -2226,9 +2226,9 @@ static noinline void trace_recursive_fail(void)
 
 static inline int trace_recursive_lock(void)
 {
-	current->trace_recursion++;
+	trace_recursion_inc();
 
-	if (likely(current->trace_recursion < TRACE_RECURSIVE_DEPTH))
+	if (likely(trace_recursion_buffer() < TRACE_RECURSIVE_DEPTH))
 		return 0;
 
 	trace_recursive_fail();
@@ -2238,9 +2238,9 @@ static inline int trace_recursive_lock(void)
 
 static inline void trace_recursive_unlock(void)
 {
-	WARN_ON_ONCE(!current->trace_recursion);
+	WARN_ON_ONCE(!trace_recursion_buffer());
 
-	current->trace_recursion--;
+	trace_recursion_dec();
 }
 
 #else
kernel/trace/trace.h +15 -0
index 6b69c4b..229f859 100644
@@ -784,4 +784,19 @@ extern const char *__stop___trace_bprintk_fmt[];
 	FTRACE_ENTRY(call, struct_name, id, PARAMS(tstruct), PARAMS(print))
 #include "trace_entries.h"
 
+/* Only current can touch trace_recursion */
+#define trace_recursion_inc() do { (current)->trace_recursion++; } while (0)
+#define trace_recursion_dec() do { (current)->trace_recursion--; } while (0)
+
+/* Ring buffer has the 10 LSB bits to count */
+#define trace_recursion_buffer() ((current)->trace_recursion & 0x3ff)
+
+/* for function tracing recursion */
+#define TRACE_INTERNAL_BIT		(1<<11)
+#define TRACE_GLOBAL_BIT		(1<<12)
+
+#define trace_recursion_set(bit)	do { (current)->trace_recursion |= (bit); } while (0)
+#define trace_recursion_clear(bit)	do { (current)->trace_recursion &= ~(bit); } while (0)
+#define trace_recursion_test(bit)	((current)->trace_recursion & (bit))
+
 #endif /* _LINUX_KERNEL_TRACE_H */
Signature · 1 line
1.7.4.4
#16 lore raw
In reply to Steven Rostedt
Quoted 73 lines · Steven Rostedt

Ingo,

This patch set contains all fixes. Some of the fixes are to get rid of warnings, but I think getting rid of warnings is a good thing during a merge window.

What's different about this and my last pull request:

1) I removed the two patches that were not bug fixes and put them in my queue for 2.6.41/3.1/2.8.1

2) I added two patches, one that was a bug fix that caused a triple fault when PROVE_RCU was set, and the other that caused problems with truncating 64bit words in the trace events.

3) I pulled out Masami's kprobe stack trace fix because it failed on other archs. My last push request, I did compile test other archs, but I only used the default config. This time, I enabled tracing as well (doing it both with and without tracing enabled) and when tracing was enabled, I detected the problem with the lack of save_stack_trace_regs() in other archs.

I tested this on x86_64 and x86_32 as well as compiled tested for the following archs, with and without tracing enabled:

alpha, arm, blackfin, ia64, parsic, ppc, ppc64, sparc

The following had failures for other reason (failed without this patch set):

cris, cris32, m68k, mips64, mips32, parsic64, sh, xtensa, uml.

Please pull the latest tip/perf/urgent tree, which can be found at:

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

Jiri Olsa (1):
jump_label: Check entries limit in __jump_label_update

Rabin Vincent (1):
ftrace/recordmcount: Avoid STT_FUNC symbols as base on ARM

Rakib Mullick (1):
x86/ftrace: Fix compiler warning in ftrace.c

Steven Rostedt (7):
maccess,probe_kernel: Make write/read src const void * scripts/tags.sh: Fix ctags for DEFINE_EVENT() scripts/tags.sh: Add magic for trace-events for etags too
ftrace: Have ftrace_startup() return failure code
tracing: Have event with function tracer check error return
ftrace: Set ops->flag to enabled even on static function tracing
ftrace: Add internal recursive checks

liubo (2):
tracing: Add __print_symbolic_u64 to avoid warnings on 32bit machine
tracing: Update btrfs's tracepoints to use u64 interface

----

 arch/blackfin/mm/maccess.c   |    4 ++--
 arch/s390/mm/maccess.c       |    4 ++--
 arch/x86/kernel/ftrace.c     |   12 ++++++------
 include/linux/ftrace_event.h |   12 ++++++++++++
 include/linux/sched.h        |    2 +-
 include/linux/uaccess.h      |    8 ++++----
 include/trace/events/btrfs.h |    4 ++--
 include/trace/ftrace.h       |   13 +++++++++++++
 kernel/jump_label.c          |   18 +++++++++++++-----
 kernel/trace/ftrace.c        |   31 ++++++++++++++++++++++++-------
Quoted 8 lines
 kernel/trace/ring_buffer.c   |   10 +++++-----
 kernel/trace/trace.h         |   15 +++++++++++++++
 kernel/trace/trace_events.c  |    7 ++++++-
 kernel/trace/trace_output.c  |   27 +++++++++++++++++++++++++++
 mm/maccess.c                 |    8 ++++----
 scripts/recordmcount.h       |    8 ++++++++
 scripts/tags.sh              |    6 ++++--
 17 files changed, 148 insertions(+), 41 deletions(-)

Pulled, thanks a lot Steve!

Ingo