Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
> +++ b/tools/perf/builtin-lock.c
> @@ -877,6 +877,9 @@ static int read_events(void)
> if (!session)
> die("Initializing perf session failed\n");
>
> + if (!perf_session__has_traces(session, "lock record"))
> + exit(1);
> +
> return perf_session__process_events(session, );
> }
This is getting out of hand, first a die(), then an exit(1) and finally
this function returns a value, ouch.
I'd rather use return to signal that something went wrong and as well
print some helpful warning to the user.
Eventually we should fix all the other offenders, but lets try not to
add even more.
Can you please resend with a pr_warning + return failure?
- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
Em Fri, Jul 06, 2012 at 11:17:41AM -0600, David Ahern escreveu:
> On 7/6/12 11:14 AM, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
> >>+++ b/tools/perf/builtin-lock.c
> >>@@ -877,6 +877,9 @@ static int read_events(void)
> >>if (!session)
> >>die("Initializing perf session failed\n");
> >>
> >>+ if (!perf_session__has_traces(session, "lock record"))
> >>+ exit(1);
> >>+
> >>return perf_session__process_events(session, );
> >> }
> >
> >This is getting out of hand, first a die(), then an exit(1) and finally
> >this function returns a value, ouch.
> >
> >I'd rather use return to signal that something went wrong and as well
> >print some helpful warning to the user.
> >
> >Eventually we should fix all the other offenders, but lets try not to
> >add even more.
>
> Agree. But
>
> >
> >Can you please resend with a pr_warning + return failure?
>
>
> This command needs some love. The rc is not checked and requires
> some rework. e.g., the report path:
>
> setup_pager();
> select_key();
> read_events();< the function I changed
> sort_result();
> print_result();
>
> and the info path:
> setup_pager();
> read_events();
> dump_info();
>
> I figured for 3.5 at least not segfault; clean up for 3.6 or later.
Fair enough, so answering that other question, you want this for
perf/urgent.
Ok, queing it up for that branch,
- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
On 7/6/12 11:14 AM, Arnaldo Carvalho de Melo wrote:
Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
+++ b/tools/perf/builtin-lock.c
@@ -877,6 +877,9 @@ static int read_events(void)
if (!session)
die("Initializing perf session failed\n");
+ if (!perf_session__has_traces(session, "lock record"))
+ exit(1);
+
return perf_session__process_events(session, );
}
This is getting out of hand, first a die(), then an exit(1) and finally
this function returns a value, ouch.
I'd rather use return to signal that something went wrong and as well
print some helpful warning to the user.
Eventually we should fix all the other offenders, but lets try not to
add even more.
Agree. But
Can you please resend with a pr_warning + return failure?
This command needs some love. The rc is not checked and requires some
rework. e.g., the report path:
setup_pager();
select_key();
read_events();< the function I changed
sort_result();
print_result();
and the info path:
setup_pager();
read_events();
dump_info();
I figured for 3.5 at least not segfault; clean up for 3.6 or later.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
Running 'perf lock info' on a "random" perf.data file crashes:
0 0x004a5618 in __parse_common (pevent=0x0, data=0x0, size=0xa0,
offset=0x9c, name=
0x556e02 "common_type") at
/opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:2851
1 0x004a56a7 in trace_parse_common_type (pevent=0x0, data=0x0)
at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:2861
2 0x004a7e42 in pevent_data_type (pevent=0x0, rec=0x7fffddf0)
at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:3952
3 0x00473f37 in trace_parse_common_type (data=0x0) at
util/trace-event-parse.c:158
4 0x004376cd in process_raw_event (data=0x0, cpu=-1,
timestamp=533548521126805, thread=
0x918450) at builtin-lock.c:727
5 0x00437df4 in process_sample_event (tool=0x7ab300,
event=0x77ff9dd8, sample=
0x7fffdf60, evsel=0x917d90, machine=0x9161c0) at builtin-lock.c:863
6 0x00470d04 in perf_session_deliver_event (session=0x916160,
event=0x77ff9dd8, sample=
0x7fffdf60, tool=0x7ab300, file_offset=11736) at util/session.c:977
7 0x0047024a in flush_sample_queue (s=0x916160, tool=0x7ab300) at
util/session.c:679
8 0x00471ba9 in __perf_session__process_events (session=0x916160,
data_offset=400, data_size=
11576, file_size=11976, tool=0x7ab300) at util/session.c:1363
9 0x00471c5e in perf_session__process_events (self=0x916160,
tool=0x7ab300)
at util/session.c:1379
10 0x00437e80 in read_events () at builtin-lock.c:880
11 0x00438275 in cmd_lock (argc=0, argv=0x7fffe420, prefix=0x0) at
builtin-lock.c:10
In this case the data file has cycles events, not traces and hence nothing
to do with lock tracepoints. Nonetheless, perf-lock should not crash. The
crash started with commit aaf045f72335653b24784d6042be8e4aee114403 - ie.,
the move to libtraceevent.
With this patch you get the more user friendly:
$ perf lock info
Fatal: Unknown type of information
Signed-off-by: David Ahern
CC: Arnaldo Carvalho de Melo
Cc: Ingo Molnar
Cc: Steven Rostedt
---
tools/perf/builtin-lock.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index fd53319..55f7961 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -877,6 +877,9 @@ static int read_events(void)
if (!session)
die("Initializing perf session failed\n");
+ if (!perf_session__has_traces(session, "lock record"))
+ exit(1);
+
return perf_session__process_events(session, );
}
--
1.7.10.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
Running 'perf lock info' on a random perf.data file crashes: 0 0x004a5618 in __parse_common (pevent=0x0, data=0x0, size=0xa0, offset=0x9c, name= 0x556e02 common_type) at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:2851 1 0x004a56a7 in trace_parse_common_type (pevent=0x0, data=0x0) at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:2861 2 0x004a7e42 in pevent_data_type (pevent=0x0, rec=0x7fffddf0) at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:3952 3 0x00473f37 in trace_parse_common_type (data=0x0) at util/trace-event-parse.c:158 4 0x004376cd in process_raw_event (data=0x0, cpu=-1, timestamp=533548521126805, thread= 0x918450) at builtin-lock.c:727 5 0x00437df4 in process_sample_event (tool=0x7ab300, event=0x77ff9dd8, sample= 0x7fffdf60, evsel=0x917d90, machine=0x9161c0) at builtin-lock.c:863 6 0x00470d04 in perf_session_deliver_event (session=0x916160, event=0x77ff9dd8, sample= 0x7fffdf60, tool=0x7ab300, file_offset=11736) at util/session.c:977 7 0x0047024a in flush_sample_queue (s=0x916160, tool=0x7ab300) at util/session.c:679 8 0x00471ba9 in __perf_session__process_events (session=0x916160, data_offset=400, data_size= 11576, file_size=11976, tool=0x7ab300) at util/session.c:1363 9 0x00471c5e in perf_session__process_events (self=0x916160, tool=0x7ab300) at util/session.c:1379 10 0x00437e80 in read_events () at builtin-lock.c:880 11 0x00438275 in cmd_lock (argc=0, argv=0x7fffe420, prefix=0x0) at builtin-lock.c:10 In this case the data file has cycles events, not traces and hence nothing to do with lock tracepoints. Nonetheless, perf-lock should not crash. The crash started with commit aaf045f72335653b24784d6042be8e4aee114403 - ie., the move to libtraceevent. With this patch you get the more user friendly: $ perf lock info Fatal: Unknown type of information Signed-off-by: David Ahern dsah...@gmail.com CC: Arnaldo Carvalho de Melo a...@redhat.com Cc: Ingo Molnar mi...@kernel.org Cc: Steven Rostedt rost...@goodmis.org --- tools/perf/builtin-lock.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c index fd53319..55f7961 100644 --- a/tools/perf/builtin-lock.c +++ b/tools/perf/builtin-lock.c @@ -877,6 +877,9 @@ static int read_events(void) if (!session) die(Initializing perf session failed\n); + if (!perf_session__has_traces(session, lock record)) + exit(1); + return perf_session__process_events(session, eops); } -- 1.7.10.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
On 7/6/12 11:14 AM, Arnaldo Carvalho de Melo wrote: Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu: +++ b/tools/perf/builtin-lock.c @@ -877,6 +877,9 @@ static int read_events(void) if (!session) die(Initializing perf session failed\n); + if (!perf_session__has_traces(session, lock record)) + exit(1); + return perf_session__process_events(session, eops); } This is getting out of hand, first a die(), then an exit(1) and finally this function returns a value, ouch. I'd rather use return to signal that something went wrong and as well print some helpful warning to the user. Eventually we should fix all the other offenders, but lets try not to add even more. Agree. But Can you please resend with a pr_warning + return failure? This command needs some love. The rc is not checked and requires some rework. e.g., the report path: setup_pager(); select_key(); read_events(); the function I changed sort_result(); print_result(); and the info path: setup_pager(); read_events(); dump_info(); I figured for 3.5 at least not segfault; clean up for 3.6 or later. David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
Em Fri, Jul 06, 2012 at 11:17:41AM -0600, David Ahern escreveu: On 7/6/12 11:14 AM, Arnaldo Carvalho de Melo wrote: Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu: +++ b/tools/perf/builtin-lock.c @@ -877,6 +877,9 @@ static int read_events(void) if (!session) die(Initializing perf session failed\n); + if (!perf_session__has_traces(session, lock record)) + exit(1); + return perf_session__process_events(session, eops); } This is getting out of hand, first a die(), then an exit(1) and finally this function returns a value, ouch. I'd rather use return to signal that something went wrong and as well print some helpful warning to the user. Eventually we should fix all the other offenders, but lets try not to add even more. Agree. But Can you please resend with a pr_warning + return failure? This command needs some love. The rc is not checked and requires some rework. e.g., the report path: setup_pager(); select_key(); read_events(); the function I changed sort_result(); print_result(); and the info path: setup_pager(); read_events(); dump_info(); I figured for 3.5 at least not segfault; clean up for 3.6 or later. Fair enough, so answering that other question, you want this for perf/urgent. Ok, queing it up for that branch, - Arnaldo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent
Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu: +++ b/tools/perf/builtin-lock.c @@ -877,6 +877,9 @@ static int read_events(void) if (!session) die(Initializing perf session failed\n); + if (!perf_session__has_traces(session, lock record)) + exit(1); + return perf_session__process_events(session, eops); } This is getting out of hand, first a die(), then an exit(1) and finally this function returns a value, ouch. I'd rather use return to signal that something went wrong and as well print some helpful warning to the user. Eventually we should fix all the other offenders, but lets try not to add even more. Can you please resend with a pr_warning + return failure? - Arnaldo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
