[PATCH 1/2] perf tools: Introduce perf_evlist__start_workload_ex()

[PATCH 1/2] perf tools: Introduce perf_evlist__start_workload_ex()

[Namhyung Kim]

The perf_evlist__start_workload_ex() does same as __start_work() but
also invokes callback which does additional work for each command.

Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evlist.c | 11 +++++++++++
 tools/perf/util/evlist.h |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a8643735dcea..12b32a059772 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1593,6 +1593,17 @@ int perf_evlist__start_workload(struct perf_evlist *evlist)
 	return 0;
 }
 
+int perf_evlist__start_workload_ex(struct perf_evlist *evlist,
+				   workload_callback_t callback, void *arg)
+{
+	int ret = callback(evlist, arg);
+
+	if (ret == 0)
+		ret = perf_evlist__start_workload(evlist);
+
+	return ret;
+}
+
 int perf_evlist__parse_sample(struct perf_evlist *evlist, union perf_event *event,
 			      struct perf_sample *sample)
 {
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 115d8b53c601..a8ee12c54195 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -128,6 +128,10 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 						     void *ucontext));
 int perf_evlist__start_workload(struct perf_evlist *evlist);
 
+typedef int (*workload_callback_t)(struct perf_evlist *evlist, void *arg);
+int perf_evlist__start_workload_ex(struct perf_evlist *evlist,
+				   workload_callback_t callback, void *arg);
+
 struct option;
 
 int __perf_evlist__parse_mmap_pages(unsigned int *mmap_pages, const char *str);
-- 
2.5.0

[PATCH 2/2] perf record: Synthesize COMM event for a command line workload

[Namhyung Kim]

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 if events are processed and displayed before the
COMM event overrides like in 'perf script'.  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.

Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-record.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 142eeb341b29..b83373adb9f8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -469,6 +469,43 @@ static void workload_exec_failed_signal(int signo __maybe_unused,
 	child_finished = 1;
 }
 
+static int synthesize_workload_comm_event(struct perf_evlist *evlist, void *arg)
+{
+	union perf_event *event;
+	struct record *rec = arg;
+	struct machine *machine = &rec->session->machines.host;
+	int pid = evlist->workload.pid;
+	const char *comm_str = program_invocation_short_name;
+	size_t comm_size, total_size;
+	int ret;
+
+	comm_size = PERF_ALIGN(strlen(comm_str) + 1, sizeof(u64));
+	total_size = sizeof(event->comm) + machine->id_hdr_size;
+	/*
+	 * (aligned) comm size might be smaller than expected size
+	 * (i.e.  size of event->comm.comm[]), in that case it needs
+	 * to shrink the total size.
+	 */
+	if (comm_size < sizeof(event->comm.comm))
+		total_size -= sizeof(event->comm.comm) - comm_size;
+
+	event = zalloc(total_size);
+	if (event == NULL)
+		return -ENOMEM;
+
+	event->comm.header.type = PERF_RECORD_COMM;
+	event->comm.header.size = total_size;
+
+	event->comm.pid = pid;
+	event->comm.tid = pid;
+	strncpy(event->comm.comm, comm_str, comm_size);
+
+	ret = record__write(rec, event, total_size);
+
+	free(event);
+	return ret;
+}
+
 static void snapshot_sig_handler(int sig);
 
 static int __cmd_record(struct record *rec, int argc, const char **argv)
@@ -637,7 +674,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	 * Let the child rip
 	 */
 	if (forks)
-		perf_evlist__start_workload(rec->evlist);
+		perf_evlist__start_workload_ex(rec->evlist,
+					       synthesize_workload_comm_event,
+					       rec);
 
 	if (opts->initial_delay) {
 		usleep(opts->initial_delay * 1000);
-- 
2.5.0

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

[Arnaldo Carvalho de Melo]

Em Mon, Sep 21, 2015 at 09:26:49AM +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 if events are processed and displayed before the
> COMM event overrides like in 'perf script'.  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.
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-record.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 142eeb341b29..b83373adb9f8 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -469,6 +469,43 @@ static void workload_exec_failed_signal(int signo __maybe_unused,
>  	child_finished = 1;
>  }
>  
> +static int synthesize_workload_comm_event(struct perf_evlist *evlist, void *arg)
> +{
> +	union perf_event *event;
> +	struct record *rec = arg;
> +	struct machine *machine = &rec->session->machines.host;
> +	int pid = evlist->workload.pid;
> +	const char *comm_str = program_invocation_short_name;
> +	size_t comm_size, total_size;
> +	int ret;
> +
> +	comm_size = PERF_ALIGN(strlen(comm_str) + 1, sizeof(u64));
> +	total_size = sizeof(event->comm) + machine->id_hdr_size;
> +	/*
> +	 * (aligned) comm size might be smaller than expected size
> +	 * (i.e.  size of event->comm.comm[]), in that case it needs
> +	 * to shrink the total size.
> +	 */
> +	if (comm_size < sizeof(event->comm.comm))
> +		total_size -= sizeof(event->comm.comm) - comm_size;
> +
> +	event = zalloc(total_size);
> +	if (event == NULL)
> +		return -ENOMEM;
> +
> +	event->comm.header.type = PERF_RECORD_COMM;
> +	event->comm.header.size = total_size;
> +
> +	event->comm.pid = pid;
> +	event->comm.tid = pid;
> +	strncpy(event->comm.comm, comm_str, comm_size);
> +
> +	ret = record__write(rec, event, total_size);
> +
> +	free(event);
> +	return ret;
> +}
> +
>  static void snapshot_sig_handler(int sig);
>  
>  static int __cmd_record(struct record *rec, int argc, const char **argv)
> @@ -637,7 +674,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  	 * Let the child rip
>  	 */
>  	if (forks)
> -		perf_evlist__start_workload(rec->evlist);
> +		perf_evlist__start_workload_ex(rec->evlist,
> +					       synthesize_workload_comm_event,
> +					       rec);

Why not call it directly? I.e.:

	if (forks) {
		err = synthesize_workload_comm_event(evlist, rec);
		if (!err)
			err = perf_evlist__start_workload(rec->evlist);
	}

Since, from what I saw, the very first thing that
perf_evlist__start_workload_ex() does is to call the callback?

Also, don't we have already a synthesize_comm routine? I.e. can't
perf_event__prepare_comm() be used here?

Something like:

	union perf_event event;
	pid_t tgid, ppid;

	err = perf_event__prepare_comm(&event, evlist->workload.pid,
				       machine, &tgid, &ppid);
	if (!err)
		err = record__write(rec, &event, sizeof(event.comm));

- Arnaldo

>  
>  	if (opts->initial_delay) {
>  		usleep(opts->initial_delay * 1000);
> -- 
> 2.5.0

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

[Namhyung Kim]

Hi Arnaldo,

On Mon, Sep 21, 2015 at 04:39:26PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 21, 2015 at 09:26:49AM +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 if events are processed and displayed before the
> > COMM event overrides like in 'perf script'.  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.
> > 
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-record.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 142eeb341b29..b83373adb9f8 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -469,6 +469,43 @@ static void workload_exec_failed_signal(int signo __maybe_unused,
> >  	child_finished = 1;
> >  }
> >  
> > +static int synthesize_workload_comm_event(struct perf_evlist *evlist, void *arg)
> > +{
> > +	union perf_event *event;
> > +	struct record *rec = arg;
> > +	struct machine *machine = &rec->session->machines.host;
> > +	int pid = evlist->workload.pid;
> > +	const char *comm_str = program_invocation_short_name;
> > +	size_t comm_size, total_size;
> > +	int ret;
> > +
> > +	comm_size = PERF_ALIGN(strlen(comm_str) + 1, sizeof(u64));
> > +	total_size = sizeof(event->comm) + machine->id_hdr_size;
> > +	/*
> > +	 * (aligned) comm size might be smaller than expected size
> > +	 * (i.e.  size of event->comm.comm[]), in that case it needs
> > +	 * to shrink the total size.
> > +	 */
> > +	if (comm_size < sizeof(event->comm.comm))
> > +		total_size -= sizeof(event->comm.comm) - comm_size;
> > +
> > +	event = zalloc(total_size);
> > +	if (event == NULL)
> > +		return -ENOMEM;
> > +
> > +	event->comm.header.type = PERF_RECORD_COMM;
> > +	event->comm.header.size = total_size;
> > +
> > +	event->comm.pid = pid;
> > +	event->comm.tid = pid;
> > +	strncpy(event->comm.comm, comm_str, comm_size);
> > +
> > +	ret = record__write(rec, event, total_size);
> > +
> > +	free(event);
> > +	return ret;
> > +}
> > +
> >  static void snapshot_sig_handler(int sig);
> >  
> >  static int __cmd_record(struct record *rec, int argc, const char **argv)
> > @@ -637,7 +674,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> >  	 * Let the child rip
> >  	 */
> >  	if (forks)
> > -		perf_evlist__start_workload(rec->evlist);
> > +		perf_evlist__start_workload_ex(rec->evlist,
> > +					       synthesize_workload_comm_event,
> > +					       rec);
> 
> Why not call it directly? I.e.:
> 
> 	if (forks) {
> 		err = synthesize_workload_comm_event(evlist, rec);
> 		if (!err)
> 			err = perf_evlist__start_workload(rec->evlist);
> 	}
> 
> Since, from what I saw, the very first thing that
> perf_evlist__start_workload_ex() does is to call the callback?

I originally thought that it'd be used by other commands too.  I
checked that 'perf trace' has similar code so I generalized it with
callbacks.  But then I realized the perf trace generates events only
after exec() so I dropped the patch for it.


> 
> Also, don't we have already a synthesize_comm routine? I.e. can't
> perf_event__prepare_comm() be used here?

Ok, it'd be better exporting perf_event__synthesize_comm() for
consistency then.  Will send v2 soon.

Thanks,
Namhyung