linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/5] perf: support specify vdso path in cmdline
@ 2024-06-25  3:37 Changbin Du
  2024-06-25  3:37 ` [PATCH v4 1/5] " Changbin Du
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Changbin Du @ 2024-06-25  3:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users, linux-kernel, llvm, Changbin Du

The vdso dumped from process memory (in buildid-cache) lacks debugging
info. To annotate vdso symbols with source lines we need specify a
debugging version.

For x86, we can find them from your local build as
'arch/x86/entry/vdso/vdso{32,64}.so.dbg'. Or they may resides in
'/lib/modules/<version>/vdso/vdso{32,64}.so' on Ubuntu. But notice that the
builid has to match. 

If user doesn't specify the path, perf will search them internally as long
as vmlinux.

Below samples are captured on my local build kernel. perf succesfully
find debugging version vdso and we can annotate with source without
specifying vdso path.

$ sudo perf record -a
$ sudo perf report --objdump=llvm-objdump

Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
__vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
Percent│       movq    -48(%rbp),%rsi
       │       testq   %rax,%rax
       │     ;               return vread_hvclock();
       │       movq    %rax,%rdx
       │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
       │     ↑ js      eb
       │     ↑ jmp     74
       │     ;               ts->tv_sec = vdso_ts->sec;
  0.02 │147:   leaq    2(%rbx),%rax
       │       shlq    $4, %rax
       │       addq    %r10,%rax
       │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
  9.38 │152:   movl    (%r10),%ecx

When doing cross platform analysis, we need to specify the vdso path if
we are interested in its symbols. At most two vdso can be given.

$ sudo perf report --objdump=llvm-objdump \
      --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg

v4:
  - split the refactoring from the actual change.
v3:
  - update documentation.
v2:
  - now search vdso automatically as long as vmlinux, as suggested by Adrian.
  - remove change 'prefer symsrc_filename for filename'.

Changbin Du (5):
  perf: support specify vdso path in cmdline
  perf: disasm: refactor function dso__disassemble_filename
  perf: disasm: use build_id_path if fallback failed
  perf: symbol: generalize vmlinux path searching
  perf: symbol: try to seach vdso path if not given by user

 tools/perf/Documentation/perf-annotate.txt |   3 +
 tools/perf/Documentation/perf-c2c.txt      |   3 +
 tools/perf/Documentation/perf-inject.txt   |   3 +
 tools/perf/Documentation/perf-report.txt   |   3 +
 tools/perf/Documentation/perf-script.txt   |   3 +
 tools/perf/Documentation/perf-top.txt      |   3 +
 tools/perf/builtin-annotate.c              |   3 +
 tools/perf/builtin-c2c.c                   |   2 +
 tools/perf/builtin-inject.c                |   2 +
 tools/perf/builtin-kallsyms.c              |   2 +
 tools/perf/builtin-probe.c                 |   2 +
 tools/perf/builtin-report.c                |   2 +
 tools/perf/builtin-script.c                |   2 +
 tools/perf/builtin-top.c                   |   4 +
 tools/perf/tests/builtin-test.c            |   3 +-
 tools/perf/util/disasm.c                   | 122 +++++++----
 tools/perf/util/machine.c                  |   4 +-
 tools/perf/util/symbol.c                   | 233 ++++++++++++++++-----
 tools/perf/util/symbol.h                   |   9 +-
 tools/perf/util/symbol_conf.h              |   6 +
 20 files changed, 312 insertions(+), 102 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-25  3:37 [PATCH v4 0/5] perf: support specify vdso path in cmdline Changbin Du
@ 2024-06-25  3:37 ` Changbin Du
  2024-06-25 13:20   ` Adrian Hunter
  2024-06-25  3:37 ` [PATCH v4 2/5] perf: disasm: refactor function dso__disassemble_filename Changbin Du
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Changbin Du @ 2024-06-25  3:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users, linux-kernel, llvm, Changbin Du

The vdso dumped from process memory (in buildid-cache) lacks debugging
info. To annotate vdso symbols with source lines we need specify a
debugging version.

For x86, we can find them from your local build as
arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
/lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
the buildid has to match.

$ sudo perf record -a
$ sudo perf report --objdump=llvm-objdump \
  --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg

Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
__vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
Percent│       movq    -48(%rbp),%rsi
       │       testq   %rax,%rax
       │     ;               return vread_hvclock();
       │       movq    %rax,%rdx
       │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
       │     ↑ js      eb
       │     ↑ jmp     74
       │     ;               ts->tv_sec = vdso_ts->sec;
  0.02 │147:   leaq    2(%rbx),%rax
       │       shlq    $4, %rax
       │       addq    %r10,%rax
       │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
  9.38 │152:   movl    (%r10),%ecx

When doing cross platform analysis, we also need specify the vdso path if
we are interested in its symbols.

v2: update documentation.

Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
 tools/perf/Documentation/perf-annotate.txt |  3 +
 tools/perf/Documentation/perf-c2c.txt      |  3 +
 tools/perf/Documentation/perf-inject.txt   |  3 +
 tools/perf/Documentation/perf-report.txt   |  3 +
 tools/perf/Documentation/perf-script.txt   |  3 +
 tools/perf/Documentation/perf-top.txt      |  3 +
 tools/perf/builtin-annotate.c              |  2 +
 tools/perf/builtin-c2c.c                   |  2 +
 tools/perf/builtin-inject.c                |  2 +
 tools/perf/builtin-report.c                |  2 +
 tools/perf/builtin-script.c                |  2 +
 tools/perf/builtin-top.c                   |  2 +
 tools/perf/util/disasm.c                   |  7 +-
 tools/perf/util/symbol.c                   | 82 +++++++++++++++++++++-
 tools/perf/util/symbol_conf.h              |  5 ++
 15 files changed, 119 insertions(+), 5 deletions(-)

diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
index b95524bea021..4b6692f9a793 100644
--- a/tools/perf/Documentation/perf-annotate.txt
+++ b/tools/perf/Documentation/perf-annotate.txt
@@ -58,6 +58,9 @@ OPTIONS
 --ignore-vmlinux::
 	Ignore vmlinux files.
 
+--vdso=<vdso1[,vdso2]>::
+	Specify vdso pathnames. You can specify up to two for multiarch-support.
+
 --itrace::
 	Options for decoding instruction tracing data. The options are:
 
diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt
index 856f0dfb8e5a..7c07efca7542 100644
--- a/tools/perf/Documentation/perf-c2c.txt
+++ b/tools/perf/Documentation/perf-c2c.txt
@@ -71,6 +71,9 @@ REPORT OPTIONS
 --vmlinux=<file>::
 	vmlinux pathname
 
+--vdso=<vdso1[,vdso2]>::
+	Specify vdso pathnames. You can specify up to two for multiarch-support.
+
 -v::
 --verbose::
 	Be more verbose (show counter open errors, etc).
diff --git a/tools/perf/Documentation/perf-inject.txt b/tools/perf/Documentation/perf-inject.txt
index c972032f4ca0..3c88967b4c7f 100644
--- a/tools/perf/Documentation/perf-inject.txt
+++ b/tools/perf/Documentation/perf-inject.txt
@@ -62,6 +62,9 @@ OPTIONS
 --kallsyms=<file>::
 	kallsyms pathname
 
+--vdso=<vdso1[,vdso2]>::
+	Specify vdso pathnames. You can specify up to two for multiarch-support.
+
 --itrace::
 	Decode Instruction Tracing data, replacing it with synthesized events.
 	Options are:
diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index d2b1593ef700..8a3ba5f74cac 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -345,6 +345,9 @@ OPTIONS
         Load module symbols. WARNING: This should only be used with -k and
         a LIVE kernel.
 
+--vdso=<vdso1[,vdso2]>::
+	Specify vdso pathnames. You can specify up to two for multiarch-support.
+
 -f::
 --force::
         Don't do ownership validation.
diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
index ff086ef05a0c..48f9974ca4c5 100644
--- a/tools/perf/Documentation/perf-script.txt
+++ b/tools/perf/Documentation/perf-script.txt
@@ -296,6 +296,9 @@ OPTIONS
 --kallsyms=<file>::
         kallsyms pathname
 
+--vdso=<vdso1[,vdso2]>::
+	Specify vdso pathnames. You can specify up to two for multiarch-support.
+
 --symfs=<directory>::
         Look for files with symbols relative to this directory.
 
diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
index a754875fa5bb..1cf3ee16a0c1 100644
--- a/tools/perf/Documentation/perf-top.txt
+++ b/tools/perf/Documentation/perf-top.txt
@@ -76,6 +76,9 @@ Default is to monitor all CPUS.
 --kallsyms=<file>::
 	kallsyms pathname
 
+--vdso=<vdso1[,vdso2]>::
+	Specify vdso pathnames. You can specify up to two for multiarch-support.
+
 -m <pages>::
 --mmap-pages=<pages>::
 	Number of mmap data pages (must be a power of two) or size
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 50d2fb222d48..ff466882065d 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -742,6 +742,8 @@ int cmd_annotate(int argc, const char **argv)
 		   "file", "vmlinux pathname"),
 	OPT_BOOLEAN('m', "modules", &symbol_conf.use_modules,
 		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
+	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
+		     parse_vdso_pathnames),
 	OPT_BOOLEAN('l', "print-line", &annotate_opts.print_lines,
 		    "print matching source lines (may be slow)"),
 	OPT_BOOLEAN('P', "full-paths", &annotate_opts.full_path,
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c157bd31f2e5..4764f9139661 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -3018,6 +3018,8 @@ static int perf_c2c__report(int argc, const char **argv)
 	const struct option options[] = {
 	OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
 		   "file", "vmlinux pathname"),
+	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
+		     parse_vdso_pathnames),
 	OPT_STRING('i', "input", &input_name, "file",
 		   "the input file to process"),
 	OPT_INCR('N', "node-info", &c2c.node_info,
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index a212678d47be..e774e83d0a0f 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -2247,6 +2247,8 @@ int cmd_inject(int argc, const char **argv)
 			    "don't load vmlinux even if found"),
 		OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name, "file",
 			   "kallsyms pathname"),
+		OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
+		     parse_vdso_pathnames),
 		OPT_BOOLEAN('f', "force", &data.force, "don't complain, do it"),
 		OPT_CALLBACK_OPTARG(0, "itrace", &inject.itrace_synth_opts,
 				    NULL, "opts", "Instruction Tracing options\n"
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 69618fb0110b..a64b48460dce 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1324,6 +1324,8 @@ int cmd_report(int argc, const char **argv)
                     "don't load vmlinux even if found"),
 	OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name,
 		   "file", "kallsyms pathname"),
+	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
+		     parse_vdso_pathnames),
 	OPT_BOOLEAN('f', "force", &symbol_conf.force, "don't complain, do it"),
 	OPT_BOOLEAN('m', "modules", &symbol_conf.use_modules,
 		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index c16224b1fef3..2e358922a8d1 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3965,6 +3965,8 @@ int cmd_script(int argc, const char **argv)
 		   "file", "vmlinux pathname"),
 	OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name,
 		   "file", "kallsyms pathname"),
+	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
+		     parse_vdso_pathnames),
 	OPT_BOOLEAN('G', "hide-call-graph", &no_callchain,
 		    "When printing symbols do not display call chain"),
 	OPT_CALLBACK(0, "symfs", NULL, "directory",
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 1d6aef51c122..a3cce4e76eb9 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1479,6 +1479,8 @@ int cmd_top(int argc, const char **argv)
 		   "file", "kallsyms pathname"),
 	OPT_BOOLEAN('K', "hide_kernel_symbols", &top.hide_kernel_symbols,
 		    "hide kernel symbols"),
+	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
+		     parse_vdso_pathnames),
 	OPT_CALLBACK('m', "mmap-pages", &opts->mmap_pages, "pages",
 		     "number of mmap data pages", evlist__parse_mmap_pages),
 	OPT_INTEGER('r', "realtime", &top.realtime_prio,
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 72aec8f61b94..8d9d78d3150c 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -16,6 +16,7 @@
 #include "debug.h"
 #include "disasm.h"
 #include "dso.h"
+#include "vdso.h"
 #include "env.h"
 #include "evsel.h"
 #include "map.h"
@@ -1126,7 +1127,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
 		dirname(build_id_path);
 
-	if (dso__is_kcore(dso))
+	if (dso__is_kcore(dso) || dso__is_vdso(dso))
 		goto fallback;
 
 	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
@@ -1134,7 +1135,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 		goto fallback;
 
 	linkname[len] = '\0';
-	if (strstr(linkname, DSO__NAME_KALLSYMS) ||
+	if (strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO) ||
 		access(filename, R_OK)) {
 fallback:
 		/*
@@ -1142,7 +1143,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 		 * cache, or is just a kallsyms file, well, lets hope that this
 		 * DSO is the same as when 'perf record' ran.
 		 */
-		if (dso__kernel(dso) && dso__long_name(dso)[0] == '/')
+		if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
 			snprintf(filename, filename_size, "%s", dso__long_name(dso));
 		else
 			__symbol__join_symfs(filename, filename_size, dso__long_name(dso));
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 9e5940b5bc59..a90c647d37e1 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -19,6 +19,7 @@
 #include "build-id.h"
 #include "cap.h"
 #include "dso.h"
+#include "vdso.h"
 #include "util.h" // lsdir()
 #include "debug.h"
 #include "event.h"
@@ -44,6 +45,7 @@
 
 static int dso__load_kernel_sym(struct dso *dso, struct map *map);
 static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map);
+static int dso__load_vdso_sym(struct dso *dso, struct map *map);
 static bool symbol__is_idle(const char *name);
 
 int vmlinux_path__nr_entries;
@@ -1833,6 +1835,12 @@ int dso__load(struct dso *dso, struct map *map)
 		goto out;
 	}
 
+	if (dso__is_vdso(dso)) {
+		ret = dso__load_vdso_sym(dso, map);
+		if (ret > 0)
+			goto out;
+	}
+
 	dso__set_adjust_symbols(dso, false);
 
 	if (perfmap) {
@@ -2017,12 +2025,14 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
 		dso__set_binary_type(dso, DSO_BINARY_TYPE__VMLINUX);
 
 	err = dso__load_sym(dso, map, &ss, &ss, 0);
-	symsrc__destroy(&ss);
-
 	if (err > 0) {
 		dso__set_loaded(dso);
 		pr_debug("Using %s for symbols\n", symfs_vmlinux);
+
+		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
+			dso__set_symsrc_filename(dso, strdup(symfs_vmlinux));
 	}
+	symsrc__destroy(&ss);
 
 	return err;
 }
@@ -2349,6 +2359,74 @@ static int vmlinux_path__init(struct perf_env *env)
 	return -1;
 }
 
+int parse_vdso_pathnames(const struct option *opt __maybe_unused,
+			 const char *arg, int unset __maybe_unused)
+{
+	char *tmp, *tok, *str = strdup(arg);
+	unsigned int i = 0;
+
+	for (tok = strtok_r(str, ",", &tmp); tok && i < ARRAY_SIZE(symbol_conf.vdso_name);
+	     tok = strtok_r(NULL, ",", &tmp)) {
+		symbol_conf.vdso_name[i++] = strdup(tok);
+	}
+
+	free(str);
+	return 0;
+}
+
+static int dso__load_vdso(struct dso *dso, struct map *map,
+			  const char *vdso)
+{
+	int err = -1;
+	struct symsrc ss;
+	char symfs_vdso[PATH_MAX];
+
+	if (vdso[0] == '/')
+		snprintf(symfs_vdso, sizeof(symfs_vdso), "%s", vdso);
+	else
+		symbol__join_symfs(symfs_vdso, vdso);
+
+	if (symsrc__init(&ss, dso, symfs_vdso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO))
+		return -1;
+
+	/*
+	 * dso__load_sym() may copy 'dso' which will result in the copies having
+	 * an incorrect long name unless we set it here first.
+	 */
+	dso__set_long_name(dso, vdso, false);
+	dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
+
+	err = dso__load_sym(dso, map, &ss, &ss, 0);
+	if (err > 0) {
+		dso__set_loaded(dso);
+		pr_debug("Using %s for %s symbols\n", symfs_vdso, dso__short_name(dso));
+
+		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
+			dso__set_symsrc_filename(dso, strdup(symfs_vdso));
+	}
+	symsrc__destroy(&ss);
+
+	return err;
+}
+
+static int dso__load_vdso_sym(struct dso *dso, struct map *map)
+{
+	int ret;
+
+	if (!dso__is_vdso(dso))
+		return -1;
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(symbol_conf.vdso_name); i++) {
+		if (symbol_conf.vdso_name[i] != NULL) {
+			ret = dso__load_vdso(dso, map, symbol_conf.vdso_name[i]);
+			if (ret > 0)
+				return ret;
+		}
+	}
+
+	return -1;
+}
+
 int setup_list(struct strlist **list, const char *list_str,
 		      const char *list_name)
 {
diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
index c114bbceef40..108356e3c981 100644
--- a/tools/perf/util/symbol_conf.h
+++ b/tools/perf/util/symbol_conf.h
@@ -3,6 +3,7 @@
 #define __PERF_SYMBOL_CONF 1
 
 #include <stdbool.h>
+#include <subcmd/parse-options.h>
 
 struct strlist;
 struct intlist;
@@ -55,6 +56,7 @@ struct symbol_conf {
 	const char	*default_guest_vmlinux_name,
 			*default_guest_kallsyms,
 			*default_guest_modules;
+	const char	*vdso_name[2];
 	const char	*guestmount;
 	const char	*dso_list_str,
 			*comm_list_str,
@@ -85,4 +87,7 @@ struct symbol_conf {
 
 extern struct symbol_conf symbol_conf;
 
+int parse_vdso_pathnames(const struct option *opt __maybe_unused,
+			 const char *arg, int unset __maybe_unused);
+
 #endif // __PERF_SYMBOL_CONF
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 2/5] perf: disasm: refactor function dso__disassemble_filename
  2024-06-25  3:37 [PATCH v4 0/5] perf: support specify vdso path in cmdline Changbin Du
  2024-06-25  3:37 ` [PATCH v4 1/5] " Changbin Du
@ 2024-06-25  3:37 ` Changbin Du
  2024-06-25  3:37 ` [PATCH v4 3/5] perf: disasm: use build_id_path if fallback failed Changbin Du
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Changbin Du @ 2024-06-25  3:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users, linux-kernel, llvm, Changbin Du

To make change easy, this change refactors the dso__disassemble_filename()
function by extracting two functions read_buildid_linkname() and
fallback_filename().

Signed-off-by: Changbin Du <changbin.du@huawei.com>

---
v2: split refactoring from logical change.
---
 tools/perf/util/disasm.c | 117 +++++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 47 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 8d9d78d3150c..c4ab98c0eaa1 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1092,28 +1092,12 @@ int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, s
 	return 0;
 }
 
-static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
+static int read_buildid_linkname(char *filename, char *linkname, size_t linkname_size)
 {
-	char linkname[PATH_MAX];
-	char *build_id_filename;
 	char *build_id_path = NULL;
 	char *pos;
 	int len;
 
-	if (dso__symtab_type(dso) == DSO_BINARY_TYPE__KALLSYMS &&
-	    !dso__is_kcore(dso))
-		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
-
-	build_id_filename = dso__build_id_filename(dso, NULL, 0, false);
-	if (build_id_filename) {
-		__symbol__join_symfs(filename, filename_size, build_id_filename);
-		free(build_id_filename);
-	} else {
-		if (dso__has_build_id(dso))
-			return ENOMEM;
-		goto fallback;
-	}
-
 	build_id_path = strdup(filename);
 	if (!build_id_path)
 		return ENOMEM;
@@ -1127,41 +1111,80 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
 		dirname(build_id_path);
 
-	if (dso__is_kcore(dso) || dso__is_vdso(dso))
-		goto fallback;
-
-	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
-	if (len < 0)
-		goto fallback;
+	len = readlink(build_id_path, linkname, linkname_size);
+	if (len < 0) {
+		free(build_id_path);
+		return -1;
+	}
 
 	linkname[len] = '\0';
-	if (strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO) ||
-		access(filename, R_OK)) {
-fallback:
-		/*
-		 * If we don't have build-ids or the build-id file isn't in the
-		 * cache, or is just a kallsyms file, well, lets hope that this
-		 * DSO is the same as when 'perf record' ran.
-		 */
-		if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
-			snprintf(filename, filename_size, "%s", dso__long_name(dso));
-		else
-			__symbol__join_symfs(filename, filename_size, dso__long_name(dso));
-
-		mutex_lock(dso__lock(dso));
-		if (access(filename, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
-			char *new_name = dso__filename_with_chroot(dso, filename);
-			if (new_name) {
-				strlcpy(filename, new_name, filename_size);
-				free(new_name);
-			}
+	free(build_id_path);
+	return 0;
+}
+
+static int fallback_filename(struct dso *dso, char *filename, size_t filename_size)
+{
+	char filepath[PATH_MAX];
+
+	/*
+	 * If we don't have build-ids or the build-id file isn't in the
+	 * cache, or is just a kallsyms file, well, lets hope that this
+	 * DSO is the same as when 'perf record' ran.
+	 */
+	if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
+		snprintf(filepath, sizeof(filepath), "%s", dso__long_name(dso));
+	else
+		__symbol__join_symfs(filepath, sizeof(filepath), dso__long_name(dso));
+
+	mutex_lock(dso__lock(dso));
+	if (access(filepath, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
+		char *new_name = dso__filename_with_chroot(dso, filepath);
+		if (new_name) {
+			strlcpy(filepath, new_name, sizeof(filepath));
+			free(new_name);
 		}
-		mutex_unlock(dso__lock(dso));
-	} else if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
-		dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
 	}
+	mutex_unlock(dso__lock(dso));
 
-	free(build_id_path);
+	if (access(filepath, R_OK) && errno == ENOENT)
+		return ENOENT;
+
+	snprintf(filename, filename_size, "%s", filepath);
+	return 0;
+}
+
+static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
+{
+	char linkname[PATH_MAX];
+	char *build_id_filename;
+
+	if (dso__symtab_type(dso) == DSO_BINARY_TYPE__KALLSYMS &&
+	    !dso__is_kcore(dso))
+		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
+
+	build_id_filename = dso__build_id_filename(dso, NULL, 0, false);
+	if (build_id_filename) {
+		__symbol__join_symfs(filename, filename_size, build_id_filename);
+		free(build_id_filename);
+	} else {
+		if (dso__has_build_id(dso))
+			return ENOMEM;
+		return fallback_filename(dso, filename, filename_size);
+	}
+
+	if (access(filename, R_OK))
+		return fallback_filename(dso, filename, filename_size);
+
+	if (dso__is_kcore(dso) || dso__is_vdso(dso))
+		return fallback_filename(dso, filename, filename_size);
+
+	if (read_buildid_linkname(filename, linkname, sizeof(linkname) - 1) ||
+	    strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO)) {
+		return fallback_filename(dso, filename, filename_size);
+	}
+
+	if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
+		dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
 	return 0;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 3/5] perf: disasm: use build_id_path if fallback failed
  2024-06-25  3:37 [PATCH v4 0/5] perf: support specify vdso path in cmdline Changbin Du
  2024-06-25  3:37 ` [PATCH v4 1/5] " Changbin Du
  2024-06-25  3:37 ` [PATCH v4 2/5] perf: disasm: refactor function dso__disassemble_filename Changbin Du
@ 2024-06-25  3:37 ` Changbin Du
  2024-06-25  3:37 ` [PATCH v4 4/5] perf: symbol: generalize vmlinux path searching Changbin Du
  2024-06-25  3:37 ` [PATCH v4 5/5] perf: symbol: try to seach vdso path if not given by user Changbin Du
  4 siblings, 0 replies; 15+ messages in thread
From: Changbin Du @ 2024-06-25  3:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users, linux-kernel, llvm, Changbin Du

If we can not fallback for special dso (vmlinx and vdso), use the
build_id_path found previously.

Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
 tools/perf/util/disasm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index c4ab98c0eaa1..f4e94ff37e50 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1176,15 +1176,21 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 		return fallback_filename(dso, filename, filename_size);
 
 	if (dso__is_kcore(dso) || dso__is_vdso(dso))
-		return fallback_filename(dso, filename, filename_size);
+		goto fallback;
 
-	if (read_buildid_linkname(filename, linkname, sizeof(linkname) - 1) ||
-	    strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO)) {
-		return fallback_filename(dso, filename, filename_size);
+	if (!read_buildid_linkname(filename, linkname, sizeof(linkname) - 1) &&
+	    (!strstr(linkname, DSO__NAME_KALLSYMS) && !strstr(linkname, DSO__NAME_VDSO))) {
+		/* It's not kallsysms or vdso, use build_id path found above */
+		goto out;
 	}
 
-	if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
-		dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
+fallback:
+	if (fallback_filename(dso, filename, filename_size)) {
+		/* if fallback failed, use build_id path found above */
+out:
+		if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
+			dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
+	}
 	return 0;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 4/5] perf: symbol: generalize vmlinux path searching
  2024-06-25  3:37 [PATCH v4 0/5] perf: support specify vdso path in cmdline Changbin Du
                   ` (2 preceding siblings ...)
  2024-06-25  3:37 ` [PATCH v4 3/5] perf: disasm: use build_id_path if fallback failed Changbin Du
@ 2024-06-25  3:37 ` Changbin Du
  2024-06-25  3:37 ` [PATCH v4 5/5] perf: symbol: try to seach vdso path if not given by user Changbin Du
  4 siblings, 0 replies; 15+ messages in thread
From: Changbin Du @ 2024-06-25  3:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users, linux-kernel, llvm, Changbin Du

This generalizes the vmlinux path searching logic. Later we will add
another instance for vdso.

The search pattern is described by struct dso_filename_pattern, and the
formatted paths are hold in struct dso_filename_paths.

Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
 tools/perf/util/machine.c |   4 +-
 tools/perf/util/symbol.c  | 112 +++++++++++++++++++++-----------------
 tools/perf/util/symbol.h  |   8 ++-
 3 files changed, 70 insertions(+), 54 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 8477edefc299..68315520f15b 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -896,9 +896,9 @@ size_t machine__fprintf_vmlinux_path(struct machine *machine, FILE *fp)
 			printed += fprintf(fp, "[0] %s\n", filename);
 	}
 
-	for (i = 0; i < vmlinux_path__nr_entries; ++i) {
+	for (i = 0; i < vmlinux_paths.nr_entries; ++i) {
 		printed += fprintf(fp, "[%d] %s\n", i + dso__has_build_id(kdso),
-				   vmlinux_path[i]);
+				   vmlinux_paths.paths[i]);
 	}
 	return printed;
 }
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a90c647d37e1..83e5c3807a2c 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -48,8 +48,7 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map);
 static int dso__load_vdso_sym(struct dso *dso, struct map *map);
 static bool symbol__is_idle(const char *name);
 
-int vmlinux_path__nr_entries;
-char **vmlinux_path;
+struct dso_filename_paths vmlinux_paths;
 
 struct symbol_conf symbol_conf = {
 	.nanosecs		= false,
@@ -2043,10 +2042,10 @@ int dso__load_vmlinux_path(struct dso *dso, struct map *map)
 	char *filename = NULL;
 
 	pr_debug("Looking at the vmlinux_path (%d entries long)\n",
-		 vmlinux_path__nr_entries + 1);
+		 vmlinux_paths.nr_entries + 1);
 
-	for (i = 0; i < vmlinux_path__nr_entries; ++i) {
-		err = dso__load_vmlinux(dso, map, vmlinux_path[i], false);
+	for (i = 0; i < vmlinux_paths.nr_entries; ++i) {
+		err = dso__load_vmlinux(dso, map, vmlinux_paths.paths[i], false);
 		if (err > 0)
 			goto out;
 	}
@@ -2210,7 +2209,7 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
 			return err;
 	}
 
-	if (!symbol_conf.ignore_vmlinux && vmlinux_path != NULL) {
+	if (!symbol_conf.ignore_vmlinux && vmlinux_paths.paths != NULL) {
 		err = dso__load_vmlinux_path(dso, map);
 		if (err > 0)
 			return err;
@@ -2285,57 +2284,55 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map)
 	return err;
 }
 
-static void vmlinux_path__exit(void)
-{
-	while (--vmlinux_path__nr_entries >= 0)
-		zfree(&vmlinux_path[vmlinux_path__nr_entries]);
-	vmlinux_path__nr_entries = 0;
-
-	zfree(&vmlinux_path);
-}
-
-static const char * const vmlinux_paths[] = {
-	"vmlinux",
-	"/boot/vmlinux"
+struct dso_filename_pattern {
+	const char *pattern;
+	/*
+	 * 0 for matching directly,
+	 * 1 for matching by kernel_version,
+	 * 2 for matching by kernel_version + arch.
+	 */
+	int match_type;
 };
 
-static const char * const vmlinux_paths_upd[] = {
-	"/boot/vmlinux-%s",
-	"/usr/lib/debug/boot/vmlinux-%s",
-	"/lib/modules/%s/build/vmlinux",
-	"/usr/lib/debug/lib/modules/%s/vmlinux",
-	"/usr/lib/debug/boot/vmlinux-%s.debug"
+struct dso_filename_pattern vmlinux_patterns[] = {
+	{"vmlinux", 0},
+	{"/boot/vmlinux", 0},
+	{"/boot/vmlinux-%s", 1},
+	{"/usr/lib/debug/boot/vmlinux-%s", 1},
+	{"/lib/modules/%s/build/vmlinux", 1},
+	{"/usr/lib/debug/lib/modules/%s/vmlinux", 1},
+	{"/usr/lib/debug/boot/vmlinux-%s.debug", 1},
 };
 
-static int vmlinux_path__add(const char *new_entry)
+static int dso_filename_path__add(struct dso_filename_paths *paths, const char *new_entry)
 {
-	vmlinux_path[vmlinux_path__nr_entries] = strdup(new_entry);
-	if (vmlinux_path[vmlinux_path__nr_entries] == NULL)
+	paths->paths[paths->nr_entries] = strdup(new_entry);
+	if (paths->paths[paths->nr_entries] == NULL)
 		return -1;
-	++vmlinux_path__nr_entries;
+	++paths->nr_entries;
 
 	return 0;
 }
 
-static int vmlinux_path__init(struct perf_env *env)
+static void dso_filename_path__exit(struct dso_filename_paths *paths)
 {
-	struct utsname uts;
-	char bf[PATH_MAX];
-	char *kernel_version;
-	unsigned int i;
+	while (--paths->nr_entries >= 0)
+		zfree(&paths->paths[paths->nr_entries]);
+	paths->nr_entries = 0;
 
-	vmlinux_path = malloc(sizeof(char *) * (ARRAY_SIZE(vmlinux_paths) +
-			      ARRAY_SIZE(vmlinux_paths_upd)));
-	if (vmlinux_path == NULL)
-		return -1;
-
-	for (i = 0; i < ARRAY_SIZE(vmlinux_paths); i++)
-		if (vmlinux_path__add(vmlinux_paths[i]) < 0)
-			goto out_fail;
+	zfree(&paths->paths);
+}
 
-	/* only try kernel version if no symfs was given */
-	if (symbol_conf.symfs[0] != 0)
-		return 0;
+static int dso_filename_path__init(struct dso_filename_paths *paths,
+				   struct dso_filename_pattern *patterns,
+				   int nr_patterns,
+				   struct perf_env *env)
+{
+	struct utsname uts;
+	char bf[PATH_MAX];
+	const char *kernel_version;
+	const char *arch = perf_env__arch(env);
+	int i;
 
 	if (env) {
 		kernel_version = env->os_release;
@@ -2346,16 +2343,28 @@ static int vmlinux_path__init(struct perf_env *env)
 		kernel_version = uts.release;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(vmlinux_paths_upd); i++) {
-		snprintf(bf, sizeof(bf), vmlinux_paths_upd[i], kernel_version);
-		if (vmlinux_path__add(bf) < 0)
+	paths->paths = malloc(sizeof(char *) * nr_patterns);
+	if (paths->paths == NULL)
+		return -1;
+
+	for (i = 0; i < nr_patterns; i++) {
+		if (patterns[i].match_type == 0)
+			strlcpy(bf, patterns[i].pattern, sizeof(bf));
+		else if (symbol_conf.symfs[0] == 0) {
+			/* only try kernel version if no symfs was given */
+			if (patterns[i].match_type == 1)
+				snprintf(bf, sizeof(bf), patterns[i].pattern, kernel_version);
+			else if (patterns[i].match_type == 2)
+				snprintf(bf, sizeof(bf), patterns[i].pattern, kernel_version, arch);
+		}
+		if (dso_filename_path__add(paths, bf) < 0)
 			goto out_fail;
 	}
 
 	return 0;
 
 out_fail:
-	vmlinux_path__exit();
+	dso_filename_path__exit(paths);
 	return -1;
 }
 
@@ -2551,8 +2560,11 @@ int symbol__init(struct perf_env *env)
 
 	symbol__elf_init();
 
-	if (symbol_conf.try_vmlinux_path && vmlinux_path__init(env) < 0)
+	if (symbol_conf.try_vmlinux_path &&
+	    dso_filename_path__init(&vmlinux_paths, vmlinux_patterns,
+				    ARRAY_SIZE(vmlinux_patterns), env) < 0) {
 		return -1;
+	}
 
 	if (symbol_conf.field_sep && *symbol_conf.field_sep == '.') {
 		pr_err("'.' is the only non valid --field-separator argument\n");
@@ -2629,7 +2641,7 @@ void symbol__exit(void)
 	intlist__delete(symbol_conf.tid_list);
 	intlist__delete(symbol_conf.pid_list);
 	intlist__delete(symbol_conf.addr_list);
-	vmlinux_path__exit();
+	dso_filename_path__exit(&vmlinux_paths);
 	symbol_conf.sym_list = symbol_conf.dso_list = symbol_conf.comm_list = NULL;
 	symbol_conf.bt_stop_list = NULL;
 	symbol_conf.initialized = false;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 3fb5d146d9b1..30056884945b 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -101,8 +101,12 @@ static inline int __symbol__join_symfs(char *bf, size_t size, const char *path)
 
 #define symbol__join_symfs(bf, path) __symbol__join_symfs(bf, sizeof(bf), path)
 
-extern int vmlinux_path__nr_entries;
-extern char **vmlinux_path;
+struct dso_filename_paths {
+	int nr_entries;
+	char **paths;
+};
+
+extern struct dso_filename_paths vmlinux_paths;
 
 static inline void *symbol__priv(struct symbol *sym)
 {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 5/5] perf: symbol: try to seach vdso path if not given by user
  2024-06-25  3:37 [PATCH v4 0/5] perf: support specify vdso path in cmdline Changbin Du
                   ` (3 preceding siblings ...)
  2024-06-25  3:37 ` [PATCH v4 4/5] perf: symbol: generalize vmlinux path searching Changbin Du
@ 2024-06-25  3:37 ` Changbin Du
  4 siblings, 0 replies; 15+ messages in thread
From: Changbin Du @ 2024-06-25  3:37 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users, linux-kernel, llvm, Changbin Du

Just like vmlinux, try to search vdso in predefined path if it's not given
by user. The searched path usually has debugging info.

For example, the vdso can be found in
/lib/modules/<version>/build/arch/x86/entry/vdso/vdso*.so.dbg for local
build on x86.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
 tools/perf/builtin-annotate.c   |  1 +
 tools/perf/builtin-kallsyms.c   |  2 ++
 tools/perf/builtin-probe.c      |  2 ++
 tools/perf/builtin-top.c        |  2 ++
 tools/perf/tests/builtin-test.c |  3 ++-
 tools/perf/util/symbol.c        | 41 +++++++++++++++++++++++++++++++++
 tools/perf/util/symbol.h        |  1 +
 tools/perf/util/symbol_conf.h   |  1 +
 8 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index ff466882065d..6c5019c39068 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -883,6 +883,7 @@ int cmd_annotate(int argc, const char **argv)
 		goto out_delete;
 
 	symbol_conf.try_vmlinux_path = true;
+	symbol_conf.try_vdso_path = true;
 
 	ret = symbol__init(&annotate.session->header.env);
 	if (ret < 0)
diff --git a/tools/perf/builtin-kallsyms.c b/tools/perf/builtin-kallsyms.c
index a3c2ffdc1af8..d049025a4959 100644
--- a/tools/perf/builtin-kallsyms.c
+++ b/tools/perf/builtin-kallsyms.c
@@ -63,6 +63,8 @@ int cmd_kallsyms(int argc, const char **argv)
 		usage_with_options(kallsyms_usage, options);
 
 	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
+	symbol_conf.try_vdso_path = (symbol_conf.vdso_name[0] == NULL &&
+				     symbol_conf.vdso_name[1] == NULL);
 	if (symbol__init(NULL) < 0)
 		return -1;
 
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 003a3bcebfdf..91770eda37c0 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -667,6 +667,8 @@ __cmd_probe(int argc, const char **argv)
 	 * Only consider the user's kernel image path if given.
 	 */
 	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
+	symbol_conf.try_vdso_path = (symbol_conf.vdso_name[0] == NULL &&
+				     symbol_conf.vdso_name[1] == NULL);
 
 	/*
 	 * Except for --list, --del and --add, other command doesn't depend
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index a3cce4e76eb9..5a2dc7a0dbb8 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1792,6 +1792,8 @@ int cmd_top(int argc, const char **argv)
 	annotation_config__init();
 
 	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
+	symbol_conf.try_vdso_path = (symbol_conf.vdso_name[0] == NULL &&
+				     symbol_conf.vdso_name[1] == NULL);
 	status = symbol__init(NULL);
 	if (status < 0)
 		goto out_delete_evlist;
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index c3d84b67ca8e..176196e3c183 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -575,7 +575,8 @@ int cmd_test(int argc, const char **argv)
 
 	symbol_conf.priv_size = sizeof(int);
 	symbol_conf.try_vmlinux_path = true;
-
+	symbol_conf.try_vdso_path = (symbol_conf.vdso_name[0] == NULL &&
+				     symbol_conf.vdso_name[1] == NULL);
 	if (symbol__init(NULL) < 0)
 		return -1;
 
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 83e5c3807a2c..4dc1a76005d5 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -49,11 +49,13 @@ static int dso__load_vdso_sym(struct dso *dso, struct map *map);
 static bool symbol__is_idle(const char *name);
 
 struct dso_filename_paths vmlinux_paths;
+struct dso_filename_paths vdso_paths;
 
 struct symbol_conf symbol_conf = {
 	.nanosecs		= false,
 	.use_modules		= true,
 	.try_vmlinux_path	= true,
+	.try_vdso_path		= true,
 	.demangle		= true,
 	.demangle_kernel	= false,
 	.cumulate_callchain	= true,
@@ -2304,6 +2306,16 @@ struct dso_filename_pattern vmlinux_patterns[] = {
 	{"/usr/lib/debug/boot/vmlinux-%s.debug", 1},
 };
 
+struct dso_filename_pattern vdso_patterns[] = {
+	{"/lib/modules/%s/vdso/vdso.so", 1},
+	{"/lib/modules/%s/vdso/vdso64.so", 1},
+	{"/lib/modules/%s/vdso/vdso32.so", 1},
+	{"/lib/modules/%s/build/arch/%s/vdso/vdso.so.dbg", 2},
+	{"/lib/modules/%s/build/arch/%s/kernel/vdso/vdso.so.dbg", 2},
+	{"/lib/modules/%s/build/arch/%s/entry/vdso/vdso32.so.dbg", 2},
+	{"/lib/modules/%s/build/arch/%s/entry/vdso/vdso64.so.dbg", 2},
+};
+
 static int dso_filename_path__add(struct dso_filename_paths *paths, const char *new_entry)
 {
 	paths->paths[paths->nr_entries] = strdup(new_entry);
@@ -2418,6 +2430,22 @@ static int dso__load_vdso(struct dso *dso, struct map *map,
 	return err;
 }
 
+static int dso__load_vdso_path(struct dso *dso, struct map *map)
+{
+	int i, ret = 0;
+
+	pr_debug("Looking at the vdso_path (%d entries long)\n",
+		 vdso_paths.nr_entries + 1);
+
+	for (i = 0; i < vdso_paths.nr_entries; ++i) {
+		ret = dso__load_vdso(dso, map, vdso_paths.paths[i]);
+		if (ret > 0)
+			return ret;
+	}
+
+	return ret;
+}
+
 static int dso__load_vdso_sym(struct dso *dso, struct map *map)
 {
 	int ret;
@@ -2433,6 +2461,12 @@ static int dso__load_vdso_sym(struct dso *dso, struct map *map)
 		}
 	}
 
+	if (vdso_paths.paths != NULL) {
+		ret = dso__load_vdso_path(dso, map);
+		if (ret > 0)
+			return ret;
+	}
+
 	return -1;
 }
 
@@ -2566,6 +2600,12 @@ int symbol__init(struct perf_env *env)
 		return -1;
 	}
 
+	if (symbol_conf.try_vdso_path &&
+	    dso_filename_path__init(&vdso_paths, vdso_patterns,
+				    ARRAY_SIZE(vdso_patterns), env) < 0) {
+		return -1;
+	}
+
 	if (symbol_conf.field_sep && *symbol_conf.field_sep == '.') {
 		pr_err("'.' is the only non valid --field-separator argument\n");
 		return -1;
@@ -2642,6 +2682,7 @@ void symbol__exit(void)
 	intlist__delete(symbol_conf.pid_list);
 	intlist__delete(symbol_conf.addr_list);
 	dso_filename_path__exit(&vmlinux_paths);
+	dso_filename_path__exit(&vdso_paths);
 	symbol_conf.sym_list = symbol_conf.dso_list = symbol_conf.comm_list = NULL;
 	symbol_conf.bt_stop_list = NULL;
 	symbol_conf.initialized = false;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 30056884945b..08c339594d4e 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -107,6 +107,7 @@ struct dso_filename_paths {
 };
 
 extern struct dso_filename_paths vmlinux_paths;
+extern struct dso_filename_paths vdso_paths;
 
 static inline void *symbol__priv(struct symbol *sym)
 {
diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
index 108356e3c981..58c4c4358ece 100644
--- a/tools/perf/util/symbol_conf.h
+++ b/tools/perf/util/symbol_conf.h
@@ -12,6 +12,7 @@ struct symbol_conf {
 	bool		nanosecs;
 	unsigned short	priv_size;
 	bool		try_vmlinux_path,
+			try_vdso_path,
 			init_annotation,
 			force,
 			ignore_vmlinux,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-25  3:37 ` [PATCH v4 1/5] " Changbin Du
@ 2024-06-25 13:20   ` Adrian Hunter
  2024-06-26  2:26     ` duchangbin
  0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2024-06-25 13:20 UTC (permalink / raw)
  To: Changbin Du, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim, Nathan Chancellor
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Liang, Kan, Nick Desaulniers, Bill Wendling, Justin Stitt,
	linux-perf-users, linux-kernel, llvm

On 25/06/24 06:37, Changbin Du wrote:
> The vdso dumped from process memory (in buildid-cache) lacks debugging
> info. To annotate vdso symbols with source lines we need specify a
> debugging version.
> 
> For x86, we can find them from your local build as
> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
> the buildid has to match.
> 
> $ sudo perf record -a
> $ sudo perf report --objdump=llvm-objdump \
>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
> 
> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
> Percent│       movq    -48(%rbp),%rsi
>        │       testq   %rax,%rax
>        │     ;               return vread_hvclock();
>        │       movq    %rax,%rdx
>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
>        │     ↑ js      eb
>        │     ↑ jmp     74
>        │     ;               ts->tv_sec = vdso_ts->sec;
>   0.02 │147:   leaq    2(%rbx),%rax
>        │       shlq    $4, %rax
>        │       addq    %r10,%rax
>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
>   9.38 │152:   movl    (%r10),%ecx
> 
> When doing cross platform analysis, we also need specify the vdso path if
> we are interested in its symbols.

Would it be possible to add vdso and vdso debug to the build-id
cache and ensure perf can find it there?

Typically, getting dsos from another machine is handled via
build-id cache e.g. what perf-archive does

> 
> v2: update documentation.
> 
> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> ---
>  tools/perf/Documentation/perf-annotate.txt |  3 +
>  tools/perf/Documentation/perf-c2c.txt      |  3 +
>  tools/perf/Documentation/perf-inject.txt   |  3 +
>  tools/perf/Documentation/perf-report.txt   |  3 +
>  tools/perf/Documentation/perf-script.txt   |  3 +
>  tools/perf/Documentation/perf-top.txt      |  3 +
>  tools/perf/builtin-annotate.c              |  2 +
>  tools/perf/builtin-c2c.c                   |  2 +
>  tools/perf/builtin-inject.c                |  2 +
>  tools/perf/builtin-report.c                |  2 +
>  tools/perf/builtin-script.c                |  2 +
>  tools/perf/builtin-top.c                   |  2 +
>  tools/perf/util/disasm.c                   |  7 +-
>  tools/perf/util/symbol.c                   | 82 +++++++++++++++++++++-
>  tools/perf/util/symbol_conf.h              |  5 ++
>  15 files changed, 119 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
> index b95524bea021..4b6692f9a793 100644
> --- a/tools/perf/Documentation/perf-annotate.txt
> +++ b/tools/perf/Documentation/perf-annotate.txt
> @@ -58,6 +58,9 @@ OPTIONS
>  --ignore-vmlinux::
>  	Ignore vmlinux files.
>  
> +--vdso=<vdso1[,vdso2]>::
> +	Specify vdso pathnames. You can specify up to two for multiarch-support.
> +
>  --itrace::
>  	Options for decoding instruction tracing data. The options are:
>  
> diff --git a/tools/perf/Documentation/perf-c2c.txt b/tools/perf/Documentation/perf-c2c.txt
> index 856f0dfb8e5a..7c07efca7542 100644
> --- a/tools/perf/Documentation/perf-c2c.txt
> +++ b/tools/perf/Documentation/perf-c2c.txt
> @@ -71,6 +71,9 @@ REPORT OPTIONS
>  --vmlinux=<file>::
>  	vmlinux pathname
>  
> +--vdso=<vdso1[,vdso2]>::
> +	Specify vdso pathnames. You can specify up to two for multiarch-support.
> +
>  -v::
>  --verbose::
>  	Be more verbose (show counter open errors, etc).
> diff --git a/tools/perf/Documentation/perf-inject.txt b/tools/perf/Documentation/perf-inject.txt
> index c972032f4ca0..3c88967b4c7f 100644
> --- a/tools/perf/Documentation/perf-inject.txt
> +++ b/tools/perf/Documentation/perf-inject.txt
> @@ -62,6 +62,9 @@ OPTIONS
>  --kallsyms=<file>::
>  	kallsyms pathname
>  
> +--vdso=<vdso1[,vdso2]>::
> +	Specify vdso pathnames. You can specify up to two for multiarch-support.
> +
>  --itrace::
>  	Decode Instruction Tracing data, replacing it with synthesized events.
>  	Options are:
> diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
> index d2b1593ef700..8a3ba5f74cac 100644
> --- a/tools/perf/Documentation/perf-report.txt
> +++ b/tools/perf/Documentation/perf-report.txt
> @@ -345,6 +345,9 @@ OPTIONS
>          Load module symbols. WARNING: This should only be used with -k and
>          a LIVE kernel.
>  
> +--vdso=<vdso1[,vdso2]>::
> +	Specify vdso pathnames. You can specify up to two for multiarch-support.
> +
>  -f::
>  --force::
>          Don't do ownership validation.
> diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
> index ff086ef05a0c..48f9974ca4c5 100644
> --- a/tools/perf/Documentation/perf-script.txt
> +++ b/tools/perf/Documentation/perf-script.txt
> @@ -296,6 +296,9 @@ OPTIONS
>  --kallsyms=<file>::
>          kallsyms pathname
>  
> +--vdso=<vdso1[,vdso2]>::
> +	Specify vdso pathnames. You can specify up to two for multiarch-support.
> +
>  --symfs=<directory>::
>          Look for files with symbols relative to this directory.
>  
> diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
> index a754875fa5bb..1cf3ee16a0c1 100644
> --- a/tools/perf/Documentation/perf-top.txt
> +++ b/tools/perf/Documentation/perf-top.txt
> @@ -76,6 +76,9 @@ Default is to monitor all CPUS.
>  --kallsyms=<file>::
>  	kallsyms pathname
>  
> +--vdso=<vdso1[,vdso2]>::
> +	Specify vdso pathnames. You can specify up to two for multiarch-support.
> +
>  -m <pages>::
>  --mmap-pages=<pages>::
>  	Number of mmap data pages (must be a power of two) or size
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 50d2fb222d48..ff466882065d 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -742,6 +742,8 @@ int cmd_annotate(int argc, const char **argv)
>  		   "file", "vmlinux pathname"),
>  	OPT_BOOLEAN('m', "modules", &symbol_conf.use_modules,
>  		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
> +	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
> +		     parse_vdso_pathnames),
>  	OPT_BOOLEAN('l', "print-line", &annotate_opts.print_lines,
>  		    "print matching source lines (may be slow)"),
>  	OPT_BOOLEAN('P', "full-paths", &annotate_opts.full_path,
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c157bd31f2e5..4764f9139661 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -3018,6 +3018,8 @@ static int perf_c2c__report(int argc, const char **argv)
>  	const struct option options[] = {
>  	OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
>  		   "file", "vmlinux pathname"),
> +	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
> +		     parse_vdso_pathnames),
>  	OPT_STRING('i', "input", &input_name, "file",
>  		   "the input file to process"),
>  	OPT_INCR('N', "node-info", &c2c.node_info,
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index a212678d47be..e774e83d0a0f 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -2247,6 +2247,8 @@ int cmd_inject(int argc, const char **argv)
>  			    "don't load vmlinux even if found"),
>  		OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name, "file",
>  			   "kallsyms pathname"),
> +		OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
> +		     parse_vdso_pathnames),
>  		OPT_BOOLEAN('f', "force", &data.force, "don't complain, do it"),
>  		OPT_CALLBACK_OPTARG(0, "itrace", &inject.itrace_synth_opts,
>  				    NULL, "opts", "Instruction Tracing options\n"
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 69618fb0110b..a64b48460dce 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1324,6 +1324,8 @@ int cmd_report(int argc, const char **argv)
>                      "don't load vmlinux even if found"),
>  	OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name,
>  		   "file", "kallsyms pathname"),
> +	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
> +		     parse_vdso_pathnames),
>  	OPT_BOOLEAN('f', "force", &symbol_conf.force, "don't complain, do it"),
>  	OPT_BOOLEAN('m', "modules", &symbol_conf.use_modules,
>  		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index c16224b1fef3..2e358922a8d1 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3965,6 +3965,8 @@ int cmd_script(int argc, const char **argv)
>  		   "file", "vmlinux pathname"),
>  	OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name,
>  		   "file", "kallsyms pathname"),
> +	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
> +		     parse_vdso_pathnames),
>  	OPT_BOOLEAN('G', "hide-call-graph", &no_callchain,
>  		    "When printing symbols do not display call chain"),
>  	OPT_CALLBACK(0, "symfs", NULL, "directory",
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 1d6aef51c122..a3cce4e76eb9 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1479,6 +1479,8 @@ int cmd_top(int argc, const char **argv)
>  		   "file", "kallsyms pathname"),
>  	OPT_BOOLEAN('K', "hide_kernel_symbols", &top.hide_kernel_symbols,
>  		    "hide kernel symbols"),
> +	OPT_CALLBACK(0, "vdso", NULL, "vdso1[,vdso2]", "vdso pathnames",
> +		     parse_vdso_pathnames),
>  	OPT_CALLBACK('m', "mmap-pages", &opts->mmap_pages, "pages",
>  		     "number of mmap data pages", evlist__parse_mmap_pages),
>  	OPT_INTEGER('r', "realtime", &top.realtime_prio,
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 72aec8f61b94..8d9d78d3150c 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -16,6 +16,7 @@
>  #include "debug.h"
>  #include "disasm.h"
>  #include "dso.h"
> +#include "vdso.h"
>  #include "env.h"
>  #include "evsel.h"
>  #include "map.h"
> @@ -1126,7 +1127,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
>  		dirname(build_id_path);
>  
> -	if (dso__is_kcore(dso))
> +	if (dso__is_kcore(dso) || dso__is_vdso(dso))
>  		goto fallback;
>  
>  	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
> @@ -1134,7 +1135,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  		goto fallback;
>  
>  	linkname[len] = '\0';
> -	if (strstr(linkname, DSO__NAME_KALLSYMS) ||
> +	if (strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO) ||
>  		access(filename, R_OK)) {
>  fallback:
>  		/*
> @@ -1142,7 +1143,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  		 * cache, or is just a kallsyms file, well, lets hope that this
>  		 * DSO is the same as when 'perf record' ran.
>  		 */
> -		if (dso__kernel(dso) && dso__long_name(dso)[0] == '/')
> +		if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
>  			snprintf(filename, filename_size, "%s", dso__long_name(dso));
>  		else
>  			__symbol__join_symfs(filename, filename_size, dso__long_name(dso));
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 9e5940b5bc59..a90c647d37e1 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -19,6 +19,7 @@
>  #include "build-id.h"
>  #include "cap.h"
>  #include "dso.h"
> +#include "vdso.h"
>  #include "util.h" // lsdir()
>  #include "debug.h"
>  #include "event.h"
> @@ -44,6 +45,7 @@
>  
>  static int dso__load_kernel_sym(struct dso *dso, struct map *map);
>  static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map);
> +static int dso__load_vdso_sym(struct dso *dso, struct map *map);
>  static bool symbol__is_idle(const char *name);
>  
>  int vmlinux_path__nr_entries;
> @@ -1833,6 +1835,12 @@ int dso__load(struct dso *dso, struct map *map)
>  		goto out;
>  	}
>  
> +	if (dso__is_vdso(dso)) {
> +		ret = dso__load_vdso_sym(dso, map);
> +		if (ret > 0)
> +			goto out;
> +	}
> +
>  	dso__set_adjust_symbols(dso, false);
>  
>  	if (perfmap) {
> @@ -2017,12 +2025,14 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
>  		dso__set_binary_type(dso, DSO_BINARY_TYPE__VMLINUX);
>  
>  	err = dso__load_sym(dso, map, &ss, &ss, 0);
> -	symsrc__destroy(&ss);
> -
>  	if (err > 0) {
>  		dso__set_loaded(dso);
>  		pr_debug("Using %s for symbols\n", symfs_vmlinux);
> +
> +		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
> +			dso__set_symsrc_filename(dso, strdup(symfs_vmlinux));
>  	}
> +	symsrc__destroy(&ss);
>  
>  	return err;
>  }
> @@ -2349,6 +2359,74 @@ static int vmlinux_path__init(struct perf_env *env)
>  	return -1;
>  }
>  
> +int parse_vdso_pathnames(const struct option *opt __maybe_unused,
> +			 const char *arg, int unset __maybe_unused)
> +{
> +	char *tmp, *tok, *str = strdup(arg);
> +	unsigned int i = 0;
> +
> +	for (tok = strtok_r(str, ",", &tmp); tok && i < ARRAY_SIZE(symbol_conf.vdso_name);
> +	     tok = strtok_r(NULL, ",", &tmp)) {
> +		symbol_conf.vdso_name[i++] = strdup(tok);
> +	}
> +
> +	free(str);
> +	return 0;
> +}
> +
> +static int dso__load_vdso(struct dso *dso, struct map *map,
> +			  const char *vdso)
> +{
> +	int err = -1;
> +	struct symsrc ss;
> +	char symfs_vdso[PATH_MAX];
> +
> +	if (vdso[0] == '/')
> +		snprintf(symfs_vdso, sizeof(symfs_vdso), "%s", vdso);
> +	else
> +		symbol__join_symfs(symfs_vdso, vdso);
> +
> +	if (symsrc__init(&ss, dso, symfs_vdso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO))
> +		return -1;
> +
> +	/*
> +	 * dso__load_sym() may copy 'dso' which will result in the copies having
> +	 * an incorrect long name unless we set it here first.
> +	 */
> +	dso__set_long_name(dso, vdso, false);
> +	dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
> +
> +	err = dso__load_sym(dso, map, &ss, &ss, 0);
> +	if (err > 0) {
> +		dso__set_loaded(dso);
> +		pr_debug("Using %s for %s symbols\n", symfs_vdso, dso__short_name(dso));
> +
> +		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
> +			dso__set_symsrc_filename(dso, strdup(symfs_vdso));
> +	}
> +	symsrc__destroy(&ss);
> +
> +	return err;
> +}
> +
> +static int dso__load_vdso_sym(struct dso *dso, struct map *map)
> +{
> +	int ret;
> +
> +	if (!dso__is_vdso(dso))
> +		return -1;
> +
> +	for (unsigned int i = 0; i < ARRAY_SIZE(symbol_conf.vdso_name); i++) {
> +		if (symbol_conf.vdso_name[i] != NULL) {
> +			ret = dso__load_vdso(dso, map, symbol_conf.vdso_name[i]);
> +			if (ret > 0)
> +				return ret;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
>  int setup_list(struct strlist **list, const char *list_str,
>  		      const char *list_name)
>  {
> diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> index c114bbceef40..108356e3c981 100644
> --- a/tools/perf/util/symbol_conf.h
> +++ b/tools/perf/util/symbol_conf.h
> @@ -3,6 +3,7 @@
>  #define __PERF_SYMBOL_CONF 1
>  
>  #include <stdbool.h>
> +#include <subcmd/parse-options.h>
>  
>  struct strlist;
>  struct intlist;
> @@ -55,6 +56,7 @@ struct symbol_conf {
>  	const char	*default_guest_vmlinux_name,
>  			*default_guest_kallsyms,
>  			*default_guest_modules;
> +	const char	*vdso_name[2];
>  	const char	*guestmount;
>  	const char	*dso_list_str,
>  			*comm_list_str,
> @@ -85,4 +87,7 @@ struct symbol_conf {
>  
>  extern struct symbol_conf symbol_conf;
>  
> +int parse_vdso_pathnames(const struct option *opt __maybe_unused,
> +			 const char *arg, int unset __maybe_unused);
> +
>  #endif // __PERF_SYMBOL_CONF


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-25 13:20   ` Adrian Hunter
@ 2024-06-26  2:26     ` duchangbin
  2024-06-26 10:32       ` Adrian Hunter
  0 siblings, 1 reply; 15+ messages in thread
From: duchangbin @ 2024-06-26  2:26 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: duchangbin, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Ian Rogers, Liang, Kan, Nick Desaulniers,
	Bill Wendling, Justin Stitt, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev

On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
> On 25/06/24 06:37, Changbin Du wrote:
> > The vdso dumped from process memory (in buildid-cache) lacks debugging
> > info. To annotate vdso symbols with source lines we need specify a
> > debugging version.
> > 
> > For x86, we can find them from your local build as
> > arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
> > /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
> > the buildid has to match.
> > 
> > $ sudo perf record -a
> > $ sudo perf report --objdump=llvm-objdump \
> >   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
> > 
> > Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
> > __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
> > Percent│       movq    -48(%rbp),%rsi
> >        │       testq   %rax,%rax
> >        │     ;               return vread_hvclock();
> >        │       movq    %rax,%rdx
> >        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
> >        │     ↑ js      eb
> >        │     ↑ jmp     74
> >        │     ;               ts->tv_sec = vdso_ts->sec;
> >   0.02 │147:   leaq    2(%rbx),%rax
> >        │       shlq    $4, %rax
> >        │       addq    %r10,%rax
> >        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
> >   9.38 │152:   movl    (%r10),%ecx
> > 
> > When doing cross platform analysis, we also need specify the vdso path if
> > we are interested in its symbols.
> 
> Would it be possible to add vdso and vdso debug to the build-id
> cache and ensure perf can find it there?
> 
> Typically, getting dsos from another machine is handled via
> build-id cache e.g. what perf-archive does
> 
Hmm. I agree this is better alternative approach for cross-machine analysis.
When collecting vdsos to buildid cache, I think we can use the local searched
objects (with debug symbols) instead if its build-id matches vdsos from process
dumping (the real code ran).

Currently I just follow what perf does for vmlinux so to reuse most of existing
code. Maybe vmlinux is too big to add to buildid-cahce?

Can we keep our current strategy for now? I'll think about above options when
I have more time.

-- 
Cheers,
Changbin Du

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-26  2:26     ` duchangbin
@ 2024-06-26 10:32       ` Adrian Hunter
  2024-06-27 23:53         ` Namhyung Kim
  0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2024-06-26 10:32 UTC (permalink / raw)
  To: duchangbin
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Nathan Chancellor, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Ian Rogers, Liang, Kan, Nick Desaulniers,
	Bill Wendling, Justin Stitt, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev

On 26/06/24 05:26, duchangbin wrote:
> On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
>> On 25/06/24 06:37, Changbin Du wrote:
>>> The vdso dumped from process memory (in buildid-cache) lacks debugging
>>> info. To annotate vdso symbols with source lines we need specify a
>>> debugging version.
>>>
>>> For x86, we can find them from your local build as
>>> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
>>> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
>>> the buildid has to match.
>>>
>>> $ sudo perf record -a
>>> $ sudo perf report --objdump=llvm-objdump \
>>>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
>>>
>>> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
>>> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
>>> Percent│       movq    -48(%rbp),%rsi
>>>        │       testq   %rax,%rax
>>>        │     ;               return vread_hvclock();
>>>        │       movq    %rax,%rdx
>>>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
>>>        │     ↑ js      eb
>>>        │     ↑ jmp     74
>>>        │     ;               ts->tv_sec = vdso_ts->sec;
>>>   0.02 │147:   leaq    2(%rbx),%rax
>>>        │       shlq    $4, %rax
>>>        │       addq    %r10,%rax
>>>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
>>>   9.38 │152:   movl    (%r10),%ecx
>>>
>>> When doing cross platform analysis, we also need specify the vdso path if
>>> we are interested in its symbols.
>>
>> Would it be possible to add vdso and vdso debug to the build-id
>> cache and ensure perf can find it there?
>>
>> Typically, getting dsos from another machine is handled via
>> build-id cache e.g. what perf-archive does
>>
> Hmm. I agree this is better alternative approach for cross-machine analysis.
> When collecting vdsos to buildid cache, I think we can use the local searched
> objects (with debug symbols) instead if its build-id matches vdsos from process
> dumping (the real code ran).
> 
> Currently I just follow what perf does for vmlinux so to reuse most of existing
> code. Maybe vmlinux is too big to add to buildid-cahce?
> 
> Can we keep our current strategy for now? I'll think about above options when
> I have more time.
> 

I tried adding vdso via perf buildid-cache.  It doesn't work only
because the lookup expects the basename to be "vdso" but it is
"elf".

Adding a link from "vdso" to "elf" made it work e.g.

$ cat gettimeofday-test.c
#include <stdio.h>
#include <sys/time.h>

int main()
{
        struct timeval tv;
        int ret;

        ret = gettimeofday(&tv, NULL);
        if (ret == -1) {
                fprintf(stderr, "gettimeofday failed\n");
                return 1;
        }

        printf("%lu.%lu\n", (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec);

        return 0;
$ perf record -e intel_pt//u ./gettimeofday-test
1719397042.892837
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.026 MB perf.data ]
$ perf script --itrace=e
$ perf buildid-cache --remove /lib/modules/6.5.0-41-generic/vdso/vdso64.so
$ perf script --itrace=e
Warning:
2 instruction trace errors
 instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
 instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
$ perf buildid-cache --add /lib/modules/6.5.0-41-generic/vdso/vdso64.so
$ perf script --itrace=e
Warning:
2 instruction trace errors
 instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
 instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
$ cd ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8
~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ln -s elf vdso
~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ls -l
total 36
-rw-r--r-- 1 ahunter ahunter 33272 Jun 26 13:17 elf
-rw-r----- 1 ahunter ahunter     0 Jun 26 13:17 probes
lrwxrwxrwx 1 ahunter ahunter     3 Jun 26 13:18 vdso -> elf
/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ cd
$ perf script --itrace=e
$ 

So maybe a change could be made to build_id_cache__add() to add
the extra link if the file name matches vdso


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-26 10:32       ` Adrian Hunter
@ 2024-06-27 23:53         ` Namhyung Kim
  2024-06-28  4:21           ` duchangbin
  0 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2024-06-27 23:53 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: duchangbin, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Nathan Chancellor, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev

Hello guys,

On Wed, Jun 26, 2024 at 01:32:42PM +0300, Adrian Hunter wrote:
> On 26/06/24 05:26, duchangbin wrote:
> > On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
> >> On 25/06/24 06:37, Changbin Du wrote:
> >>> The vdso dumped from process memory (in buildid-cache) lacks debugging
> >>> info. To annotate vdso symbols with source lines we need specify a
> >>> debugging version.
> >>>
> >>> For x86, we can find them from your local build as
> >>> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
> >>> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
> >>> the buildid has to match.
> >>>
> >>> $ sudo perf record -a
> >>> $ sudo perf report --objdump=llvm-objdump \
> >>>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
> >>>
> >>> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
> >>> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
> >>> Percent│       movq    -48(%rbp),%rsi
> >>>        │       testq   %rax,%rax
> >>>        │     ;               return vread_hvclock();
> >>>        │       movq    %rax,%rdx
> >>>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
> >>>        │     ↑ js      eb
> >>>        │     ↑ jmp     74
> >>>        │     ;               ts->tv_sec = vdso_ts->sec;
> >>>   0.02 │147:   leaq    2(%rbx),%rax
> >>>        │       shlq    $4, %rax
> >>>        │       addq    %r10,%rax
> >>>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
> >>>   9.38 │152:   movl    (%r10),%ecx
> >>>
> >>> When doing cross platform analysis, we also need specify the vdso path if
> >>> we are interested in its symbols.
> >>
> >> Would it be possible to add vdso and vdso debug to the build-id
> >> cache and ensure perf can find it there?
> >>
> >> Typically, getting dsos from another machine is handled via
> >> build-id cache e.g. what perf-archive does
> >>
> > Hmm. I agree this is better alternative approach for cross-machine analysis.
> > When collecting vdsos to buildid cache, I think we can use the local searched
> > objects (with debug symbols) instead if its build-id matches vdsos from process
> > dumping (the real code ran).
> > 
> > Currently I just follow what perf does for vmlinux so to reuse most of existing
> > code. Maybe vmlinux is too big to add to buildid-cahce?
> > 
> > Can we keep our current strategy for now? I'll think about above options when
> > I have more time.
> > 
> 
> I tried adding vdso via perf buildid-cache.  It doesn't work only
> because the lookup expects the basename to be "vdso" but it is
> "elf".
> 
> Adding a link from "vdso" to "elf" made it work e.g.
> 
> $ cat gettimeofday-test.c
> #include <stdio.h>
> #include <sys/time.h>
> 
> int main()
> {
>         struct timeval tv;
>         int ret;
> 
>         ret = gettimeofday(&tv, NULL);
>         if (ret == -1) {
>                 fprintf(stderr, "gettimeofday failed\n");
>                 return 1;
>         }
> 
>         printf("%lu.%lu\n", (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec);
> 
>         return 0;
> $ perf record -e intel_pt//u ./gettimeofday-test
> 1719397042.892837
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.026 MB perf.data ]
> $ perf script --itrace=e
> $ perf buildid-cache --remove /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> $ perf script --itrace=e
> Warning:
> 2 instruction trace errors
>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> $ perf buildid-cache --add /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> $ perf script --itrace=e
> Warning:
> 2 instruction trace errors
>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> $ cd ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8
> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ln -s elf vdso
> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ls -l
> total 36
> -rw-r--r-- 1 ahunter ahunter 33272 Jun 26 13:17 elf
> -rw-r----- 1 ahunter ahunter     0 Jun 26 13:17 probes
> lrwxrwxrwx 1 ahunter ahunter     3 Jun 26 13:18 vdso -> elf
> /.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ cd
> $ perf script --itrace=e
> $ 
> 
> So maybe a change could be made to build_id_cache__add() to add
> the extra link if the file name matches vdso
 
Thanks for doing this!  I noticed buildid_cache__basename() will handle
the name properly once it realizes the file is a vdso.  Maybe we can
check the filepath with some patterns, or simply add an command line
option to say it's a vdso.

Thanks,
Namhyung


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-27 23:53         ` Namhyung Kim
@ 2024-06-28  4:21           ` duchangbin
  2024-06-28 17:27             ` Adrian Hunter
  0 siblings, 1 reply; 15+ messages in thread
From: duchangbin @ 2024-06-28  4:21 UTC (permalink / raw)
  To: Namhyung Kim, Adrian Hunter
  Cc: Adrian Hunter, duchangbin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Nathan Chancellor, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Liang, Kan,
	Nick Desaulniers, Bill Wendling, Justin Stitt,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev

On Thu, Jun 27, 2024 at 04:53:18PM -0700, Namhyung Kim wrote:
> Hello guys,
> 
> On Wed, Jun 26, 2024 at 01:32:42PM +0300, Adrian Hunter wrote:
> > On 26/06/24 05:26, duchangbin wrote:
> > > On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
> > >> On 25/06/24 06:37, Changbin Du wrote:
> > >>> The vdso dumped from process memory (in buildid-cache) lacks debugging
> > >>> info. To annotate vdso symbols with source lines we need specify a
> > >>> debugging version.
> > >>>
> > >>> For x86, we can find them from your local build as
> > >>> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
> > >>> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
> > >>> the buildid has to match.
> > >>>
> > >>> $ sudo perf record -a
> > >>> $ sudo perf report --objdump=llvm-objdump \
> > >>>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
> > >>>
> > >>> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
> > >>> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
> > >>> Percent│       movq    -48(%rbp),%rsi
> > >>>        │       testq   %rax,%rax
> > >>>        │     ;               return vread_hvclock();
> > >>>        │       movq    %rax,%rdx
> > >>>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
> > >>>        │     ↑ js      eb
> > >>>        │     ↑ jmp     74
> > >>>        │     ;               ts->tv_sec = vdso_ts->sec;
> > >>>   0.02 │147:   leaq    2(%rbx),%rax
> > >>>        │       shlq    $4, %rax
> > >>>        │       addq    %r10,%rax
> > >>>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
> > >>>   9.38 │152:   movl    (%r10),%ecx
> > >>>
> > >>> When doing cross platform analysis, we also need specify the vdso path if
> > >>> we are interested in its symbols.
> > >>
> > >> Would it be possible to add vdso and vdso debug to the build-id
> > >> cache and ensure perf can find it there?
> > >>
> > >> Typically, getting dsos from another machine is handled via
> > >> build-id cache e.g. what perf-archive does
> > >>
> > > Hmm. I agree this is better alternative approach for cross-machine analysis.
> > > When collecting vdsos to buildid cache, I think we can use the local searched
> > > objects (with debug symbols) instead if its build-id matches vdsos from process
> > > dumping (the real code ran).
> > > 
> > > Currently I just follow what perf does for vmlinux so to reuse most of existing
> > > code. Maybe vmlinux is too big to add to buildid-cahce?
> > > 
> > > Can we keep our current strategy for now? I'll think about above options when
> > > I have more time.
> > > 
> > 
> > I tried adding vdso via perf buildid-cache.  It doesn't work only
> > because the lookup expects the basename to be "vdso" but it is
> > "elf".
> > 
> > Adding a link from "vdso" to "elf" made it work e.g.
> > 
> > $ cat gettimeofday-test.c
> > #include <stdio.h>
> > #include <sys/time.h>
> > 
> > int main()
> > {
> >         struct timeval tv;
> >         int ret;
> > 
> >         ret = gettimeofday(&tv, NULL);
> >         if (ret == -1) {
> >                 fprintf(stderr, "gettimeofday failed\n");
> >                 return 1;
> >         }
> > 
> >         printf("%lu.%lu\n", (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec);
> > 
> >         return 0;
> > $ perf record -e intel_pt//u ./gettimeofday-test
> > 1719397042.892837
> > [ perf record: Woken up 1 times to write data ]
> > [ perf record: Captured and wrote 0.026 MB perf.data ]
> > $ perf script --itrace=e
> > $ perf buildid-cache --remove /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> > $ perf script --itrace=e
> > Warning:
> > 2 instruction trace errors
> >  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
> >  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> > $ perf buildid-cache --add /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> > $ perf script --itrace=e
> > Warning:
> > 2 instruction trace errors
> >  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
> >  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> > $ cd ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8
> > ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ln -s elf vdso
> > ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ls -l
> > total 36
> > -rw-r--r-- 1 ahunter ahunter 33272 Jun 26 13:17 elf
> > -rw-r----- 1 ahunter ahunter     0 Jun 26 13:17 probes
> > lrwxrwxrwx 1 ahunter ahunter     3 Jun 26 13:18 vdso -> elf
> > /.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ cd
> > $ perf script --itrace=e
> > $ 
> > 
> > So maybe a change could be made to build_id_cache__add() to add
> > the extra link if the file name matches vdso
>  
> Thanks for doing this!  I noticed buildid_cache__basename() will handle
> the name properly once it realizes the file is a vdso.  Maybe we can
> check the filepath with some patterns, or simply add an command line
> option to say it's a vdso.
> 
> Thanks,
> Namhyung
>
I added some tricks for vdso in build_id_cache__add(). It replace vdso object
with debugging one if found. Is this okay?

+static char *build_id_cache__find_debug_vdso(const char *sbuild_id)
+{
+       char sbuild_id_tmp[SBUILD_ID_SIZE];
+       struct build_id bid;
+       int i, ret = 0;
+
+       printf("Looking at the vdso_path (%d entries long)\n",
+                vdso_paths.nr_entries + 1);
+
+       for (i = 0; i < vdso_paths.nr_entries; ++i) {
+               ret = filename__read_build_id(vdso_paths.paths[i], &bid);
+               if (ret < 0)
+                       continue;
+
+               build_id__sprintf(&bid, sbuild_id_tmp);
+               if (!strcmp(sbuild_id, sbuild_id_tmp)) {
+                       printf("Found debugging vdso %s\n", vdso_paths.paths[i]);
+                       return strdup(vdso_paths.paths[i]);
+               }
+       }
+
+       return NULL;
+}
+
 int
 build_id_cache__add(const char *sbuild_id, const char *name, const char *realname,
                    struct nsinfo *nsi, bool is_kallsyms, bool is_vdso,
                    const char *proper_name, const char *root_dir)
 {
        const size_t size = PATH_MAX;
-       char *filename = NULL, *dir_name = NULL, *linkname = zalloc(size), *tmp;
+       char *filename = NULL, *dir_name = NULL, *vdso_name = NULL, *linkname = zalloc(size), *tmp;
        char *debugfile = NULL;
        int err = -1;

+       /* replace vdso object with debugging one if found */
+       if (is_vdso) {
+               vdso_name = build_id_cache__find_debug_vdso(sbuild_id);
+               if (vdso_name)
+                       name = realname = vdso_name;
+       }
+
        if (!proper_name)
                proper_name = name;

> 

-- 
Cheers,
Changbin Du

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-28  4:21           ` duchangbin
@ 2024-06-28 17:27             ` Adrian Hunter
  2024-06-29  2:32               ` duchangbin
  0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2024-06-28 17:27 UTC (permalink / raw)
  To: duchangbin, Namhyung Kim
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Nathan Chancellor, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Liang, Kan, Nick Desaulniers, Bill Wendling,
	Justin Stitt, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev

On 28/06/24 07:21, duchangbin wrote:
> On Thu, Jun 27, 2024 at 04:53:18PM -0700, Namhyung Kim wrote:
>> Hello guys,
>>
>> On Wed, Jun 26, 2024 at 01:32:42PM +0300, Adrian Hunter wrote:
>>> On 26/06/24 05:26, duchangbin wrote:
>>>> On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
>>>>> On 25/06/24 06:37, Changbin Du wrote:
>>>>>> The vdso dumped from process memory (in buildid-cache) lacks debugging
>>>>>> info. To annotate vdso symbols with source lines we need specify a
>>>>>> debugging version.
>>>>>>
>>>>>> For x86, we can find them from your local build as
>>>>>> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
>>>>>> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
>>>>>> the buildid has to match.
>>>>>>
>>>>>> $ sudo perf record -a
>>>>>> $ sudo perf report --objdump=llvm-objdump \
>>>>>>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
>>>>>>
>>>>>> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
>>>>>> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
>>>>>> Percent│       movq    -48(%rbp),%rsi
>>>>>>        │       testq   %rax,%rax
>>>>>>        │     ;               return vread_hvclock();
>>>>>>        │       movq    %rax,%rdx
>>>>>>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
>>>>>>        │     ↑ js      eb
>>>>>>        │     ↑ jmp     74
>>>>>>        │     ;               ts->tv_sec = vdso_ts->sec;
>>>>>>   0.02 │147:   leaq    2(%rbx),%rax
>>>>>>        │       shlq    $4, %rax
>>>>>>        │       addq    %r10,%rax
>>>>>>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
>>>>>>   9.38 │152:   movl    (%r10),%ecx
>>>>>>
>>>>>> When doing cross platform analysis, we also need specify the vdso path if
>>>>>> we are interested in its symbols.
>>>>>
>>>>> Would it be possible to add vdso and vdso debug to the build-id
>>>>> cache and ensure perf can find it there?
>>>>>
>>>>> Typically, getting dsos from another machine is handled via
>>>>> build-id cache e.g. what perf-archive does
>>>>>
>>>> Hmm. I agree this is better alternative approach for cross-machine analysis.
>>>> When collecting vdsos to buildid cache, I think we can use the local searched
>>>> objects (with debug symbols) instead if its build-id matches vdsos from process
>>>> dumping (the real code ran).
>>>>
>>>> Currently I just follow what perf does for vmlinux so to reuse most of existing
>>>> code. Maybe vmlinux is too big to add to buildid-cahce?
>>>>
>>>> Can we keep our current strategy for now? I'll think about above options when
>>>> I have more time.
>>>>
>>>
>>> I tried adding vdso via perf buildid-cache.  It doesn't work only
>>> because the lookup expects the basename to be "vdso" but it is
>>> "elf".
>>>
>>> Adding a link from "vdso" to "elf" made it work e.g.
>>>
>>> $ cat gettimeofday-test.c
>>> #include <stdio.h>
>>> #include <sys/time.h>
>>>
>>> int main()
>>> {
>>>         struct timeval tv;
>>>         int ret;
>>>
>>>         ret = gettimeofday(&tv, NULL);
>>>         if (ret == -1) {
>>>                 fprintf(stderr, "gettimeofday failed\n");
>>>                 return 1;
>>>         }
>>>
>>>         printf("%lu.%lu\n", (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec);
>>>
>>>         return 0;
>>> $ perf record -e intel_pt//u ./gettimeofday-test
>>> 1719397042.892837
>>> [ perf record: Woken up 1 times to write data ]
>>> [ perf record: Captured and wrote 0.026 MB perf.data ]
>>> $ perf script --itrace=e
>>> $ perf buildid-cache --remove /lib/modules/6.5.0-41-generic/vdso/vdso64.so
>>> $ perf script --itrace=e
>>> Warning:
>>> 2 instruction trace errors
>>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
>>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
>>> $ perf buildid-cache --add /lib/modules/6.5.0-41-generic/vdso/vdso64.so
>>> $ perf script --itrace=e
>>> Warning:
>>> 2 instruction trace errors
>>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
>>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
>>> $ cd ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8
>>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ln -s elf vdso
>>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ls -l
>>> total 36
>>> -rw-r--r-- 1 ahunter ahunter 33272 Jun 26 13:17 elf
>>> -rw-r----- 1 ahunter ahunter     0 Jun 26 13:17 probes
>>> lrwxrwxrwx 1 ahunter ahunter     3 Jun 26 13:18 vdso -> elf
>>> /.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ cd
>>> $ perf script --itrace=e
>>> $ 
>>>
>>> So maybe a change could be made to build_id_cache__add() to add
>>> the extra link if the file name matches vdso
>>  
>> Thanks for doing this!  I noticed buildid_cache__basename() will handle
>> the name properly once it realizes the file is a vdso.  Maybe we can
>> check the filepath with some patterns, or simply add an command line
>> option to say it's a vdso.
>>
>> Thanks,
>> Namhyung
>>
> I added some tricks for vdso in build_id_cache__add(). It replace vdso object
> with debugging one if found. Is this okay?

perf has support for storing debug files in the build-id
cache using the base name "debug" instead of "elf", so really
it would be better to make that work

> 
> +static char *build_id_cache__find_debug_vdso(const char *sbuild_id)
> +{
> +       char sbuild_id_tmp[SBUILD_ID_SIZE];
> +       struct build_id bid;
> +       int i, ret = 0;
> +
> +       printf("Looking at the vdso_path (%d entries long)\n",
> +                vdso_paths.nr_entries + 1);
> +
> +       for (i = 0; i < vdso_paths.nr_entries; ++i) {
> +               ret = filename__read_build_id(vdso_paths.paths[i], &bid);
> +               if (ret < 0)
> +                       continue;
> +
> +               build_id__sprintf(&bid, sbuild_id_tmp);
> +               if (!strcmp(sbuild_id, sbuild_id_tmp)) {
> +                       printf("Found debugging vdso %s\n", vdso_paths.paths[i]);
> +                       return strdup(vdso_paths.paths[i]);
> +               }
> +       }
> +
> +       return NULL;
> +}
> +
>  int
>  build_id_cache__add(const char *sbuild_id, const char *name, const char *realname,
>                     struct nsinfo *nsi, bool is_kallsyms, bool is_vdso,
>                     const char *proper_name, const char *root_dir)
>  {
>         const size_t size = PATH_MAX;
> -       char *filename = NULL, *dir_name = NULL, *linkname = zalloc(size), *tmp;
> +       char *filename = NULL, *dir_name = NULL, *vdso_name = NULL, *linkname = zalloc(size), *tmp;
>         char *debugfile = NULL;
>         int err = -1;
> 
> +       /* replace vdso object with debugging one if found */
> +       if (is_vdso) {
> +               vdso_name = build_id_cache__find_debug_vdso(sbuild_id);
> +               if (vdso_name)
> +                       name = realname = vdso_name;
> +       }
> +
>         if (!proper_name)
>                 proper_name = name;
> 
>>
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-28 17:27             ` Adrian Hunter
@ 2024-06-29  2:32               ` duchangbin
  2024-06-29 18:47                 ` Adrian Hunter
  0 siblings, 1 reply; 15+ messages in thread
From: duchangbin @ 2024-06-29  2:32 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: duchangbin, Namhyung Kim, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Nathan Chancellor, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Liang, Kan,
	Nick Desaulniers, Bill Wendling, Justin Stitt,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev

On Fri, Jun 28, 2024 at 08:27:55PM +0300, Adrian Hunter wrote:
> On 28/06/24 07:21, duchangbin wrote:
> > On Thu, Jun 27, 2024 at 04:53:18PM -0700, Namhyung Kim wrote:
> >> Hello guys,
> >>
> >> On Wed, Jun 26, 2024 at 01:32:42PM +0300, Adrian Hunter wrote:
> >>> On 26/06/24 05:26, duchangbin wrote:
> >>>> On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
> >>>>> On 25/06/24 06:37, Changbin Du wrote:
> >>>>>> The vdso dumped from process memory (in buildid-cache) lacks debugging
> >>>>>> info. To annotate vdso symbols with source lines we need specify a
> >>>>>> debugging version.
> >>>>>>
> >>>>>> For x86, we can find them from your local build as
> >>>>>> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
> >>>>>> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
> >>>>>> the buildid has to match.
> >>>>>>
> >>>>>> $ sudo perf record -a
> >>>>>> $ sudo perf report --objdump=llvm-objdump \
> >>>>>>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
> >>>>>>
> >>>>>> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
> >>>>>> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
> >>>>>> Percent│       movq    -48(%rbp),%rsi
> >>>>>>        │       testq   %rax,%rax
> >>>>>>        │     ;               return vread_hvclock();
> >>>>>>        │       movq    %rax,%rdx
> >>>>>>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
> >>>>>>        │     ↑ js      eb
> >>>>>>        │     ↑ jmp     74
> >>>>>>        │     ;               ts->tv_sec = vdso_ts->sec;
> >>>>>>   0.02 │147:   leaq    2(%rbx),%rax
> >>>>>>        │       shlq    $4, %rax
> >>>>>>        │       addq    %r10,%rax
> >>>>>>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
> >>>>>>   9.38 │152:   movl    (%r10),%ecx
> >>>>>>
> >>>>>> When doing cross platform analysis, we also need specify the vdso path if
> >>>>>> we are interested in its symbols.
> >>>>>
> >>>>> Would it be possible to add vdso and vdso debug to the build-id
> >>>>> cache and ensure perf can find it there?
> >>>>>
> >>>>> Typically, getting dsos from another machine is handled via
> >>>>> build-id cache e.g. what perf-archive does
> >>>>>
> >>>> Hmm. I agree this is better alternative approach for cross-machine analysis.
> >>>> When collecting vdsos to buildid cache, I think we can use the local searched
> >>>> objects (with debug symbols) instead if its build-id matches vdsos from process
> >>>> dumping (the real code ran).
> >>>>
> >>>> Currently I just follow what perf does for vmlinux so to reuse most of existing
> >>>> code. Maybe vmlinux is too big to add to buildid-cahce?
> >>>>
> >>>> Can we keep our current strategy for now? I'll think about above options when
> >>>> I have more time.
> >>>>
> >>>
> >>> I tried adding vdso via perf buildid-cache.  It doesn't work only
> >>> because the lookup expects the basename to be "vdso" but it is
> >>> "elf".
> >>>
> >>> Adding a link from "vdso" to "elf" made it work e.g.
> >>>
> >>> $ cat gettimeofday-test.c
> >>> #include <stdio.h>
> >>> #include <sys/time.h>
> >>>
> >>> int main()
> >>> {
> >>>         struct timeval tv;
> >>>         int ret;
> >>>
> >>>         ret = gettimeofday(&tv, NULL);
> >>>         if (ret == -1) {
> >>>                 fprintf(stderr, "gettimeofday failed\n");
> >>>                 return 1;
> >>>         }
> >>>
> >>>         printf("%lu.%lu\n", (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec);
> >>>
> >>>         return 0;
> >>> $ perf record -e intel_pt//u ./gettimeofday-test
> >>> 1719397042.892837
> >>> [ perf record: Woken up 1 times to write data ]
> >>> [ perf record: Captured and wrote 0.026 MB perf.data ]
> >>> $ perf script --itrace=e
> >>> $ perf buildid-cache --remove /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> >>> $ perf script --itrace=e
> >>> Warning:
> >>> 2 instruction trace errors
> >>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
> >>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> >>> $ perf buildid-cache --add /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> >>> $ perf script --itrace=e
> >>> Warning:
> >>> 2 instruction trace errors
> >>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
> >>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> >>> $ cd ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8
> >>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ln -s elf vdso
> >>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ls -l
> >>> total 36
> >>> -rw-r--r-- 1 ahunter ahunter 33272 Jun 26 13:17 elf
> >>> -rw-r----- 1 ahunter ahunter     0 Jun 26 13:17 probes
> >>> lrwxrwxrwx 1 ahunter ahunter     3 Jun 26 13:18 vdso -> elf
> >>> /.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ cd
> >>> $ perf script --itrace=e
> >>> $ 
> >>>
> >>> So maybe a change could be made to build_id_cache__add() to add
> >>> the extra link if the file name matches vdso
> >>  
> >> Thanks for doing this!  I noticed buildid_cache__basename() will handle
> >> the name properly once it realizes the file is a vdso.  Maybe we can
> >> check the filepath with some patterns, or simply add an command line
> >> option to say it's a vdso.
> >>
> >> Thanks,
> >> Namhyung
> >>
> > I added some tricks for vdso in build_id_cache__add(). It replace vdso object
> > with debugging one if found. Is this okay?
> 
> perf has support for storing debug files in the build-id
> cache using the base name "debug" instead of "elf", so really
> it would be better to make that work
>
My thoughts are:
  1. add debugging vdso file as "debug" in buildid cache.
  2. add a new perf config item 'annotate.prefer_debug_file' (default false) to
     annotate with debugging object files.

-- 
Cheers,
Changbin Du

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-29  2:32               ` duchangbin
@ 2024-06-29 18:47                 ` Adrian Hunter
  2024-07-01  2:30                   ` duchangbin
  0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2024-06-29 18:47 UTC (permalink / raw)
  To: duchangbin
  Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Nathan Chancellor, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Liang, Kan,
	Nick Desaulniers, Bill Wendling, Justin Stitt,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev

On 29/06/24 05:32, duchangbin wrote:
> On Fri, Jun 28, 2024 at 08:27:55PM +0300, Adrian Hunter wrote:
>> On 28/06/24 07:21, duchangbin wrote:
>>> On Thu, Jun 27, 2024 at 04:53:18PM -0700, Namhyung Kim wrote:
>>>> Hello guys,
>>>>
>>>> On Wed, Jun 26, 2024 at 01:32:42PM +0300, Adrian Hunter wrote:
>>>>> On 26/06/24 05:26, duchangbin wrote:
>>>>>> On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
>>>>>>> On 25/06/24 06:37, Changbin Du wrote:
>>>>>>>> The vdso dumped from process memory (in buildid-cache) lacks debugging
>>>>>>>> info. To annotate vdso symbols with source lines we need specify a
>>>>>>>> debugging version.
>>>>>>>>
>>>>>>>> For x86, we can find them from your local build as
>>>>>>>> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
>>>>>>>> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
>>>>>>>> the buildid has to match.
>>>>>>>>
>>>>>>>> $ sudo perf record -a
>>>>>>>> $ sudo perf report --objdump=llvm-objdump \
>>>>>>>>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
>>>>>>>>
>>>>>>>> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
>>>>>>>> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
>>>>>>>> Percent│       movq    -48(%rbp),%rsi
>>>>>>>>        │       testq   %rax,%rax
>>>>>>>>        │     ;               return vread_hvclock();
>>>>>>>>        │       movq    %rax,%rdx
>>>>>>>>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
>>>>>>>>        │     ↑ js      eb
>>>>>>>>        │     ↑ jmp     74
>>>>>>>>        │     ;               ts->tv_sec = vdso_ts->sec;
>>>>>>>>   0.02 │147:   leaq    2(%rbx),%rax
>>>>>>>>        │       shlq    $4, %rax
>>>>>>>>        │       addq    %r10,%rax
>>>>>>>>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
>>>>>>>>   9.38 │152:   movl    (%r10),%ecx
>>>>>>>>
>>>>>>>> When doing cross platform analysis, we also need specify the vdso path if
>>>>>>>> we are interested in its symbols.
>>>>>>>
>>>>>>> Would it be possible to add vdso and vdso debug to the build-id
>>>>>>> cache and ensure perf can find it there?
>>>>>>>
>>>>>>> Typically, getting dsos from another machine is handled via
>>>>>>> build-id cache e.g. what perf-archive does
>>>>>>>
>>>>>> Hmm. I agree this is better alternative approach for cross-machine analysis.
>>>>>> When collecting vdsos to buildid cache, I think we can use the local searched
>>>>>> objects (with debug symbols) instead if its build-id matches vdsos from process
>>>>>> dumping (the real code ran).
>>>>>>
>>>>>> Currently I just follow what perf does for vmlinux so to reuse most of existing
>>>>>> code. Maybe vmlinux is too big to add to buildid-cahce?
>>>>>>
>>>>>> Can we keep our current strategy for now? I'll think about above options when
>>>>>> I have more time.
>>>>>>
>>>>>
>>>>> I tried adding vdso via perf buildid-cache.  It doesn't work only
>>>>> because the lookup expects the basename to be "vdso" but it is
>>>>> "elf".
>>>>>
>>>>> Adding a link from "vdso" to "elf" made it work e.g.
>>>>>
>>>>> $ cat gettimeofday-test.c
>>>>> #include <stdio.h>
>>>>> #include <sys/time.h>
>>>>>
>>>>> int main()
>>>>> {
>>>>>         struct timeval tv;
>>>>>         int ret;
>>>>>
>>>>>         ret = gettimeofday(&tv, NULL);
>>>>>         if (ret == -1) {
>>>>>                 fprintf(stderr, "gettimeofday failed\n");
>>>>>                 return 1;
>>>>>         }
>>>>>
>>>>>         printf("%lu.%lu\n", (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec);
>>>>>
>>>>>         return 0;
>>>>> $ perf record -e intel_pt//u ./gettimeofday-test
>>>>> 1719397042.892837
>>>>> [ perf record: Woken up 1 times to write data ]
>>>>> [ perf record: Captured and wrote 0.026 MB perf.data ]
>>>>> $ perf script --itrace=e
>>>>> $ perf buildid-cache --remove /lib/modules/6.5.0-41-generic/vdso/vdso64.so
>>>>> $ perf script --itrace=e
>>>>> Warning:
>>>>> 2 instruction trace errors
>>>>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
>>>>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
>>>>> $ perf buildid-cache --add /lib/modules/6.5.0-41-generic/vdso/vdso64.so
>>>>> $ perf script --itrace=e
>>>>> Warning:
>>>>> 2 instruction trace errors
>>>>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
>>>>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
>>>>> $ cd ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8
>>>>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ln -s elf vdso
>>>>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ls -l
>>>>> total 36
>>>>> -rw-r--r-- 1 ahunter ahunter 33272 Jun 26 13:17 elf
>>>>> -rw-r----- 1 ahunter ahunter     0 Jun 26 13:17 probes
>>>>> lrwxrwxrwx 1 ahunter ahunter     3 Jun 26 13:18 vdso -> elf
>>>>> /.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ cd
>>>>> $ perf script --itrace=e
>>>>> $ 
>>>>>
>>>>> So maybe a change could be made to build_id_cache__add() to add
>>>>> the extra link if the file name matches vdso
>>>>  
>>>> Thanks for doing this!  I noticed buildid_cache__basename() will handle
>>>> the name properly once it realizes the file is a vdso.  Maybe we can
>>>> check the filepath with some patterns, or simply add an command line
>>>> option to say it's a vdso.
>>>>
>>>> Thanks,
>>>> Namhyung
>>>>
>>> I added some tricks for vdso in build_id_cache__add(). It replace vdso object
>>> with debugging one if found. Is this okay?
>>
>> perf has support for storing debug files in the build-id
>> cache using the base name "debug" instead of "elf", so really
>> it would be better to make that work
>>
> My thoughts are:
>   1. add debugging vdso file as "debug" in buildid cache.
>   2. add a new perf config item 'annotate.prefer_debug_file' (default false) to
>      annotate with debugging object files.

Is that needed?  Isn't the debug information read from
the "debug" file?  If not, does that need fixing?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/5] perf: support specify vdso path in cmdline
  2024-06-29 18:47                 ` Adrian Hunter
@ 2024-07-01  2:30                   ` duchangbin
  0 siblings, 0 replies; 15+ messages in thread
From: duchangbin @ 2024-07-01  2:30 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: duchangbin, Namhyung Kim, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Nathan Chancellor, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Liang, Kan,
	Nick Desaulniers, Bill Wendling, Justin Stitt,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev

On Sat, Jun 29, 2024 at 09:47:10PM +0300, Adrian Hunter wrote:
> On 29/06/24 05:32, duchangbin wrote:
> > On Fri, Jun 28, 2024 at 08:27:55PM +0300, Adrian Hunter wrote:
> >> On 28/06/24 07:21, duchangbin wrote:
> >>> On Thu, Jun 27, 2024 at 04:53:18PM -0700, Namhyung Kim wrote:
> >>>> Hello guys,
> >>>>
> >>>> On Wed, Jun 26, 2024 at 01:32:42PM +0300, Adrian Hunter wrote:
> >>>>> On 26/06/24 05:26, duchangbin wrote:
> >>>>>> On Tue, Jun 25, 2024 at 04:20:49PM +0300, Adrian Hunter wrote:
> >>>>>>> On 25/06/24 06:37, Changbin Du wrote:
> >>>>>>>> The vdso dumped from process memory (in buildid-cache) lacks debugging
> >>>>>>>> info. To annotate vdso symbols with source lines we need specify a
> >>>>>>>> debugging version.
> >>>>>>>>
> >>>>>>>> For x86, we can find them from your local build as
> >>>>>>>> arch/x86/entry/vdso/vdso{32,64}.so.dbg. Or they may reside in
> >>>>>>>> /lib/modules/<version>/vdso/vdso{32,64}.so on Ubuntu. But notice that
> >>>>>>>> the buildid has to match.
> >>>>>>>>
> >>>>>>>> $ sudo perf record -a
> >>>>>>>> $ sudo perf report --objdump=llvm-objdump \
> >>>>>>>>   --vdso arch/x86/entry/vdso/vdso64.so.dbg,arch/x86/entry/vdso/vdso32.so.dbg
> >>>>>>>>
> >>>>>>>> Samples: 17K of event 'cycles:P', 4000 Hz, Event count (approx.): 1760
> >>>>>>>> __vdso_clock_gettime  /work/linux-host/arch/x86/entry/vdso/vdso64.so.d
> >>>>>>>> Percent│       movq    -48(%rbp),%rsi
> >>>>>>>>        │       testq   %rax,%rax
> >>>>>>>>        │     ;               return vread_hvclock();
> >>>>>>>>        │       movq    %rax,%rdx
> >>>>>>>>        │     ;               if (unlikely(!vdso_cycles_ok(cycles)))
> >>>>>>>>        │     ↑ js      eb
> >>>>>>>>        │     ↑ jmp     74
> >>>>>>>>        │     ;               ts->tv_sec = vdso_ts->sec;
> >>>>>>>>   0.02 │147:   leaq    2(%rbx),%rax
> >>>>>>>>        │       shlq    $4, %rax
> >>>>>>>>        │       addq    %r10,%rax
> >>>>>>>>        │     ;               while ((seq = READ_ONCE(vd->seq)) & 1) {
> >>>>>>>>   9.38 │152:   movl    (%r10),%ecx
> >>>>>>>>
> >>>>>>>> When doing cross platform analysis, we also need specify the vdso path if
> >>>>>>>> we are interested in its symbols.
> >>>>>>>
> >>>>>>> Would it be possible to add vdso and vdso debug to the build-id
> >>>>>>> cache and ensure perf can find it there?
> >>>>>>>
> >>>>>>> Typically, getting dsos from another machine is handled via
> >>>>>>> build-id cache e.g. what perf-archive does
> >>>>>>>
> >>>>>> Hmm. I agree this is better alternative approach for cross-machine analysis.
> >>>>>> When collecting vdsos to buildid cache, I think we can use the local searched
> >>>>>> objects (with debug symbols) instead if its build-id matches vdsos from process
> >>>>>> dumping (the real code ran).
> >>>>>>
> >>>>>> Currently I just follow what perf does for vmlinux so to reuse most of existing
> >>>>>> code. Maybe vmlinux is too big to add to buildid-cahce?
> >>>>>>
> >>>>>> Can we keep our current strategy for now? I'll think about above options when
> >>>>>> I have more time.
> >>>>>>
> >>>>>
> >>>>> I tried adding vdso via perf buildid-cache.  It doesn't work only
> >>>>> because the lookup expects the basename to be "vdso" but it is
> >>>>> "elf".
> >>>>>
> >>>>> Adding a link from "vdso" to "elf" made it work e.g.
> >>>>>
> >>>>> $ cat gettimeofday-test.c
> >>>>> #include <stdio.h>
> >>>>> #include <sys/time.h>
> >>>>>
> >>>>> int main()
> >>>>> {
> >>>>>         struct timeval tv;
> >>>>>         int ret;
> >>>>>
> >>>>>         ret = gettimeofday(&tv, NULL);
> >>>>>         if (ret == -1) {
> >>>>>                 fprintf(stderr, "gettimeofday failed\n");
> >>>>>                 return 1;
> >>>>>         }
> >>>>>
> >>>>>         printf("%lu.%lu\n", (unsigned long)tv.tv_sec, (unsigned long)tv.tv_usec);
> >>>>>
> >>>>>         return 0;
> >>>>> $ perf record -e intel_pt//u ./gettimeofday-test
> >>>>> 1719397042.892837
> >>>>> [ perf record: Woken up 1 times to write data ]
> >>>>> [ perf record: Captured and wrote 0.026 MB perf.data ]
> >>>>> $ perf script --itrace=e
> >>>>> $ perf buildid-cache --remove /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> >>>>> $ perf script --itrace=e
> >>>>> Warning:
> >>>>> 2 instruction trace errors
> >>>>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
> >>>>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> >>>>> $ perf buildid-cache --add /lib/modules/6.5.0-41-generic/vdso/vdso64.so
> >>>>> $ perf script --itrace=e
> >>>>> Warning:
> >>>>> 2 instruction trace errors
> >>>>>  instruction trace error type 1 time 525345.386424204 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e8e00 code 5: Failed to get instruction
> >>>>>  instruction trace error type 1 time 525345.386424829 cpu 4 pid 198976 tid 198976 ip 0x7ffddb0e884d code 5: Failed to get instruction
> >>>>> $ cd ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8
> >>>>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ln -s elf vdso
> >>>>> ~/.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ ls -l
> >>>>> total 36
> >>>>> -rw-r--r-- 1 ahunter ahunter 33272 Jun 26 13:17 elf
> >>>>> -rw-r----- 1 ahunter ahunter     0 Jun 26 13:17 probes
> >>>>> lrwxrwxrwx 1 ahunter ahunter     3 Jun 26 13:18 vdso -> elf
> >>>>> /.debug/.build-id/c3/530aed66e71bfd10af66039f58cc7c4d2eaba8$ cd
> >>>>> $ perf script --itrace=e
> >>>>> $ 
> >>>>>
> >>>>> So maybe a change could be made to build_id_cache__add() to add
> >>>>> the extra link if the file name matches vdso
> >>>>  
> >>>> Thanks for doing this!  I noticed buildid_cache__basename() will handle
> >>>> the name properly once it realizes the file is a vdso.  Maybe we can
> >>>> check the filepath with some patterns, or simply add an command line
> >>>> option to say it's a vdso.
> >>>>
> >>>> Thanks,
> >>>> Namhyung
> >>>>
> >>> I added some tricks for vdso in build_id_cache__add(). It replace vdso object
> >>> with debugging one if found. Is this okay?
> >>
> >> perf has support for storing debug files in the build-id
> >> cache using the base name "debug" instead of "elf", so really
> >> it would be better to make that work
> >>
> > My thoughts are:
> >   1. add debugging vdso file as "debug" in buildid cache.
> >   2. add a new perf config item 'annotate.prefer_debug_file' (default false) to
> >      annotate with debugging object files.
> 
> Is that needed?  Isn't the debug information read from
> the "debug" file?  If not, does that need fixing?
>

Is it by design as you mentioned before?

https://lore.kernel.org/r/all/395cfff7-9692-4123-96b6-353752007f46@intel.com/:

"In the past, there have been cases where the debugging version has not
worked for reading object code.  I don't remember the details, but the
symbols and debugging information was OK while the object code was not.

In general, using anything other than the file that was actually executed
for reading object code seems like a bad idea."


I don't know whether this really matter mostly. I can fix it if it's a bug.

https://lore.kernel.org/r/all/395cfff7-9692-4123-96b6-353752007f46@intel.com/

> 

-- 
Cheers,
Changbin Du

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2024-07-01  2:30 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-25  3:37 [PATCH v4 0/5] perf: support specify vdso path in cmdline Changbin Du
2024-06-25  3:37 ` [PATCH v4 1/5] " Changbin Du
2024-06-25 13:20   ` Adrian Hunter
2024-06-26  2:26     ` duchangbin
2024-06-26 10:32       ` Adrian Hunter
2024-06-27 23:53         ` Namhyung Kim
2024-06-28  4:21           ` duchangbin
2024-06-28 17:27             ` Adrian Hunter
2024-06-29  2:32               ` duchangbin
2024-06-29 18:47                 ` Adrian Hunter
2024-07-01  2:30                   ` duchangbin
2024-06-25  3:37 ` [PATCH v4 2/5] perf: disasm: refactor function dso__disassemble_filename Changbin Du
2024-06-25  3:37 ` [PATCH v4 3/5] perf: disasm: use build_id_path if fallback failed Changbin Du
2024-06-25  3:37 ` [PATCH v4 4/5] perf: symbol: generalize vmlinux path searching Changbin Du
2024-06-25  3:37 ` [PATCH v4 5/5] perf: symbol: try to seach vdso path if not given by user Changbin Du

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).