* [PATCH] perf dso: Fix dso comparison @ 2020-03-24 4:24 Ravi Bangoria 2020-03-24 10:48 ` Jiri Olsa ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ravi Bangoria @ 2020-03-24 4:24 UTC (permalink / raw) To: acme, jolsa Cc: linux-kernel, namhyung, mark.rutland, naveen.n.rao, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 4:24 [PATCH] perf dso: Fix dso comparison Ravi Bangoria @ 2020-03-24 10:48 ` Jiri Olsa 2020-03-24 12:37 ` Ravi Bangoria 2020-03-24 13:00 ` Arnaldo Carvalho de Melo 2020-03-24 13:44 ` Naveen N. Rao 2020-04-04 8:41 ` [tip: perf/urgent] " tip-bot2 for Ravi Bangoria 2 siblings, 2 replies; 11+ messages in thread From: Jiri Olsa @ 2020-03-24 10:48 UTC (permalink / raw) To: Ravi Bangoria; +Cc: acme, linux-kernel, namhyung, mark.rutland, naveen.n.rao 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 10:48 ` Jiri Olsa @ 2020-03-24 12:37 ` Ravi Bangoria 2020-03-24 13:22 ` Jiri Olsa 2020-03-24 13:00 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 11+ messages in thread From: Ravi Bangoria @ 2020-03-24 12:37 UTC (permalink / raw) To: Jiri Olsa Cc: acme, linux-kernel, namhyung, mark.rutland, naveen.n.rao, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 12:37 ` Ravi Bangoria @ 2020-03-24 13:22 ` Jiri Olsa 2020-03-24 13:42 ` Arnaldo Carvalho de Melo 2020-03-24 13:42 ` Ravi Bangoria 0 siblings, 2 replies; 11+ messages in thread From: Jiri Olsa @ 2020-03-24 13:22 UTC (permalink / raw) To: Ravi Bangoria; +Cc: acme, linux-kernel, namhyung, mark.rutland, naveen.n.rao 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 13:22 ` Jiri Olsa @ 2020-03-24 13:42 ` Arnaldo Carvalho de Melo 2020-03-24 13:42 ` Ravi Bangoria 1 sibling, 0 replies; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-03-24 13:42 UTC (permalink / raw) To: Jiri Olsa Cc: Ravi Bangoria, linux-kernel, namhyung, mark.rutland, naveen.n.rao 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 13:22 ` Jiri Olsa 2020-03-24 13:42 ` Arnaldo Carvalho de Melo @ 2020-03-24 13:42 ` Ravi Bangoria 1 sibling, 0 replies; 11+ messages in thread From: Ravi Bangoria @ 2020-03-24 13:42 UTC (permalink / raw) To: Jiri Olsa Cc: acme, linux-kernel, namhyung, mark.rutland, naveen.n.rao, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 10:48 ` Jiri Olsa 2020-03-24 12:37 ` Ravi Bangoria @ 2020-03-24 13:00 ` Arnaldo Carvalho de Melo 2020-03-24 13:20 ` Jiri Olsa 2020-03-24 13:21 ` Arnaldo Carvalho de Melo 1 sibling, 2 replies; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-03-24 13:00 UTC (permalink / raw) To: Jiri Olsa Cc: Ravi Bangoria, linux-kernel, namhyung, mark.rutland, naveen.n.rao 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); } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 13:00 ` Arnaldo Carvalho de Melo @ 2020-03-24 13:20 ` Jiri Olsa 2020-03-24 13:21 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 11+ messages in thread From: Jiri Olsa @ 2020-03-24 13:20 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ravi Bangoria, linux-kernel, namhyung, mark.rutland, naveen.n.rao 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); > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 13:00 ` Arnaldo Carvalho de Melo 2020-03-24 13:20 ` Jiri Olsa @ 2020-03-24 13:21 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-03-24 13:21 UTC (permalink / raw) To: Jiri Olsa Cc: Ravi Bangoria, linux-kernel, namhyung, mark.rutland, naveen.n.rao 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf dso: Fix dso comparison 2020-03-24 4:24 [PATCH] perf dso: Fix dso comparison Ravi Bangoria 2020-03-24 10:48 ` Jiri Olsa @ 2020-03-24 13:44 ` Naveen N. Rao 2020-04-04 8:41 ` [tip: perf/urgent] " tip-bot2 for Ravi Bangoria 2 siblings, 0 replies; 11+ messages in thread From: Naveen N. Rao @ 2020-03-24 13:44 UTC (permalink / raw) To: acme, jolsa, Ravi Bangoria; +Cc: linux-kernel, mark.rutland, namhyung 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip: perf/urgent] perf dso: Fix dso comparison 2020-03-24 4:24 [PATCH] perf dso: Fix dso comparison Ravi Bangoria 2020-03-24 10:48 ` Jiri Olsa 2020-03-24 13:44 ` Naveen N. Rao @ 2020-04-04 8:41 ` tip-bot2 for Ravi Bangoria 2 siblings, 0 replies; 11+ messages in thread From: tip-bot2 for Ravi Bangoria @ 2020-04-04 8:41 UTC (permalink / raw) To: linux-tip-commits Cc: Naveen N. Rao, Ravi Bangoria, Arnaldo Carvalho de Melo, Jiri Olsa, Mark Rutland, Namhyung Kim, x86, LKML 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); } ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-04 8:44 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-24 4:24 [PATCH] perf dso: Fix dso comparison Ravi Bangoria 2020-03-24 10:48 ` Jiri Olsa 2020-03-24 12:37 ` Ravi Bangoria 2020-03-24 13:22 ` Jiri Olsa 2020-03-24 13:42 ` Arnaldo Carvalho de Melo 2020-03-24 13:42 ` Ravi Bangoria 2020-03-24 13:00 ` Arnaldo Carvalho de Melo 2020-03-24 13:20 ` Jiri Olsa 2020-03-24 13:21 ` Arnaldo Carvalho de Melo 2020-03-24 13:44 ` Naveen N. Rao 2020-04-04 8:41 ` [tip: perf/urgent] " tip-bot2 for Ravi Bangoria
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).