From: Namhyung Kim <[email protected]>
perf-record updates the header in the perf.data file at termination.
Without this update perf-report (and other processing built-ins) it
caused an infinite loop when perf report (or something like) called.
This is because the algorithm in __perf_session__process_events()
depends on the data_size which is read from file header. Use file
size directly instead in this case to do the best-effort processing.
Cc: David Ahern <[email protected]>
Cc: Sonny Rao <[email protected]>
Signed-off-by: David Ahern <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/header.c | 9 +++++++++
tools/perf/util/session.c | 2 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 26441d0e571b..cedccde6a6b2 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2753,6 +2753,15 @@ int perf_session__read_header(struct perf_session *session)
if (perf_file_header__read(&f_header, header, fd) < 0)
return -EINVAL;
+ /*
+ * Sanity check that perf.data was written cleanly; data size is
+ * initialized to 0 and updated only if the on_exit function is run.
+ * If data size is still 0 then the file contains only partial
+ * information. Just warn user and process it as much as it can.
+ */
+ if (f_header.data.size == 0)
+ pr_warning("Data size is 0. Was the record command properly terminated?\n");
+
nr_attrs = f_header.attrs.size / f_header.attr_size;
lseek(fd, f_header.attrs.offset, SEEK_SET);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 6c1d4447c5b4..7bbd60c0a0ed 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1323,7 +1323,7 @@ int __perf_session__process_events(struct perf_session *session,
file_offset = page_offset;
head = data_offset - page_offset;
- if (data_offset + data_size < file_size)
+ if (data_size && (data_offset + data_size < file_size))
file_size = data_offset + data_size;
progress_next = file_size / 16;
--
1.7.11.7
On 9/30/13 2:19 AM, Namhyung Kim wrote:
> From: Namhyung Kim <[email protected]>
>
> perf-record updates the header in the perf.data file at termination.
> Without this update perf-report (and other processing built-ins) it
> caused an infinite loop when perf report (or something like) called.
>
> This is because the algorithm in __perf_session__process_events()
> depends on the data_size which is read from file header. Use file
> size directly instead in this case to do the best-effort processing.
>
> Cc: David Ahern <[email protected]>
> Cc: Sonny Rao <[email protected]>
> Signed-off-by: David Ahern <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
worked ok for me. Sonny can you verify?
David
On Mon, Sep 30, 2013 at 6:49 AM, David Ahern <[email protected]> wrote:
> On 9/30/13 2:19 AM, Namhyung Kim wrote:
>>
>> From: Namhyung Kim <[email protected]>
>>
>> perf-record updates the header in the perf.data file at termination.
>> Without this update perf-report (and other processing built-ins) it
>> caused an infinite loop when perf report (or something like) called.
>>
>> This is because the algorithm in __perf_session__process_events()
>> depends on the data_size which is read from file header. Use file
>> size directly instead in this case to do the best-effort processing.
>>
>> Cc: David Ahern <[email protected]>
>> Cc: Sonny Rao <[email protected]>
>> Signed-off-by: David Ahern <[email protected]>
>> Signed-off-by: Namhyung Kim <[email protected]>
>
>
> worked ok for me. Sonny can you verify?
Yes, it works for me as well, thanks!
> David
>
* Namhyung Kim <[email protected]> wrote:
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2753,6 +2753,15 @@ int perf_session__read_header(struct perf_session *session)
> if (perf_file_header__read(&f_header, header, fd) < 0)
> return -EINVAL;
>
> + /*
> + * Sanity check that perf.data was written cleanly; data size is
> + * initialized to 0 and updated only if the on_exit function is run.
> + * If data size is still 0 then the file contains only partial
> + * information. Just warn user and process it as much as it can.
> + */
> + if (f_header.data.size == 0)
> + pr_warning("Data size is 0. Was the record command properly terminated?\n");
Just a detail: it would be nice to make all the user facing messages in
tools/perf/util/header.c more specific and more structured. For example
prefixing it with 'perf header:' would be fine:
WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
or something like that.
etc.
Ingo
On 10/1/13 1:16 AM, Ingo Molnar wrote:
> Just a detail: it would be nice to make all the user facing messages in
> tools/perf/util/header.c more specific and more structured. For example
> prefixing it with 'perf header:' would be fine:
>
> WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
>
Why put code references in the messages? The message is all on one line
so grep finds it quickly for people working on the code.
David
* David Ahern <[email protected]> wrote:
> On 10/1/13 1:16 AM, Ingo Molnar wrote:
> >Just a detail: it would be nice to make all the user facing messages in
> >tools/perf/util/header.c more specific and more structured. For example
> >prefixing it with 'perf header:' would be fine:
> >
> > WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
> >
>
> Why put code references in the messages? The message is all on one
> line so grep finds it quickly for people working on the code.
I'd agree if this was some internal error that should never really trigger
(so at most developers see it).
But if I understood it correctly this particular message could trigger for
regular users of perf as well, of the perf record is terminated in some
unusual fashion. Regular users might not have the perf code handy (and
might not know about git grep either).
Thanks,
Ingo
On 10/1/13 8:21 AM, Ingo Molnar wrote:
> But if I understood it correctly this particular message could trigger for
> regular users of perf as well, of the perf record is terminated in some
> unusual fashion. Regular users might not have the perf code handy (and
> might not know about git grep either).
This is the case I was referring to -- normal users don't care about the
code reference, hence the more specific question about how the
perf-record session ended.
* David Ahern <[email protected]> wrote:
> On 10/1/13 8:21 AM, Ingo Molnar wrote:
> >But if I understood it correctly this particular message could trigger for
> >regular users of perf as well, of the perf record is terminated in some
> >unusual fashion. Regular users might not have the perf code handy (and
> >might not know about git grep either).
>
> This is the case I was referring to -- normal users don't care about
> the code reference, hence the more specific question about how the
> perf-record session ended.
Hm, what do you call 'code reference'?
The message I suggested is:
WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
I didn't intend 'perf/header' to be a code reference - it wanted to refer
to the perf.data header. Maybe that should be formulated in a less
confusing manner? Something like:
WARNING: The perf.data file's data size field is 0 which is unexpected.
Was the 'perf record' command properly terminated?
Thanks,
Ingo
Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
>
> * David Ahern <[email protected]> wrote:
>
> > On 10/1/13 8:21 AM, Ingo Molnar wrote:
> > >But if I understood it correctly this particular message could trigger for
> > >regular users of perf as well, of the perf record is terminated in some
> > >unusual fashion. Regular users might not have the perf code handy (and
> > >might not know about git grep either).
> >
> > This is the case I was referring to -- normal users don't care about
> > the code reference, hence the more specific question about how the
> > perf-record session ended.
>
> Hm, what do you call 'code reference'?
>
> The message I suggested is:
>
> WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
>
> I didn't intend 'perf/header' to be a code reference - it wanted to refer
> to the perf.data header. Maybe that should be formulated in a less
> confusing manner? Something like:
I liked this last one:
> WARNING: The perf.data file's data size field is 0 which is unexpected.
> Was the 'perf record' command properly terminated?
Can I patch that up into Namhyung's latest patch?
Sonny, David, from your replies I think I can add Tested-by: tags for
both of you?
Thanks,
- Arnaldo
On 10/1/13 10:54 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
>>
>> * David Ahern <[email protected]> wrote:
>>
>>> On 10/1/13 8:21 AM, Ingo Molnar wrote:
>>>> But if I understood it correctly this particular message could trigger for
>>>> regular users of perf as well, of the perf record is terminated in some
>>>> unusual fashion. Regular users might not have the perf code handy (and
>>>> might not know about git grep either).
>>>
>>> This is the case I was referring to -- normal users don't care about
>>> the code reference, hence the more specific question about how the
>>> perf-record session ended.
>>
>> Hm, what do you call 'code reference'?
>>
>> The message I suggested is:
>>
>> WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
>>
>> I didn't intend 'perf/header' to be a code reference - it wanted to refer
>> to the perf.data header. Maybe that should be formulated in a less
>> confusing manner? Something like:
>
> I liked this last one:
>
>> WARNING: The perf.data file's data size field is 0 which is unexpected.
>> Was the 'perf record' command properly terminated?
>
> Can I patch that up into Namhyung's latest patch?
>
> Sonny, David, from your replies I think I can add Tested-by: tags for
> both of you?
I'm fine with the last message. And yes a Tested by
* Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
> >
> > * David Ahern <[email protected]> wrote:
> >
> > > On 10/1/13 8:21 AM, Ingo Molnar wrote:
> > > >But if I understood it correctly this particular message could trigger for
> > > >regular users of perf as well, of the perf record is terminated in some
> > > >unusual fashion. Regular users might not have the perf code handy (and
> > > >might not know about git grep either).
> > >
> > > This is the case I was referring to -- normal users don't care about
> > > the code reference, hence the more specific question about how the
> > > perf-record session ended.
> >
> > Hm, what do you call 'code reference'?
> >
> > The message I suggested is:
> >
> > WARNING: perf/header: Data size is 0. Was the 'perf record' command properly terminated?
> >
> > I didn't intend 'perf/header' to be a code reference - it wanted to refer
> > to the perf.data header. Maybe that should be formulated in a less
> > confusing manner? Something like:
>
> I liked this last one:
>
> > WARNING: The perf.data file's data size field is 0 which is unexpected.
> > Was the 'perf record' command properly terminated?
>
> Can I patch that up into Namhyung's latest patch?
Sure, feel free!
Thanks,
Ingo
Em Tue, Oct 01, 2013 at 10:59:06AM -0600, David Ahern escreveu:
> On 10/1/13 10:54 AM, Arnaldo Carvalho de Melo wrote:
> >Em Tue, Oct 01, 2013 at 05:33:09PM +0200, Ingo Molnar escreveu:
> >>* David Ahern <[email protected]> wrote:
> >>>On 10/1/13 8:21 AM, Ingo Molnar wrote:
> >I liked this last one:
> >
> >> WARNING: The perf.data file's data size field is 0 which is unexpected.
> >> Was the 'perf record' command properly terminated?
> >
> >Can I patch that up into Namhyung's latest patch?
> >
> >Sonny, David, from your replies I think I can add Tested-by: tags for
> >both of you?
>
> I'm fine with the last message. And yes a Tested by
Got to process this now, changing it a bit to replace "perf.data" with
"%s" + session->filename, ack?
Otherwise the user may have a 'perf.data' file and despite having
informed -i with a different file name still get confused... ;-)
- Arnaldo
On 10/4/13 9:25 AM, Arnaldo Carvalho de Melo wrote:
> Got to process this now, changing it a bit to replace "perf.data" with
> "%s" + session->filename, ack?
>
> Otherwise the user may have a 'perf.data' file and despite having
> informed -i with a different file name still get confused... ;-)
>
ack. agreed that's the better way to go.
David
Commit-ID: b314e5cfd11fd78545ce6c2be42646254390c1aa
Gitweb: http://git.kernel.org/tip/b314e5cfd11fd78545ce6c2be42646254390c1aa
Author: Namhyung Kim <[email protected]>
AuthorDate: Mon, 30 Sep 2013 17:19:48 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 4 Oct 2013 15:17:46 -0300
perf session: Fix infinite loop on invalid perf.data file
perf-record updates the header in the perf.data file at termination.
Without this update perf-report (and other processing built-ins) it
caused an infinite loop when perf report (or something like) called.
This is because the algorithm in __perf_session__process_events()
depends on the data_size which is read from file header. Use file size
directly instead in this case to do the best-effort processing.
Signed-off-by: Namhyung Kim <[email protected]>
Tested-by: David Ahern <[email protected]>
Tested-by: Sonny Rao <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sonny Rao <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: David Ahern <[email protected]>
[ Reworded warning as per Ingo Molnar suggestion, replaces 'perf.data'
with session->filename, to precisely identify the data file involved ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/header.c | 12 ++++++++++++
tools/perf/util/session.c | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index ce69901..c3e5a3b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2768,6 +2768,18 @@ int perf_session__read_header(struct perf_session *session)
if (perf_file_header__read(&f_header, header, fd) < 0)
return -EINVAL;
+ /*
+ * Sanity check that perf.data was written cleanly; data size is
+ * initialized to 0 and updated only if the on_exit function is run.
+ * If data size is still 0 then the file contains only partial
+ * information. Just warn user and process it as much as it can.
+ */
+ if (f_header.data.size == 0) {
+ pr_warning("WARNING: The %s file's data size field is 0 which is unexpected.\n"
+ "Was the 'perf record' command properly terminated?\n",
+ session->filename);
+ }
+
nr_attrs = f_header.attrs.size / f_header.attr_size;
lseek(fd, f_header.attrs.offset, SEEK_SET);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 37c4718..568b750 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1312,7 +1312,7 @@ int __perf_session__process_events(struct perf_session *session,
file_offset = page_offset;
head = data_offset - page_offset;
- if (data_offset + data_size < file_size)
+ if (data_size && (data_offset + data_size < file_size))
file_size = data_offset + data_size;
progress_next = file_size / 16;