[GIT PULL 0/5] perf/urgent fixes

[GIT PULL 0/5] perf/urgent 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/urgent

Regards,

- Arnaldo

Arnaldo Carvalho de Melo (4):
  perf tools: Make sure kptr_restrict warnings fit 80 col terms
  perf top: Remove unused macro
  perf top: Handle kptr_restrict
  perf top: Don't stop if no kernel symtab is found

David Ahern (1):
  perf events: initialize fd array to -1 instead of 0

 tools/perf/builtin-record.c |   19 ++++++++-----------
 tools/perf/builtin-report.c |   17 +++++++----------
 tools/perf/builtin-top.c    |   37 ++++++++++++++++++++++++++++++-------
 tools/perf/util/evsel.c     |   10 ++++++++++
 4 files changed, 55 insertions(+), 28 deletions(-)

[PATCH 1/5] perf tools: Make sure kptr_restrict warnings fit 80 col terms

[Arnaldo Carvalho de Melo]

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

Suggested-by: Ingo Molnar <mingo@elte.hu>
Cc: Frederic Weisbecker <fweisbec@gmail.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>
Link: http://lkml.kernel.org/n/tip-i1p8vrhq7xveyui6t1sc914e@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c |   19 ++++++++-----------
 tools/perf/builtin-report.c |   17 +++++++----------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2ca107f..8e2c857 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -824,17 +824,14 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 	symbol__init();
 
 	if (symbol_conf.kptr_restrict)
-		pr_warning("WARNING: Kernel address maps "
-			   "(/proc/{kallsyms,modules}) are restricted, "
-			   "check /proc/sys/kernel/kptr_restrict.\n\n"
-			   "Samples in kernel functions may not be resolved "
-			   "if a suitable vmlinux file is not found in the "
-			   "buildid cache or in the vmlinux path.\n\n"
-			   "Samples in kernel modules won't be resolved "
-			   "at all.\n\n"
-			   "If some relocation was applied (e.g. kexec) "
-			   "symbols may be misresolved even with a suitable "
-			   "vmlinux or kallsyms file.\n\n");
+		pr_warning(
+"WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,\n"
+"check /proc/sys/kernel/kptr_restrict.\n\n"
+"Samples in kernel functions may not be resolved if a suitable vmlinux\n"
+"file is not found in the buildid cache or in the vmlinux path.\n\n"
+"Samples in kernel modules won't be resolved at all.\n\n"
+"If some relocation was applied (e.g. kexec) symbols may be misresolved\n"
+"even with a suitable vmlinux or kallsyms file.\n\n");
 
 	if (no_buildid_cache || no_buildid)
 		disable_buildid_cache();
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 99156c3..287a173 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -281,17 +281,14 @@ static int __cmd_report(void)
 	      kernel_kmap->ref_reloc_sym->addr == 0))) {
 		const struct dso *kdso = kernel_map->dso;
 
-		ui__warning("Kernel address maps "
-			    "(/proc/{kallsyms,modules}) were restricted, "
-			    "check /proc/sys/kernel/kptr_restrict before "
-			    "running 'perf record'.\n\n%s\n\n"
-			    "Samples in kernel modules can't be resolved "
-			    "as well.\n\n",
+		ui__warning(
+"Kernel address maps (/proc/{kallsyms,modules}) were restricted.\n\n"
+"Check /proc/sys/kernel/kptr_restrict before running 'perf record'.\n\n%s\n\n"
+"Samples in kernel modules can't be resolved as well.\n\n",
 			    RB_EMPTY_ROOT(&kdso->symbols[MAP__FUNCTION]) ?
-			    "As no suitable kallsyms nor vmlinux was found, "
-			    "kernel samples can't be resolved." :
-			    "If some relocation was applied (e.g. kexec) "
-			    "symbols may be misresolved.");
+"As no suitable kallsyms nor vmlinux was found, kernel samples\n"
+"can't be resolved." :
+"If some relocation was applied (e.g. kexec) symbols may be misresolved.");
 	}
 
 	if (dump_trace) {
-- 
1.6.2.5

[PATCH 2/5] perf events: initialize fd array to -1 instead of 0

[Arnaldo Carvalho de Melo]

From: David Ahern <dsahern@gmail.com>

perf_evsel__alloc_fd allocates an array of file descriptors with the
memory initialized to 0. The array has dimensions for cpus and threads.

Later, __perf_evsel__open calls sys_perf_event_open for each cpu and thread
dimensions. If the open fails for any of the cpus or threads then the fd's
for this event are closed and the fd entry in the array is set to -1. Now,
if the first attempt fails for the event (e.g., the event is not supported)
the remaining dimensions (cpu > 0 and thread > 0) are not touched and left
at the initialized value of 0.

builtin-stat catches ENOENT and ENOSYS failures and allows the command to
continue. The end result is that stat attempts to read from an fd of 0 which
of course is stdin and so the command hangs until you type ctrl-D.

Resolve by initializing the array to -1 since an fd < 0 is already
handled.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1306511914-8016-1-git-send-email-dsahern@gmail.com
Signed-off-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ee0fe0d..cca29ed 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -35,7 +35,17 @@ struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr, int idx)
 
 int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
 {
+	int cpu, thread;
 	evsel->fd = xyarray__new(ncpus, nthreads, sizeof(int));
+
+	if (evsel->fd) {
+		for (cpu = 0; cpu < ncpus; cpu++) {
+			for (thread = 0; thread < nthreads; thread++) {
+				FD(evsel, cpu, thread) = -1;
+			}
+		}
+	}
+
 	return evsel->fd != NULL ? 0 : -ENOMEM;
 }
 
-- 
1.6.2.5

[PATCH 3/5] perf top: Remove unused macro

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.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>
Link: http://lkml.kernel.org/n/tip-weqbs0tkk2u0qp1xxdxxosfg@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 2d7934e..375ed16 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -62,8 +62,6 @@
 #include <linux/unistd.h>
 #include <linux/types.h>
 
-#define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
-
 static struct perf_top top = {
 	.count_filter		= 5,
 	.delay_secs		= 2,
-- 
1.6.2.5

[PATCH 4/5] perf top: Handle kptr_restrict

[Arnaldo Carvalho de Melo]

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

Reported-by: Ingo Molnar <mingo@elte.hu>
Cc: Frederic Weisbecker <fweisbec@gmail.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>
Link: http://lkml.kernel.org/n/tip-cyl5zmi1nu35vyu7l5im2pyv@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 375ed16..472f627 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -80,6 +80,7 @@ static bool			use_tui, use_stdio;
 
 static int			default_interval		=      0;
 
+static bool			kptr_restrict_warned;
 static bool			inherit				=  false;
 static int			realtime_prio			=      0;
 static bool			group				=  false;
@@ -738,6 +739,20 @@ static void perf_event__process_sample(const union perf_event *event,
 	    al.filtered)
 		return;
 
+	if (!kptr_restrict_warned &&
+	    symbol_conf.kptr_restrict &&
+	    al.cpumode == PERF_RECORD_MISC_KERNEL) {
+		ui__warning(
+"Kernel address maps (/proc/{kallsyms,modules}) are restricted.\n\n"
+"Check /proc/sys/kernel/kptr_restrict.\n\n"
+"Kernel%s samples will not be resolved.\n",
+			  !RB_EMPTY_ROOT(&al.map->dso->symbols[MAP__FUNCTION]) ?
+			  " modules" : "");
+		if (use_browser <= 0)
+			sleep(5);
+		kptr_restrict_warned = true;
+	}
+
 	if (al.sym == NULL) {
 		/*
 		 * As we do lazy loading of symtabs we only will know if the
-- 
1.6.2.5

[PATCH 5/5] perf top: Don't stop if no kernel symtab is found

[Arnaldo Carvalho de Melo]

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

We now just warn the user about the fact and go on providing just
userspace samples.

This fixes a problem when no vmlinux is explicetely passed by the user,
thus symbol_conf.vmlinux_name is NULL, no suitable vmlinux is found, and
then we get:

 aldebaran:~> perf top -p 7557
 [kernel.kallsyms] with build id 44d9a989eabbd79e486bc079d6b743d397c204e0
 not found, continuing without symbols
 The (null) file can't be used

Reported-by: Ingo Molnar <mingo@elte.hu>
Cc: Frederic Weisbecker <fweisbec@gmail.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>
Link: http://lkml.kernel.org/n/tip-cj2g81hn64wv2bipmqk4fy2m@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 472f627..f2f3f49 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -81,6 +81,7 @@ static bool			use_tui, use_stdio;
 static int			default_interval		=      0;
 
 static bool			kptr_restrict_warned;
+static bool			vmlinux_warned;
 static bool			inherit				=  false;
 static int			realtime_prio			=      0;
 static bool			group				=  false;
@@ -754,6 +755,7 @@ static void perf_event__process_sample(const union perf_event *event,
 	}
 
 	if (al.sym == NULL) {
+		const char *msg = "Kernel samples will not be resolved.\n";
 		/*
 		 * As we do lazy loading of symtabs we only will know if the
 		 * specified vmlinux file is invalid when we actually have a
@@ -765,12 +767,20 @@ static void perf_event__process_sample(const union perf_event *event,
 		 * --hide-kernel-symbols, even if the user specifies an
 		 * invalid --vmlinux ;-)
 		 */
-		if (al.map == machine->vmlinux_maps[MAP__FUNCTION] &&
+		if (!kptr_restrict_warned && !vmlinux_warned &&
+		    al.map == machine->vmlinux_maps[MAP__FUNCTION] &&
 		    RB_EMPTY_ROOT(&al.map->dso->symbols[MAP__FUNCTION])) {
-			ui__warning("The %s file can't be used\n",
-				    symbol_conf.vmlinux_name);
-			exit_browser(0);
-			exit(1);
+			if (symbol_conf.vmlinux_name) {
+				ui__warning("The %s file can't be used.\n%s",
+					    symbol_conf.vmlinux_name, msg);
+			} else {
+				ui__warning("A vmlinux file was not found.\n%s",
+					    msg);
+			}
+
+			if (use_browser <= 0)
+				sleep(5);
+			vmlinux_warned = true;
 		}
 
 		return;
-- 
1.6.2.5

Re: [GIT PULL 0/5] perf/urgent 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/urgent
> 
> Regards,
> 
> - Arnaldo
> 
> Arnaldo Carvalho de Melo (4):
>   perf tools: Make sure kptr_restrict warnings fit 80 col terms
>   perf top: Remove unused macro
>   perf top: Handle kptr_restrict
>   perf top: Don't stop if no kernel symtab is found
> 
> David Ahern (1):
>   perf events: initialize fd array to -1 instead of 0
> 
>  tools/perf/builtin-record.c |   19 ++++++++-----------
>  tools/perf/builtin-report.c |   17 +++++++----------
>  tools/perf/builtin-top.c    |   37 ++++++++++++++++++++++++++++++-------
>  tools/perf/util/evsel.c     |   10 ++++++++++
>  4 files changed, 55 insertions(+), 28 deletions(-)

Pulled, thanks a lot Arnaldo!

The various variations of kptr_restrict now work fine AFAICS - will 
send the fixes to Linus today.

One (very) small issue i noticed, if the user only samples user-space 
events then i still get the warning:

 aldebaran:~> perf record -e cycles:upp sleep 1
 WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,

although perf does not record any kernel samples so kptr_restrict is 
irrelevant in this case.

	Ingo

Re: [GIT PULL 0/5] perf/urgent fixes

[Arnaldo Carvalho de Melo]

Em Sat, May 28, 2011 at 11:41:50AM +0200, Ingo Molnar escreveu:
> The various variations of kptr_restrict now work fine AFAICS - will 
> send the fixes to Linus today.
> 
> One (very) small issue i noticed, if the user only samples user-space 
> events then i still get the warning:
> 
>  aldebaran:~> perf record -e cycles:upp sleep 1
>  WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,
> 
> although perf does not record any kernel samples so kptr_restrict is 
> irrelevant in this case.

I'll have that fixed too, oversight. We really should avoid confusing
messages, and this one, as you mention, seems just noise as I still have
to look again how vsyscall symbols get resolved, IIRC they need a kernel
symtab, but then, if that is the case, the message should be changed
accordingly.

- Arnaldo

Re: [GIT PULL 0/5] perf/urgent fixes

[Ingo Molnar]

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

> Em Sat, May 28, 2011 at 11:41:50AM +0200, Ingo Molnar escreveu:
> > The various variations of kptr_restrict now work fine AFAICS - will 
> > send the fixes to Linus today.
> > 
> > One (very) small issue i noticed, if the user only samples user-space 
> > events then i still get the warning:
> > 
> >  aldebaran:~> perf record -e cycles:upp sleep 1
> >  WARNING: Kernel address maps (/proc/{kallsyms,modules}) are restricted,
> > 
> > although perf does not record any kernel samples so kptr_restrict is 
> > irrelevant in this case.
> 
> I'll have that fixed too, oversight. We really should avoid 
> confusing messages, and this one, as you mention, seems just noise 
> as I still have to look again how vsyscall symbols get resolved, 
> IIRC they need a kernel symtab, but then, if that is the case, the 
> message should be changed accordingly.

Btw., if you touch that code, would you be interested in adding a 
--user-events kind of option which would have the implicit effect of 
adding :u to every event that is listed (or implied)? (and a 
--kernel-events counterpart, for :k)

The reason is that right now there's no easy way to run say 'perf 
stat -ddd' but measure userspace events only.

[ perf record --repeat would be nice as well :-) And a pony! ]

Thanks,

	Ingo