Em Wed, May 20, 2015 at 09:56:25AM +0900, Namhyung Kim escreveu:
> On Tue, May 19, 2015 at 09:22:10PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, May 20, 2015 at 08:56:58AM +0900, Namhyung Kim escreveu:
> > > On Tue, May 19, 2015 at 05:18:54PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Tue, May 19, 2015 at 09:49:03PM +0200, Jiri Olsa escreveu:
> > > > > > > Humm, you're thinking about where you managed to reproduce the
> > > > > > > problem,
> > > > > > > I am thinking outside indexing, etc, i.e. by definition we either
> > > > > > > enable
> > > > > > > the event before we fork, so that we get the
> > > > > > > PERF_RECORD_FORK/COMM or we
> > > > > > > synthesize it either from /proc or directly (preferred) if we
> > > > > > > decide to
> > > > > > > do it after the fork/exec, right?
> >
> > > > > > But as I said before, later COMM event will override thread->comm
> > > > > > to a
> > > > > > proper string as long as it can find a matching thread. So I think
> > > > > > it
> > > > > > has no problem in the current code.
> >
> > > > > I can see the issue in the current script code and the patch cured it
> > > > > ;-)
> >
> > > > Exactly, this is my point, this is not something new :-)
> >
> > > Ah, okay. The perf script shows samples before processing comm events
> > > while perf report shows after processing all events.
> >
> > I.e. 'perf script' behaves like 'perf trace' and 'perf top'. 'perf
> > report' is the odd one out, and I think it should be not, i.e. you
> > should try to think more about the non 'report' use cases when thinking
> > about how to improve report :-)
>
> I'll keep it in mind.
>
> >
> > But I digress, lets get back to the question at hand...
> >
> > > But to move it under generic place like perf_evlist__{prepare,start}_
> > > workload(), it seems we need to pass an additional callback and data.
> >
> > Only if you want to do it with perf_event__synthesize_comm(). I
> > suggested writing a new synthesize routine that doesn't parses /proc, as
> > we have all that we need, no?
>
> Agreed.
>
> >
> > I think that just doing something like:
> >
> > thread = machine__findnew_thread(evlist->workload.pid,
> > evlist->workload.pid);
> > if (thread)
> > thread__set_comm(thread, argv[0], timestamp);
> >
> > Should be enough, no? I.e. no need for setting up a PERF_RECORD_FORK and
> > a PERF_RECORD_COMM, read /proc, etc, just do it directly with the info
> > we used to do the fork in perf_evlist__prepare_workload(), etc.
>
> For non-record use case it'd be enough. But for record, it needs to
> synthesize/write an event to data file so that perf report canout
> recognize it later. That's why I think it needs callback.
You have a point, then probably it is better to have it as a function to
be called by each kind of tool, as needed.
- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
