Re: [PATCH 35/40] perf record: Synthesize COMM event for a command line workload

[Arnaldo Carvalho de Melo]

Em Mon, May 18, 2015 at 09:30:50AM +0900, Namhyung Kim escreveu:
> When perf creates a new child to profile, the events are enabled on
> exec().  And in this case, it doesn't synthesize any event for the
> child since they'll be generated during exec().  But there's an window
> between the enabling and the event generation.
> 
> It used to be overcome since samples are only in kernel (so we always
> have the map) and the comm is overridden by a later COMM event.
> However it won't work anymore since those samples will go to a missing
> thread now but the COMM event will create a (current) thread.  This
> leads to those early samples (like native_write_msr_safe) not having a
> comm but pid (like ':15328').
 
> So it needs to synthesize COMM event for the child explicitly before
> enabling so that it can have a correct comm.  But at this time, the
> comm will be "perf" since it's not exec-ed yet.

This looks reasonable, but I think it probably needs to be done
somewhere in perf_evlist__prepare_workload() or
perf_evlist__start_workload(), as this affects other tools as well, like
'top', 'trace' and any other that may want to do this start-workload use
case.

I also wonder if we can't overcome this without using /proc, i.e.
actually moving the "start the workload" to just before the fork, so
that the kernel covers that as well.

Or, alternatively, the thread can be created without having to look at
/proc at all, but by directly creating the struct thread, with the
correct COMM, pid, etc, that we know, since we forked it, etc.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
>  tools/perf/builtin-record.c | 18 +++++++++++++++++-
>  tools/perf/util/event.c     |  2 +-
>  tools/perf/util/event.h     |  5 +++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 978ebf648aab..153f38e35555 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -766,8 +766,24 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  	/*
>  	 * Let the child rip
>  	 */
> -	if (forks)
> +	if (forks) {
> +		union perf_event *comm_event;
> +
> +		comm_event = malloc(sizeof(*comm_event) + machine->id_hdr_size);
> +		if (comm_event == NULL)
> +			goto out_child;
> +
> +		err = perf_event__synthesize_comm(tool, comm_event,
> +						  rec->evlist->workload.pid,
> +						  process_synthesized_event,
> +						  machine);
> +		free(comm_event);
> +
> +		if (err < 0)
> +			goto out_child;
> +
>  		perf_evlist__start_workload(rec->evlist);
> +	}
>  
>  	if (opts->initial_delay) {
>  		usleep(opts->initial_delay * 1000);
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 930d45d5a37a..3adca1302150 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -165,7 +165,7 @@ static int perf_event__prepare_comm(union perf_event *event, pid_t pid,
>  	return 0;
>  }
>  
> -static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> +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)
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index 40e02544f861..9aacd558ac0f 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -452,6 +452,11 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
>  				       struct machine *machine,
>  				       bool mmap_data);
>  
> +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);
> +
>  size_t perf_event__fprintf_comm(union perf_event *event, FILE *fp);
>  size_t perf_event__fprintf_mmap(union perf_event *event, FILE *fp);
>  size_t perf_event__fprintf_mmap2(union perf_event *event, FILE *fp);
> -- 
> 2.4.0
--
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/