Re: [PATCH] perf: add PERF_RECORD_EXEC type, to distinguish from PERF_RECORD_COMM (DO NOT APPLY)

[Namhyung Kim <namhyung.kim@lge.com>]

Hi, again.

2012-03-03 4:30 AM, Luigi Semenzato wrote:
> ---- NOT FINISHED - NOT TESTED ---- rfc only
>
> I agree with others that adding a new record type is the cleanest solution.
> This is more or less what it takes to add a new record type.  It may be
> more than we like but that's a separate problem.  I am open to other
> solutions.  I may be able to do a bit of refactoring to reduce the
> copy-paste, but of course the CL will grow as the refactoring would
> not be limited to COMM and EXEC.
>
> ---- actual commit message below ----
>
> Currently the kernel produces a PERF_RECORD_COMM type record both when
> a process execs and when it renames its "comm" name.  The "perf report"
> command interprets each COMM record as an exec, and flushes all
> mappings to executables when it encounters one.  This can result in the
> inability to correlate IP samples to function symbols.
>
> This CL adds a PERF_RECORD_EXEC type, which doesn't contain the process
> name (the comm field).  Thus, an exec now must send two records, one EXEC
> and one COMM, whereas a rename sends only a COMM.
>
> The change is mostly straightforward, but there are some complications
> in the synthesized events sent when "perf record" starts to account for
> existing processes.
>
> Signed-off-by: Luigi Semenzato<semenzato@chromium.org>
> ---
>   fs/exec.c                  |    1 +
>   include/linux/perf_event.h |   19 +++++-
>   kernel/events/core.c       |  153 +++++++++++++++++++++++++++++++++++++++++---
>   tools/perf/builtin-test.c  |   24 ++-----
>   tools/perf/builtin-top.c   |    5 +-
>   tools/perf/util/event.c    |  148 +++++++++++++++++++++++++++++-------------
>   tools/perf/util/event.h    |   11 +++
>   tools/perf/util/evsel.c    |    5 +-
>   tools/perf/util/python.c   |   42 +++++++++++-
>   tools/perf/util/session.c  |   11 +++
>   tools/perf/util/thread.c   |    1 -
>   tools/perf/util/tool.h     |    1 +
>   12 files changed, 338 insertions(+), 83 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index e33501a..077d199 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1144,6 +1144,7 @@ void setup_new_exec(struct linux_binprm * bprm)
>   	else
>   		set_dumpable(current->mm, suid_dumpable);
>
> +        perf_event_exec(current);
>   	set_task_comm(current, bprm->tcomm);
>
>   	/* Set the new mm task size. We have to do that late because it may
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 64426b7..8e4a472 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -200,8 +200,8 @@ struct perf_event_attr {
>   				exclude_kernel :  1, /* ditto kernel          */
>   				exclude_hv     :  1, /* ditto hypervisor      */
>   				exclude_idle   :  1, /* don't count when idle */
> -				mmap           :  1, /* include mmap data     */
> -				comm	       :  1, /* include comm data     */
> +				mmap_attr      :  1, /* include mmap data     */
> +				comm_attr      :  1, /* include comm data     */
>   				freq           :  1, /* use freq, not period  */
>   				inherit_stat   :  1, /* per task counts       */
>   				enable_on_exec :  1, /* next exec enables     */
> @@ -223,8 +223,10 @@ struct perf_event_attr {
>
>   				exclude_host   :  1, /* don't count in host   */
>   				exclude_guest  :  1, /* don't count in guest  */
> +                                /* COMM used to mean exec in older versions   */
> +                                exec_attr      :  1, /* include exec data     */
>
> -				__reserved_1   : 43;
> +				__reserved_1   : 42;
>

This new bit will cause the same issue with ->sample_id_all and 
->exclude_{host,guest} on old kernels. It needs to be handled too in a similar 
way of perf record/top IMHO.

Thanks,
Namhyung