[PATCH] perf tools: Fix use of wrong event when processing exit events

[PATCH] perf tools: Fix use of wrong event when processing exit events

[Adrian Hunter]

In a couple of cases the 'comm' member of 'union event' has
been used instead of the correct member ('fork') when processing
exit events.

In the cases where it has been used incorrectly, only the 'pid'
and 'tid' are affected.  The 'pid' value would be correct anyway
because it is in the same position in 'comm' and 'fork' events,
but the 'tid' would have been incorrectly assigned from 'ppid'.
However, for exit events, the kernel puts the current task in
the 'ppid' and 'ttid' which is the same as the exiting task.
That is 'ppid' == 'pid' and if the task is not multi-threaded,
'pid' == 'tid' i.e. the data goes wrong only when tracing
multi-threaded programs.

It is hard to find an example of how this would produce an
error in practice.  There are 3 occurences of the fix:
1. perf script is only affected if !sample_id_all which only
happens on old kernels
2. intel_pt is only affected when decoding without timestamps
and would probably still decode correctly - the exit event is
only used to flush out data which anyway gets flushed at the
end of the session
3. intel_bts also uses the exit event to flush data which
would probably not cause errors as it would get flushed at
the end of the session instead

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-script.c | 4 ++--
 tools/perf/util/intel-bts.c | 2 +-
 tools/perf/util/intel-pt.c  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 105332e950a9..17030c6c64b6 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -739,8 +739,8 @@ static int process_exit_event(struct perf_tool *tool,
 	if (!evsel->attr.sample_id_all) {
 		sample->cpu = 0;
 		sample->time = 0;
-		sample->tid = event->comm.tid;
-		sample->pid = event->comm.pid;
+		sample->tid = event->fork.tid;
+		sample->pid = event->fork.pid;
 	}
 	print_sample_start(sample, thread, evsel);
 	perf_event__fprintf(event, stdout);
diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index dce99cfb1309..09d142b362fa 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -623,7 +623,7 @@ static int intel_bts_process_event(struct perf_session *session,
 	if (err)
 		return err;
 	if (event->header.type == PERF_RECORD_EXIT) {
-		err = intel_bts_process_tid_exit(bts, event->comm.tid);
+		err = intel_bts_process_tid_exit(bts, event->fork.tid);
 		if (err)
 			return err;
 	}
diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 2a4a4120473b..ef2fb13f1dfa 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -1464,7 +1464,7 @@ static int intel_pt_process_event(struct perf_session *session,
 	if (pt->timeless_decoding) {
 		if (event->header.type == PERF_RECORD_EXIT) {
 			err = intel_pt_process_timeless_queues(pt,
-							       event->comm.tid,
+							       event->fork.tid,
 							       sample->time);
 		}
 	} else if (timestamp) {
-- 
1.9.1

Re: [PATCH] perf tools: Fix use of wrong event when processing exit events

[Adrian Hunter]

On 18/08/15 12:07, Adrian Hunter wrote:
> In a couple of cases the 'comm' member of 'union event' has
> been used instead of the correct member ('fork') when processing
> exit events.
> 
> In the cases where it has been used incorrectly, only the 'pid'
> and 'tid' are affected.  The 'pid' value would be correct anyway
> because it is in the same position in 'comm' and 'fork' events,
> but the 'tid' would have been incorrectly assigned from 'ppid'.
> However, for exit events, the kernel puts the current task in
> the 'ppid' and 'ttid' which is the same as the exiting task.
> That is 'ppid' == 'pid' and if the task is not multi-threaded,
> 'pid' == 'tid' i.e. the data goes wrong only when tracing
> multi-threaded programs.
> 
> It is hard to find an example of how this would produce an
> error in practice.  There are 3 occurences of the fix:
> 1. perf script is only affected if !sample_id_all which only
> happens on old kernels
> 2. intel_pt is only affected when decoding without timestamps
> and would probably still decode correctly - the exit event is
> only used to flush out data which anyway gets flushed at the
> end of the session
> 3. intel_bts also uses the exit event to flush data which
> would probably not cause errors as it would get flushed at
> the end of the session instead
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

I think it would be worth picking this one up for 4.3

> ---
>  tools/perf/builtin-script.c | 4 ++--
>  tools/perf/util/intel-bts.c | 2 +-
>  tools/perf/util/intel-pt.c  | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 105332e950a9..17030c6c64b6 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -739,8 +739,8 @@ static int process_exit_event(struct perf_tool *tool,
>  	if (!evsel->attr.sample_id_all) {
>  		sample->cpu = 0;
>  		sample->time = 0;
> -		sample->tid = event->comm.tid;
> -		sample->pid = event->comm.pid;
> +		sample->tid = event->fork.tid;
> +		sample->pid = event->fork.pid;
>  	}
>  	print_sample_start(sample, thread, evsel);
>  	perf_event__fprintf(event, stdout);
> diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> index dce99cfb1309..09d142b362fa 100644
> --- a/tools/perf/util/intel-bts.c
> +++ b/tools/perf/util/intel-bts.c
> @@ -623,7 +623,7 @@ static int intel_bts_process_event(struct perf_session *session,
>  	if (err)
>  		return err;
>  	if (event->header.type == PERF_RECORD_EXIT) {
> -		err = intel_bts_process_tid_exit(bts, event->comm.tid);
> +		err = intel_bts_process_tid_exit(bts, event->fork.tid);
>  		if (err)
>  			return err;
>  	}
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 2a4a4120473b..ef2fb13f1dfa 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -1464,7 +1464,7 @@ static int intel_pt_process_event(struct perf_session *session,
>  	if (pt->timeless_decoding) {
>  		if (event->header.type == PERF_RECORD_EXIT) {
>  			err = intel_pt_process_timeless_queues(pt,
> -							       event->comm.tid,
> +							       event->fork.tid,
>  							       sample->time);
>  		}
>  	} else if (timestamp) {
> 

Re: [PATCH] perf tools: Fix use of wrong event when processing exit events

[Arnaldo Carvalho de Melo]

Em Wed, Sep 02, 2015 at 04:00:08PM +0300, Adrian Hunter escreveu:
> On 18/08/15 12:07, Adrian Hunter wrote:
> > In a couple of cases the 'comm' member of 'union event' has
> > been used instead of the correct member ('fork') when processing
> > exit events.
> > 
> > In the cases where it has been used incorrectly, only the 'pid'
> > and 'tid' are affected.  The 'pid' value would be correct anyway
> > because it is in the same position in 'comm' and 'fork' events,
> > but the 'tid' would have been incorrectly assigned from 'ppid'.
> > However, for exit events, the kernel puts the current task in
> > the 'ppid' and 'ttid' which is the same as the exiting task.
> > That is 'ppid' == 'pid' and if the task is not multi-threaded,
> > 'pid' == 'tid' i.e. the data goes wrong only when tracing
> > multi-threaded programs.
> > 
> > It is hard to find an example of how this would produce an
> > error in practice.  There are 3 occurences of the fix:
> > 1. perf script is only affected if !sample_id_all which only
> > happens on old kernels
> > 2. intel_pt is only affected when decoding without timestamps
> > and would probably still decode correctly - the exit event is
> > only used to flush out data which anyway gets flushed at the
> > end of the session
> > 3. intel_bts also uses the exit event to flush data which
> > would probably not cause errors as it would get flushed at
> > the end of the session instead
> > 
> > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> I think it would be worth picking this one up for 4.3

I guess I had this picked up, is there anything else from you that is
outstanding?

- Arnaldo
 
> > ---
> >  tools/perf/builtin-script.c | 4 ++--
> >  tools/perf/util/intel-bts.c | 2 +-
> >  tools/perf/util/intel-pt.c  | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 105332e950a9..17030c6c64b6 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -739,8 +739,8 @@ static int process_exit_event(struct perf_tool *tool,
> >  	if (!evsel->attr.sample_id_all) {
> >  		sample->cpu = 0;
> >  		sample->time = 0;
> > -		sample->tid = event->comm.tid;
> > -		sample->pid = event->comm.pid;
> > +		sample->tid = event->fork.tid;
> > +		sample->pid = event->fork.pid;
> >  	}
> >  	print_sample_start(sample, thread, evsel);
> >  	perf_event__fprintf(event, stdout);
> > diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> > index dce99cfb1309..09d142b362fa 100644
> > --- a/tools/perf/util/intel-bts.c
> > +++ b/tools/perf/util/intel-bts.c
> > @@ -623,7 +623,7 @@ static int intel_bts_process_event(struct perf_session *session,
> >  	if (err)
> >  		return err;
> >  	if (event->header.type == PERF_RECORD_EXIT) {
> > -		err = intel_bts_process_tid_exit(bts, event->comm.tid);
> > +		err = intel_bts_process_tid_exit(bts, event->fork.tid);
> >  		if (err)
> >  			return err;
> >  	}
> > diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> > index 2a4a4120473b..ef2fb13f1dfa 100644
> > --- a/tools/perf/util/intel-pt.c
> > +++ b/tools/perf/util/intel-pt.c
> > @@ -1464,7 +1464,7 @@ static int intel_pt_process_event(struct perf_session *session,
> >  	if (pt->timeless_decoding) {
> >  		if (event->header.type == PERF_RECORD_EXIT) {
> >  			err = intel_pt_process_timeless_queues(pt,
> > -							       event->comm.tid,
> > +							       event->fork.tid,
> >  							       sample->time);
> >  		}
> >  	} else if (timestamp) {
> > 

Re: [PATCH] perf tools: Fix use of wrong event when processing exit events

[Adrian Hunter]

On 02/09/15 22:55, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 02, 2015 at 04:00:08PM +0300, Adrian Hunter escreveu:
>> On 18/08/15 12:07, Adrian Hunter wrote:
>>> In a couple of cases the 'comm' member of 'union event' has
>>> been used instead of the correct member ('fork') when processing
>>> exit events.
>>>
>>> In the cases where it has been used incorrectly, only the 'pid'
>>> and 'tid' are affected.  The 'pid' value would be correct anyway
>>> because it is in the same position in 'comm' and 'fork' events,
>>> but the 'tid' would have been incorrectly assigned from 'ppid'.
>>> However, for exit events, the kernel puts the current task in
>>> the 'ppid' and 'ttid' which is the same as the exiting task.
>>> That is 'ppid' == 'pid' and if the task is not multi-threaded,
>>> 'pid' == 'tid' i.e. the data goes wrong only when tracing
>>> multi-threaded programs.
>>>
>>> It is hard to find an example of how this would produce an
>>> error in practice.  There are 3 occurences of the fix:
>>> 1. perf script is only affected if !sample_id_all which only
>>> happens on old kernels
>>> 2. intel_pt is only affected when decoding without timestamps
>>> and would probably still decode correctly - the exit event is
>>> only used to flush out data which anyway gets flushed at the
>>> end of the session
>>> 3. intel_bts also uses the exit event to flush data which
>>> would probably not cause errors as it would get flushed at
>>> the end of the session instead
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>
>> I think it would be worth picking this one up for 4.3
> 
> I guess I had this picked up, is there anything else from you that is
> outstanding?

Only "perf tools: Add support for PERF_RECORD_SWITCH to Intel PT" but that
is a feature not a bug.

[tip:perf/urgent] perf tools: Fix use of wrong event when processing exit events

[tip-bot for Adrian Hunter]

Commit-ID:  53ff6bc37be449f546158a39c528d7814dfb15a1
Gitweb:     http://git.kernel.org/tip/53ff6bc37be449f546158a39c528d7814dfb15a1
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Tue, 18 Aug 2015 12:07:05 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 2 Sep 2015 17:46:26 -0300

perf tools: Fix use of wrong event when processing exit events

In a couple of cases the 'comm' member of 'union event' has been used
instead of the correct member ('fork') when processing exit events.

In the cases where it has been used incorrectly, only the 'pid' and
'tid' are affected.  The 'pid' value would be correct anyway because it
is in the same position in 'comm' and 'fork' events, but the 'tid' would
have been incorrectly assigned from 'ppid'.

However, for exit events, the kernel puts the current task in the 'ppid'
and 'ttid' which is the same as the exiting task.  That is 'ppid' ==
'pid' and if the task is not multi-threaded, 'pid' == 'tid' i.e. the
data goes wrong only when tracing multi-threaded programs.

It is hard to find an example of how this would produce an error in
practice.  There are 3 occurences of the fix:

1. perf script is only affected if !sample_id_all which only happens on
  old kernels.

2. intel_pt is only affected when decoding without timestamps
   and would probably still decode correctly - the exit event is
   only used to flush out data which anyway gets flushed at the
   end of the session

3. intel_bts also uses the exit event to flush data which
   would probably not cause errors as it would get flushed at
   the end of the session instead

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Link: http://lkml.kernel.org/r/1439888825-27708-1-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-script.c | 4 ++--
 tools/perf/util/intel-bts.c | 2 +-
 tools/perf/util/intel-pt.c  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index eb51325..284a76e 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -768,8 +768,8 @@ static int process_exit_event(struct perf_tool *tool,
 	if (!evsel->attr.sample_id_all) {
 		sample->cpu = 0;
 		sample->time = 0;
-		sample->tid = event->comm.tid;
-		sample->pid = event->comm.pid;
+		sample->tid = event->fork.tid;
+		sample->pid = event->fork.pid;
 	}
 	print_sample_start(sample, thread, evsel);
 	perf_event__fprintf(event, stdout);
diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index ea76862..eb0e7f8 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -623,7 +623,7 @@ static int intel_bts_process_event(struct perf_session *session,
 	if (err)
 		return err;
 	if (event->header.type == PERF_RECORD_EXIT) {
-		err = intel_bts_process_tid_exit(bts, event->comm.tid);
+		err = intel_bts_process_tid_exit(bts, event->fork.tid);
 		if (err)
 			return err;
 	}
diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index bb41c20..535d86f 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -1494,7 +1494,7 @@ static int intel_pt_process_event(struct perf_session *session,
 	if (pt->timeless_decoding) {
 		if (event->header.type == PERF_RECORD_EXIT) {
 			err = intel_pt_process_timeless_queues(pt,
-							       event->comm.tid,
+							       event->fork.tid,
 							       sample->time);
 		}
 	} else if (timestamp) {