From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754572AbbIBNC5 (ORCPT ); Wed, 2 Sep 2015 09:02:57 -0400 Received: from mga09.intel.com ([134.134.136.24]:36832 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752689AbbIBNCz (ORCPT ); Wed, 2 Sep 2015 09:02:55 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,453,1437462000"; d="scan'208";a="760822141" Subject: Re: [PATCH] perf tools: Fix use of wrong event when processing exit events To: Arnaldo Carvalho de Melo References: <1439888825-27708-1-git-send-email-adrian.hunter@intel.com> Cc: linux-kernel@vger.kernel.org, Jiri Olsa From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <55E6F2D8.30306@intel.com> Date: Wed, 2 Sep 2015 16:00:08 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1439888825-27708-1-git-send-email-adrian.hunter@intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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) { >