[GIT PULL 0/2] perf/core fixes

[GIT PULL 0/2] perf/core fixes

[Arnaldo Carvalho de Melo]

Hi Ingo,

        Please consider pulling from:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6 perf/core

Regards,

- Arnaldo

Arnaldo Carvalho de Melo (2):
  perf probe: Fix error propagation leading to segfault
  perf evsel: Fix inverted test for fixing up attr.inherit flag

 tools/perf/util/evsel.c        |   15 +++++++++++++--
 tools/perf/util/probe-event.c  |    5 ++++-
 tools/perf/util/probe-finder.c |    4 +++-
 3 files changed, 20 insertions(+), 4 deletions(-)

[PATCH 1/2] perf probe: Fix error propagation leading to segfault

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

There are two hunks in this patch that stops probe processing as soon as one
error is found, breaking out of loops, the other fix an error propagation that
should return a negative error number but instead was returning the result of
"ret < 0", which is 1 and thus made several error checks fail because they test
agains < 0.

The problem could be triggered by asking for a variable that was optimized out,
fact that should stop the whole probe processing but instead was segfaulting
while installing broken probes:

[root@emilia ~]# probe perf_mmap:55 user_lock_limit
Failed to find the location of user_lock_limit at this address.
 Perhaps, it has been optimized out.
Failed to find 'user_lock_limit' in this function.
Add new events:
  probe:perf_mmap      (on perf_mmap:55 with user_lock_limit)
  probe:perf_mmap_1    (on perf_mmap:55 with user_lock_limit)
Segmentation fault (core dumped)
[root@emilia ~]# perf probe -l
  probe:perf_mmap      (on perf_mmap:55@git/linux/kernel/perf_event.c with user_lock_limit)
  probe:perf_mmap_1    (on perf_mmap:55@git/linux/kernel/perf_event.c with user_lock_limit)
[root@emilia ~]#

After the fix:

[root@emilia ~]# probe perf_mmap:55 user_lock_limit
Failed to find the location of user_lock_limit at this address.
 Perhaps, it has been optimized out.
Failed to find 'user_lock_limit' in this function.
  Error: Failed to add events. (-2)
[root@emilia ~]#

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-event.c  |    5 ++++-
 tools/perf/util/probe-finder.c |    4 +++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 0e3ea13..369ddc6 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1832,9 +1832,12 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
 	}
 
 	/* Loop 2: add all events */
-	for (i = 0; i < npevs && ret >= 0; i++)
+	for (i = 0; i < npevs && ret >= 0; i++) {
 		ret = __add_probe_trace_events(pkgs[i].pev, pkgs[i].tevs,
 						pkgs[i].ntevs, force_add);
+		if (ret < 0)
+			break;
+	}
 end:
 	/* Loop 3: cleanup and free trace events  */
 	for (i = 0; i < npevs; i++) {
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index fe461f6..eecbdca 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1262,7 +1262,7 @@ static int probe_point_line_walker(const char *fname, int lineno,
 	ret = call_probe_finder(NULL, pf);
 
 	/* Continue if no error, because the line will be in inline function */
-	return ret < 0 ?: 0;
+	return ret < 0 ? ret : 0;
 }
 
 /* Find probe point from its line number */
@@ -1484,6 +1484,8 @@ static int find_probes(int fd, struct probe_finder *pf)
 				pf->lno = pp->line;
 				ret = find_probe_point_by_line(pf);
 			}
+			if (ret != DWARF_CB_OK)
+				break;
 		}
 		off = noff;
 	}
-- 
1.6.2.5

[PATCH 2/2] perf evsel: Fix inverted test for fixing up attr.inherit flag

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The kernel refuses mmapping an event with the inherit flag set for
something that is systemwide (cpu == -1), and the evsel layer got this
reversed at some point, fix it.

The symtom was that the --pid and --tid parameters for 'perf record' and
'perf top' returned with -EINVAL, like:

 # /tmp/build-perf/perf record -v -fo/tmp/perf.data -p 1042
   Warning:  ... trying to fall back to cpu-clock-ticks

   Fatal: failed to mmap with 22 (Invalid argument)

Reported-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 63cadaf..8083d51 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -179,8 +179,19 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 
 	for (cpu = 0; cpu < cpus->nr; cpu++) {
 		int group_fd = -1;
-
-		evsel->attr.inherit = (cpus->map[cpu] < 0) && inherit;
+		/*
+		 * Don't allow mmap() of inherited per-task counters. This
+		 * would create a performance issue due to all children writing
+		 * to the same buffer.
+		 *
+		 * FIXME:
+		 * Proper fix is not to pass 'inherit' to perf_evsel__open*,
+		 * but a 'flags' parameter, with 'group' folded there as well,
+		 * then introduce a PERF_O_{MMAP,GROUP,INHERIT} enum, and if
+		 * O_MMAP is set, emit a warning if cpu < 0 and O_INHERIT is
+		 * set. Lets go for the minimal fix first tho.
+		 */
+		evsel->attr.inherit = (cpus->map[cpu] >= 0) && inherit;
 
 		for (thread = 0; thread < threads->nr; thread++) {
 
-- 
1.6.2.5

Re: [PATCH 1/2] perf probe: Fix error propagation leading to segfault

[Masami Hiramatsu]

(2011/02/22 10:31), Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> There are two hunks in this patch that stops probe processing as soon as one
> error is found, breaking out of loops, the other fix an error propagation that
> should return a negative error number but instead was returning the result of
> "ret < 0", which is 1 and thus made several error checks fail because they test
> agains < 0.
> 
> The problem could be triggered by asking for a variable that was optimized out,
> fact that should stop the whole probe processing but instead was segfaulting
> while installing broken probes:
> 
> [root@emilia ~]# probe perf_mmap:55 user_lock_limit
> Failed to find the location of user_lock_limit at this address.
>  Perhaps, it has been optimized out.
> Failed to find 'user_lock_limit' in this function.
> Add new events:
>   probe:perf_mmap      (on perf_mmap:55 with user_lock_limit)
>   probe:perf_mmap_1    (on perf_mmap:55 with user_lock_limit)
> Segmentation fault (core dumped)
> [root@emilia ~]# perf probe -l
>   probe:perf_mmap      (on perf_mmap:55@git/linux/kernel/perf_event.c with user_lock_limit)
>   probe:perf_mmap_1    (on perf_mmap:55@git/linux/kernel/perf_event.c with user_lock_limit)
> [root@emilia ~]#
> 
> After the fix:
> 
> [root@emilia ~]# probe perf_mmap:55 user_lock_limit
> Failed to find the location of user_lock_limit at this address.
>  Perhaps, it has been optimized out.
> Failed to find 'user_lock_limit' in this function.
>   Error: Failed to add events. (-2)
> [root@emilia ~]#

Oops, thanks! But I've also found this fix including some
redundant checks.

> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: Tom Zanussi <tzanussi@gmail.com>
> LKML-Reference: <new-submission>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/probe-event.c  |    5 ++++-
>  tools/perf/util/probe-finder.c |    4 +++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 0e3ea13..369ddc6 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1832,9 +1832,12 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
>  	}
>  
>  	/* Loop 2: add all events */
> -	for (i = 0; i < npevs && ret >= 0; i++)
> +	for (i = 0; i < npevs && ret >= 0; i++) {
>  		ret = __add_probe_trace_events(pkgs[i].pev, pkgs[i].tevs,
>  						pkgs[i].ntevs, force_add);
> +		if (ret < 0)
> +			break;
> +	}

Hmm, we've already checked ret >= 0 in for().

>  end:
>  	/* Loop 3: cleanup and free trace events  */
>  	for (i = 0; i < npevs; i++) {
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index fe461f6..eecbdca 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1262,7 +1262,7 @@ static int probe_point_line_walker(const char *fname, int lineno,
>  	ret = call_probe_finder(NULL, pf);
>  
>  	/* Continue if no error, because the line will be in inline function */
> -	return ret < 0 ?: 0;
> +	return ret < 0 ? ret : 0;

I think the problem is only here.

>  }
>  
>  /* Find probe point from its line number */
> @@ -1484,6 +1484,8 @@ static int find_probes(int fd, struct probe_finder *pf)
>  				pf->lno = pp->line;
>  				ret = find_probe_point_by_line(pf);
>  			}
> +			if (ret != DWARF_CB_OK)
> +				break;

Actually, we must check that ret < 0 here, and that has been checked
in while().

>  		}
>  		off = noff;
>  	}

Only with the second hunk of the patch, I've checked that is enough to fix
the problem.

$ ./perf probe -vv perf_mmap:55 user_lock_limit
probe-definition(0): perf_mmap:55 user_lock_limit
symbol:perf_mmap file:(null) line:55 offset:0 return:0 lazy:(null)
parsing arg: user_lock_limit into user_lock_limit
1 arguments
Looking at the vmlinux_path (6 entries long)
Using //lib/modules/2.6.38-rc5-tip+/build/vmlinux for symbols
Try to open /lib/modules/2.6.38-rc5-tip+/build/vmlinux
Get 4514 lines from this CU
Probe point found: perf_mmap+352
Searching 'user_lock_limit' variable in context.
Converting variable user_lock_limit into trace event.
user_lock_limit type is long unsigned int.
Probe point found: perf_mmap+359
Searching 'user_lock_limit' variable in context.
Converting variable user_lock_limit into trace event.
user_lock_limit type is long unsigned int.
Probe point found: perf_mmap+322
Searching 'user_lock_limit' variable in context.
Converting variable user_lock_limit into trace event.
Failed to find the location of user_lock_limit at this address.
 Perhaps, it has been optimized out.
Failed to find 'user_lock_limit' in this function.
An error occurred in debuginfo analysis (-2).
  Error: Failed to add events. (-2)


Thank you,

-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [GIT PULL 0/2] perf/core fixes

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@infradead.org> wrote:

> Hi Ingo,
> 
>         Please consider pulling from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6 perf/core
> 
> Regards,
> 
> - Arnaldo
> 
> Arnaldo Carvalho de Melo (2):
>   perf probe: Fix error propagation leading to segfault
>   perf evsel: Fix inverted test for fixing up attr.inherit flag
> 
>  tools/perf/util/evsel.c        |   15 +++++++++++++--
>  tools/perf/util/probe-event.c  |    5 ++++-
>  tools/perf/util/probe-finder.c |    4 +++-
>  3 files changed, 20 insertions(+), 4 deletions(-)

Pulled, thanks a lot Arnaldo!

Thanks,

	Ingo

Re: [PATCH 1/2] perf probe: Fix error propagation leading to segfault

[Arnaldo Carvalho de Melo]

Em Tue, Feb 22, 2011 at 12:20:22PM +0900, Masami Hiramatsu escreveu:
> (2011/02/22 10:31), Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > There are two hunks in this patch that stops probe processing as soon as one
> > error is found, breaking out of loops, the other fix an error propagation that
> > should return a negative error number but instead was returning the result of
> > "ret < 0", which is 1 and thus made several error checks fail because they test
> > agains < 0.
> > 
> > The problem could be triggered by asking for a variable that was optimized out,
> > fact that should stop the whole probe processing but instead was segfaulting
> > while installing broken probes:
> > 
> > [root@emilia ~]# probe perf_mmap:55 user_lock_limit
> > Failed to find the location of user_lock_limit at this address.
> >  Perhaps, it has been optimized out.
> > Failed to find 'user_lock_limit' in this function.
> > Add new events:
> >   probe:perf_mmap      (on perf_mmap:55 with user_lock_limit)
> >   probe:perf_mmap_1    (on perf_mmap:55 with user_lock_limit)
> > Segmentation fault (core dumped)
> > [root@emilia ~]# perf probe -l
> >   probe:perf_mmap      (on perf_mmap:55@git/linux/kernel/perf_event.c with user_lock_limit)
> >   probe:perf_mmap_1    (on perf_mmap:55@git/linux/kernel/perf_event.c with user_lock_limit)
> > [root@emilia ~]#
> > 
> > After the fix:
> > 
> > [root@emilia ~]# probe perf_mmap:55 user_lock_limit
> > Failed to find the location of user_lock_limit at this address.
> >  Perhaps, it has been optimized out.
> > Failed to find 'user_lock_limit' in this function.
> >   Error: Failed to add events. (-2)
> > [root@emilia ~]#
> 
> Oops, thanks! But I've also found this fix including some
> redundant checks.

I'll remove the redundancies as I describe later on this message.

> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Cc: Mike Galbraith <efault@gmx.de>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Stephane Eranian <eranian@google.com>
> > Cc: Tom Zanussi <tzanussi@gmail.com>
> > LKML-Reference: <new-submission>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/util/probe-event.c  |    5 ++++-
> >  tools/perf/util/probe-finder.c |    4 +++-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 0e3ea13..369ddc6 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -1832,9 +1832,12 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
> >  	}
> >  
> >  	/* Loop 2: add all events */
> > -	for (i = 0; i < npevs && ret >= 0; i++)
> > +	for (i = 0; i < npevs && ret >= 0; i++) {
> >  		ret = __add_probe_trace_events(pkgs[i].pev, pkgs[i].tevs,
> >  						pkgs[i].ntevs, force_add);
> > +		if (ret < 0)
> > +			break;
> > +	}
> 
> Hmm, we've already checked ret >= 0 in for().

You see? There is value in sticking to common practices, even being one
line above I automatically looked after the assignment, not before. :-\

For that to work one needs to make sure to have ret initialized to zero
before the loop and do an unneeded test before we start the
__add_probe_trace_events calls.
 
> >  end:
> >  	/* Loop 3: cleanup and free trace events  */
> >  	for (i = 0; i < npevs; i++) {
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index fe461f6..eecbdca 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -1262,7 +1262,7 @@ static int probe_point_line_walker(const char *fname, int lineno,
> >  	ret = call_probe_finder(NULL, pf);
> >  
> >  	/* Continue if no error, because the line will be in inline function */
> > -	return ret < 0 ?: 0;
> > +	return ret < 0 ? ret : 0;
> 
> I think the problem is only here.
> 
> >  }
> >  
> >  /* Find probe point from its line number */
> > @@ -1484,6 +1484,8 @@ static int find_probes(int fd, struct probe_finder *pf)
> >  				pf->lno = pp->line;
> >  				ret = find_probe_point_by_line(pf);
> >  			}
> > +			if (ret != DWARF_CB_OK)
> > +				break;
> 
> Actually, we must check that ret < 0 here, and that has been checked
> in while().

Ok, another instance of the above pet peeve of mine :-)

> >  		}
> >  		off = noff;
> >  	}
> 
> Only with the second hunk of the patch, I've checked that is enough to fix
> the problem.
> 
> $ ./perf probe -vv perf_mmap:55 user_lock_limit
> probe-definition(0): perf_mmap:55 user_lock_limit
> symbol:perf_mmap file:(null) line:55 offset:0 return:0 lazy:(null)
> parsing arg: user_lock_limit into user_lock_limit
> 1 arguments
> Looking at the vmlinux_path (6 entries long)
> Using //lib/modules/2.6.38-rc5-tip+/build/vmlinux for symbols
> Try to open /lib/modules/2.6.38-rc5-tip+/build/vmlinux
> Get 4514 lines from this CU
> Probe point found: perf_mmap+352
> Searching 'user_lock_limit' variable in context.
> Converting variable user_lock_limit into trace event.
> user_lock_limit type is long unsigned int.
> Probe point found: perf_mmap+359
> Searching 'user_lock_limit' variable in context.
> Converting variable user_lock_limit into trace event.
> user_lock_limit type is long unsigned int.
> Probe point found: perf_mmap+322
> Searching 'user_lock_limit' variable in context.
> Converting variable user_lock_limit into trace event.
> Failed to find the location of user_lock_limit at this address.
>  Perhaps, it has been optimized out.
> Failed to find 'user_lock_limit' in this function.
> An error occurred in debuginfo analysis (-2).
>   Error: Failed to add events. (-2)

Thanks for checking,

- Arnaldo