perf tool: pipe-mode fixes
cover perf tool: pipe-mode fixes
Various fixes for perf tool pipe-mode for bugs that arose while trying to make this work:
perf record -o - noploop | perf inject -b | perf annotate -i -
David Carrillo-Cisneros (7):
perf inject: don't proceed if perf_session__process_event fails perf inject: copy events when reordering events in pipe mode perf tool: describe pipe mode in perf.data-file-fomat.txt perf annotate: process attr and build_id records
perf session: don't rely on evlist in pipe mode
perf tool: protect empty evlists
perf tool: do not print missing features in pipe-mode
tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++++++++++++-- tools/perf/builtin-annotate.c | 2 ++ tools/perf/builtin-inject.c | 2 ++ tools/perf/util/evlist.h | 4 ++++ tools/perf/util/header.c | 13 +++++++------ tools/perf/util/ordered-events.c | 3 ++- tools/perf/util/session.c | 17 ++++++++++++++--- 7 files changed, 48 insertions(+), 12 deletions(-)
Signature · 1 line
2.12.2.715.g7642488e1d-goog
PATCH 1/7 perf inject: don't proceed if perf_session__process_event fails
All paths following perf_session__process_event in __cmd_inject are useless if __cmd_inject is to fail, some depend on a correct session->evlist.
First commit to add code that depends on session->evlist without checking error was commmit e558a5bd8b ("perf inject: Work with files"). It has grown since then.
Change __cmd_inject to fail immediately after perf_session__process_event fails.
--- tools/perf/builtin-inject.c | 2 ++ 1 file changed, 2 insertions(+)
tools/perf/builtin-inject.c +2 -0
@@ -694,6 +694,8 @@ static int __cmd_inject(struct perf_inject *inject)lseek (fd, output_data_offset,SEEK_SET ); ret =perf_session__process_events (session); +if (ret) +return ret;if (!file_out->is_pipe) {if (inject->build_ids)
Signature · 1 line
2.12.2.715.g7642488e1d-goog
tip-bot for David Carrillo-Cisneros
merge bot[tip:perf/core] perf inject: Don't proceed if perf_session__process_event() fails
perf inject: Don't proceed if perf_session__process_event() fails
Commit-ID: bb8d521f77f3e68a713456b7fb1e99f52ff3342c Gitweb: http://git.kernel.org/tip/bb8d521f77f3e68a713456b7fb1e99f52ff3342c Author: David Carrillo-Cisneros <[email protected]> AuthorDate: Mon, 10 Apr 2017 13:14:26 -0700 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 11 Apr 2017 15:23:40 -0300
perf inject: Don't proceed if perf_session__process_event() fails
All paths following perf_session__process_event() in __cmd_inject() are useless if __cmd_inject() is to fail, some depend on a correct session->evlist.
First commit to add code that depends on session->evlist without checking error was commmit e558a5bd8b ("perf inject: Work with files"). It has grown since then.
Change __cmd_inject() to fail immediately afterperf_session__process_event() fails.
--- tools/perf/builtin-inject.c | 2 ++ 1 file changed, 2 insertions(+)
tools/perf/builtin-inject.c +2 -0
@@ -694,6 +694,8 @@ static int __cmd_inject(struct perf_inject *inject)lseek (fd, output_data_offset,SEEK_SET ); ret =perf_session__process_events (session); +if (ret) +return ret;if (!file_out->is_pipe) {if (inject->build_ids)
PATCH 2/7 perf inject: copy events when reordering events in pipe mode
In reply to David Carrillo-Cisneros__perf_session__process_pipe_events reuses the same memory buffer to process all events in the pipe.
When reordering is needed (e.g. -b option), events are not immediately flushed, but kept around until reordering is possible, causing memory corruption.
The problem is usually observed by a "Unknown sample error" output. It can easily be reproduced by:
perf record -o - noploop | perf inject -b > output
--- tools/perf/util/ordered-events.c | 3 ++- tools/perf/util/session.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
tools/perf/util/ordered-events.c +2 -1
@@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,static void free_dup_event (struct ordered_events *oe,union perf_event *event) { -if (oe->copy_on_queue) { +if (event && oe->copy_on_queue) { oe->cur_alloc_size -= event->header.size;free (event); }
@@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *evelist_move (&event->list, &oe->cache); oe->nr_events--;free_dup_event (oe, event->event); + event->event =NULL ; } int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
tools/perf/util/session.c +1 -0
@@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session) buf =malloc (cur_size);if (!buf)return -errno; +ordered_events__set_copy_on_queue (oe, true); more: event = buf; err =readn (fd, event,sizeof (struct perf_event_header ));
Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [PATCH 2/7] perf inject: copy events when reordering events in pipe mode
Quoted 32 lines · David Carrillo-Cisneros
__perf_session__process_pipe_eventsreuses the same memory buffer to process all events in the pipe.When reordering is needed (e.g. -b option), events are not immediately flushed, but kept around until reordering is possible, causing memory corruption.
The problem is usually observed by a "Unknown sample error" output. It can easily be reproduced by:
perf record -o - noploop | perf inject -b > output
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/ordered-events.c | 3 ++- tools/perf/util/session.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)tools/perf/util/ordered-events.c +1
-1@@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,static void free_dup_event (struct ordered_events *oe,union perf_event *event) { -if (oe->copy_on_queue) { +if (event && oe->copy_on_queue) { oe->cur_alloc_size -= event->header.size;free (event); }
tools/perf/util/ordered-events.c void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve · line 153
Quoted 4 lines
tools/perf/util/ordered-events.c +1
-0@@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *evelist_move (&event->list, &oe->cache); oe->nr_events--;free_dup_event (oe, event->event); + event->event =NULL ;
based on the changelog I understand the need for the change below, but I dont get why do we need this cleanu and check above
thanks,
jirka
Quoted 18 lines
tools/perf/util/ordered-events.c +0
-0} int ordered_events__queue(struct ordered_events *oe, union perf_event *event,tools/perf/util/session.c +1
-0@@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session) buf =malloc (cur_size);if (!buf)return -errno; +ordered_events__set_copy_on_queue (oe, true); more: event = buf; err =readn (fd, event,sizeof (struct perf_event_header ));Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [PATCH 2/7] perf inject: copy events when reordering events in pipe mode
free_dup_event is called in two places
1) ordered_events__free
2) ordered_events__delete
without this patch, calling 1) after 2) would cause a double de-allocation, but that's exactly what happens in __perf_session__process_pipe_events. It wasn't triggered before because events where not copied when queued.
With this patch, the event->event = NULL in ordered_events__delete will clear event pointer that is checked in free_dup_event, avoiding de-allocating the event a second time.
Thanks,
David
Quoted 62 lines · Jiri Olsa
Quoted 32 lines · David Carrillo-Cisneros
__perf_session__process_pipe_eventsreuses the same memory buffer to process all events in the pipe.When reordering is needed (e.g. -b option), events are not immediately flushed, but kept around until reordering is possible, causing memory corruption.
The problem is usually observed by a "Unknown sample error" output. It can easily be reproduced by:
perf record -o - noploop | perf inject -b > output
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/ordered-events.c | 3 ++- tools/perf/util/session.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)tools/perf/util/ordered-events.c +1
-1@@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,static void free_dup_event (struct ordered_events *oe,union perf_event *event) { -if (oe->copy_on_queue) { +if (event && oe->copy_on_queue) { oe->cur_alloc_size -= event->header.size;free (event); }tools/perf/util/ordered-events.c void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve · line 153
Quoted 4 lines
tools/perf/util/ordered-events.c +1
-0@@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *evelist_move (&event->list, &oe->cache); oe->nr_events--;free_dup_event (oe, event->event); + event->event =NULL ;based on the changelog I understand the need for the change below, but I dont get why do we need this cleanu and check above
thanks,
jirkaQuoted 18 lines
tools/perf/util/ordered-events.c +0
-0} int ordered_events__queue(struct ordered_events *oe, union perf_event *event,tools/perf/util/session.c +1
-1@@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session) buf =malloc (cur_size);if (!buf)return -errno; +ordered_events__set_copy_on_queue (oe, true); more: event = buf; err =readn (fd, event,sizeof (struct perf_event_header )); --2.12.2.715.g7642488e1d-goog
tip-bot for David Carrillo-Cisneros
merge bot[tip:perf/core] perf inject: Copy events when reordering events in pipe mode
In reply to David Carrillo-Cisnerosperf inject: Copy events when reordering events in pipe mode
Commit-ID: 1e0d4f0200e4dbdfc38d818f329d8a0955f7c6f5 Gitweb: http://git.kernel.org/tip/1e0d4f0200e4dbdfc38d818f329d8a0955f7c6f5 Author: David Carrillo-Cisneros <[email protected]> AuthorDate: Mon, 10 Apr 2017 13:14:27 -0700 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 11 Apr 2017 15:23:41 -0300
perf inject: Copy events when reordering events in pipe mode
__perf_session__process_pipe_events reuses the same memory buffer to process all events in the pipe.
When reordering is needed (e.g. -b option), events are not immediately flushed, but kept around until reordering is possible, causing memory corruption.
The problem is usually observed by a "Unknown sample error" output. It can easily be reproduced by:
perf record -o - noploop | perf inject -b > output
Committer testing:
Before:
$ perf record -o - stress -t 2 -c 2 | perf inject -b > /dev/null stress: info: [8297] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd stress: info: [8297] successful run completed in 2s [ perf record: Woken up 3 times to write data ] [ perf record: Captured and wrote 0.000 MB - ] Warning: Found 1 unknown events!
Is this an older tool processing a perf.data file generated by a more recent tool?
If that is not the case, consider reporting to [email protected].
$
After:
$ perf record -o - stress -t 2 -c 2 | perf inject -b > /dev/null stress: info: [9027] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd stress: info: [9027] successful run completed in 2s [ perf record: Woken up 3 times to write data ] [ perf record: Captured and wrote 0.000 MB - ] no symbols found in /usr/bin/stress, maybe install a debug package? no symbols found in /usr/bin/stress, maybe install a debug package? $
--- tools/perf/util/ordered-events.c | 3 ++- tools/perf/util/session.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
tools/perf/util/ordered-events.c +2 -1
@@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,static void free_dup_event (struct ordered_events *oe,union perf_event *event) { -if (oe->copy_on_queue) { +if (event && oe->copy_on_queue) { oe->cur_alloc_size -= event->header.size;free (event); }
@@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *evelist_move (&event->list, &oe->cache); oe->nr_events--;free_dup_event (oe, event->event); + event->event =NULL ; } int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
tools/perf/util/session.c +1 -0
@@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session) buf =malloc (cur_size);if (!buf)return -errno; +ordered_events__set_copy_on_queue (oe, true); more: event = buf; err =readn (fd, event,sizeof (struct perf_event_header ));
PATCH 3/7 perf tool: describe pipe mode in perf.data-file-fomat.txt
In reply to David Carrillo-CisnerosAdd a minimal description of pipe's data format.
--- tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
tools/perf/Documentation/perf.data-file-format.txt +17 -2
@@ -11,8 +11,8 @@ All fields are in native-endian of the machine that generated the perf.data. When perf is writing to a pipe it uses a special version of the file format that does not rely on seeking to adjust data offsets. This -format is not described here. The pipe version can be converted to -normal perf.data with perf inject. +format is described in "Pipe-mode data" section. The pipe data version can be +augmented with additional events using perf inject. The file starts with a perf_header:
@@ -411,6 +411,21 @@ An array bound by the perf_file_section size. ids points to a array of uint64_t defining the ids for event attr attr. +Pipe-mode data + +Pipe-mode avoid seeks in the file by removing the perf_file_section and flags +from the struct perf_header. The trimmed header is: + +struct perf_pipe_file_header { + u64 magic; + u64 size; +}; + +The information about attrs, data, and event_types is instead in the +synthesized events PERF_RECORD_ATTR, PERF_RECORD_HEADER_TRACING_DATA and +PERF_RECORD_HEADER_EVENT_TYPE that are generated by perf record in pipe-mode. + + References: include/uapi/linux/perf_event.h
Signature · 1 line
2.12.2.715.g7642488e1d-goog
tip-bot for David Carrillo-Cisneros
merge bot[tip:perf/core] perf tools: Describe pipe mode in perf.data-file-fomat.txt
perf tools: Describe pipe mode in perf.data-file-fomat.txt
Commit-ID: 6d13491e2d4944180c9b4fb6ddca4e34b1537836 Gitweb: http://git.kernel.org/tip/6d13491e2d4944180c9b4fb6ddca4e34b1537836 Author: David Carrillo-Cisneros <[email protected]> AuthorDate: Mon, 10 Apr 2017 13:14:28 -0700 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 11 Apr 2017 15:23:41 -0300
perf tools: Describe pipe mode in perf.data-file-fomat.txt
Add a minimal description of pipe's data format.
--- tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
tools/perf/Documentation/perf.data-file-format.txt +17 -2
@@ -11,8 +11,8 @@ All fields are in native-endian of the machine that generated the perf.data. When perf is writing to a pipe it uses a special version of the file format that does not rely on seeking to adjust data offsets. This -format is not described here. The pipe version can be converted to -normal perf.data with perf inject. +format is described in "Pipe-mode data" section. The pipe data version can be +augmented with additional events using perf inject. The file starts with a perf_header:
@@ -411,6 +411,21 @@ An array bound by the perf_file_section size. ids points to a array of uint64_t defining the ids for event attr attr. +Pipe-mode data + +Pipe-mode avoid seeks in the file by removing the perf_file_section and flags +from the struct perf_header. The trimmed header is: + +struct perf_pipe_file_header { + u64 magic; + u64 size; +}; + +The information about attrs, data, and event_types is instead in the +synthesized events PERF_RECORD_ATTR, PERF_RECORD_HEADER_TRACING_DATA and +PERF_RECORD_HEADER_EVENT_TYPE that are generated by perf record in pipe-mode. + + References: include/uapi/linux/perf_event.h
PATCH 4/7 perf annotate: process attr and build_id records
In reply to David Carrillo-Cisnerosperf annotate did not get some love for pipe-mode, and did not have .attr and .buil_id setup (while record and inject did. Fix that.
It can easily be reproduced by:
perf record -o - noploop | perf annotate
that in my system shows:
0xd8 [0x28]: failed to process type: 9
--- tools/perf/builtin-annotate.c | 2 ++ 1 file changed, 2 insertions(+)
tools/perf/builtin-annotate.c +2 -0
@@ -394,6 +394,8 @@ int cmd_annotate(int argc, const char **argv) .exit = perf_event__process_exit, .fork = perf_event__process_fork, .namespaces = perf_event__process_namespaces, + .attr = perf_event__process_attr, + .build_id = perf_event__process_build_id, .ordered_events = true, .ordering_requires_timestamps = true, },
Signature · 1 line
2.12.2.715.g7642488e1d-goog
tip-bot for David Carrillo-Cisneros
merge bot[tip:perf/core] perf annotate: Process attr and build_id records
perf annotate: Process attr and build_id records
Commit-ID: 6ab11f3a35aa07be2ff167b9de37e6c1eb58396b Gitweb: http://git.kernel.org/tip/6ab11f3a35aa07be2ff167b9de37e6c1eb58396b Author: David Carrillo-Cisneros <[email protected]> AuthorDate: Mon, 10 Apr 2017 13:14:29 -0700 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 11 Apr 2017 15:23:42 -0300
perf annotate: Process attr and build_id records
perf annotate did not get some love for pipe-mode, and did not have .attr and .buil_id setup (while record and inject did. Fix that.
It can easily be reproduced by:
perf record -o - noploop | perf annotate
that in my system shows:
0xd8 [0x28]: failed to process type: 9
Committer Testing:
Before:
$ perf record -o - stress -t 2 -c 2 | perf annotate --stdio stress: info: [11060] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd 0x4470 [0x28]: failed to process type: 9 $ stress: info: [11060] successful run completed in 2s
$
After:
$ perf record -o - stress -t 2 -c 2 | perf annotate --stdio
stress: info: [11871] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
stress: info: [11871] successful run completed in 2s
[ perf record: Woken up 2 times to write data ]
[ perf record: Captured and wrote 0.000 MB - ]
no symbols found in /usr/bin/stress, maybe install a debug package?
Percent | Source code & Disassembly of libc-2.24.so for cycles:uhH (6117 samples)
---------------------------------------------------------------------------------------
:
: Disassembly of section .text:
:
: 000000000003b050 <random_r>:
: __random_r():
10.56 : 3b050: test %rdi,%rdi
0.00 : 3b053: je 3b0d0 <random_r+0x80>
0.34 : 3b055: test %rsi,%rsi
0.00 : 3b058: je 3b0d0 <random_r+0x80>
0.46 : 3b05a: mov 0x18(%rdi),%eax
12.44 : 3b05d: mov 0x10(%rdi),%r8
0.18 : 3b061: test %eax,%eax
0.00 : 3b063: je 3b0b0 <random_r+0x60>
<SNIP>--- tools/perf/builtin-annotate.c | 2 ++ 1 file changed, 2 insertions(+)
tools/perf/builtin-annotate.c +2 -0
@@ -394,6 +394,8 @@ int cmd_annotate(int argc, const char **argv) .exit = perf_event__process_exit, .fork = perf_event__process_fork, .namespaces = perf_event__process_namespaces, + .attr = perf_event__process_attr, + .build_id = perf_event__process_build_id, .ordered_events = true, .ordering_requires_timestamps = true, },
PATCH 5/7 perf session: don't rely on evlist in pipe mode
In reply to David Carrillo-CisnerosSession sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
--- tools/perf/util/session.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
tools/perf/util/session.c +13 -3
@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,if (perf_session__open (session) <0 ) goto out_close; -perf_session__set_id_hdr_size (session); -perf_session__set_comm_exec (session); +/* +* set session attributes that are present in perf.data +* but not in pipe-mode. +*/ +if (!file->is_pipe) { +perf_session__set_id_hdr_size (session); +perf_session__set_comm_exec (session); + } } }else { session->machines.host.env = &perf_env;
@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,pr_warning ("Cannot read kernel map\n" ); } -if (tool && tool->ordering_requires_timestamps && +/* +* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is +* processed, so perf_evlist__sample_id_all is not meaningful here. +*/ +if (!file->is_pipe && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false;
Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [PATCH 5/7] perf session: don't rely on evlist in pipe mode
Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
Quoted 3 lines
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
Ditching this one as well, until further investigation, well, file must be null in this case...
[acme@jouet linux]$ git bisect bad 5d71c27204d51492abe9fc69064fd36a0bda410b is the first bad commit commit 5d71c27204d51492abe9fc69064fd36a0bda410b
Author: David Carrillo-Cisneros <[email protected]> Date: Mon Apr 10 13:14:30 2017 -0700
perf session: Don't rely on evlist in pipe mode
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
:040000 040000 7df28fe9d7c9d962b78c0532f4961caea2737b59 34e4fed0042b1401c1a1a247d8ae40df17f9d272 M tools
[acme@jouet linux]$
Quoted 41 lines
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/session.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)tools/perf/util/session.c +13
-3@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,if (perf_session__open (session) <0 ) goto out_close; -perf_session__set_id_hdr_size (session); -perf_session__set_comm_exec (session); +/* +* set session attributes that are present in perf.data +* but not in pipe-mode. +*/ +if (!file->is_pipe) { +perf_session__set_id_hdr_size (session); +perf_session__set_comm_exec (session); + } } }else { session->machines.host.env = &perf_env;@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,pr_warning ("Cannot read kernel map\n" ); } -if (tool && tool->ordering_requires_timestamps && +/* +* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is +* processed, so perf_evlist__sample_id_all is not meaningful here. +*/ +if (!file->is_pipe && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false;Signature · 1 line
2.12.2.715.g7642488e1d-goog
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 5/7] perf session: don't rely on evlist in pipe mode
Em Tue, Apr 11, 2017 at 03:40:14PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 7 lines
Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
Quoted 3 lines
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
Ditching this one as well, until further investigation, well, file must be null in this case...
Ah, this is the problem:
[root@jouet ~]# perf top
perf: Segmentation fault
-------- backtrace --------
perf[0x57eddb] /lib64/libc.so.6(+0x35990)[0x7fb435d06990] perf(perf_session__new+0xb7)[0x4eb547] perf(cmd_top+0xde9)[0x44ce89]
perf[0x49953f]
perf[0x499831]
perf(main+0x252)[0x428dd2] /lib64/libc.so.6(__libc_start_main+0xf1)[0x7fb435cf1401] perf(_start+0x2a)[0x4292da]
[0x0]
[root@jouet ~]#
Quoted 70 lines
[acme@jouet linux]$ git bisect bad
5d71c27204d51492abe9fc69064fd36a0bda410bis the first bad commit commit5d71c27204d51492abe9fc69064fd36a0bda410bAuthor: David Carrillo-Cisneros <[email protected]> Date: Mon Apr 10 13:14:30 2017 -0700perf session: Don't rely on evlist in pipe mode
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
Signed-off-by David Carrillo-Cisneros <[email protected]>Acked-by Jiri Olsa <[email protected]>Cc Alexander Shishkin <[email protected]>Cc Andi Kleen <[email protected]>Cc He Kuang <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Paul Turner <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Simon Que <[email protected]>Cc Stephane Eranian <[email protected]>Cc Wang Nan <[email protected]>Signed-off-by Arnaldo Carvalho de Melo <[email protected]>:040000 040000
7df28fe9d7c9d962b78c0532f4961caea2737b5934e4fed0042b1401c1a1a247d8ae40df17f9d272M tools
[acme@jouet linux]$Quoted 41 lines
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/session.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)tools/perf/util/session.c +13
-3@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,if (perf_session__open (session) <0 ) goto out_close; -perf_session__set_id_hdr_size (session); -perf_session__set_comm_exec (session); +/* +* set session attributes that are present in perf.data +* but not in pipe-mode. +*/ +if (!file->is_pipe) { +perf_session__set_id_hdr_size (session); +perf_session__set_comm_exec (session); + } } }else { session->machines.host.env = &perf_env;@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,pr_warning ("Cannot read kernel map\n" ); } -if (tool && tool->ordering_requires_timestamps && +/* +* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is +* processed, so perf_evlist__sample_id_all is not meaningful here. +*/ +if (!file->is_pipe && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false;Signature · 1 line
2.12.2.715.g7642488e1d-goog
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 5/7] perf session: don't rely on evlist in pipe mode
Em Tue, Apr 11, 2017 at 03:40:50PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 8 lines
Em Tue, Apr 11, 2017 at 03:40:14PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 7 lines
Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
Quoted 3 lines
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
Ditching this one as well, until further investigation, well, file must be null in this case...
This should be enough, right?
tools/perf/util/session.c +2 -2
@@ -144,7 +144,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,* set session attributes that are present in perf.data * but not in pipe-mode. */ -if (!file->is_pipe) { +if (!file || !file->is_pipe) {perf_session__set_id_hdr_size (session);perf_session__set_comm_exec (session); }
@@ -166,7 +166,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is * processed, so perf_evlist__sample_id_all is not meaningful here. */ -if (!file->is_pipe && tool && tool->ordering_requires_timestamps && +if ((!file || !file->is_pipe) && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false;
Quoted 87 lines
Ah, this is the problem:
[root@jouet ~]# perf top
perf: Segmentation fault
-------- backtrace --------
perf[0x57eddb] /lib64/libc.so.6(+0x35990)[0x7fb435d06990] perf(perf_session__new+0xb7)[0x4eb547] perf(cmd_top+0xde9)[0x44ce89]
perf[0x49953f]
perf[0x499831]
perf(main+0x252)[0x428dd2] /lib64/libc.so.6(__libc_start_main+0xf1)[0x7fb435cf1401] perf(_start+0x2a)[0x4292da]
[0x0]
[root@jouet ~]#Quoted 70 lines
[acme@jouet linux]$ git bisect bad
5d71c27204d51492abe9fc69064fd36a0bda410bis the first bad commit commit5d71c27204d51492abe9fc69064fd36a0bda410bAuthor: David Carrillo-Cisneros <[email protected]> Date: Mon Apr 10 13:14:30 2017 -0700perf session: Don't rely on evlist in pipe mode
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
Signed-off-by David Carrillo-Cisneros <[email protected]>Acked-by Jiri Olsa <[email protected]>Cc Alexander Shishkin <[email protected]>Cc Andi Kleen <[email protected]>Cc He Kuang <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Paul Turner <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Simon Que <[email protected]>Cc Stephane Eranian <[email protected]>Cc Wang Nan <[email protected]>Signed-off-by Arnaldo Carvalho de Melo <[email protected]>:040000 040000
7df28fe9d7c9d962b78c0532f4961caea2737b5934e4fed0042b1401c1a1a247d8ae40df17f9d272M tools
[acme@jouet linux]$Quoted 41 lines
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/session.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)tools/perf/util/session.c +13
-3@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,if (perf_session__open (session) <0 ) goto out_close; -perf_session__set_id_hdr_size (session); -perf_session__set_comm_exec (session); +/* +* set session attributes that are present in perf.data +* but not in pipe-mode. +*/ +if (!file->is_pipe) { +perf_session__set_id_hdr_size (session); +perf_session__set_comm_exec (session); + } } }else { session->machines.host.env = &perf_env;@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,pr_warning ("Cannot read kernel map\n" ); } -if (tool && tool->ordering_requires_timestamps && +/* +* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is +* processed, so perf_evlist__sample_id_all is not meaningful here. +*/ +if (!file->is_pipe && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false;Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [PATCH 5/7] perf session: don't rely on evlist in pipe mode
Only the second one is needed. file is already checked for the first one.
Quoted 122 lines · Arnaldo Carvalho de Melo
Em Tue, Apr 11, 2017 at 03:40:50PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 8 lines
Em Tue, Apr 11, 2017 at 03:40:14PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 7 lines
Em Mon, Apr 10, 2017 at 01:14:30PM -0700, David Carrillo-Cisneros escreveu:
Quoted 3 lines
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
Ditching this one as well, until further investigation, well, file must be null in this case...
This should be enough, right?
tools/perf/util/session.c +2
-2@@ -144,7 +144,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,* set session attributes that are present in perf.data * but not in pipe-mode. */ -if (!file->is_pipe) { +if (!file || !file->is_pipe) {perf_session__set_id_hdr_size (session);perf_session__set_comm_exec (session); }@@ -166,7 +166,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is * processed, so perf_evlist__sample_id_all is not meaningful here. */ -if (!file->is_pipe && tool && tool->ordering_requires_timestamps && +if ((!file || !file->is_pipe) && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false;Quoted 88 lines
Ah, this is the problem:
[root@jouet ~]# perf top
perf: Segmentation fault
-------- backtrace --------
perf[0x57eddb] /lib64/libc.so.6(+0x35990)[0x7fb435d06990] perf(perf_session__new+0xb7)[0x4eb547] perf(cmd_top+0xde9)[0x44ce89]
perf[0x49953f]
perf[0x499831]
perf(main+0x252)[0x428dd2] /lib64/libc.so.6(__libc_start_main+0xf1)[0x7fb435cf1401] perf(_start+0x2a)[0x4292da]
[0x0]
[root@jouet ~]#Quoted 70 lines
[acme@jouet linux]$ git bisect bad
5d71c27204d51492abe9fc69064fd36a0bda410bis the first bad commit commit5d71c27204d51492abe9fc69064fd36a0bda410bAuthor: David Carrillo-Cisneros <[email protected]> Date: Mon Apr 10 13:14:30 2017 -0700perf session: Don't rely on evlist in pipe mode
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
Signed-off-by David Carrillo-Cisneros <[email protected]>Acked-by Jiri Olsa <[email protected]>Cc Alexander Shishkin <[email protected]>Cc Andi Kleen <[email protected]>Cc He Kuang <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Paul Turner <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Simon Que <[email protected]>Cc Stephane Eranian <[email protected]>Cc Wang Nan <[email protected]>Signed-off-by Arnaldo Carvalho de Melo <[email protected]>:040000 040000
7df28fe9d7c9d962b78c0532f4961caea2737b5934e4fed0042b1401c1a1a247d8ae40df17f9d272M tools [acme@jouet linux]$Quoted 41 lines
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/session.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)tools/perf/util/session.c +13
-4@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,if (perf_session__open (session) <0 ) goto out_close; -perf_session__set_id_hdr_size (session); -perf_session__set_comm_exec (session); +/* +* set session attributes that are present in perf.data +* but not in pipe-mode. +*/ +if (!file->is_pipe) { +perf_session__set_id_hdr_size (session); +perf_session__set_comm_exec (session); + } } }else { session->machines.host.env = &perf_env;@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,pr_warning ("Cannot read kernel map\n" ); } -if (tool && tool->ordering_requires_timestamps && +/* +* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is +* processed, so perf_evlist__sample_id_all is not meaningful here. +*/ +if (!file->is_pipe && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false; --2.12.2.715.g7642488e1d-goog
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 5/7] perf session: don't rely on evlist in pipe mode
Em Tue, Apr 11, 2017 at 11:59:18AM -0700, David Carrillo-Cisneros escreveu:
Quoted 1 line
Only the second one is needed. file is already checked for the first one.
Right, fixed, tested.
- Arnaldo
tip-bot for David Carrillo-Cisneros
merge bot[tip:perf/core] perf session: Don't rely on evlist in pipe mode
In reply to David Carrillo-Cisnerosperf session: Don't rely on evlist in pipe mode
Commit-ID: 0973ad97c187e06aece61f685b9c3b2d93290a73 Gitweb: http://git.kernel.org/tip/0973ad97c187e06aece61f685b9c3b2d93290a73 Author: David Carrillo-Cisneros <[email protected]> AuthorDate: Mon, 10 Apr 2017 13:14:30 -0700 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 11 Apr 2017 16:22:20 -0300
perf session: Don't rely on evlist in pipe mode
Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.
--- tools/perf/util/session.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
tools/perf/util/session.c +13 -3
@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,if (perf_session__open (session) <0 ) goto out_close; -perf_session__set_id_hdr_size (session); -perf_session__set_comm_exec (session); +/* +* set session attributes that are present in perf.data +* but not in pipe-mode. +*/ +if (!file->is_pipe) { +perf_session__set_id_hdr_size (session); +perf_session__set_comm_exec (session); + } } }else { session->machines.host.env = &perf_env;
@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,pr_warning ("Cannot read kernel map\n" ); } -if (tool && tool->ordering_requires_timestamps && +/* +* In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is +* processed, so perf_evlist__sample_id_all is not meaningful here. +*/ +if ((!file || !file->is_pipe) && tool && tool->ordering_requires_timestamps && tool->ordered_events && !perf_evlist__sample_id_all (session->evlist)) {dump_printf ("WARNING: No sample_id_all support, falling back to unordered processing\n" ); tool->ordered_events = false;
PATCH 6/7 perf tool: protect empty evlists
In reply to David Carrillo-CisnerosA common pattern in in pipe-mode bugs is accessing an empty evlist. Return NULL to make it easier to catch this problems.
--- tools/perf/util/evlist.h | 4 ++++ 1 file changed, 4 insertions(+)
tools/perf/util/evlist.h +4 -0
@@ -257,11 +257,15 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,static inline struct perf_evsel *perf_evlist__first (struct perf_evlist *evlist) { +if (list_empty (&evlist->entries)) +return NULL ; return list_entry(evlist->entries.next, struct perf_evsel, node); }static inline struct perf_evsel *perf_evlist__last (struct perf_evlist *evlist) { +if (list_empty (&evlist->entries)) +return NULL ; return list_entry(evlist->entries.prev, struct perf_evsel, node); }
Signature · 1 line
2.12.2.715.g7642488e1d-goog
Arnaldo Carvalho de Melo
maintainerRe: [PATCH 6/7] perf tool: protect empty evlists
Em Mon, Apr 10, 2017 at 01:14:31PM -0700, David Carrillo-Cisneros escreveu:
Quoted 2 lines
A common pattern in in pipe-mode bugs is accessing an empty evlist. Return NULL to make it easier to catch this problems.
This one is far reaching, we need to take care handling all places using these functions, albeit probably most cases will always have an evsel, I looked at builtin-top.c for instance, and I think we need to be more careful here, will leave this for later.
- Arnaldo
Quoted 27 lines
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/evlist.h | 4 ++++ 1 file changed, 4 insertions(+)tools/perf/util/evlist.h +4
-0@@ -257,11 +257,15 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,static inline struct perf_evsel *perf_evlist__first (struct perf_evlist *evlist) { +if (list_empty (&evlist->entries)) +return NULL ; return list_entry(evlist->entries.next, struct perf_evsel, node); }static inline struct perf_evsel *perf_evlist__last (struct perf_evlist *evlist) { +if (list_empty (&evlist->entries)) +return NULL ; return list_entry(evlist->entries.prev, struct perf_evsel, node); }Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [PATCH 6/7] perf tool: protect empty evlists
I did my best to fix those issues in perf inject and perf report, but yeah, other commands may have issues.
Quoted 38 lines · Arnaldo Carvalho de Melo
Em Mon, Apr 10, 2017 at 01:14:31PM -0700, David Carrillo-Cisneros escreveu:
Quoted 2 lines
A common pattern in in pipe-mode bugs is accessing an empty evlist. Return NULL to make it easier to catch this problems.
This one is far reaching, we need to take care handling all places using these functions, albeit probably most cases will always have an evsel, I looked at
builtin-top.cfor instance, and I think we need to be more careful here, will leave this for later.- Arnaldo
Quoted 27 lines
Signed-off-by David Carrillo-Cisneros <[email protected]>--- tools/perf/util/evlist.h | 4 ++++ 1 file changed, 4 insertions(+)tools/perf/util/evlist.h +4
-1@@ -257,11 +257,15 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,static inline struct perf_evsel *perf_evlist__first (struct perf_evlist *evlist) { +if (list_empty (&evlist->entries)) +return NULL ; return list_entry(evlist->entries.next, struct perf_evsel, node); }static inline struct perf_evsel *perf_evlist__last (struct perf_evlist *evlist) { +if (list_empty (&evlist->entries)) +return NULL ; return list_entry(evlist->entries.prev, struct perf_evsel, node); } --2.12.2.715.g7642488e1d-goog
PATCH 7/7 perf tool: do not print missing features in pipe-mode
In reply to David Carrillo-CisnerosPipe-mode has no perf.data header, hence no upfront knowledge of presend and missing features, hence, do not print missing features in pipe-mode.
--- tools/perf/util/header.c | 3 +++ 1 file changed, 3 insertions(+)
tools/perf/util/header.c +3 -0
@@ -2270,6 +2270,9 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)perf_header__process_sections (header, fd, &hd, perf_file_section__fprintf_info); +if (session->file->is_pipe) +return 0 ; +fprintf (fp,"# missing features: " );for_each_clear_bit (bit, header->adds_features,HEADER_LAST_FEATURE ) {if (bit)
Signature · 1 line
2.12.2.715.g7642488e1d-goog
tip-bot for David Carrillo-Cisneros
merge bot[tip:perf/core] perf tools: Do not print missing features in pipe-mode
perf tools: Do not print missing features in pipe-mode
Commit-ID: c9d1c93421e3b3c7051b193c9cf648a3bc55cb3e Gitweb: http://git.kernel.org/tip/c9d1c93421e3b3c7051b193c9cf648a3bc55cb3e Author: David Carrillo-Cisneros <[email protected]> AuthorDate: Mon, 10 Apr 2017 13:14:32 -0700 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Tue, 11 Apr 2017 16:22:22 -0300
perf tools: Do not print missing features in pipe-mode
Pipe-mode has no perf.data header, hence no upfront knowledge of presend and missing features, hence, do not print missing features in pipe-mode.
--- tools/perf/util/header.c | 3 +++ 1 file changed, 3 insertions(+)
tools/perf/util/header.c +3 -0
@@ -2270,6 +2270,9 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)perf_header__process_sections (header, fd, &hd, perf_file_section__fprintf_info); +if (session->file->is_pipe) +return 0 ; +fprintf (fp,"# missing features: " );for_each_clear_bit (bit, header->adds_features,HEADER_LAST_FEATURE ) {if (bit)
Quoted 5 lines · David Carrillo-Cisneros
Various fixes for perf tool pipe-mode for bugs that arose while trying to make this work:
perf record -o - noploop | perf inject -b | perf annotate -i -
Quoted 8 lines
David Carrillo-Cisneros (7):
perf inject: don't proceed ifperf_session__process_eventfails perf inject: copy events when reordering events in pipe mode perf tool: describe pipe mode in perf.data-file-fomat.txt perf annotate: process attr andbuild_idrecords
perf session: don't rely on evlist in pipe mode
perf tool: protect empty evlists
perf tool: do not print missing features in pipe-mode
thanks,
jirka
Arnaldo Carvalho de Melo
maintainerEm Tue, Apr 11, 2017 at 11:49:06AM +0200, Jiri Olsa escreveu:
Quoted 15 lines · David Carrillo-Cisneros
Quoted 5 lines
Various fixes for perf tool pipe-mode for bugs that arose while trying to make this work:
perf record -o - noploop | perf inject -b | perf annotate -i -
Quoted 8 lines
David Carrillo-Cisneros (7):
perf inject: don't proceed ifperf_session__process_eventfails perf inject: copy events when reordering events in pipe mode perf tool: describe pipe mode in perf.data-file-fomat.txt perf annotate: process attr andbuild_idrecords
perf session: don't rely on evlist in pipe mode
perf tool: protect empty evlists
perf tool: do not print missing features in pipe-mode
Quoted 1 line
Acked-by Jiri Olsa <[email protected]>
Applied all, except 6/7, will audit all users of those first() and last() methods to check if this is ok or what changes are needed to robustify code assuming they always return an evsel (which is false, and this patch fixes, as the evlist may be empty).
- Arnaldo.