[PATCH] perf dso: Fix dso comparison

[PATCH] perf dso: Fix dso comparison

[Ravi Bangoria]

Perf gets dso details from two different sources. 1st, from builid
headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
header does not have dso_id detail. And dso from MMAP2 samples does
not have buildid information. If detail of the same dso is present
at both the places, filename is common.

Previously, __dsos__findnew_link_by_longname_id() used to compare only
long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
from 'struct map' to 'struct dso'") also added a dso_id comparison.
Because of that, now perf is creating two different dso objects of the
same file, one from buildid header (with dso_id but without buildid)
and second from MMAP2 sample (with buildid but without dso_id).

This is causing issues with archive, buildid-list etc subcommands. Fix
this by comparing dso_id only when it's present. And incase dso is
present in 'dsos' list without dso_id, inject dso_id detail as well.

Before:

  $ sudo ./perf buildid-list -H
  0000000000000000000000000000000000000000 /usr/bin/ls
  0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
  0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so

  $ ./perf archive
  perf archive: no build-ids found

After:

  $ ./perf buildid-list -H
  b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
  641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
  675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so

  $ ./perf archive
  Now please run:

  $ tar xvf perf.data.tar.bz2 -C ~/.debug

  wherever you need to run 'perf report' on.

Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'")
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 tools/perf/util/dsos.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index 591707c69c39..5c5bfa2538a9 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -26,13 +26,29 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b)
 	return 0;
 }
 
+static bool is_empty_dso_id(struct dso_id *id)
+{
+	if (!id)
+		return true;
+
+	return !id->maj && !id->min && !id->ino && !id->ino_generation;
+}
+
+static void inject_dso_id(struct dso *dso, struct dso_id *id)
+{
+	dso->id.maj = id->maj;
+	dso->id.min = id->min;
+	dso->id.ino = id->ino;
+	dso->id.ino_generation = id->ino_generation;
+}
+
 static int dso_id__cmp(struct dso_id *a, struct dso_id *b)
 {
 	/*
 	 * The second is always dso->id, so zeroes if not set, assume passing
 	 * NULL for a means a zeroed id
 	 */
-	if (a == NULL)
+	if (is_empty_dso_id(a) || is_empty_dso_id(b))
 		return 0;
 
 	return __dso_id__cmp(a, b);
@@ -249,6 +265,10 @@ struct dso *__dsos__addnew(struct dsos *dsos, const char *name)
 static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, struct dso_id *id)
 {
 	struct dso *dso = __dsos__find_id(dsos, name, id, false);
+
+	if (dso && is_empty_dso_id(&dso->id) && !is_empty_dso_id(id))
+		inject_dso_id(dso, id);
+
 	return dso ? dso : __dsos__addnew_id(dsos, name, id);
 }
 
-- 
2.24.1

Re: [PATCH] perf dso: Fix dso comparison

[Jiri Olsa]

On Tue, Mar 24, 2020 at 09:54:24AM +0530, Ravi Bangoria wrote:
> Perf gets dso details from two different sources. 1st, from builid
> headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
> header does not have dso_id detail. And dso from MMAP2 samples does
> not have buildid information. If detail of the same dso is present
> at both the places, filename is common.
> 
> Previously, __dsos__findnew_link_by_longname_id() used to compare only
> long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
> from 'struct map' to 'struct dso'") also added a dso_id comparison.
> Because of that, now perf is creating two different dso objects of the
> same file, one from buildid header (with dso_id but without buildid)
> and second from MMAP2 sample (with buildid but without dso_id).
> 
> This is causing issues with archive, buildid-list etc subcommands. Fix
> this by comparing dso_id only when it's present. And incase dso is
> present in 'dsos' list without dso_id, inject dso_id detail as well.
> 
> Before:
> 
>   $ sudo ./perf buildid-list -H
>   0000000000000000000000000000000000000000 /usr/bin/ls
>   0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
>   0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
> 
>   $ ./perf archive
>   perf archive: no build-ids found
> 
> After:
> 
>   $ ./perf buildid-list -H
>   b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
>   641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
>   675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
> 
>   $ ./perf archive
>   Now please run:
> 
>   $ tar xvf perf.data.tar.bz2 -C ~/.debug
> 
>   wherever you need to run 'perf report' on.
> 
> Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

looks good, do we need to add the dso_id check to sort__dso_cmp?

thanks,
jirka

Re: [PATCH] perf dso: Fix dso comparison

[Ravi Bangoria]

On 3/24/20 4:18 PM, Jiri Olsa wrote:
> On Tue, Mar 24, 2020 at 09:54:24AM +0530, Ravi Bangoria wrote:
>> Perf gets dso details from two different sources. 1st, from builid
>> headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
>> header does not have dso_id detail. And dso from MMAP2 samples does
>> not have buildid information. If detail of the same dso is present
>> at both the places, filename is common.
>>
>> Previously, __dsos__findnew_link_by_longname_id() used to compare only
>> long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
>> from 'struct map' to 'struct dso'") also added a dso_id comparison.
>> Because of that, now perf is creating two different dso objects of the
>> same file, one from buildid header (with dso_id but without buildid)
>> and second from MMAP2 sample (with buildid but without dso_id).
>>
>> This is causing issues with archive, buildid-list etc subcommands. Fix
>> this by comparing dso_id only when it's present. And incase dso is
>> present in 'dsos' list without dso_id, inject dso_id detail as well.
>>
>> Before:
>>
>>    $ sudo ./perf buildid-list -H
>>    0000000000000000000000000000000000000000 /usr/bin/ls
>>    0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
>>    0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
>>
>>    $ ./perf archive
>>    perf archive: no build-ids found
>>
>> After:
>>
>>    $ ./perf buildid-list -H
>>    b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
>>    641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
>>    675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
>>
>>    $ ./perf archive
>>    Now please run:
>>
>>    $ tar xvf perf.data.tar.bz2 -C ~/.debug
>>
>>    wherever you need to run 'perf report' on.
>>
>> Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> 
> looks good, do we need to add the dso_id check to sort__dso_cmp?

I guess with different filename there is no need to compare dso_id.
But for same filename, adding dso_id cmp will separate out the
samples:

Ex, Without dso_id compare:

   $ ./perf report -s dso,dso_size -v
     66.63%  /home/ravi/a.out                                  4096
     33.36%  /home/ravi/Workspace/linux/tools/perf/a.out       4096

   $ ./perf report -s dso,dso_size
     99.99%  a.out                 4096


With below diff:

   -       return strcmp(dso_name_l, dso_name_r);
   +       ret = strcmp(dso_name_l, dso_name_r);
   +       if (ret)
   +               return ret;
   +       else
   +               return dso__cmp_id(dso_l, dso_r);


   $ ./perf report -s dso,dso_size
     99.99%  a.out                 4096
     33.36%  a.out                 4096

though, the o/p also depends which other sort keys are used along
with dso key. Do you think this change makes sense?

Ravi

Re: [PATCH] perf dso: Fix dso comparison

[Jiri Olsa]

On Tue, Mar 24, 2020 at 06:07:23PM +0530, Ravi Bangoria wrote:

SNIP

> > looks good, do we need to add the dso_id check to sort__dso_cmp?
> 
> I guess with different filename there is no need to compare dso_id.
> But for same filename, adding dso_id cmp will separate out the
> samples:
> 
> Ex, Without dso_id compare:
> 
>   $ ./perf report -s dso,dso_size -v
>     66.63%  /home/ravi/a.out                                  4096
>     33.36%  /home/ravi/Workspace/linux/tools/perf/a.out       4096
> 
>   $ ./perf report -s dso,dso_size
>     99.99%  a.out                 4096
> 
> 
> With below diff:
> 
>   -       return strcmp(dso_name_l, dso_name_r);
>   +       ret = strcmp(dso_name_l, dso_name_r);
>   +       if (ret)
>   +               return ret;
>   +       else
>   +               return dso__cmp_id(dso_l, dso_r);
> 
> 
>   $ ./perf report -s dso,dso_size
>     99.99%  a.out                 4096
>     33.36%  a.out                 4096
> 
> though, the o/p also depends which other sort keys are used along
> with dso key. Do you think this change makes sense?

the above behaviour is something I'd expect from 'dso'
sort key to do - separate out different dsos, even with
the same name

jirka

Re: [PATCH] perf dso: Fix dso comparison

[Arnaldo Carvalho de Melo]

Em Tue, Mar 24, 2020 at 02:22:58PM +0100, Jiri Olsa escreveu:
> On Tue, Mar 24, 2020 at 06:07:23PM +0530, Ravi Bangoria wrote:
> 
> SNIP
> 
> > > looks good, do we need to add the dso_id check to sort__dso_cmp?
> > 
> > I guess with different filename there is no need to compare dso_id.
> > But for same filename, adding dso_id cmp will separate out the
> > samples:
> > 
> > Ex, Without dso_id compare:
> > 
> >   $ ./perf report -s dso,dso_size -v
> >     66.63%  /home/ravi/a.out                                  4096
> >     33.36%  /home/ravi/Workspace/linux/tools/perf/a.out       4096
> > 
> >   $ ./perf report -s dso,dso_size
> >     99.99%  a.out                 4096
> > 
> > 
> > With below diff:
> > 
> >   -       return strcmp(dso_name_l, dso_name_r);
> >   +       ret = strcmp(dso_name_l, dso_name_r);
> >   +       if (ret)
> >   +               return ret;
> >   +       else
> >   +               return dso__cmp_id(dso_l, dso_r);
> > 
> > 
> >   $ ./perf report -s dso,dso_size
> >     99.99%  a.out                 4096
> >     33.36%  a.out                 4096
> > 
> > though, the o/p also depends which other sort keys are used along
> > with dso key. Do you think this change makes sense?
> 
> the above behaviour is something I'd expect from 'dso'
> sort key to do - separate out different dsos, even with
> the same name

This specific one can be resolved using -v when long_name will be used,
the biggest problem is when long_name is the same (and thus short_name),
which can happen when developing some software, i.e. compile+rebuild and
get a different content, same short/long name, in that case we should
use some diferentiator, the build-id comes to mind, but one that could
be more useful would be file timestamp, meaning, hey, the older version
is actually better, which one, lemme look at the build-id, and even the
source code if developed with -g, by using the copy we stored in the
build-id cache (~/.debug), which would be really useful workflow.

- Arnaldo

> jirka
> 

-- 

- Arnaldo

Re: [PATCH] perf dso: Fix dso comparison

[Ravi Bangoria]

On 3/24/20 6:52 PM, Jiri Olsa wrote:
> On Tue, Mar 24, 2020 at 06:07:23PM +0530, Ravi Bangoria wrote:
> 
> SNIP
> 
>>> looks good, do we need to add the dso_id check to sort__dso_cmp?
>>
>> I guess with different filename there is no need to compare dso_id.
>> But for same filename, adding dso_id cmp will separate out the
>> samples:
>>
>> Ex, Without dso_id compare:
>>
>>    $ ./perf report -s dso,dso_size -v
>>      66.63%  /home/ravi/a.out                                  4096
>>      33.36%  /home/ravi/Workspace/linux/tools/perf/a.out       4096
>>
>>    $ ./perf report -s dso,dso_size
>>      99.99%  a.out                 4096
>>
>>
>> With below diff:
>>
>>    -       return strcmp(dso_name_l, dso_name_r);
>>    +       ret = strcmp(dso_name_l, dso_name_r);
>>    +       if (ret)
>>    +               return ret;
>>    +       else
>>    +               return dso__cmp_id(dso_l, dso_r);
>>
>>
>>    $ ./perf report -s dso,dso_size
>>      99.99%  a.out                 4096
>>      33.36%  a.out                 4096
>>
>> though, the o/p also depends which other sort keys are used along
>> with dso key. Do you think this change makes sense?
> 
> the above behaviour is something I'd expect from 'dso'
> sort key to do - separate out different dsos, even with
> the same name

Yes it does that as well...

   $ ./perf report -s dso
     66.63%  a.out
     33.36%  a.out

Ravi

Re: [PATCH] perf dso: Fix dso comparison

[Arnaldo Carvalho de Melo]

Em Tue, Mar 24, 2020 at 11:48:43AM +0100, Jiri Olsa escreveu:
> On Tue, Mar 24, 2020 at 09:54:24AM +0530, Ravi Bangoria wrote:
> > Perf gets dso details from two different sources. 1st, from builid
> > headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
> > header does not have dso_id detail. And dso from MMAP2 samples does
> > not have buildid information. If detail of the same dso is present
> > at both the places, filename is common.
> > 
> > Previously, __dsos__findnew_link_by_longname_id() used to compare only
> > long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
> > from 'struct map' to 'struct dso'") also added a dso_id comparison.
> > Because of that, now perf is creating two different dso objects of the
> > same file, one from buildid header (with dso_id but without buildid)
> > and second from MMAP2 sample (with buildid but without dso_id).
> > 
> > This is causing issues with archive, buildid-list etc subcommands. Fix
> > this by comparing dso_id only when it's present. And incase dso is
> > present in 'dsos' list without dso_id, inject dso_id detail as well.
> > 
> > Before:
> > 
> >   $ sudo ./perf buildid-list -H
> >   0000000000000000000000000000000000000000 /usr/bin/ls
> >   0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
> >   0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
> > 
> >   $ ./perf archive
> >   perf archive: no build-ids found
> > 
> > After:
> > 
> >   $ ./perf buildid-list -H
> >   b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
> >   641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
> >   675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
> > 
> >   $ ./perf archive
> >   Now please run:
> > 
> >   $ tar xvf perf.data.tar.bz2 -C ~/.debug
> > 
> >   wherever you need to run 'perf report' on.
> > 
> > Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> 
> looks good, do we need to add the dso_id check to sort__dso_cmp?

Jiri:

Humm, you mean sort__dso_cmp() -> _sort__dso_cmp() should consider the
dso_id and not just its name? Humm, when "dso" sort key is used that
means just the short_name (or long_name, if verbose), if we use the ID
for "dso" then we need to somehow show the id in the output, otherwise
we'd have multiple lines with the same DSO name, when multiple versions
exist... Perhaps we should do a first pass, figure out if there are DSOs
with the same name/different IDs and mark them for showing the ID to
differentiate them on the output? But this is something that should be
dealt with in a separece cset, I think.

With that in mind, can I add your Acked-by for this patch, with my
changes described below?

Ravi:

I'm applying it with the changes below, to keep namespacing consistency, ok?

- Arnaldo

diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index 5c5bfa2538a9..939471731ea6 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -26,7 +26,7 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b)
 	return 0;
 }
 
-static bool is_empty_dso_id(struct dso_id *id)
+static bool dso_id__empty(struct dso_id *id)
 {
 	if (!id)
 		return true;
@@ -34,7 +34,7 @@ static bool is_empty_dso_id(struct dso_id *id)
 	return !id->maj && !id->min && !id->ino && !id->ino_generation;
 }
 
-static void inject_dso_id(struct dso *dso, struct dso_id *id)
+static void dso__inject_id(struct dso *dso, struct dso_id *id)
 {
 	dso->id.maj = id->maj;
 	dso->id.min = id->min;
@@ -48,7 +48,7 @@ static int dso_id__cmp(struct dso_id *a, struct dso_id *b)
 	 * The second is always dso->id, so zeroes if not set, assume passing
 	 * NULL for a means a zeroed id
 	 */
-	if (is_empty_dso_id(a) || is_empty_dso_id(b))
+	if (dso_id__empty(a) || dso_id__empty(b))
 		return 0;
 
 	return __dso_id__cmp(a, b);
@@ -266,8 +266,8 @@ static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, struc
 {
 	struct dso *dso = __dsos__find_id(dsos, name, id, false);
 
-	if (dso && is_empty_dso_id(&dso->id) && !is_empty_dso_id(id))
-		inject_dso_id(dso, id);
+	if (dso && dso_id__empty(&dso->id) && !dso_id__empty(id))
+		dso__inject_id(dso, id);
 
 	return dso ? dso : __dsos__addnew_id(dsos, name, id);
 }

Re: [PATCH] perf dso: Fix dso comparison

[Jiri Olsa]

On Tue, Mar 24, 2020 at 10:00:52AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 24, 2020 at 11:48:43AM +0100, Jiri Olsa escreveu:
> > On Tue, Mar 24, 2020 at 09:54:24AM +0530, Ravi Bangoria wrote:
> > > Perf gets dso details from two different sources. 1st, from builid
> > > headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
> > > header does not have dso_id detail. And dso from MMAP2 samples does
> > > not have buildid information. If detail of the same dso is present
> > > at both the places, filename is common.
> > > 
> > > Previously, __dsos__findnew_link_by_longname_id() used to compare only
> > > long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
> > > from 'struct map' to 'struct dso'") also added a dso_id comparison.
> > > Because of that, now perf is creating two different dso objects of the
> > > same file, one from buildid header (with dso_id but without buildid)
> > > and second from MMAP2 sample (with buildid but without dso_id).
> > > 
> > > This is causing issues with archive, buildid-list etc subcommands. Fix
> > > this by comparing dso_id only when it's present. And incase dso is
> > > present in 'dsos' list without dso_id, inject dso_id detail as well.
> > > 
> > > Before:
> > > 
> > >   $ sudo ./perf buildid-list -H
> > >   0000000000000000000000000000000000000000 /usr/bin/ls
> > >   0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
> > >   0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
> > > 
> > >   $ ./perf archive
> > >   perf archive: no build-ids found
> > > 
> > > After:
> > > 
> > >   $ ./perf buildid-list -H
> > >   b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
> > >   641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
> > >   675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
> > > 
> > >   $ ./perf archive
> > >   Now please run:
> > > 
> > >   $ tar xvf perf.data.tar.bz2 -C ~/.debug
> > > 
> > >   wherever you need to run 'perf report' on.
> > > 
> > > Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > 
> > looks good, do we need to add the dso_id check to sort__dso_cmp?
> 
> Jiri:
> 
> Humm, you mean sort__dso_cmp() -> _sort__dso_cmp() should consider the
> dso_id and not just its name? Humm, when "dso" sort key is used that
> means just the short_name (or long_name, if verbose), if we use the ID
> for "dso" then we need to somehow show the id in the output, otherwise
> we'd have multiple lines with the same DSO name, when multiple versions
> exist... Perhaps we should do a first pass, figure out if there are DSOs
> with the same name/different IDs and mark them for showing the ID to
> differentiate them on the output? But this is something that should be
> dealt with in a separece cset, I think.

true, also Ravi pointed out in the other answer,
it depends on what we want.. compare the name or
different binaries with same name

> 
> With that in mind, can I add your Acked-by for this patch, with my
> changes described below?

yep,

Acked-by: Jiri Olsa <jolsa@redhat.com>

jirka

> 
> Ravi:
> 
> I'm applying it with the changes below, to keep namespacing consistency, ok?
> 
> - Arnaldo
> 
> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> index 5c5bfa2538a9..939471731ea6 100644
> --- a/tools/perf/util/dsos.c
> +++ b/tools/perf/util/dsos.c
> @@ -26,7 +26,7 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b)
>  	return 0;
>  }
>  
> -static bool is_empty_dso_id(struct dso_id *id)
> +static bool dso_id__empty(struct dso_id *id)
>  {
>  	if (!id)
>  		return true;
> @@ -34,7 +34,7 @@ static bool is_empty_dso_id(struct dso_id *id)
>  	return !id->maj && !id->min && !id->ino && !id->ino_generation;
>  }
>  
> -static void inject_dso_id(struct dso *dso, struct dso_id *id)
> +static void dso__inject_id(struct dso *dso, struct dso_id *id)
>  {
>  	dso->id.maj = id->maj;
>  	dso->id.min = id->min;
> @@ -48,7 +48,7 @@ static int dso_id__cmp(struct dso_id *a, struct dso_id *b)
>  	 * The second is always dso->id, so zeroes if not set, assume passing
>  	 * NULL for a means a zeroed id
>  	 */
> -	if (is_empty_dso_id(a) || is_empty_dso_id(b))
> +	if (dso_id__empty(a) || dso_id__empty(b))
>  		return 0;
>  
>  	return __dso_id__cmp(a, b);
> @@ -266,8 +266,8 @@ static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, struc
>  {
>  	struct dso *dso = __dsos__find_id(dsos, name, id, false);
>  
> -	if (dso && is_empty_dso_id(&dso->id) && !is_empty_dso_id(id))
> -		inject_dso_id(dso, id);
> +	if (dso && dso_id__empty(&dso->id) && !dso_id__empty(id))
> +		dso__inject_id(dso, id);
>  
>  	return dso ? dso : __dsos__addnew_id(dsos, name, id);
>  }
> 

Re: [PATCH] perf dso: Fix dso comparison

[Arnaldo Carvalho de Melo]

Em Tue, Mar 24, 2020 at 10:00:52AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 24, 2020 at 11:48:43AM +0100, Jiri Olsa escreveu:
> > On Tue, Mar 24, 2020 at 09:54:24AM +0530, Ravi Bangoria wrote:
> > > Perf gets dso details from two different sources. 1st, from builid
> > > headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
> > > header does not have dso_id detail. And dso from MMAP2 samples does
> > > not have buildid information. If detail of the same dso is present
> > > at both the places, filename is common.
> > > 
> > > Previously, __dsos__findnew_link_by_longname_id() used to compare only
> > > long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
> > > from 'struct map' to 'struct dso'") also added a dso_id comparison.
> > > Because of that, now perf is creating two different dso objects of the
> > > same file, one from buildid header (with dso_id but without buildid)
> > > and second from MMAP2 sample (with buildid but without dso_id).
> > > 
> > > This is causing issues with archive, buildid-list etc subcommands. Fix
> > > this by comparing dso_id only when it's present. And incase dso is
> > > present in 'dsos' list without dso_id, inject dso_id detail as well.
> > > 
> > > Before:
> > > 
> > >   $ sudo ./perf buildid-list -H
> > >   0000000000000000000000000000000000000000 /usr/bin/ls
> > >   0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
> > >   0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
> > > 
> > >   $ ./perf archive
> > >   perf archive: no build-ids found
> > > 
> > > After:
> > > 
> > >   $ ./perf buildid-list -H
> > >   b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
> > >   641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
> > >   675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
> > > 
> > >   $ ./perf archive
> > >   Now please run:
> > > 
> > >   $ tar xvf perf.data.tar.bz2 -C ~/.debug
> > > 
> > >   wherever you need to run 'perf report' on.

This improves the situation, but something else is still amiss:

[root@five ~]# perf buildid-list -H | grep ^000000000000 | while read -a line ; do path=${line[1]} ; nlines=$(perf buildid-list -H -i perf.data | grep $path | tee /tmp/lines | wc -l) ;[ $nlines -eq 2 ] && cat /tmp/lines ; done
641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
d40c8da7371d8adea464ae2b099590b2c4465574 /usr/lib64/libpthread-2.30.so
0000000000000000000000000000000000000000 /usr/lib64/libpthread-2.30.so
aec38b95c2b305c9f1943b2dd988aeb58c17a5d9 /usr/lib64/libm-2.30.so
0000000000000000000000000000000000000000 /usr/lib64/libm-2.30.so
675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
8286f22591b0be26730eea306a22a0f30475590b /usr/bin/bash
0000000000000000000000000000000000000000 /usr/bin/bash
[root@five ~]#

I.e. DSOs that have not changed:

[root@five ~]# grep libpthread /proc/*/maps | cut -d' ' -f6- | sort  | uniq -c
    690                    /usr/lib64/libpthread-2.30.so
[root@five ~]#

[root@five ~]# file /usr/lib64/libpthread-2.30.so
/usr/lib64/libpthread-2.30.so: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=d40c8da7371d8adea464ae2b099590b2c4465574, for GNU/Linux 3.2.0, not stripped
[root@five ~]#
[root@five ~]# perf buildid-list -i /usr/lib64/libpthread-2.30.so
d40c8da7371d8adea464ae2b099590b2c4465574
[root@five ~]#

Appear with/without build-id in 'perf buildid-list'.

I'm checking a bit more to see if I figure this out, I'll keep your
patch in, maybe this is a diferent issue, will combine if needed.

- Arnaldo

Re: [PATCH] perf dso: Fix dso comparison

[Naveen N. Rao]

Ravi Bangoria wrote:
> Perf gets dso details from two different sources. 1st, from builid
> headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
> header does not have dso_id detail. And dso from MMAP2 samples does
> not have buildid information. If detail of the same dso is present
> at both the places, filename is common.
> 
> Previously, __dsos__findnew_link_by_longname_id() used to compare only
> long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
> from 'struct map' to 'struct dso'") also added a dso_id comparison.
> Because of that, now perf is creating two different dso objects of the
> same file, one from buildid header (with dso_id but without buildid)
> and second from MMAP2 sample (with buildid but without dso_id).
> 
> This is causing issues with archive, buildid-list etc subcommands. Fix
> this by comparing dso_id only when it's present. And incase dso is
> present in 'dsos' list without dso_id, inject dso_id detail as well.
> 
> Before:
> 
>   $ sudo ./perf buildid-list -H
>   0000000000000000000000000000000000000000 /usr/bin/ls
>   0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
>   0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
> 
>   $ ./perf archive
>   perf archive: no build-ids found
> 
> After:
> 
>   $ ./perf buildid-list -H
>   b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
>   641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
>   675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
> 
>   $ ./perf archive
>   Now please run:
> 
>   $ tar xvf perf.data.tar.bz2 -C ~/.debug
> 
>   wherever you need to run 'perf report' on.
> 
> Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  tools/perf/util/dsos.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)

Thanks. This fixes the issue I was facing with 'perf archive' not 
picking up the right binaries.  So:
Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>


- Naveen

[tip: perf/urgent] perf dso: Fix dso comparison

[tip-bot2 for Ravi Bangoria]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     0d33b34352531ff7029c58eda2321340c0ea3f5f
Gitweb:        https://git.kernel.org/tip/0d33b34352531ff7029c58eda2321340c0ea3f5f
Author:        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
AuthorDate:    Tue, 24 Mar 2020 09:54:24 +05:30
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 24 Mar 2020 10:57:38 -03:00

perf dso: Fix dso comparison

Perf gets dso details from two different sources. 1st, from builid
headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
header does not have dso_id detail. And dso from MMAP2 samples does
not have buildid information. If detail of the same dso is present
at both the places, filename is common.

Previously, __dsos__findnew_link_by_longname_id() used to compare only
long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
from 'struct map' to 'struct dso'") also added a dso_id comparison.
Because of that, now perf is creating two different dso objects of the
same file, one from buildid header (with dso_id but without buildid)
and second from MMAP2 sample (with buildid but without dso_id).

This is causing issues with archive, buildid-list etc subcommands. Fix
this by comparing dso_id only when it's present. And incase dso is
present in 'dsos' list without dso_id, inject dso_id detail as well.

Before:

  $ sudo ./perf buildid-list -H
  0000000000000000000000000000000000000000 /usr/bin/ls
  0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
  0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so

  $ ./perf archive
  perf archive: no build-ids found

After:

  $ ./perf buildid-list -H
  b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
  641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
  675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so

  $ ./perf archive
  Now please run:

  $ tar xvf perf.data.tar.bz2 -C ~/.debug

  wherever you need to run 'perf report' on.

Committer notes:

Renamed is_empty_dso_id() to dso_id__empty() and inject_dso_id() to
dso__inject_id() to keep namespacing consistent.

Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'")
Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lore.kernel.org/lkml/20200324042424.68366-1-ravi.bangoria@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dsos.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index 591707c..9394717 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -26,13 +26,29 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b)
 	return 0;
 }
 
+static bool dso_id__empty(struct dso_id *id)
+{
+	if (!id)
+		return true;
+
+	return !id->maj && !id->min && !id->ino && !id->ino_generation;
+}
+
+static void dso__inject_id(struct dso *dso, struct dso_id *id)
+{
+	dso->id.maj = id->maj;
+	dso->id.min = id->min;
+	dso->id.ino = id->ino;
+	dso->id.ino_generation = id->ino_generation;
+}
+
 static int dso_id__cmp(struct dso_id *a, struct dso_id *b)
 {
 	/*
 	 * The second is always dso->id, so zeroes if not set, assume passing
 	 * NULL for a means a zeroed id
 	 */
-	if (a == NULL)
+	if (dso_id__empty(a) || dso_id__empty(b))
 		return 0;
 
 	return __dso_id__cmp(a, b);
@@ -249,6 +265,10 @@ struct dso *__dsos__addnew(struct dsos *dsos, const char *name)
 static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, struct dso_id *id)
 {
 	struct dso *dso = __dsos__find_id(dsos, name, id, false);
+
+	if (dso && dso_id__empty(&dso->id) && !dso_id__empty(id))
+		dso__inject_id(dso, id);
+
 	return dso ? dso : __dsos__addnew_id(dsos, name, id);
 }