Message-ID: <CAH0uvojWYbaeDi2=MLkNi+R+N3yAE5rNEV2Ube0Xm21pz6nTTA@mail.gmail.com>
Date: Fri, 14 Feb 2025 09:06:23 -0800
From: Howard Chu <howardchu95@...il.com>
To: Krzysztof Łopatowski <krzysztof.m.lopatowski@...il.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Ian Rogers <irogers@...gle.com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: perf: Question about machine__create_extra_kernel_maps and
 trampoline symbols

Hello Krzysztof,

Thanks for looking into this.

On Thu, Feb 13, 2025 at 5:10 AM Krzysztof Łopatowski
<krzysztof.m.lopatowski@...il.com> wrote:
>
> Hi,
>
> I'm investigating performance issues with perf's kallsyms parsing. Running
> `perf record -g perf trace -a --max-events 1` on an x86_64 Ubuntu 24.10 on a VM

This might be off-topic, but since you (and I think most of the people
as well) didn't use perf trace with --call-graph and
--kernel-syscall-graph.

sudo ./perf trace --call-graph fp --kernel-syscall-graph -a -- sleep 10

Can we do something like:

 tools/perf/builtin-buildid-list.c     | 2 +-
 tools/perf/builtin-trace.c            | 6 +++---
 tools/perf/tests/code-reading.c       | 2 +-
 tools/perf/tests/dlfilter-test.c      | 2 +-
 tools/perf/tests/dwarf-unwind.c       | 2 +-
 tools/perf/tests/mmap-thread-lookup.c | 2 +-
 tools/perf/tests/symbols.c            | 2 +-
 tools/perf/util/machine.c             | 6 +++---
 tools/perf/util/machine.h             | 2 +-
 tools/perf/util/probe-event.c         | 2 +-
 10 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-buildid-list.c
b/tools/perf/builtin-buildid-list.c
index 52dfacaff8e3..357201d8ef0c 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -47,7 +47,7 @@ static void buildid__show_kernel_maps(void)
 {
         struct machine *machine;

-        machine = machine__new_host();
+        machine = machine__new_host(true);
         machine__for_each_kernel_map(machine, buildid__map_cb, NULL);
         machine__delete(machine);
 }
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index f55a8a6481f2..e2d840d4787a 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1956,14 +1956,14 @@ static char
*trace__machine__resolve_kernel_addr(void *vmachine, unsigned long l
         return machine__resolve_kernel_addr(vmachine, addrp, modp);
 }

-static int trace__symbols_init(struct trace *trace, struct evlist *evlist)
+static int trace__symbols_init(struct trace *trace, struct evlist
*evlist, bool create_kernel_maps)
 {
         int err = symbol__init(NULL);

         if (err)
                 return err;

-        trace->host = machine__new_host();
+        trace->host = machine__new_host(create_kernel_maps);
         if (trace->host == NULL)
                 return -ENOMEM;

@@ -4340,7 +4340,7 @@ static int trace__run(struct trace *trace, int
argc, const char **argv)
                 goto out_delete_evlist;
         }

-        err = trace__symbols_init(trace, evlist);
+        err = trace__symbols_init(trace, evlist,
callchain_param.enabled && trace->kernel_syscallchains);
         if (err < 0) {
                 fprintf(trace->output, "Problems initializing symbol
libraries!\n");
                 goto out_delete_evlist;
diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index cf6edbe697b2..17c7b9f95532 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -654,7 +654,7 @@ static int do_test_code_reading(bool try_kcore)

         pid = getpid();

-        machine = machine__new_host();
+        machine = machine__new_host(true);
         machine->env = &perf_env;

         ret = machine__create_kernel_maps(machine);
diff --git a/tools/perf/tests/dlfilter-test.c b/tools/perf/tests/dlfilter-test.c
index 54f59d1246bc..11f70ebabacf 100644
--- a/tools/perf/tests/dlfilter-test.c
+++ b/tools/perf/tests/dlfilter-test.c
@@ -352,7 +352,7 @@ static int test__dlfilter_test(struct test_data *td)
                 return test_result("Failed to find program symbols",
TEST_FAIL);

         pr_debug("Creating new host machine structure\n");
-        td->machine = machine__new_host();
+        td->machine = machine__new_host(true);
         td->machine->env = &perf_env;

         td->fd = creat(td->perf_data_file_name, 0644);
diff --git a/tools/perf/tests/dwarf-unwind.c b/tools/perf/tests/dwarf-unwind.c
index 4803ab2d97ba..b76712f68403 100644
--- a/tools/perf/tests/dwarf-unwind.c
+++ b/tools/perf/tests/dwarf-unwind.c
@@ -203,7 +203,7 @@ noinline int test__dwarf_unwind(struct test_suite
*test __maybe_unused,
         struct thread *thread;
         int err = -1;

-        machine = machine__new_host();
+        machine = machine__new_host(true);
         if (!machine) {
                 pr_err("Could not get machine\n");
                 return -1;
diff --git a/tools/perf/tests/mmap-thread-lookup.c
b/tools/perf/tests/mmap-thread-lookup.c
index ddd1da9a4ba9..194b5affaa41 100644
--- a/tools/perf/tests/mmap-thread-lookup.c
+++ b/tools/perf/tests/mmap-thread-lookup.c
@@ -167,7 +167,7 @@ static int mmap_events(synth_cb synth)
          */
         TEST_ASSERT_VAL("failed to create threads", !threads_create());

-        machine = machine__new_host();
+        machine = machine__new_host(true);

         dump_trace = verbose > 1 ? 1 : 0;

diff --git a/tools/perf/tests/symbols.c b/tools/perf/tests/symbols.c
index ee20a366f32f..6b22a451211a 100644
--- a/tools/perf/tests/symbols.c
+++ b/tools/perf/tests/symbols.c
@@ -19,7 +19,7 @@ struct test_info {

 static int init_test_info(struct test_info *ti)
 {
-        ti->machine = machine__new_host();
+        ti->machine = machine__new_host(true);
         if (!ti->machine) {
                 pr_debug("machine__new_host() failed!\n");
                 return TEST_FAIL;
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 55d4977b9913..bfa0dc98264a 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -125,14 +125,14 @@ int machine__init(struct machine *machine, const
char *root_dir, pid_t pid)
         return 0;
 }

-struct machine *machine__new_host(void)
+struct machine *machine__new_host(bool create_kernel_maps)
 {
         struct machine *machine = malloc(sizeof(*machine));

         if (machine != NULL) {
                 machine__init(machine, "", HOST_KERNEL_ID);

-                if (machine__create_kernel_maps(machine) < 0)
+                if (create_kernel_maps &&
machine__create_kernel_maps(machine) < 0)
                         goto out_delete;

                 machine->env = &perf_env;
@@ -146,7 +146,7 @@ struct machine *machine__new_host(void)

 struct machine *machine__new_kallsyms(void)
 {
-        struct machine *machine = machine__new_host();
+        struct machine *machine = machine__new_host(true);
         /*
          * FIXME:
          * 1) We should switch to machine__load_kallsyms(), i.e. not explicitly
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index ae3e5542d57d..444e1961acf7 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -163,7 +163,7 @@ struct thread *machine__findnew_guest_code(struct
machine *machine, pid_t pid);
 void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size);
 void machines__set_comm_exec(struct machines *machines, bool comm_exec);

-struct machine *machine__new_host(void);
+struct machine *machine__new_host(bool create_kernel_maps);
 struct machine *machine__new_kallsyms(void);
 int machine__init(struct machine *machine, const char *root_dir, pid_t pid);
 void machine__exit(struct machine *machine);
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 307ad6242a4e..6b5b5542f454 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -94,7 +94,7 @@ int init_probe_symbol_maps(bool user_only)
         if (symbol_conf.vmlinux_name)
                 pr_debug("Use vmlinux: %s\n", symbol_conf.vmlinux_name);

-        host_machine = machine__new_host();
+        host_machine = machine__new_host(true);
         if (!host_machine) {
                 pr_debug("machine__new_host() failed.\n");
                 symbol__exit();
-- 
2.45.2

But I also like the idea of optimizing the ksym parsing/preprocessing.

I can't answer your questions yet, but I'll look into it.

Thanks,
Howard


> (perf version 6.11) showed that about 61% of time was spent in
> 'kallsyms__parse'.
> Total execution time was 370 ms. When running latest version from
> tmp.perf-tools-next
> It's 530ms total and 38% in 'kallsyms__parse' because the old version
> doesn't have
> bpf skeletons enabled.
> During regular execution this function is called three times:
>
> 1. In machine__get_running_kernel_start - searching for _text
> 2. In machine__get_running_kernel_start - searching for _edata
> 3. In machine__create_extra_kernel_maps - which is the focus of my question
>
> Regarding the third call (implemented in tools/perf/arch/x86/util/machine.c),
> I notice it searches for:
> - _entry_trampoline
> - __entry_SYSCALL_64_trampoline
>
> I'm puzzled by the dynamic allocation in add_extra_kernel_map, which seems to
> expect multiple __entry_SYSCALL_64_trampoline symbols. This functionality was
> introduced in:
> https://lore.kernel.org/all/1526986485-6562-1-git-send-email-adrian.hunter@intel.com/
>
> I've attempted to trigger the trampoline logic in two ways:
>
> 1. Using the example provided (uname_x_n.c), which only recorded these symbols:
>    - entry_SYSCALL_64_after_hwframe
>    - entry_SYSCALL_64
>    - entry_SYSCALL_64_safe_stack
>
> 2. Setting kprobes and kretprobes to try to make the kernel create these special
>    trampoline symbols, but this approach also didn't work.
>
> Questions for the perf developer community:
> 1. Is there a reliable way to trigger this trampoline logic in perf? I'd like to
>    create a perf test for this functionality.
> 2. If machine__create_extra_kernel_maps is obsolete (since it's
> x86_64-specific),
>    could we remove it to reduce /proc/kallsyms parsing time by at least 50%?
>
> I'm working on a patch to simplify machine__create_kernel_maps to call
> kallsyms__parse only once. However, I would appreciate guidance from those more
> familiar with perf.
>
> Side note: Could exposing the kernel's lookup_symbol_name function
> (from kernel/kallsyms.c) to userspace eliminate the need for reading
> /proc/kallsyms?
>
> Best regards,
> Krzysztof Łopatowski
>

Powered by blists - more mailing lists