tracing: fixes

GIT PULL Superseded Steven Rostedt 5 participants 10 messages Jun 7, 2011 – Jun 9, 2011

Subsystems (2)

Tracing ftrace

lore

Share & tools

lore

Steven Rostedt

author maintainer

2011-06-07 17:08 UTC #0 lore raw

Ingo,

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

GuoWen Li (1):
ftrace: Fix possible undefined return code

Steven Rostedt (2):
kprobes/trace: Fix kprobe selftest for gcc 4.6
ftrace: Revert 8ab2b7efd ftrace: Remove unnecessary disabling of irqs

----

 kernel/trace/ftrace.c       |    9 ++++++++-
 kernel/trace/trace_kprobe.c |    8 ++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

Steven Rostedt

author maintainer

2011-06-07 17:08 UTC #1 lore raw

PATCH 1/3 ftrace: Fix possible undefined return code

From: GuoWen Li <[email protected]>

kernel/trace/ftrace.c: In function 'ftrace_regex_write.clone.15':
kernel/trace/ftrace.c:2743:6: warning: 'ret' may be used uninitialized in this function

Signed-off-by GuoWen Li <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

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

---
 kernel/trace/ftrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/trace/ftrace.c +1 -1

index 1ee417f..204b3eb 100644
@@ -2740,7 +2740,7 @@ static int ftrace_process_regex(struct ftrace_hash *hash,
 {
 	char *func, *command, *next = buff;
 	struct ftrace_func_command *p;
-	int ret;
+	int ret = -EINVAL;
 
 	func = strsep(&next, ":");

Signature · 1 line

1.7.4.4

Steven Rostedt

author maintainer

2011-06-07 17:08 UTC #2 lore raw

PATCH 2/3 kprobes/trace: Fix kprobe selftest for gcc 4.6

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

With gcc 4.6, the self test kprobe function:

kprobe_trace_selftest_target()

is optimized such that kallsyms does not list it. The kprobes test uses this function to insert a probe and test it. But it will fail the test if the function is not listed in kallsyms.

Because the name is rather unique, converting it from static to global should not be a problem. This prevents gcc from removing it from kallsyms.

Cc Masami Hiramatsu <[email protected]>

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

---
 kernel/trace/trace_kprobe.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/trace/trace_kprobe.c +6 -2

index f925c45..2c14378 100644
@@ -1870,8 +1870,12 @@ fs_initcall(init_kprobe_trace);
 
 #ifdef CONFIG_FTRACE_STARTUP_TEST
 
-static int kprobe_trace_selftest_target(int a1, int a2, int a3,
-					int a4, int a5, int a6)
+/*
+ * Can't be static, otherwise gcc might optimize this to
+ * not be in the kallsyms table.
+ */
+int kprobe_trace_selftest_target(int a1, int a2, int a3,
+				 int a4, int a5, int a6)
 {
 	return a1 + a2 + a3 + a4 + a5 + a6;
 }

Signature · 1 line

1.7.4.4

David Daney

2011-06-07 18:08 UTC #3 lore raw

Re: [PATCH 2/3] kprobes/trace: Fix kprobe selftest for gcc 4.6

Quoted 25 lines · Steven Rostedt

From: Steven Rostedt<[email protected]>
With gcc 4.6, the self test kprobe function:
kprobe_trace_selftest_target()
is optimized such that kallsyms does not list it. The kprobes test uses this function to insert a probe and test it. But it will fail the test if the function is not listed in kallsyms.
Because the name is rather unique, converting it from static to global should not be a problem. This prevents gcc from removing it from kallsyms.
Cc Masami Hiramatsu<[email protected]>
Signed-off-by Steven Rostedt<[email protected]>
---
  kernel/trace/trace_kprobe.c |    8 ++++++--
  1 files changed, 6 insertions(+), 2 deletions(-)
kernel/trace/trace_kprobe.c +0 -0
index f925c45..2c14378 100644

kernel/trace/trace_kprobe.c fs_initcall(init_kprobe_trace); · line 1876

Quoted 9 lines

kernel/trace/trace_kprobe.c +4 -2

@@ -1870,8 +1870,12 @@ fs_initcall(init_kprobe_trace);
 
  #ifdef CONFIG_FTRACE_STARTUP_TEST
 
-static int kprobe_trace_selftest_target(int a1, int a2, int a3,
-					int a4, int a5, int a6)
+/*
+ * Can't be static, otherwise gcc might optimize this to
+ * not be in the kallsyms table.
+ */

Could you make it '__used' instead?

David Daney

Quoted 5 lines

kernel/trace/trace_kprobe.c +2 -0

+int kprobe_trace_selftest_target(int a1, int a2, int a3,
+				 int a4, int a5, int a6)
  {
  	return a1 + a2 + a3 + a4 + a5 + a6;
  }

Steven Rostedt

author maintainer

2011-06-07 18:13 UTC #4 lore raw

Re: [PATCH 2/3] kprobes/trace: Fix kprobe selftest for gcc 4.6

Quoted 9 lines · David Daney

kernel/trace/trace_kprobe.c fs_initcall(init_kprobe_trace); · line 1876
Quoted 6 lines
kernel/trace/trace_kprobe.c +4 -2
-static int kprobe_trace_selftest_target(int a1, int a2, int a3,
-					int a4, int a5, int a6)
+/*
+ * Can't be static, otherwise gcc might optimize this to
+ * not be in the kallsyms table.
+ */
Could you make it '__used' instead?

I can try, but the problem is not that the function itself is being optimized out. It looks like its being turned into anonymous text. That is, it optimized out the symbol name, not the code itself.

-- Steve

David Daney

2011-06-07 18:23 UTC #5 lore raw

Re: [PATCH 2/3] kprobes/trace: Fix kprobe selftest for gcc 4.6

Quoted 12 lines · Steven Rostedt

Quoted 9 lines · David Daney
kernel/trace/trace_kprobe.c fs_initcall(init_kprobe_trace); · line 1876
Quoted 6 lines
kernel/trace/trace_kprobe.c +4 -2
-static int kprobe_trace_selftest_target(int a1, int a2, int a3,
-					int a4, int a5, int a6)
+/*
+ * Can't be static, otherwise gcc might optimize this to
+ * not be in the kallsyms table.
+ */
Could you make it '__used' instead?

Quoted 4 lines

I can try, but the problem is not that the function itself is being optimized out. It looks like its being turned into anonymous text. That is, it optimized out the symbol name, not the code itself.

Really it is no big deal either way. Just a thought I had.

David Daney

Steven Rostedt

author maintainer

2011-06-07 18:44 UTC #6 lore raw

Re: [PATCH 2/3] kprobes/trace: Fix kprobe selftest for gcc 4.6

Quoted 13 lines · David Daney

Quoted 12 lines · Steven Rostedt
Quoted 9 lines · David Daney
kernel/trace/trace_kprobe.c fs_initcall(init_kprobe_trace); · line 1876
Quoted 6 lines
kernel/trace/trace_kprobe.c +4 -2
-static int kprobe_trace_selftest_target(int a1, int a2, int a3,
-					int a4, int a5, int a6)
+/*
+ * Can't be static, otherwise gcc might optimize this to
+ * not be in the kallsyms table.
+ */
Could you make it '__used' instead?

Quoted 6 lines

Quoted 4 lines
I can try, but the problem is not that the function itself is being optimized out. It looks like its being turned into anonymous text. That is, it optimized out the symbol name, not the code itself.
Really it is no big deal either way. Just a thought I had.

This actually works! I like it better.

Ingo, can you hold off on pulling, while I rebase to do it David's way? I can add tested-by tags and fix misspellings of my other change logs as well.

Thanks!

-- Steve

Steven Rostedt

author maintainer

2011-06-07 17:08 UTC #7 lore raw

PATCH 3/3 ftrace: Revert 8ab2b7efd ftrace: Remove unnecessary disabling of

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Revert the commit that removed the disabling of interrupts around the initial modifying of mcount callers to nops, and update the comment.

The original comment was outdated and stated that the interrupts were being disabled to prevent kstop machine, which was required with the old ftrace daemon, but was no longer the case.

What the comment failed to mention was that interrupts needed to be disabled to keep interrupts from preempting the modifying of the code and then executing the code that was partially modified.

Revert the commint and update the comment.

Reported-by Richard W.M. Jones <[email protected]>

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

---
 kernel/trace/ftrace.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

kernel/trace/ftrace.c +7 -0

index 204b3eb..908038f 100644
@@ -3330,6 +3330,7 @@ static int ftrace_process_locs(struct module *mod,
 {
 	unsigned long *p;
 	unsigned long addr;
+	unsigned long flags;
 
 	mutex_lock(&ftrace_lock);
 	p = start;

@@ -3346,7 +3347,13 @@ static int ftrace_process_locs(struct module *mod,
 		ftrace_record_ip(addr);
 	}
 
+	/*
+	 * Disable interrupts to prevent interrupts from executing
+	 * code that is being modified.
+	 */
+	local_irq_save(flags);
 	ftrace_update_code(mod);
+	local_irq_restore(flags);
 	mutex_unlock(&ftrace_lock);
 
 	return 0;

Signature · 1 line

1.7.4.4

Richard W.M. Jones

2011-06-07 17:20 UTC #8 lore raw

Re: [PATCH 3/3] ftrace: Revert 8ab2b7efd ftrace: Remove unnecessary disabling of

Quoted 49 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>
Revert the commit that removed the disabling of interrupts around the initial modifying of mcount callers to nops, and update the comment.
The original comment was outdated and stated that the interrupts were being disabled to prevent kstop machine, which was required with the old ftrace daemon, but was no longer the case.
What the comment failed to mention was that interrupts needed to be disabled to keep interrupts from preempting the modifying of the code and then executing the code that was partially modified.
Revert the commint and update the comment.
Reported-by Richard W.M. Jones <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
kernel/trace/ftrace.c +7 -0
index 204b3eb..908038f 100644
@@ -3330,6 +3330,7 @@ static int ftrace_process_locs(struct module *mod,
 {
 	unsigned long *p;
 	unsigned long addr;
+	unsigned long flags;
 
 	mutex_lock(&ftrace_lock);
 	p = start;
@@ -3346,7 +3347,13 @@ static int ftrace_process_locs(struct module *mod,
 		ftrace_record_ip(addr);
 	}
 
+	/*
+	 * Disable interrupts to prevent interrupts from executing
+	 * code that is being modified.
+	 */
+	local_irq_save(flags);
 	ftrace_update_code(mod);
+	local_irq_restore(flags);
 	mutex_unlock(&ftrace_lock);
 
 	return 0;
Signature · 1 line
1.7.4.4

ACK.

I have extensively tested this patch because it was required in order to fix a boot problem with libguestfs on Fedora Rawhide:

https://bugzilla.redhat.com/show_bug.cgi?id=710921

Rich.

Signature · 4 lines

Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

Ingo Molnar

2011-06-09 07:07 UTC #9 lore raw

In reply to Steven Rostedt

Quoted 11 lines · Steven Rostedt

Ingo,
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
GuoWen Li (1):
ftrace: Fix possible undefined return code

Quoted 8 lines

Steven Rostedt (2):
kprobes/trace: Fix kprobe selftest for gcc 4.6
ftrace: Revert 8ab2b7efd ftrace: Remove unnecessary disabling of irqs
----
 kernel/trace/ftrace.c       |    9 ++++++++-
 kernel/trace/trace_kprobe.c |    8 ++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

Pulled, thanks Steve!

Ingo