From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933060AbbCEQJE (ORCPT ); Thu, 5 Mar 2015 11:09:04 -0500 Received: from mail-wg0-f51.google.com ([74.125.82.51]:42924 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933028AbbCEQJB (ORCPT ); Thu, 5 Mar 2015 11:09:01 -0500 Date: Thu, 5 Mar 2015 17:08:56 +0100 From: Frederic Weisbecker To: Namhyung Kim Cc: Arnaldo Carvalho de Melo , Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , Adrian Hunter , Stephane Eranian , Andi Kleen , David Ahern Subject: Re: [PATCH 12/38] perf tools: Introduce thread__comm_time() helpers Message-ID: <20150305160854.GF5074@lerouge> References: <1425352070-1115-1-git-send-email-namhyung@kernel.org> <1425352070-1115-13-git-send-email-namhyung@kernel.org> <20150303162837.GA26830@lerouge> <20150304000255.GF27046@danjae> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150304000255.GF27046@danjae> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 04, 2015 at 09:02:55AM +0900, Namhyung Kim wrote: > Hi Frederic, > > On Tue, Mar 03, 2015 at 05:28:40PM +0100, Frederic Weisbecker wrote: > > On Tue, Mar 03, 2015 at 12:07:24PM +0900, Namhyung Kim wrote: > > > When data file indexing is enabled, it processes all task, comm and mmap > > > events first and then goes to the sample events. So all it sees is the > > > last comm of a thread although it has information at the time of sample. > > > > > > Sort thread's comm by time so that it can find appropriate comm at the > > > sample time. The thread__comm_time() will mostly work even if > > > PERF_SAMPLE_TIME bit is off since in that case, sample->time will be > > > -1 so it'll take the last comm anyway. > > > > > > Cc: Frederic Weisbecker > > > Signed-off-by: Namhyung Kim > > > --- > > > tools/perf/util/thread.c | 33 ++++++++++++++++++++++++++++++++- > > > tools/perf/util/thread.h | 2 ++ > > > 2 files changed, 34 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c > > > index 9ebc8b1f9be5..ad96725105c2 100644 > > > --- a/tools/perf/util/thread.c > > > +++ b/tools/perf/util/thread.c > > > @@ -103,6 +103,21 @@ struct comm *thread__exec_comm(const struct thread *thread) > > > return last; > > > } > > > > > > +struct comm *thread__comm_time(const struct thread *thread, u64 timestamp) > > > > Usually thread__comm_foo() would suggest that we return the "foo" from a thread comm. > > For example thread__comm_len() returns the len of the last thread comm. > > thread__comm_str() returns the string of the last thread comm. > > Ah, okay. I mean, that's just an impression, others may have a different one :o) > > > > > So thread__comm_time() suggests that we return the timestamp of the default thread comm. > > Ideally all thread__comm_foo() accessors should now take a timestamp as an argument. Now there > > are quite some callers of such APIs, I'm not sure they will all turn into passing a precise timestamp, > > but the current callers are interested in the last comm so perhaps those can be turned into thread__last_comm[_str](). > > The call would gain some clarity. > > I'm fine with this change. Actually I also don't like the _time > suffix but couldn't come up with a better name. ;-) > > I also think the last comm also be thought as current comm. So how > about thread__curr_comm[_str]() then? Yeah, curr works for me too. Thanks! > > > > > > > +{ > > > + struct comm *comm; > > > + > > > + list_for_each_entry(comm, &thread->comm_list, list) { > > > + if (timestamp >= comm->start) > > > + return comm; > > > + } > > > + > > > + if (list_empty(&thread->comm_list)) > > > + return NULL; > > > + > > > + return list_last_entry(&thread->comm_list, struct comm, list); > > > +} > > > > Yes, handling the thread's comm lifecycle correctly with fetching the right comm at a given time is > > the evolution I had in mind. I haven't looked at the rest of your patchset but this > > change alone seem to go to the right direction. > > The idea is extending it to find thread and maps so that we can > process samples parallelly. Ok! Thanks! > > Thanks for your review! > Namhyung