Re: [PATCH 1/3] perf machine: Add machine->has_data_mmap field

From: Ian Rogers
Date: Thu Jun 22 2023 - 13:17:50 EST

Next message: Yonghong Song: "Re: [PATCH bpf-next] bpf, docs: document existing macros instead of deprecated"
Previous message: Mathieu Poirier: "Re: [PATCH v13 00/11] Add support for MT8195 SCP 2nd core"
In reply to: Ian Rogers: "Re: [PATCH 3/3] perf machine: Include data symbols in the kernel map"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jun 20, 2023 at 1:18 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> So that it can indicate the it needs to collect data mappings. This is
> needed especially for kernel maps.
>
> At first, I just wanted to add it to struct machines only and to use the
> machine->machines to check the value. But it turned out that some of
> machine didn't belong to any machines (eg. some test codes), so I just
> copied the value to individual struct machine.
>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Acked-by: Ian Rogers <irogers@xxxxxxxxxx>

> ---
> tools/perf/builtin-inject.c | 2 +-
> tools/perf/builtin-record.c | 2 +-
> tools/perf/tests/vmlinux-kallsyms.c | 4 ++--
> tools/perf/util/machine.c | 12 +++++++-----
> tools/perf/util/machine.h | 10 ++++++++--
> tools/perf/util/session.c | 4 ++--
> tools/perf/util/session.h | 4 ++--
> 7 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index c8cf2fdd9cff..481adaa97a68 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -2325,7 +2325,7 @@ int cmd_inject(int argc, const char **argv)
> }
>
> inject.session = __perf_session__new(&data, repipe,
> - output_fd(&inject),
> + output_fd(&inject), false,
> &inject.tool);
> if (IS_ERR(inject.session)) {
> ret = PTR_ERR(inject.session);
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index aec18db7ff23..b4d0154dcb18 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2373,7 +2373,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> signal(SIGUSR2, SIG_IGN);
> }
>
> - session = perf_session__new(data, tool);
> + session = __perf_session__new(data, false, -1, rec->opts.sample_address, tool);
> if (IS_ERR(session)) {
> pr_err("Perf session creation failed.\n");
> return PTR_ERR(session);
> diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> index 1078a93b01aa..1f1ba3ae88aa 100644
> --- a/tools/perf/tests/vmlinux-kallsyms.c
> +++ b/tools/perf/tests/vmlinux-kallsyms.c
> @@ -131,8 +131,8 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> * Init the machines that will hold kernel, modules obtained from
> * both vmlinux + .ko files and from /proc/kallsyms split by modules.
> */
> - machine__init(&kallsyms, "", HOST_KERNEL_ID);
> - machine__init(&vmlinux, "", HOST_KERNEL_ID);
> + machine__init(&kallsyms, "", HOST_KERNEL_ID, false);
> + machine__init(&vmlinux, "", HOST_KERNEL_ID, false);
>
> maps = machine__kernel_maps(&vmlinux);
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 4e62843d51b7..ddc0a2130caf 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -110,7 +110,7 @@ static void thread__set_guest_comm(struct thread *thread, pid_t pid)
> thread__set_comm(thread, comm, 0);
> }
>
> -int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
> +int machine__init(struct machine *machine, const char *root_dir, pid_t pid, bool has_data_mmap)
> {
> int err = -ENOMEM;
>
> @@ -128,6 +128,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
> machine->env = NULL;
>
> machine->pid = pid;
> + machine->has_data_mmap = has_data_mmap;
>
> machine->id_hdr_size = 0;
> machine->kptr_restrict_warned = false;
> @@ -170,7 +171,7 @@ struct machine *machine__new_host(void)
> struct machine *machine = malloc(sizeof(*machine));
>
> if (machine != NULL) {
> - machine__init(machine, "", HOST_KERNEL_ID);
> + machine__init(machine, "", HOST_KERNEL_ID, false);

nit: when passing constants it can be nice to name the parameter, so
here "/*data_mmap=*/false". There are some checkers based on this
like:
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html

Thanks,
Ian

>
> if (machine__create_kernel_maps(machine) < 0)
> goto out_delete;
> @@ -272,10 +273,11 @@ void machine__delete(struct machine *machine)
> }
> }
>
> -void machines__init(struct machines *machines)
> +void __machines__init(struct machines *machines, bool have_data_mmap)
> {
> - machine__init(&machines->host, "", HOST_KERNEL_ID);
> + machine__init(&machines->host, "", HOST_KERNEL_ID, have_data_mmap);
> machines->guests = RB_ROOT_CACHED;
> + machines->have_data_mmaps = have_data_mmap;
> }
>
> void machines__exit(struct machines *machines)
> @@ -295,7 +297,7 @@ struct machine *machines__add(struct machines *machines, pid_t pid,
> if (machine == NULL)
> return NULL;
>
> - if (machine__init(machine, root_dir, pid) != 0) {
> + if (machine__init(machine, root_dir, pid, machines->have_data_mmaps) != 0) {
> free(machine);
> return NULL;
> }
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index d034ecaf89c1..f54e5c888a99 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -46,6 +46,7 @@ struct machine {
> bool comm_exec;
> bool kptr_restrict_warned;
> bool single_address_space;
> + bool has_data_mmap;
> char *root_dir;
> char *mmap_name;
> char *kallsyms_filename;
> @@ -160,9 +161,14 @@ typedef void (*machine__process_t)(struct machine *machine, void *data);
> struct machines {
> struct machine host;
> struct rb_root_cached guests;
> + bool have_data_mmaps;
> };
>
> -void machines__init(struct machines *machines);
> +void __machines__init(struct machines *machines, bool data_mmap);
> +static inline void machines__init(struct machines *machines)
> +{
> + __machines__init(machines, false);
> +}
> void machines__exit(struct machines *machines);
>
> void machines__process_guests(struct machines *machines,
> @@ -181,7 +187,7 @@ void machines__set_comm_exec(struct machines *machines, bool comm_exec);
>
> struct machine *machine__new_host(void);
> struct machine *machine__new_kallsyms(void);
> -int machine__init(struct machine *machine, const char *root_dir, pid_t pid);
> +int machine__init(struct machine *machine, const char *root_dir, pid_t pid, bool has_data_mmap);
> void machine__exit(struct machine *machine);
> void machine__delete_threads(struct machine *machine);
> void machine__delete(struct machine *machine);
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 00d18c74c090..e09a02ec8f28 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -191,7 +191,7 @@ static int ordered_events__deliver_event(struct ordered_events *oe,
> }
>
> struct perf_session *__perf_session__new(struct perf_data *data,
> - bool repipe, int repipe_fd,
> + bool repipe, int repipe_fd, bool data_mmap,
> struct perf_tool *tool)
> {
> int ret = -ENOMEM;
> @@ -205,7 +205,7 @@ struct perf_session *__perf_session__new(struct perf_data *data,
> session->decomp_data.zstd_decomp = &session->zstd_data;
> session->active_decomp = &session->decomp_data;
> INIT_LIST_HEAD(&session->auxtrace_index);
> - machines__init(&session->machines);
> + __machines__init(&session->machines, data_mmap);
> ordered_events__init(&session->ordered_events,
> ordered_events__deliver_event, NULL);
>
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index ee3715e8563b..465610e81e95 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -64,13 +64,13 @@ struct decomp {
> struct perf_tool;
>
> struct perf_session *__perf_session__new(struct perf_data *data,
> - bool repipe, int repipe_fd,
> + bool repipe, int repipe_fd, bool data_mmap,
> struct perf_tool *tool);
>
> static inline struct perf_session *perf_session__new(struct perf_data *data,
> struct perf_tool *tool)
> {
> - return __perf_session__new(data, false, -1, tool);
> + return __perf_session__new(data, false, -1, false, tool);
> }
>
> void perf_session__delete(struct perf_session *session);
> --
> 2.41.0.185.g7c58973941-goog
>

Next message: Yonghong Song: "Re: [PATCH bpf-next] bpf, docs: document existing macros instead of deprecated"
Previous message: Mathieu Poirier: "Re: [PATCH v13 00/11] Add support for MT8195 SCP 2nd core"
In reply to: Ian Rogers: "Re: [PATCH 3/3] perf machine: Include data symbols in the kernel map"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]