[PATCH 0/3] [GIT PULL][v3.0] tracing: fixes

[PATCH 0/3] [GIT PULL][v3.0] tracing: fixes

[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

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(-)

[PATCH 1/3] ftrace: Fix possible undefined return code

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-Fix-possible-undefined-return-code.patch --]
[-- Type: text/plain, Size: 847 bytes --]

From: GuoWen Li <guowen.li.linux@gmail.com>

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 <guowen.li.linux@gmail.com>
Link: http://lkml.kernel.org/r/201106011918.47939.guowen.li.linux@gmail.com
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 1ee417f..204b3eb 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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, ":");
 
-- 
1.7.4.4

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

[Steven Rostedt]

[-- Attachment #1: 0002-kprobes-trace-Fix-kprobe-selftest-for-gcc-4.6.patch --]
[-- Type: text/plain, Size: 1269 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

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 <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_kprobe.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index f925c45..2c14378 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -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;
 }
-- 
1.7.4.4

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

[Steven Rostedt]

[-- Attachment #1: 0003-ftrace-Revert-8ab2b7efd-ftrace-Remove-unnecessary-di.patch irqs --]
[-- Type: text/plain, Size: 1488 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

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 <rjones@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 204b3eb..908038f 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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;
-- 
1.7.4.4

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

[Richard W.M. Jones]

On Tue, Jun 07, 2011 at 01:07:43PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> 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 <rjones@redhat.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/ftrace.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 204b3eb..908038f 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -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;
> -- 
> 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.

-- 
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

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

[David Daney]

On 06/07/2011 10:07 AM, Steven Rostedt wrote:
> From: Steven Rostedt<srostedt@redhat.com>
>
> 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<masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Steven Rostedt<rostedt@goodmis.org>
> ---
>   kernel/trace/trace_kprobe.c |    8 ++++++--
>   1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index f925c45..2c14378 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -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


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

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

[Steven Rostedt]

On Tue, 2011-06-07 at 11:08 -0700, David Daney wrote:

> > -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

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

[David Daney]

On 06/07/2011 11:12 AM, Steven Rostedt wrote:
> On Tue, 2011-06-07 at 11:08 -0700, David Daney wrote:
>
>>> -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.
>

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

David Daney

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

[Steven Rostedt]

On Tue, 2011-06-07 at 11:22 -0700, David Daney wrote:
> On 06/07/2011 11:12 AM, Steven Rostedt wrote:
> > On Tue, 2011-06-07 at 11:08 -0700, David Daney wrote:
> >
> >>> -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.
> >
> 
> 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

Re: [PATCH 0/3] [GIT PULL][v3.0] tracing: fixes

[Ingo Molnar]

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

> 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(-)

Pulled, thanks Steve!

	Ingo