[PATCH] perf tool: more user-friendly errors from trace

[PATCH] perf tool: more user-friendly errors from trace

[Ramkumar Ramachandra]

Currently, execution of 'perf trace' reports the following cryptic
message to the user:

$ perf trace
Couldn't read the raw_syscalls tracepoints information!

Now, it prints a detailed message:

$ perf trace
Error:  No permissions to read $debugfs/tracing/events/raw_syscalls
Hint:   Change the permissions of debugfs: /sys/kernel/debug
        The directory will be present if your kernel was compiled with debugfs support.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/builtin-trace.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 71aa3e3..544707a 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -916,14 +916,18 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 
 	if (perf_evlist__add_newtp(evlist, "raw_syscalls", "sys_enter", trace__sys_enter) ||
 	    perf_evlist__add_newtp(evlist, "raw_syscalls", "sys_exit", trace__sys_exit)) {
-		fprintf(trace->output, "Couldn't read the raw_syscalls tracepoints information!\n");
+		fprintf(trace->output, "Error:\tNo permissions to read $debugfs/tracing/events/raw_syscalls\n");
+		fprintf(trace->output, "Hint:\tChange the permissions of debugfs: /sys/kernel/debug\n");
+		fprintf(trace->output, "\tThe directory will be present if your kernel was compiled with debugfs support.\n");
 		goto out_delete_evlist;
 	}
 
 	if (trace->sched &&
 	    perf_evlist__add_newtp(evlist, "sched", "sched_stat_runtime",
 				   trace__sched_stat_runtime)) {
-		fprintf(trace->output, "Couldn't read the sched_stat_runtime tracepoint information!\n");
+		fprintf(trace->output, "Error:\tNo permissions to read $debugfs/tracing/events/sched/sched_stat_runtime\n");
+		fprintf(trace->output, "Hint:\tChange the permissions of debugfs: /sys/kernel/debug\n");
+		fprintf(trace->output, "\tThe directory will be present if your kernel was compiled with debugfs support.\n");
 		goto out_delete_evlist;
 	}
 
-- 
1.8.4.477.g5d89aa9

Re: [PATCH] perf tool: more user-friendly errors from trace

[Arnaldo Carvalho de Melo]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

Em Thu, Oct 03, 2013 at 01:58:11PM +0530, Ramkumar Ramachandra escreveu:
> Currently, execution of 'perf trace' reports the following cryptic
> message to the user:
> 
> $ perf trace
> Couldn't read the raw_syscalls tracepoints information!
> 
> Now, it prints a detailed message:

What about the one attached instead? It 

[acme@zoo ~]$ mount | grep debugfs
[acme@zoo ~]$ 
[acme@zoo ~]$ perf trace usleep 1
Is debugfs mounted? Try 'sudo mount -t debugfs nodev /sys/kernel/debug'
[acme@zoo ~]$ sudo mkdir /d
[acme@zoo ~]$ sudo mount -t debugfs nodev /d
[acme@zoo ~]$ mount | grep debugfs
nodev on /d type debugfs (rw,relatime)
[acme@zoo ~]$ 
[acme@zoo ~]$ perf trace usleep 1
Couldn't access debugfs. Try 'sudo mount -o remount,mode=755 /d'
[acme@zoo ~]$ sudo mount -o remount,mode=755 /d
[acme@zoo ~]$ perf trace -e mmap usleep 1
     0.956 ( 0.004 ms): mmap(len: 4096, prot: READ|WRITE, flags: PRIVATE|ANONYMOUS, fd: 4294967295) = 0x46dfc000
     0.995 ( 0.004 ms): mmap(len: 125871, prot: READ, flags: PRIVATE, fd: 3                   ) = 0x46ddd000
     1.045 ( 0.005 ms): mmap(addr: 0x3da4800000, len: 2135088, prot: EXEC|READ, flags: PRIVATE|DENYWRITE, fd: 3) = 0xa4800000
     1.068 ( 0.005 ms): mmap(addr: 0x3da4a08000, len: 8192, prot: READ|WRITE, flags: PRIVATE|DENYWRITE|FIXED, fd: 3, off: 32768) = 0xa4a08000
     1.109 ( 0.005 ms): mmap(addr: 0x3d93400000, len: 3896312, prot: EXEC|READ, flags: PRIVATE|DENYWRITE, fd: 3) = 0x93400000
     1.125 ( 0.006 ms): mmap(addr: 0x3d937ad000, len: 24576, prot: READ|WRITE, flags: PRIVATE|DENYWRITE|FIXED, fd: 3, off: 1757184) = 0x937ad000
     1.134 ( 0.004 ms): mmap(addr: 0x3d937b3000, len: 17400, prot: READ|WRITE, flags: PRIVATE|ANONYMOUS|FIXED, fd: 4294967295) = 0x937b3000
     1.147 ( 0.003 ms): mmap(len: 4096, prot: READ|WRITE, flags: PRIVATE|ANONYMOUS, fd: 4294967295) = 0x46ddc000
     1.160 ( 0.003 ms): mmap(len: 8192, prot: READ|WRITE, flags: PRIVATE|ANONYMOUS, fd: 4294967295) = 0x46dda000
[acme@zoo ~]$

[-- Attachment #2: tracepoint_error_message.patch --]
[-- Type: text/plain, Size: 1628 bytes --]

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 1bb8f15..8a35943 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1501,17 +1501,13 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 	}
 
 	if (perf_evlist__add_newtp(evlist, "raw_syscalls", "sys_enter", trace__sys_enter) ||
-	    perf_evlist__add_newtp(evlist, "raw_syscalls", "sys_exit", trace__sys_exit)) {
-		fprintf(trace->output, "Couldn't read the raw_syscalls tracepoints information!\n");
-		goto out_delete_evlist;
-	}
+	    perf_evlist__add_newtp(evlist, "raw_syscalls", "sys_exit", trace__sys_exit))
+		goto out_error_tp;
 
 	if (trace->sched &&
 	    perf_evlist__add_newtp(evlist, "sched", "sched_stat_runtime",
-				   trace__sched_stat_runtime)) {
-		fprintf(trace->output, "Couldn't read the sched_stat_runtime tracepoint information!\n");
-		goto out_delete_evlist;
-	}
+				   trace__sched_stat_runtime))
+		goto out_error_tp;
 
 	err = perf_evlist__create_maps(evlist, &trace->opts.target);
 	if (err < 0) {
@@ -1628,6 +1624,23 @@ out_delete_evlist:
 out:
 	trace->live = false;
 	return err;
+out_error_tp:
+	switch (errno) {
+	case ENOENT:
+		fputs("Is debugfs mounted? Try 'sudo mount -t debugfs nodev /sys/kernel/debug'\n", trace->output);
+		break;
+	case EACCES:
+		fprintf(trace->output,
+			"Couldn't access debugfs. Try 'sudo mount -o remount,mode=755 %s'\n",
+			debugfs_mountpoint);
+		break;
+	default:
+		fprintf(trace->output, "Can't trace: %s!\n", sys_errlist[errno]);
+		break;
+
+	}
+
+	goto out_unmap_evlist;
 }
 
 static int trace__replay(struct trace *trace)

Re: [PATCH] perf tool: more user-friendly errors from trace

[David Ahern]

On 10/3/13 10:58 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 03, 2013 at 01:58:11PM +0530, Ramkumar Ramachandra escreveu:
>> Currently, execution of 'perf trace' reports the following cryptic
>> message to the user:
>>
>> $ perf trace
>> Couldn't read the raw_syscalls tracepoints information!
>>
>> Now, it prints a detailed message:
>
> What about the one attached instead? It

Isnt' strerror(errno) preferred over sys_errlist[errno]?

David

Re: [PATCH] perf tool: more user-friendly errors from trace

[Arnaldo Carvalho de Melo]

Em Thu, Oct 03, 2013 at 11:18:49AM -0600, David Ahern escreveu:
> On 10/3/13 10:58 AM, Arnaldo Carvalho de Melo wrote:
> >What about the one attached instead? It

> Isnt' strerror(errno) preferred over sys_errlist[errno]?

I found it much simpler to index the array, but can change to using
strerror_r (which I think is preferred over strerror, but one needs to
be careful as there are some subtleties there as well...).

Apart from that, do you think using errno for such simple functions is
ok? i.e. just use the mechanism in place, errno is _already_ set up and
reflects exactly what we need to get propagated to those functions
callers.

- ARnaldo

Re: [PATCH] perf tool: more user-friendly errors from trace

[Ramkumar Ramachandra]

Arnaldo Carvalho de Melo wrote:
> [acme@zoo ~]$ mount | grep debugfs
> [acme@zoo ~]$
> [acme@zoo ~]$ perf trace usleep 1
> Is debugfs mounted? Try 'sudo mount -t debugfs nodev /sys/kernel/debug'
> [acme@zoo ~]$ sudo mkdir /d
> [acme@zoo ~]$ sudo mount -t debugfs nodev /d
> [acme@zoo ~]$ mount | grep debugfs
> nodev on /d type debugfs (rw,relatime)
> [acme@zoo ~]$
> [acme@zoo ~]$ perf trace usleep 1
> Couldn't access debugfs. Try 'sudo mount -o remount,mode=755 /d'
> [acme@zoo ~]$ sudo mount -o remount,mode=755 /d

On Arch, I just had to `chmod go+rx /sys/kernel/debug`, and add an
entry to fstab so it works across reboots. Remounting by hand works
too, but it's likely that users are more familiar with simple
filesystem permissions.

> + case ENOENT:
> + fputs("Is debugfs mounted? Try 'sudo mount -t debugfs nodev /sys/kernel/debug'\n", trace->output);

Doesn't an ENOENT indicate that the kernel was not compiled with
debugfs support in the first place? Isn't it systemd's job to check
the kernel for the feature and do the mounting?

> + case EACCES:
> + fprintf(trace->output,
> + "Couldn't access debugfs. Try 'sudo mount -o remount,mode=755 %s'\n",
> + debugfs_mountpoint);

Instead of giving a specific line to execute, I'd prefer if the error
described that the current user does not have permissions to read
$debugfs in words.

Your patch splits the error into two different bits, which I like. But
I prefer the wording in my original patch: so, I'll re-work my patch
and re-submit.

Thanks.