2013-10-03 08:35:19

by Ramkumar Ramachandra

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

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 <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: Ramkumar Ramachandra <[email protected]>
---
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


2013-10-03 16:58:46

by Arnaldo Carvalho de Melo

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

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 ~]$


Attachments:
(No filename) (1.93 kB)
tracepoint_error_message.patch (1.59 kB)
Download all attachments

2013-10-03 17:18:54

by David Ahern

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

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

2013-10-03 17:27:41

by Arnaldo Carvalho de Melo

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

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

2013-10-04 04:53:18

by Ramkumar Ramachandra

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

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.