Re: [PATCH v6 1/8] perf: support specify vdso path in cmdline

[duchangbin]

Hi, Adrian,

On Wed, Sep 11, 2024 at 11:03:21AM +0300, Adrian Hunter wrote:
> On 25/07/24 05:15, 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.
> > 
> > v2: update documentation.
> > 
> > Signed-off-by: Changbin Du <changbin.du@xxxxxxxxxx>
> > ---
> >  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:
> >  
> 
> <SNIP>
> 
> > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > index b10b7f005658..e0aa657e6ca0 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,
> 
> <SNIP>
> 
> > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > index e10558b79504..7e26d5215640 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))
> 
> Sorry for very long delay.
> 
> This patch (probably this bit here) breaks annotation of vdso.
> To allow for bisection, you need to arrange changes so that each
> patch leaves things in a working state.
> 
> However, I disagree with adding --vdso option since with just
> patch 8 alone, it would be possible to do:
> 
>   perf buildid-cache --remove /work/linux/arch/x86/entry/vdso/vdso64.so.dbg
>   perf buildid-cache --add /work/linux/arch/x86/entry/vdso/vdso64.so.dbg
> 
> and same of vdso32.
> 
> That would leave the buildid-cache containing only the debug versions,
> which would mean you will only get the debug versions, and it would only
> need to be done once per kernel instead of having to add --vdso to
> every perf command.
>

I may send patch 8 alone first and suspend the rest. I suppose mananging buildid-cache
for vdso mannually is enough for most case. So maybe it's better not get things
more complex.

> 

-- 
Cheers,
Changbin Du