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

[David Ahern]

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.

> SNIP
>
> > @@ -365,14 +387,12 @@ static int __event__synthesize_thread(union perf_event *comm_event,
> >   	char filename[PATH_MAX];
> >   	DIR *tasks;
> >   	struct dirent dirent, *next;
> > -	pid_t tgid;
> > +	pid_t tgid, ppid;
> >
> >   	/* special case: only send one comm event using passed in pid */
> >   	if (!full) {
> > -		tgid = perf_event__synthesize_comm(tool, comm_event, pid,
> > -						   process, machine);
> > -
> > -		if (tgid == -1)
> > +		if (perf_event__synthesize_comm(tool, comm_event, pid,
> > +						process, machine, &tgid, &ppid) != 0)
>
> why did you add *ppid arg into perf_event__synthesize_comm?
> I see no use for it in the rest of the caller..

Because of the twisted state that code is in. Yes, I can remove that one.

David
--
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/