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@xxxxxxxxx>

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) {
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/