Re: [PATCH 12/38] perf tools: Introduce thread__comm_time() helpers

[Frederic Weisbecker <fweisbec@gmail.com>]

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 <fweisbec@gmail.com>
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  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