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

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

[Namhyung Kim]

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@kernel.org>
---
 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);
 
 		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

[PATCH 2/3] perf tools: Add kallsyms__get_symbol_start()

[Namhyung Kim]

The kallsyms__get_symbol_start() to get any symbol address from
kallsyms.  The existing kallsyms__get_function_start() only allows text
symbols so create this to allow data symbols too.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/event.c | 30 +++++++++++++++++++++++++++---
 tools/perf/util/event.h |  2 ++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 3860b0c74829..6fdda0eb3854 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -93,8 +93,8 @@ struct process_symbol_args {
 	u64	   start;
 };
 
-static int find_symbol_cb(void *arg, const char *name, char type,
-			  u64 start)
+static int find_func_symbol_cb(void *arg, const char *name, char type,
+			       u64 start)
 {
 	struct process_symbol_args *args = arg;
 
@@ -110,12 +110,36 @@ static int find_symbol_cb(void *arg, const char *name, char type,
 	return 1;
 }
 
+static int find_any_symbol_cb(void *arg, const char *name,
+			      char type __maybe_unused, u64 start)
+{
+	struct process_symbol_args *args = arg;
+
+	if (strcmp(name, args->name))
+		return 0;
+
+	args->start = start;
+	return 1;
+}
+
 int kallsyms__get_function_start(const char *kallsyms_filename,
 				 const char *symbol_name, u64 *addr)
 {
 	struct process_symbol_args args = { .name = symbol_name, };
 
-	if (kallsyms__parse(kallsyms_filename, &args, find_symbol_cb) <= 0)
+	if (kallsyms__parse(kallsyms_filename, &args, find_func_symbol_cb) <= 0)
+		return -1;
+
+	*addr = args.start;
+	return 0;
+}
+
+int kallsyms__get_symbol_start(const char *kallsyms_filename,
+			       const char *symbol_name, u64 *addr)
+{
+	struct process_symbol_args args = { .name = symbol_name, };
+
+	if (kallsyms__parse(kallsyms_filename, &args, find_any_symbol_cb) <= 0)
 		return -1;
 
 	*addr = args.start;
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index de20e01c9d72..d8bcee2e9b93 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -360,6 +360,8 @@ size_t perf_event__fprintf(union perf_event *event, struct machine *machine, FIL
 
 int kallsyms__get_function_start(const char *kallsyms_filename,
 				 const char *symbol_name, u64 *addr);
+int kallsyms__get_symbol_start(const char *kallsyms_filename,
+			       const char *symbol_name, u64 *addr);
 
 void event_attr_init(struct perf_event_attr *attr);
 
-- 
2.41.0.185.g7c58973941-goog

[PATCH 3/3] perf machine: Include data symbols in the kernel map

[Namhyung Kim]

When perf record -d is used, it needs data mmaps to symbolize global data.
But it missed to collect kernel data maps so it cannot symbolize them.
Instead of having a separate map, just increase the kernel map size to
include the data section.

Probably we can have a separate kernel map for data, but the current
code assumes a single kernel map.  So it'd require more changes in other
places and looks error-prone.  I decided not to go that way for now.

Also it seems the kernel module size already includes the data section.

For example, my system has the following.

  $ grep -e _stext -e _etext -e _edata /proc/kallsyms
  ffffffff99800000 T _stext
  ffffffff9a601ac8 T _etext
  ffffffff9b446a00 D _edata

Size of the text section is (0x9a601ac8 - 0x99800000 = 0xe01ac8) and
size of the data section is (0x9b446a00 - 0x99800000 = 0x1c46a00).

Before:
  $ perf record -d true

  $ perf report -D | grep MMAP | head -1
  0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0xe01ac8) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
                                                               ^^^^^^^^
                                                                 here
After:
  $ perf report -D | grep MMAP | head -1
  0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0x1c46a00) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
                                                               ^^^^^^^^^

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/machine.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index ddc0a2130caf..e93a66f6e0b3 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1218,7 +1218,10 @@ static int machine__get_running_kernel_start(struct machine *machine,
 
 	*start = addr;
 
-	err = kallsyms__get_function_start(filename, "_etext", &addr);
+	if (machine->has_data_mmap)
+		err = kallsyms__get_symbol_start(filename, "_edata", &addr);
+	else
+		err = kallsyms__get_function_start(filename, "_etext", &addr);
 	if (!err)
 		*end = addr;
 
-- 
2.41.0.185.g7c58973941-goog

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

[Ian Rogers]

On Tue, Jun 20, 2023 at 1:18 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

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

Re: [PATCH 2/3] perf tools: Add kallsyms__get_symbol_start()

[Ian Rogers]

On Tue, Jun 20, 2023 at 1:18 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The kallsyms__get_symbol_start() to get any symbol address from
> kallsyms.  The existing kallsyms__get_function_start() only allows text
> symbols so create this to allow data symbols too.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/event.c | 30 +++++++++++++++++++++++++++---
>  tools/perf/util/event.h |  2 ++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 3860b0c74829..6fdda0eb3854 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -93,8 +93,8 @@ struct process_symbol_args {
>         u64        start;
>  };
>
> -static int find_symbol_cb(void *arg, const char *name, char type,
> -                         u64 start)
> +static int find_func_symbol_cb(void *arg, const char *name, char type,
> +                              u64 start)
>  {
>         struct process_symbol_args *args = arg;
>
> @@ -110,12 +110,36 @@ static int find_symbol_cb(void *arg, const char *name, char type,
>         return 1;
>  }
>
> +static int find_any_symbol_cb(void *arg, const char *name,
> +                             char type __maybe_unused, u64 start)
> +{
> +       struct process_symbol_args *args = arg;
> +
> +       if (strcmp(name, args->name))
> +               return 0;
> +
> +       args->start = start;
> +       return 1;
> +}
> +
>  int kallsyms__get_function_start(const char *kallsyms_filename,
>                                  const char *symbol_name, u64 *addr)
>  {
>         struct process_symbol_args args = { .name = symbol_name, };
>
> -       if (kallsyms__parse(kallsyms_filename, &args, find_symbol_cb) <= 0)
> +       if (kallsyms__parse(kallsyms_filename, &args, find_func_symbol_cb) <= 0)
> +               return -1;
> +
> +       *addr = args.start;
> +       return 0;
> +}
> +
> +int kallsyms__get_symbol_start(const char *kallsyms_filename,
> +                              const char *symbol_name, u64 *addr)
> +{
> +       struct process_symbol_args args = { .name = symbol_name, };
> +
> +       if (kallsyms__parse(kallsyms_filename, &args, find_any_symbol_cb) <= 0)
>                 return -1;
>
>         *addr = args.start;
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index de20e01c9d72..d8bcee2e9b93 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -360,6 +360,8 @@ size_t perf_event__fprintf(union perf_event *event, struct machine *machine, FIL
>
>  int kallsyms__get_function_start(const char *kallsyms_filename,
>                                  const char *symbol_name, u64 *addr);
> +int kallsyms__get_symbol_start(const char *kallsyms_filename,
> +                              const char *symbol_name, u64 *addr);
>
>  void event_attr_init(struct perf_event_attr *attr);
>
> --
> 2.41.0.185.g7c58973941-goog
>

Re: [PATCH 3/3] perf machine: Include data symbols in the kernel map

[Ian Rogers]

On Tue, Jun 20, 2023 at 1:18 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> When perf record -d is used, it needs data mmaps to symbolize global data.
> But it missed to collect kernel data maps so it cannot symbolize them.
> Instead of having a separate map, just increase the kernel map size to
> include the data section.
>
> Probably we can have a separate kernel map for data, but the current
> code assumes a single kernel map.  So it'd require more changes in other
> places and looks error-prone.  I decided not to go that way for now.
>
> Also it seems the kernel module size already includes the data section.
>
> For example, my system has the following.
>
>   $ grep -e _stext -e _etext -e _edata /proc/kallsyms
>   ffffffff99800000 T _stext
>   ffffffff9a601ac8 T _etext
>   ffffffff9b446a00 D _edata
>
> Size of the text section is (0x9a601ac8 - 0x99800000 = 0xe01ac8) and
> size of the data section is (0x9b446a00 - 0x99800000 = 0x1c46a00).
>
> Before:
>   $ perf record -d true
>
>   $ perf report -D | grep MMAP | head -1
>   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0xe01ac8) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
>                                                                ^^^^^^^^
>                                                                  here

nit: should the ^^^ be under 0xe01ac8?

> After:
>   $ perf report -D | grep MMAP | head -1
>   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0x1c46a00) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
>                                                                ^^^^^^^^^

nit: and here under 0x1c46a00 ?

>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/machine.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index ddc0a2130caf..e93a66f6e0b3 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1218,7 +1218,10 @@ static int machine__get_running_kernel_start(struct machine *machine,
>
>         *start = addr;
>
> -       err = kallsyms__get_function_start(filename, "_etext", &addr);
> +       if (machine->has_data_mmap)
> +               err = kallsyms__get_symbol_start(filename, "_edata", &addr);
> +       else
> +               err = kallsyms__get_function_start(filename, "_etext", &addr);
>         if (!err)
>                 *end = addr;
>
> --
> 2.41.0.185.g7c58973941-goog
>

Re: [PATCH 3/3] perf machine: Include data symbols in the kernel map

[Adrian Hunter]

On 20/06/23 23:18, Namhyung Kim wrote:
> When perf record -d is used, it needs data mmaps to symbolize global data.
> But it missed to collect kernel data maps so it cannot symbolize them.
> Instead of having a separate map, just increase the kernel map size to
> include the data section.
> 
> Probably we can have a separate kernel map for data, but the current
> code assumes a single kernel map.  So it'd require more changes in other
> places and looks error-prone.  I decided not to go that way for now.
> 
> Also it seems the kernel module size already includes the data section.
> 
> For example, my system has the following.
> 
>   $ grep -e _stext -e _etext -e _edata /proc/kallsyms
>   ffffffff99800000 T _stext
>   ffffffff9a601ac8 T _etext
>   ffffffff9b446a00 D _edata
> 
> Size of the text section is (0x9a601ac8 - 0x99800000 = 0xe01ac8) and
> size of the data section is (0x9b446a00 - 0x99800000 = 0x1c46a00).
> 
> Before:
>   $ perf record -d true
> 
>   $ perf report -D | grep MMAP | head -1
>   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0xe01ac8) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
>                                                                ^^^^^^^^
>                                                                  here
> After:
>   $ perf report -D | grep MMAP | head -1
>   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0x1c46a00) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
>                                                                ^^^^^^^^^
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/machine.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index ddc0a2130caf..e93a66f6e0b3 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1218,7 +1218,10 @@ static int machine__get_running_kernel_start(struct machine *machine,
>  
>  	*start = addr;
>  
> -	err = kallsyms__get_function_start(filename, "_etext", &addr);
> +	if (machine->has_data_mmap)
> +		err = kallsyms__get_symbol_start(filename, "_edata", &addr);
> +	else
> +		err = kallsyms__get_function_start(filename, "_etext", &addr);

What is the downside of just extending it unconditionally?

>  	if (!err)
>  		*end = addr;
>  

Re: [PATCH 3/3] perf machine: Include data symbols in the kernel map

[Namhyung Kim]

Hi Adrian,

On Tue, Jul 11, 2023 at 8:19 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 20/06/23 23:18, Namhyung Kim wrote:
> > When perf record -d is used, it needs data mmaps to symbolize global data.
> > But it missed to collect kernel data maps so it cannot symbolize them.
> > Instead of having a separate map, just increase the kernel map size to
> > include the data section.
> >
> > Probably we can have a separate kernel map for data, but the current
> > code assumes a single kernel map.  So it'd require more changes in other
> > places and looks error-prone.  I decided not to go that way for now.
> >
> > Also it seems the kernel module size already includes the data section.
> >
> > For example, my system has the following.
> >
> >   $ grep -e _stext -e _etext -e _edata /proc/kallsyms
> >   ffffffff99800000 T _stext
> >   ffffffff9a601ac8 T _etext
> >   ffffffff9b446a00 D _edata
> >
> > Size of the text section is (0x9a601ac8 - 0x99800000 = 0xe01ac8) and
> > size of the data section is (0x9b446a00 - 0x99800000 = 0x1c46a00).
> >
> > Before:
> >   $ perf record -d true
> >
> >   $ perf report -D | grep MMAP | head -1
> >   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0xe01ac8) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
> >                                                                ^^^^^^^^
> >                                                                  here
> > After:
> >   $ perf report -D | grep MMAP | head -1
> >   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0x1c46a00) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
> >                                                                ^^^^^^^^^
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/machine.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index ddc0a2130caf..e93a66f6e0b3 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1218,7 +1218,10 @@ static int machine__get_running_kernel_start(struct machine *machine,
> >
> >       *start = addr;
> >
> > -     err = kallsyms__get_function_start(filename, "_etext", &addr);
> > +     if (machine->has_data_mmap)
> > +             err = kallsyms__get_symbol_start(filename, "_edata", &addr);
> > +     else
> > +             err = kallsyms__get_function_start(filename, "_etext", &addr);
>
> What is the downside of just extending it unconditionally?

I don't know.. maybe some people would argue it needs the
proper protection bits other than 'x' but this patch also breaks it.
But as I said, I'm not sure if we really want to change that now.

That said, we can make it unconditional. :)

Thanks,
Namhyung

>
> >       if (!err)
> >               *end = addr;
> >
>

Re: [PATCH 3/3] perf machine: Include data symbols in the kernel map

[Adrian Hunter]

On 11/07/23 20:30, Namhyung Kim wrote:
> Hi Adrian,
> 
> On Tue, Jul 11, 2023 at 8:19 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 20/06/23 23:18, Namhyung Kim wrote:
>>> When perf record -d is used, it needs data mmaps to symbolize global data.
>>> But it missed to collect kernel data maps so it cannot symbolize them.
>>> Instead of having a separate map, just increase the kernel map size to
>>> include the data section.
>>>
>>> Probably we can have a separate kernel map for data, but the current
>>> code assumes a single kernel map.  So it'd require more changes in other
>>> places and looks error-prone.  I decided not to go that way for now.
>>>
>>> Also it seems the kernel module size already includes the data section.
>>>
>>> For example, my system has the following.
>>>
>>>   $ grep -e _stext -e _etext -e _edata /proc/kallsyms
>>>   ffffffff99800000 T _stext
>>>   ffffffff9a601ac8 T _etext
>>>   ffffffff9b446a00 D _edata
>>>
>>> Size of the text section is (0x9a601ac8 - 0x99800000 = 0xe01ac8) and
>>> size of the data section is (0x9b446a00 - 0x99800000 = 0x1c46a00).
>>>
>>> Before:
>>>   $ perf record -d true
>>>
>>>   $ perf report -D | grep MMAP | head -1
>>>   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0xe01ac8) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
>>>                                                                ^^^^^^^^
>>>                                                                  here
>>> After:
>>>   $ perf report -D | grep MMAP | head -1
>>>   0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0x1c46a00) @ 0xffffffff99800000]: x [kernel.kallsyms]_text
>>>                                                                ^^^^^^^^^
>>>
>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>> ---
>>>  tools/perf/util/machine.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>>> index ddc0a2130caf..e93a66f6e0b3 100644
>>> --- a/tools/perf/util/machine.c
>>> +++ b/tools/perf/util/machine.c
>>> @@ -1218,7 +1218,10 @@ static int machine__get_running_kernel_start(struct machine *machine,
>>>
>>>       *start = addr;
>>>
>>> -     err = kallsyms__get_function_start(filename, "_etext", &addr);
>>> +     if (machine->has_data_mmap)
>>> +             err = kallsyms__get_symbol_start(filename, "_edata", &addr);
>>> +     else
>>> +             err = kallsyms__get_function_start(filename, "_etext", &addr);
>>
>> What is the downside of just extending it unconditionally?
> 
> I don't know.. maybe some people would argue it needs the
> proper protection bits other than 'x' but this patch also breaks it.
> But as I said, I'm not sure if we really want to change that now.
> 
> That said, we can make it unconditional. :)

Might as well to start with.  Will need a big comment.

Also do we know if all arch's do it like that? Perhaps
need to fallback to _etext if _edata is not found?

> 
> Thanks,
> Namhyung
> 
>>
>>>       if (!err)
>>>               *end = addr;
>>>
>>