Re: [PATCH 2/2] perf tool: Fix ppid for synthesized fork events

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Mar 30, 2015 at 09:23:21AM -0600, David Ahern escreveu:
> On 3/30/15 2:04 AM, Jiri Olsa wrote:
> >On Sun, Mar 29, 2015 at 04:30:02PM -0600, David Ahern wrote:
> >
> >SNIP
> >
> >>-static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> >>-					 union perf_event *event, pid_t pid,
> >>-					 perf_event__handler_t process,
> >>-					 struct machine *machine)
> >>+static int perf_event__synthesize_comm(struct perf_tool *tool,
> >>+				       union perf_event *event, pid_t pid,
> >>+				       perf_event__handler_t process,
> >>+				       struct machine *machine,
> >>+				       pid_t *tgid, pid_t *ppid)
> >>  {
> >>-	pid_t tgid = perf_event__prepare_comm(event, pid, machine);
> >>-
> >>-	if (tgid == -1)
> >>-		goto out;
> >>+	if (perf_event__prepare_comm(event, pid, machine, tgid, ppid) != 0)
> >>+		return -1;
> >
> >why dont we set ppid also for single comm event? seems to me
> >like following assignments:
> >
> >         event->fork.ppid = ppid;
> >         event->fork.ptid = ppid;
> >         event->fork.pid  = tgid;
> >         event->fork.tid  = pid;
> >         event->fork.header.type = PERF_RECORD_FORK;
> >
> >should be part of perf_event__prepare_comm function..?
> 
> comm events versus fork events. Only fork has the ppid. That loop that
> generates COMM and FORK events needs some work. e.g., FORK should come
> before COMM. But that is not the point of this patch which is to fix ppid.

Yes, and COMM nowadays creates the thread (uses findnew_thread) where it
should really use just find_thread(), i.e. the fork event is the only
one that should create it, I guess it was like that because it made the
code a bit more robust, i.e. would handle COMMs without FORKs.

This one:

commit 4aa5f4f7bb8bc41cba15bcd0d80c4fb085027d6b
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Fri Feb 27 19:52:10 2015 -0300

    perf tools: Fix FORK after COMM when synthesizing records for pre-existing threads

Should have fixed a bit of this...

- Arnaldo