| Message ID | 20220711093218.10967-2-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf intel-pt: Add support for tracing virtual machine user space on the host | expand |
Em Mon, Jul 11, 2022 at 12:31:44PM +0300, Adrian Hunter escreveu: > Synthesized MMAP events have zero ino_generation, so do not compare zero > values. > > Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'") > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > tools/perf/util/dsos.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c > index b97366f77bbf..839a1f384733 100644 > --- a/tools/perf/util/dsos.c > +++ b/tools/perf/util/dsos.c > @@ -23,8 +23,14 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b) > if (a->ino > b->ino) return -1; > if (a->ino < b->ino) return 1; > > - if (a->ino_generation > b->ino_generation) return -1; > - if (a->ino_generation < b->ino_generation) return 1; > + /* > + * Synthesized MMAP events have zero ino_generation, so do not compare > + * zero values. > + */ > + if (a->ino_generation && b->ino_generation) { > + if (a->ino_generation > b->ino_generation) return -1; > + if (a->ino_generation < b->ino_generation) return 1; > + } But comparing didn't harm right? when its !0 now we may have three comparisions instead of 2 :-\ The comment has some value tho, so I'm merging this :-) - Arnaldo
On 18/07/22 17:57, Arnaldo Carvalho de Melo wrote: > Em Mon, Jul 11, 2022 at 12:31:44PM +0300, Adrian Hunter escreveu: >> Synthesized MMAP events have zero ino_generation, so do not compare zero >> values. >> >> Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'") >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> tools/perf/util/dsos.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c >> index b97366f77bbf..839a1f384733 100644 >> --- a/tools/perf/util/dsos.c >> +++ b/tools/perf/util/dsos.c >> @@ -23,8 +23,14 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b) >> if (a->ino > b->ino) return -1; >> if (a->ino < b->ino) return 1; >> >> - if (a->ino_generation > b->ino_generation) return -1; >> - if (a->ino_generation < b->ino_generation) return 1; >> + /* >> + * Synthesized MMAP events have zero ino_generation, so do not compare >> + * zero values. >> + */ >> + if (a->ino_generation && b->ino_generation) { >> + if (a->ino_generation > b->ino_generation) return -1; >> + if (a->ino_generation < b->ino_generation) return 1; >> + } > > But comparing didn't harm right? when its !0 now we may have three > comparisions instead of 2 :-\ > > The comment has some value tho, so I'm merging this :-) Thanks. I found it harmful because the mismatch resulted in a new dso that did not have a build ID whereas the original dso did have a build ID. The build ID was essential because the object was not found otherwise.
On Tue, Jul 19, 2022 at 3:18 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 18/07/22 17:57, Arnaldo Carvalho de Melo wrote: > > Em Mon, Jul 11, 2022 at 12:31:44PM +0300, Adrian Hunter escreveu: > >> Synthesized MMAP events have zero ino_generation, so do not compare zero > >> values. > >> > >> Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'") > >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > >> --- > >> tools/perf/util/dsos.c | 10 ++++++++-- > >> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c > >> index b97366f77bbf..839a1f384733 100644 > >> --- a/tools/perf/util/dsos.c > >> +++ b/tools/perf/util/dsos.c > >> @@ -23,8 +23,14 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b) > >> if (a->ino > b->ino) return -1; > >> if (a->ino < b->ino) return 1; > >> > >> - if (a->ino_generation > b->ino_generation) return -1; > >> - if (a->ino_generation < b->ino_generation) return 1; > >> + /* > >> + * Synthesized MMAP events have zero ino_generation, so do not compare > >> + * zero values. > >> + */ > >> + if (a->ino_generation && b->ino_generation) { > >> + if (a->ino_generation > b->ino_generation) return -1; > >> + if (a->ino_generation < b->ino_generation) return 1; > >> + } > > > > But comparing didn't harm right? when its !0 now we may have three > > comparisions instead of 2 :-\ > > > > The comment has some value tho, so I'm merging this :-) > > Thanks. I found it harmful because the mismatch resulted in a new > dso that did not have a build ID whereas the original dso did have > a build ID. The build ID was essential because the object was not > found otherwise. That's good to know, could we add that also to the comment? Perhaps: Synthesized MMAP events have zero ino_generation, avoid comparing them with MMAP events with actual ino_generation. Thanks, Ian
Em Tue, Jul 19, 2022 at 08:13:18AM -0700, Ian Rogers escreveu: > On Tue, Jul 19, 2022 at 3:18 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > > > On 18/07/22 17:57, Arnaldo Carvalho de Melo wrote: > > > Em Mon, Jul 11, 2022 at 12:31:44PM +0300, Adrian Hunter escreveu: > > >> Synthesized MMAP events have zero ino_generation, so do not compare zero > > >> values. > > >> > > >> Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'") > > >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > > >> --- > > >> tools/perf/util/dsos.c | 10 ++++++++-- > > >> 1 file changed, 8 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c > > >> index b97366f77bbf..839a1f384733 100644 > > >> --- a/tools/perf/util/dsos.c > > >> +++ b/tools/perf/util/dsos.c > > >> @@ -23,8 +23,14 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b) > > >> if (a->ino > b->ino) return -1; > > >> if (a->ino < b->ino) return 1; > > >> > > >> - if (a->ino_generation > b->ino_generation) return -1; > > >> - if (a->ino_generation < b->ino_generation) return 1; > > >> + /* > > >> + * Synthesized MMAP events have zero ino_generation, so do not compare > > >> + * zero values. > > >> + */ > > >> + if (a->ino_generation && b->ino_generation) { > > >> + if (a->ino_generation > b->ino_generation) return -1; > > >> + if (a->ino_generation < b->ino_generation) return 1; > > >> + } > > > > > > But comparing didn't harm right? when its !0 now we may have three > > > comparisions instead of 2 :-\ > > > > > > The comment has some value tho, so I'm merging this :-) > > > > Thanks. I found it harmful because the mismatch resulted in a new > > dso that did not have a build ID whereas the original dso did have > > a build ID. The build ID was essential because the object was not > > found otherwise. > > That's good to know, could we add that also to the comment? Perhaps: > > Synthesized MMAP events have zero ino_generation, avoid comparing them > with MMAP events with actual ino_generation. I see now, thanks, adding this comment. - Arnaldo
diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c index b97366f77bbf..839a1f384733 100644 --- a/tools/perf/util/dsos.c +++ b/tools/perf/util/dsos.c @@ -23,8 +23,14 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b) if (a->ino > b->ino) return -1; if (a->ino < b->ino) return 1; - if (a->ino_generation > b->ino_generation) return -1; - if (a->ino_generation < b->ino_generation) return 1; + /* + * Synthesized MMAP events have zero ino_generation, so do not compare + * zero values. + */ + if (a->ino_generation && b->ino_generation) { + if (a->ino_generation > b->ino_generation) return -1; + if (a->ino_generation < b->ino_generation) return 1; + } return 0; }
Synthesized MMAP events have zero ino_generation, so do not compare zero values. Fixes: 0e3149f86b99 ("perf dso: Move dso_id from 'struct map' to 'struct dso'") Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- tools/perf/util/dsos.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)