[PATCH v2] perf tools: Fix build-id matching on vmlinux

[PATCH v2] perf tools: Fix build-id matching on vmlinux

[Namhyung Kim]

There's a problem on finding correct kernel symbols when perf report
runs on a different kernel.  Although a part of the problem was solved
by the prior commit 0a7e6d1b6844 ("perf tools: Check recorded kernel
version when finding vmlinux"), there's a remaining problem still.

When perf records samples, it synthesizes the kernel map using
machine__mmap_name() and ref_reloc_sym like "[kernel.kallsyms]_text".
You can easily see it using 'perf report -D' command.

After finishing record, it goes through the recorded events to find
maps/dsos actually used.  And then record build-id info of them.

During this process, it needs to load symbols in a dso and it'd call
dso__load_vmlinux() since the default value of the symbol_conf.try_
vmlinux_path is true.  However it changes dso->long_name to a real
path of the vmlinux file (e.g. /lib/modules/3.16.0-rc2+/build/vmlinux)
if one is running on a custom kernel.

It resulted in that perf report reads the build-id of the vmlinux, but
cannot use it since it only knows about the [kernel.kallsyms] map.  It
then falls back to possible vmlinux paths by using the recorded kernel
version (in case of a recent version) or a running kernel silently.

Even with the recent tools, this still has a possibility of breaking
the result.  As the build directory is a symbolic link, if one built a
new kernel in the same directory with different source/config, the old
link to vmlinux will point the new file.  So it's absolutely needed to
use build-id when finding a kernel image.

In this patch, it's now changed to try to search a kernel dso using
"vmlinux" shortname (which always has a build-id) and, if not found,
search "[kernel.kallsyms]".

Before:

  $ perf report
  # Children      Self  Command  Shared Object      Symbol
  # ........  ........  .......  .................  ...............................
  #
      72.15%     0.00%  swapper  [kernel.kallsyms]  [k] set_curr_task_rt
      72.15%     0.00%  swapper  [kernel.kallsyms]  [k] native_calibrate_tsc
      72.15%     0.00%  swapper  [kernel.kallsyms]  [k] tsc_refine_calibration_work
      71.87%    71.87%  swapper  [kernel.kallsyms]  [k] module_finalize
   ...

After (for the same perf.data):

      72.15%     0.00%  swapper  vmlinux  [k] cpu_startup_entry
      72.15%     0.00%  swapper  vmlinux  [k] arch_cpu_idle
      72.15%     0.00%  swapper  vmlinux  [k] default_idle
      71.87%    71.87%  swapper  vmlinux  [k] native_safe_halt
   ...

Cc: stable@vger.kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/machine.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index b2ec38bf211e..d9f828f3b54f 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
 				strlen(kmmap_prefix));
 		/*
 		 * Should be there already, from the build-id table in
-		 * the header.
+		 * the header (but maybe with a different name: "vmlinux").
 		 */
-		struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
-						     kmmap_prefix);
+		struct dso *kernel = dsos__find(&machine->kernel_dsos,
+						"vmlinux", true);
+
+		if (kernel == NULL)
+			kernel = __dsos__findnew(&machine->kernel_dsos,
+						 kmmap_prefix);
 		if (kernel == NULL)
 			goto out_problem;
 
-- 
2.1.0

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Adrian Hunter]

On 09/05/2014 07:59 AM, Namhyung Kim wrote:
> There's a problem on finding correct kernel symbols when perf report
> runs on a different kernel.  Although a part of the problem was solved
> by the prior commit 0a7e6d1b6844 ("perf tools: Check recorded kernel
> version when finding vmlinux"), there's a remaining problem still.
> 
> When perf records samples, it synthesizes the kernel map using
> machine__mmap_name() and ref_reloc_sym like "[kernel.kallsyms]_text".
> You can easily see it using 'perf report -D' command.
> 
> After finishing record, it goes through the recorded events to find
> maps/dsos actually used.  And then record build-id info of them.
> 
> During this process, it needs to load symbols in a dso and it'd call
> dso__load_vmlinux() since the default value of the symbol_conf.try_
> vmlinux_path is true.  However it changes dso->long_name to a real
> path of the vmlinux file (e.g. /lib/modules/3.16.0-rc2+/build/vmlinux)
> if one is running on a custom kernel.
> 
> It resulted in that perf report reads the build-id of the vmlinux, but
> cannot use it since it only knows about the [kernel.kallsyms] map.  It
> then falls back to possible vmlinux paths by using the recorded kernel
> version (in case of a recent version) or a running kernel silently.
> 
> Even with the recent tools, this still has a possibility of breaking
> the result.  As the build directory is a symbolic link, if one built a
> new kernel in the same directory with different source/config, the old
> link to vmlinux will point the new file.  So it's absolutely needed to
> use build-id when finding a kernel image.
> 
> In this patch, it's now changed to try to search a kernel dso using
> "vmlinux" shortname (which always has a build-id) and, if not found,
> search "[kernel.kallsyms]".
> 
> Before:
> 
>   $ perf report
>   # Children      Self  Command  Shared Object      Symbol
>   # ........  ........  .......  .................  ...............................
>   #
>       72.15%     0.00%  swapper  [kernel.kallsyms]  [k] set_curr_task_rt
>       72.15%     0.00%  swapper  [kernel.kallsyms]  [k] native_calibrate_tsc
>       72.15%     0.00%  swapper  [kernel.kallsyms]  [k] tsc_refine_calibration_work
>       71.87%    71.87%  swapper  [kernel.kallsyms]  [k] module_finalize
>    ...
> 
> After (for the same perf.data):
> 
>       72.15%     0.00%  swapper  vmlinux  [k] cpu_startup_entry
>       72.15%     0.00%  swapper  vmlinux  [k] arch_cpu_idle
>       72.15%     0.00%  swapper  vmlinux  [k] default_idle
>       71.87%    71.87%  swapper  vmlinux  [k] native_safe_halt
>    ...
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/machine.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b2ec38bf211e..d9f828f3b54f 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
>  				strlen(kmmap_prefix));
>  		/*
>  		 * Should be there already, from the build-id table in
> -		 * the header.
> +		 * the header (but maybe with a different name: "vmlinux").
>  		 */
> -		struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
> -						     kmmap_prefix);
> +		struct dso *kernel = dsos__find(&machine->kernel_dsos,
> +						"vmlinux", true);

Isn't "vmlinux" just the basename of the original file name, so if it had a
different name this wouldn't work e.g. if the filename had been
/boot/vmlinuz-3.11.0-26-generic then you would need
"vmlinuz-3.11.0-26-generic" for this to work?

> +
> +		if (kernel == NULL)
> +			kernel = __dsos__findnew(&machine->kernel_dsos,
> +						 kmmap_prefix);
>  		if (kernel == NULL)
>  			goto out_problem;
>  
> 

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Arnaldo Carvalho de Melo]

Em Fri, Sep 05, 2014 at 10:22:40AM +0300, Adrian Hunter escreveu:
> On 09/05/2014 07:59 AM, Namhyung Kim wrote:
> > +++ b/tools/perf/util/machine.c
> > @@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
> >  				strlen(kmmap_prefix));
> >  		/*
> >  		 * Should be there already, from the build-id table in
> > -		 * the header.
> > +		 * the header (but maybe with a different name: "vmlinux").
> >  		 */
> > -		struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
> > -						     kmmap_prefix);
> > +		struct dso *kernel = dsos__find(&machine->kernel_dsos,
> > +						"vmlinux", true);
 
> Isn't "vmlinux" just the basename of the original file name, so if it had a
> different name this wouldn't work e.g. if the filename had been
> /boot/vmlinuz-3.11.0-26-generic then you would need
> "vmlinuz-3.11.0-26-generic" for this to work?

Yeah, looking for well known pathnames to then check if the build-id
matches the one we're looking for, be it because we obtained it from
/sys/kernel/notes (for the running kernel), or from the perf.data file
build-id table is ok, as we don't know where it is.

Plain sticking "vmlinux" there is not.

- Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Namhyung Kim]

Hi Adrian and Arnaldo,

2014-09-05 (금), 11:11 -0300, Arnaldo Carvalho de Melo:
> Em Fri, Sep 05, 2014 at 10:22:40AM +0300, Adrian Hunter escreveu:
> > On 09/05/2014 07:59 AM, Namhyung Kim wrote:
> > > +++ b/tools/perf/util/machine.c
> > > @@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
> > >  				strlen(kmmap_prefix));
> > >  		/*
> > >  		 * Should be there already, from the build-id table in
> > > -		 * the header.
> > > +		 * the header (but maybe with a different name: "vmlinux").
> > >  		 */
> > > -		struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
> > > -						     kmmap_prefix);
> > > +		struct dso *kernel = dsos__find(&machine->kernel_dsos,
> > > +						"vmlinux", true);
>  
> > Isn't "vmlinux" just the basename of the original file name, so if it had a
> > different name this wouldn't work e.g. if the filename had been
> > /boot/vmlinuz-3.11.0-26-generic then you would need
> > "vmlinuz-3.11.0-26-generic" for this to work?
> 
> Yeah, looking for well known pathnames to then check if the build-id
> matches the one we're looking for, be it because we obtained it from
> /sys/kernel/notes (for the running kernel), or from the perf.data file
> build-id table is ok, as we don't know where it is.
> 
> Plain sticking "vmlinux" there is not.

I don't get it.  AFAIK when perf record runs, it uses [kernel.kallsyms]
name for kernel map.  But it can be changed only if it found a "vmlinux"
file in the vmlinux_path[].  So short name will always be "vmlinux" -
okay, it might be vmlinux-$(uname -r) too; I can add it.

As you know, the vmlinux file is a ELF image that created during kernel
build process so I guess its name is fixed.  It's different from the
vmlinu"z" which resides in /boot directory.

Am I missing something?

Thanks,
Namhyung

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Arnaldo Carvalho de Melo]

Em Sat, Sep 06, 2014 at 12:16:40AM +0900, Namhyung Kim escreveu:
> Hi Adrian and Arnaldo,
> 
> 2014-09-05 (금), 11:11 -0300, Arnaldo Carvalho de Melo:
> > Em Fri, Sep 05, 2014 at 10:22:40AM +0300, Adrian Hunter escreveu:
> > > On 09/05/2014 07:59 AM, Namhyung Kim wrote:
> > > > +++ b/tools/perf/util/machine.c
> > > > @@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
> > > >  				strlen(kmmap_prefix));
> > > >  		/*
> > > >  		 * Should be there already, from the build-id table in
> > > > -		 * the header.
> > > > +		 * the header (but maybe with a different name: "vmlinux").
> > > >  		 */
> > > > -		struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
> > > > -						     kmmap_prefix);
> > > > +		struct dso *kernel = dsos__find(&machine->kernel_dsos,
> > > > +						"vmlinux", true);
> >  
> > > Isn't "vmlinux" just the basename of the original file name, so if it had a
> > > different name this wouldn't work e.g. if the filename had been
> > > /boot/vmlinuz-3.11.0-26-generic then you would need
> > > "vmlinuz-3.11.0-26-generic" for this to work?
> > 
> > Yeah, looking for well known pathnames to then check if the build-id
> > matches the one we're looking for, be it because we obtained it from
> > /sys/kernel/notes (for the running kernel), or from the perf.data file
> > build-id table is ok, as we don't know where it is.
> > 
> > Plain sticking "vmlinux" there is not.
> 
> I don't get it.  AFAIK when perf record runs, it uses [kernel.kallsyms]
> name for kernel map.  But it can be changed only if it found a "vmlinux"
> file in the vmlinux_path[].  So short name will always be "vmlinux" -
> okay, it might be vmlinux-$(uname -r) too; I can add it.
> 
> As you know, the vmlinux file is a ELF image that created during kernel
> build process so I guess its name is fixed.  It's different from the
> vmlinu"z" which resides in /boot directory.
> 
> Am I missing something?

[acme@zoo linux]$ perf report --help 2>&1 | grep vmlin
       -k, --vmlinux=<file>
           vmlinux pathname
[acme@zoo linux]$

Yes, it is using [kernel.kallsyms].ref_reloc_sym for the synthesized
KERNEL mmap event, but it stores the full path in the build-id table.

It seems we need a way to state that an entry in the build-id table is
for the kernel, without looking at its file name. That or to put the
build-id into the synthesized kernel mmap event. Which is better,
because:

That leads to another problem that needs to get solved eventually: We
need to have the build-id into PERF_RECORD_MMAP, because we're now using
just the mmap filename as the key, not the contents, and for long
running sessions, DSOs can get updated, etc.

- Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Stephane Eranian]

On Fri, Sep 5, 2014 at 5:44 PM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Sat, Sep 06, 2014 at 12:16:40AM +0900, Namhyung Kim escreveu:
>> Hi Adrian and Arnaldo,
>>
>> 2014-09-05 (금), 11:11 -0300, Arnaldo Carvalho de Melo:
>> > Em Fri, Sep 05, 2014 at 10:22:40AM +0300, Adrian Hunter escreveu:
>> > > On 09/05/2014 07:59 AM, Namhyung Kim wrote:
>> > > > +++ b/tools/perf/util/machine.c
>> > > > @@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
>> > > >                                 strlen(kmmap_prefix));
>> > > >                 /*
>> > > >                  * Should be there already, from the build-id table in
>> > > > -                * the header.
>> > > > +                * the header (but maybe with a different name: "vmlinux").
>> > > >                  */
>> > > > -               struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
>> > > > -                                                    kmmap_prefix);
>> > > > +               struct dso *kernel = dsos__find(&machine->kernel_dsos,
>> > > > +                                               "vmlinux", true);
>> >
>> > > Isn't "vmlinux" just the basename of the original file name, so if it had a
>> > > different name this wouldn't work e.g. if the filename had been
>> > > /boot/vmlinuz-3.11.0-26-generic then you would need
>> > > "vmlinuz-3.11.0-26-generic" for this to work?
>> >
>> > Yeah, looking for well known pathnames to then check if the build-id
>> > matches the one we're looking for, be it because we obtained it from
>> > /sys/kernel/notes (for the running kernel), or from the perf.data file
>> > build-id table is ok, as we don't know where it is.
>> >
>> > Plain sticking "vmlinux" there is not.
>>
>> I don't get it.  AFAIK when perf record runs, it uses [kernel.kallsyms]
>> name for kernel map.  But it can be changed only if it found a "vmlinux"
>> file in the vmlinux_path[].  So short name will always be "vmlinux" -
>> okay, it might be vmlinux-$(uname -r) too; I can add it.
>>
>> As you know, the vmlinux file is a ELF image that created during kernel
>> build process so I guess its name is fixed.  It's different from the
>> vmlinu"z" which resides in /boot directory.
>>
>> Am I missing something?
>
> [acme@zoo linux]$ perf report --help 2>&1 | grep vmlin
>        -k, --vmlinux=<file>
>            vmlinux pathname
> [acme@zoo linux]$
>
> Yes, it is using [kernel.kallsyms].ref_reloc_sym for the synthesized
> KERNEL mmap event, but it stores the full path in the build-id table.
>
> It seems we need a way to state that an entry in the build-id table is
> for the kernel, without looking at its file name. That or to put the
> build-id into the synthesized kernel mmap event. Which is better,
> because:
>
Can't you figure out it is from the kernel (in perf record) by remembering
you got the buildid from /sys/kernel/notes?


> That leads to another problem that needs to get solved eventually: We
> need to have the build-id into PERF_RECORD_MMAP, because we're now using
> just the mmap filename as the key, not the contents, and for long
> running sessions, DSOs can get updated, etc.
>
> - Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Arnaldo Carvalho de Melo]

Em Sun, Sep 07, 2014 at 10:24:12PM +0200, Stephane Eranian escreveu:
> On Fri, Sep 5, 2014 at 5:44 PM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> > Em Sat, Sep 06, 2014 at 12:16:40AM +0900, Namhyung Kim escreveu:
> >> Hi Adrian and Arnaldo,
> >>
> >> 2014-09-05 (금), 11:11 -0300, Arnaldo Carvalho de Melo:
> >> > Em Fri, Sep 05, 2014 at 10:22:40AM +0300, Adrian Hunter escreveu:
> >> > > On 09/05/2014 07:59 AM, Namhyung Kim wrote:
> >> > > > +++ b/tools/perf/util/machine.c
> >> > > > @@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
> >> > > >                                 strlen(kmmap_prefix));
> >> > > >                 /*
> >> > > >                  * Should be there already, from the build-id table in
> >> > > > -                * the header.
> >> > > > +                * the header (but maybe with a different name: "vmlinux").
> >> > > >                  */
> >> > > > -               struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
> >> > > > -                                                    kmmap_prefix);
> >> > > > +               struct dso *kernel = dsos__find(&machine->kernel_dsos,
> >> > > > +                                               "vmlinux", true);
> >> >
> >> > > Isn't "vmlinux" just the basename of the original file name, so if it had a
> >> > > different name this wouldn't work e.g. if the filename had been
> >> > > /boot/vmlinuz-3.11.0-26-generic then you would need
> >> > > "vmlinuz-3.11.0-26-generic" for this to work?
> >> >
> >> > Yeah, looking for well known pathnames to then check if the build-id
> >> > matches the one we're looking for, be it because we obtained it from
> >> > /sys/kernel/notes (for the running kernel), or from the perf.data file
> >> > build-id table is ok, as we don't know where it is.
> >> >
> >> > Plain sticking "vmlinux" there is not.
> >>
> >> I don't get it.  AFAIK when perf record runs, it uses [kernel.kallsyms]
> >> name for kernel map.  But it can be changed only if it found a "vmlinux"
> >> file in the vmlinux_path[].  So short name will always be "vmlinux" -
> >> okay, it might be vmlinux-$(uname -r) too; I can add it.
> >>
> >> As you know, the vmlinux file is a ELF image that created during kernel
> >> build process so I guess its name is fixed.  It's different from the
> >> vmlinu"z" which resides in /boot directory.
> >>
> >> Am I missing something?
> >
> > [acme@zoo linux]$ perf report --help 2>&1 | grep vmlin
> >        -k, --vmlinux=<file>
> >            vmlinux pathname
> > [acme@zoo linux]$
> >
> > Yes, it is using [kernel.kallsyms].ref_reloc_sym for the synthesized
> > KERNEL mmap event, but it stores the full path in the build-id table.
> >
> > It seems we need a way to state that an entry in the build-id table is
> > for the kernel, without looking at its file name. That or to put the
> > build-id into the synthesized kernel mmap event. Which is better,
> > because:
> >
> Can't you figure out it is from the kernel (in perf record) by remembering
> you got the buildid from /sys/kernel/notes?

Sure, the question was more about how to encode that in the build-id
table in the perf.data file, i.e. we don't have a flags field there that
we could use a bit for this purpose (hey, this entry is for the kernel,
please match it to the PERF_RECORD_MMAP [kernel_kallsyms] synthesized
for the kernel).

We know it is a kernel because the name is [kernel.kallsyms] in the
synthesized PERF_RECORD_MMAP. So perhaps we should have the kernel
pathname right after the reference relocation symbol, that way we would
use it to get the entry in the build-id table.

I.e. parts of what we need are in the synthesized PERF_RECORD_MMAP (how
to do relocation, start-end kernel mmap, and we have parts of it in the
build-id table: the build-id and the full pathname. How to connect both
is what we're trying to achieve here, and in a backwards compatible
way.

The (ugly) way I found to achieve all this is to concatenate the full
pathname right after the ref reloc symbol in the synthesized
PERF_RECORD_MMAP for the kernel mmap.

Older tools would just ignore what is after the ref reloc symbol (need
to check if we put the zero there, if not, we can add the zero + the
full pathname.).

Newer tools would look if the size of the record is longer than the
strlen(ref_reloc_symbol), and if it is, it means we have the kernel full
pathname after the ref_reloc_symbol, that we can use to find the
build-id in the build-id table.

In the future this is all moot if we add the build-id to all
PERF_RECORD_MMAP events, as described below:

> > That leads to another problem that needs to get solved eventually: We
> > need to have the build-id into PERF_RECORD_MMAP, because we're now using
> > just the mmap filename as the key, not the contents, and for long
> > running sessions, DSOs can get updated, etc.

- Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Namhyung Kim]

On Mon, 8 Sep 2014 10:38:47 -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Sep 07, 2014 at 10:24:12PM +0200, Stephane Eranian escreveu:
>> Can't you figure out it is from the kernel (in perf record) by remembering
>> you got the buildid from /sys/kernel/notes?
>
> Sure, the question was more about how to encode that in the build-id
> table in the perf.data file, i.e. we don't have a flags field there that
> we could use a bit for this purpose (hey, this entry is for the kernel,
> please match it to the PERF_RECORD_MMAP [kernel_kallsyms] synthesized
> for the kernel).

So why not adding a new build_id2_event? :)


>
> We know it is a kernel because the name is [kernel.kallsyms] in the
> synthesized PERF_RECORD_MMAP. So perhaps we should have the kernel
> pathname right after the reference relocation symbol, that way we would
> use it to get the entry in the build-id table.
>
> I.e. parts of what we need are in the synthesized PERF_RECORD_MMAP (how
> to do relocation, start-end kernel mmap, and we have parts of it in the
> build-id table: the build-id and the full pathname. How to connect both
> is what we're trying to achieve here, and in a backwards compatible
> way.
>
> The (ugly) way I found to achieve all this is to concatenate the full
> pathname right after the ref reloc symbol in the synthesized
> PERF_RECORD_MMAP for the kernel mmap.
>
> Older tools would just ignore what is after the ref reloc symbol (need
> to check if we put the zero there, if not, we can add the zero + the
> full pathname.).
>
> Newer tools would look if the size of the record is longer than the
> strlen(ref_reloc_symbol), and if it is, it means we have the kernel full
> pathname after the ref_reloc_symbol, that we can use to find the
> build-id in the build-id table.

Or else, what about synthesizing a build-id event right after the kernel
mmap event (in perf_event__synthesize_kernel_mmap)?  This way we can
have same effect of extending the mmap event while keeping backward
compatibility IMHO.  At report time, we might need to set a flag that it
has just seen the kernel mmap event so that it expects to see a matching
build-id event (use it as a kernel dso).

Thanks,
Namhyung


>
> In the future this is all moot if we add the build-id to all
> PERF_RECORD_MMAP events, as described below:
>
>> > That leads to another problem that needs to get solved eventually: We
>> > need to have the build-id into PERF_RECORD_MMAP, because we're now using
>> > just the mmap filename as the key, not the contents, and for long
>> > running sessions, DSOs can get updated, etc.
>
> - Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Namhyung Kim]

Hi Arnaldo,

On Fri, 5 Sep 2014 12:44:02 -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Sep 06, 2014 at 12:16:40AM +0900, Namhyung Kim escreveu:
>> Hi Adrian and Arnaldo,
>> 
>> 2014-09-05 (금), 11:11 -0300, Arnaldo Carvalho de Melo:
>> > Em Fri, Sep 05, 2014 at 10:22:40AM +0300, Adrian Hunter escreveu:
>> > > On 09/05/2014 07:59 AM, Namhyung Kim wrote:
>> > > > +++ b/tools/perf/util/machine.c
>> > > > @@ -1060,10 +1060,14 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
>> > > >  				strlen(kmmap_prefix));
>> > > >  		/*
>> > > >  		 * Should be there already, from the build-id table in
>> > > > -		 * the header.
>> > > > +		 * the header (but maybe with a different name: "vmlinux").
>> > > >  		 */
>> > > > -		struct dso *kernel = __dsos__findnew(&machine->kernel_dsos,
>> > > > -						     kmmap_prefix);
>> > > > +		struct dso *kernel = dsos__find(&machine->kernel_dsos,
>> > > > +						"vmlinux", true);
>> >  
>> > > Isn't "vmlinux" just the basename of the original file name, so if it had a
>> > > different name this wouldn't work e.g. if the filename had been
>> > > /boot/vmlinuz-3.11.0-26-generic then you would need
>> > > "vmlinuz-3.11.0-26-generic" for this to work?
>> > 
>> > Yeah, looking for well known pathnames to then check if the build-id
>> > matches the one we're looking for, be it because we obtained it from
>> > /sys/kernel/notes (for the running kernel), or from the perf.data file
>> > build-id table is ok, as we don't know where it is.
>> > 
>> > Plain sticking "vmlinux" there is not.
>> 
>> I don't get it.  AFAIK when perf record runs, it uses [kernel.kallsyms]
>> name for kernel map.  But it can be changed only if it found a "vmlinux"
>> file in the vmlinux_path[].  So short name will always be "vmlinux" -
>> okay, it might be vmlinux-$(uname -r) too; I can add it.
>> 
>> As you know, the vmlinux file is a ELF image that created during kernel
>> build process so I guess its name is fixed.  It's different from the
>> vmlinu"z" which resides in /boot directory.
>> 
>> Am I missing something?
>
> [acme@zoo linux]$ perf report --help 2>&1 | grep vmlin
>        -k, --vmlinux=<file>
>            vmlinux pathname
> [acme@zoo linux]$

Hmm.. this is a report option to specify a vmlinux file rather than
finding it using recorded info.  When this option was given by user, it
doesn't even try to search the vmlinux path (or build-id table) and just
uses the given path.


>
> Yes, it is using [kernel.kallsyms].ref_reloc_sym for the synthesized
> KERNEL mmap event, but it stores the full path in the build-id table.
>
> It seems we need a way to state that an entry in the build-id table is
> for the kernel, without looking at its file name.

Maybe we can add a new build_id2_event (like mmap2) that has a new field
to record that info.


> That or to put the build-id into the synthesized kernel mmap
> event. Which is better, because:
>
> That leads to another problem that needs to get solved eventually: We
> need to have the build-id into PERF_RECORD_MMAP, because we're now using
> just the mmap filename as the key, not the contents, and for long
> running sessions, DSOs can get updated, etc.

But it'd require a realtime processing of events at record time.

Thanks,
Namhyung

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Arnaldo Carvalho de Melo]

Em Fri, Sep 12, 2014 at 03:14:51PM +0900, Namhyung Kim escreveu:
> On Fri, 5 Sep 2014 12:44:02 -0300, Arnaldo Carvalho de Melo wrote:
> > It seems we need a way to state that an entry in the build-id table is
> > for the kernel, without looking at its file name.
 
> Maybe we can add a new build_id2_event (like mmap2) that has a new field
> to record that info.

Humm, take a look at machine__write_buildid_table() -> write_buildid(),
we already seem to set build_id_event.header.misc suitably, no?

> > That or to put the build-id into the synthesized kernel mmap
> > event. Which is better, because:

> > That leads to another problem that needs to get solved eventually: We
> > need to have the build-id into PERF_RECORD_MMAP, because we're now using
> > just the mmap filename as the key, not the contents, and for long
> > running sessions, DSOs can get updated, etc.
 
> But it'd require a realtime processing of events at record time.

Been there, done that, backtracked, yes, we can't do that at 'record
time', else we would be adding way too much noise to the recording
phase.

The idea is for the _kernel_ to do that, would take sizeof(buildid)
(about 20 bytes) on the per-DSO structure in the kernel.

When ELF loading it, stashing the contents of an ELF session
(.note.gnu.build-id) somewhere accessible at PERF_RECORD_MMAP3
generation time.

Doing that at the time we load the DSO from disk should add negligible
overhead.

With that we would not need to go over all the perf.data stream to
generate the build-id table after all record sessions, they would be at
the struct map already.

We could then have multiple entries in the build-id table for the same
pathname, with different build ids, that is something we don't support
now and that provides bogus results when it happens.

- Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Namhyung Kim]

On Fri, 12 Sep 2014 11:11:16 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 12, 2014 at 03:14:51PM +0900, Namhyung Kim escreveu:
>> On Fri, 5 Sep 2014 12:44:02 -0300, Arnaldo Carvalho de Melo wrote:
>> > It seems we need a way to state that an entry in the build-id table is
>> > for the kernel, without looking at its file name.
>  
>> Maybe we can add a new build_id2_event (like mmap2) that has a new field
>> to record that info.
>
> Humm, take a look at machine__write_buildid_table() -> write_buildid(),
> we already seem to set build_id_event.header.misc suitably, no?

AFAIK the PERF_RECORD_MISC_KERNEL is set for both of kernel image and
modules so we cannot distinguish a kernel from others (without checking
names).

>
>> > That or to put the build-id into the synthesized kernel mmap
>> > event. Which is better, because:
>
>> > That leads to another problem that needs to get solved eventually: We
>> > need to have the build-id into PERF_RECORD_MMAP, because we're now using
>> > just the mmap filename as the key, not the contents, and for long
>> > running sessions, DSOs can get updated, etc.
>  
>> But it'd require a realtime processing of events at record time.
>
> Been there, done that, backtracked, yes, we can't do that at 'record
> time', else we would be adding way too much noise to the recording
> phase.
>
> The idea is for the _kernel_ to do that, would take sizeof(buildid)
> (about 20 bytes) on the per-DSO structure in the kernel.
>
> When ELF loading it, stashing the contents of an ELF session
> (.note.gnu.build-id) somewhere accessible at PERF_RECORD_MMAP3
> generation time.
>
> Doing that at the time we load the DSO from disk should add negligible
> overhead.
>
> With that we would not need to go over all the perf.data stream to
> generate the build-id table after all record sessions, they would be at
> the struct map already.

Yes, it'll solve the problem and simplifies the perf record.  However I
suspect it's not acceptable to keep such info in the kernel only for the
sake of perf.


>
> We could then have multiple entries in the build-id table for the same
> pathname, with different build ids, that is something we don't support
> now and that provides bogus results when it happens.

Maybe we can save and compare timestamp of build-id/map event and sample
event.  I'm experimenting to separate meta events (comm, mmap, ...) from
sample events with a similar idea... :)

Thanks,
Namhyung

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Arnaldo Carvalho de Melo]

Em Fri, Sep 19, 2014 at 03:26:25PM +0900, Namhyung Kim escreveu:
> On Fri, 12 Sep 2014 11:11:16 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Sep 12, 2014 at 03:14:51PM +0900, Namhyung Kim escreveu:
> >> On Fri, 5 Sep 2014 12:44:02 -0300, Arnaldo Carvalho de Melo wrote:
> >> > It seems we need a way to state that an entry in the build-id table is
> >> > for the kernel, without looking at its file name.

> >> Maybe we can add a new build_id2_event (like mmap2) that has a new field
> >> to record that info.

> > Humm, take a look at machine__write_buildid_table() -> write_buildid(),
> > we already seem to set build_id_event.header.misc suitably, no?
 
> AFAIK the PERF_RECORD_MISC_KERNEL is set for both of kernel image and
> modules so we cannot distinguish a kernel from others (without checking
> names).

Ok, not perfect, but I think we improve the situation by using this
piece of information while looking if the file ends in .ko, in which
case we can mark it as a kernel module, if it doesn't end in .ko, we
have a kernel.

Better than today, probably good enough, no?
 
> >> > That or to put the build-id into the synthesized kernel mmap
> >> > event. Which is better, because:

> >> > That leads to another problem that needs to get solved eventually: We
> >> > need to have the build-id into PERF_RECORD_MMAP, because we're now using
> >> > just the mmap filename as the key, not the contents, and for long
> >> > running sessions, DSOs can get updated, etc.

> >> But it'd require a realtime processing of events at record time.

> > Been there, done that, backtracked, yes, we can't do that at 'record
> > time', else we would be adding way too much noise to the recording
> > phase.

> > The idea is for the _kernel_ to do that, would take sizeof(buildid)
> > (about 20 bytes) on the per-DSO structure in the kernel.

> > When ELF loading it, stashing the contents of an ELF session
> > (.note.gnu.build-id) somewhere accessible at PERF_RECORD_MMAP3
> > generation time.

> > Doing that at the time we load the DSO from disk should add negligible
> > overhead.

> > With that we would not need to go over all the perf.data stream to
> > generate the build-id table after all record sessions, they would be at
> > the struct map already.
 
> Yes, it'll solve the problem and simplifies the perf record.  However I
> suspect it's not acceptable to keep such info in the kernel only for the
> sake of perf.

Hey, perf is a crucial part of development, even if it was the sole
user, I think this would be acceptable.

But this is not perf specific at all, any other tool that needs to map
from sample to a symtab _needs_ this, there is no other way to, on a
system with running apps and its libraries, to safely go from DSO
pathname to its corresponding binary.

Also this enables one to unambiguously describe a running environment
without packing tons of duplicated payloads for multiple workload runs,
be it for profiling or for any other problem characterization task.

And we have build-ids available for quite a while in all DSOs in the
system.

I bet gdb, systemtap, etc would be glad to use this if available.
 
> > We could then have multiple entries in the build-id table for the same
> > pathname, with different build ids, that is something we don't support
> > now and that provides bogus results when it happens.
 
> Maybe we can save and compare timestamp of build-id/map event and sample
> event.  I'm experimenting to separate meta events (comm, mmap, ...) from
> sample events with a similar idea... :)

The point is to go from an entry in /proc/pid/maps to the file where it
was loaded from, in a system where updates may have taken place for that
pathname.

- Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Namhyung Kim]

2014-09-19 (금), 11:16 -0300, Arnaldo Carvalho de Melo:
> Em Fri, Sep 19, 2014 at 03:26:25PM +0900, Namhyung Kim escreveu:
> > On Fri, 12 Sep 2014 11:11:16 -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Sep 12, 2014 at 03:14:51PM +0900, Namhyung Kim escreveu:
> > >> On Fri, 5 Sep 2014 12:44:02 -0300, Arnaldo Carvalho de Melo wrote:
> > >> > It seems we need a way to state that an entry in the build-id table is
> > >> > for the kernel, without looking at its file name.
> 
> > >> Maybe we can add a new build_id2_event (like mmap2) that has a new field
> > >> to record that info.
> 
> > > Humm, take a look at machine__write_buildid_table() -> write_buildid(),
> > > we already seem to set build_id_event.header.misc suitably, no?
>  
> > AFAIK the PERF_RECORD_MISC_KERNEL is set for both of kernel image and
> > modules so we cannot distinguish a kernel from others (without checking
> > names).
> 
> Ok, not perfect, but I think we improve the situation by using this
> piece of information while looking if the file ends in .ko, in which
> case we can mark it as a kernel module, if it doesn't end in .ko, we
> have a kernel.
> 
> Better than today, probably good enough, no?

Yep, I'll check .ko suffix in v3.


>  
> > >> > That or to put the build-id into the synthesized kernel mmap
> > >> > event. Which is better, because:
> 
> > >> > That leads to another problem that needs to get solved eventually: We
> > >> > need to have the build-id into PERF_RECORD_MMAP, because we're now using
> > >> > just the mmap filename as the key, not the contents, and for long
> > >> > running sessions, DSOs can get updated, etc.
> 
> > >> But it'd require a realtime processing of events at record time.
> 
> > > Been there, done that, backtracked, yes, we can't do that at 'record
> > > time', else we would be adding way too much noise to the recording
> > > phase.
> 
> > > The idea is for the _kernel_ to do that, would take sizeof(buildid)
> > > (about 20 bytes) on the per-DSO structure in the kernel.
> 
> > > When ELF loading it, stashing the contents of an ELF session
> > > (.note.gnu.build-id) somewhere accessible at PERF_RECORD_MMAP3
> > > generation time.
> 
> > > Doing that at the time we load the DSO from disk should add negligible
> > > overhead.
> 
> > > With that we would not need to go over all the perf.data stream to
> > > generate the build-id table after all record sessions, they would be at
> > > the struct map already.
>  
> > Yes, it'll solve the problem and simplifies the perf record.  However I
> > suspect it's not acceptable to keep such info in the kernel only for the
> > sake of perf.
> 
> Hey, perf is a crucial part of development, even if it was the sole
> user, I think this would be acceptable.
> 
> But this is not perf specific at all, any other tool that needs to map
> from sample to a symtab _needs_ this, there is no other way to, on a
> system with running apps and its libraries, to safely go from DSO
> pathname to its corresponding binary.
> 
> Also this enables one to unambiguously describe a running environment
> without packing tons of duplicated payloads for multiple workload runs,
> be it for profiling or for any other problem characterization task.
> 
> And we have build-ids available for quite a while in all DSOs in the
> system.
> 
> I bet gdb, systemtap, etc would be glad to use this if available.

Hmm.. okay then.  But IIUC kernel only knows about an executable (and
dynamic linker) at load time not libraries as they're loaded by the
linker.  So kernel needs to check whether a file is an ELF and has
build-id for every file, right?

Thanks,
Namhyung


>  
> > > We could then have multiple entries in the build-id table for the same
> > > pathname, with different build ids, that is something we don't support
> > > now and that provides bogus results when it happens.
>  
> > Maybe we can save and compare timestamp of build-id/map event and sample
> > event.  I'm experimenting to separate meta events (comm, mmap, ...) from
> > sample events with a similar idea... :)
> 
> The point is to go from an entry in /proc/pid/maps to the file where it
> was loaded from, in a system where updates may have taken place for that
> pathname.
> 
> - Arnaldo

Re: [PATCH v2] perf tools: Fix build-id matching on vmlinux

[Arnaldo Carvalho de Melo]

Em Sat, Sep 20, 2014 at 12:42:46AM +0900, Namhyung Kim escreveu:
> 2014-09-19 (금), 11:16 -0300, Arnaldo Carvalho de Melo:
> > Em Fri, Sep 19, 2014 at 03:26:25PM +0900, Namhyung Kim escreveu:
> > > On Fri, 12 Sep 2014 11:11:16 -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Fri, Sep 12, 2014 at 03:14:51PM +0900, Namhyung Kim escreveu:
> > > >> On Fri, 5 Sep 2014 12:44:02 -0300, Arnaldo Carvalho de Melo wrote:
> > > > Humm, take a look at machine__write_buildid_table() -> write_buildid(),
> > > > we already seem to set build_id_event.header.misc suitably, no?
> >  
> > > AFAIK the PERF_RECORD_MISC_KERNEL is set for both of kernel image and
> > > modules so we cannot distinguish a kernel from others (without checking
> > > names).
> > 
> > Ok, not perfect, but I think we improve the situation by using this
> > piece of information while looking if the file ends in .ko, in which
> > case we can mark it as a kernel module, if it doesn't end in .ko, we
> > have a kernel.
> > 
> > Better than today, probably good enough, no?
> 
> Yep, I'll check .ko suffix in v3.

Thanks!

> > > >> > That or to put the build-id into the synthesized kernel mmap
> > > >> > event. Which is better, because:

> > > >> > That leads to another problem that needs to get solved eventually: We
> > > >> > need to have the build-id into PERF_RECORD_MMAP, because we're now using
> > > >> > just the mmap filename as the key, not the contents, and for long
> > > >> > running sessions, DSOs can get updated, etc.
> > 
> > > >> But it'd require a realtime processing of events at record time.
> > 
> > > > Been there, done that, backtracked, yes, we can't do that at 'record
> > > > time', else we would be adding way too much noise to the recording
> > > > phase.
> > 
> > > > The idea is for the _kernel_ to do that, would take sizeof(buildid)
> > > > (about 20 bytes) on the per-DSO structure in the kernel.
> > 
> > > > When ELF loading it, stashing the contents of an ELF session
> > > > (.note.gnu.build-id) somewhere accessible at PERF_RECORD_MMAP3
> > > > generation time.
> > 
> > > > Doing that at the time we load the DSO from disk should add negligible
> > > > overhead.
> > 
> > > > With that we would not need to go over all the perf.data stream to
> > > > generate the build-id table after all record sessions, they would be at
> > > > the struct map already.
> >  
> > > Yes, it'll solve the problem and simplifies the perf record.  However I
> > > suspect it's not acceptable to keep such info in the kernel only for the
> > > sake of perf.
> > 
> > Hey, perf is a crucial part of development, even if it was the sole
> > user, I think this would be acceptable.
> > 
> > But this is not perf specific at all, any other tool that needs to map
> > from sample to a symtab _needs_ this, there is no other way to, on a
> > system with running apps and its libraries, to safely go from DSO
> > pathname to its corresponding binary.
> > 
> > Also this enables one to unambiguously describe a running environment
> > without packing tons of duplicated payloads for multiple workload runs,
> > be it for profiling or for any other problem characterization task.
> > 
> > And we have build-ids available for quite a while in all DSOs in the
> > system.
> > 
> > I bet gdb, systemtap, etc would be glad to use this if available.
> 
> Hmm.. okay then.  But IIUC kernel only knows about an executable (and
> dynamic linker) at load time not libraries as they're loaded by the
> linker.  So kernel needs to check whether a file is an ELF and has
> build-id for every file, right?

Yes, when it figures out it is an ELF file and invokes this DSO loading
routines, there is where we should insert the code that will read/store
the build id so that when we emit the PERF_RECORD_MMAP3 we can read it.
 
- Arnaldo