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

[Namhyung Kim]

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@xxxxxxxxx>
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  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.

> 
> 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?


> 
> > +{
> > +	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.

Thanks for your review!
Namhyung
--
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/