Re: [PATCH v12 12/20] perf dsos: Use read lock fast path in dsos__findnew_id()

[Ian Rogers]

On Wed, Nov 11, 2026 at 10:53 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Wed, Nov 11, 2026 at 09:42:04AM -0800, Ian Rogers wrote:
> > In dsos__findnew_id(), try finding the DSO under the read lock first and
> > only acquire the write lock when a new DSO must be inserted or an
> > existing DSO needs its ID updated (checked via dso__needs_improve_id()).
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/util/dso.c  | 16 +++++++++++++++-
> >  tools/perf/util/dso.h  |  1 +
> >  tools/perf/util/dsos.c | 10 ++++++++++
> >  3 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index f0e02062c09c..4dc518f7cb7d 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -1685,6 +1685,18 @@ const struct dso_id dso_id_empty = {
> >       }
> >  };
> >
> > +bool dso__needs_improve_id(const struct dso *dso, const struct dso_id *id)
> > +{
> > +     const struct dso_id *dso_id = dso__id_const(dso);
> > +
> > +     if (!id)
> > +             return false;
> > +
> > +     return (id->mmap2_valid && !dso_id->mmap2_valid) ||
> > +            (id->mmap2_ino_generation_valid && !dso_id->mmap2_ino_generation_valid) ||
> > +            (build_id__is_defined(&id->build_id) && !build_id__is_defined(&dso_id->build_id));
>
> Don't we prefer build-ID over inode?  I'm curious if it needs to check
> existing build-ID is defined first and return true.  Any chance it'd
> overwrite a build-ID with an inode?

struct dso_id keeps the maj/min/ino fields and build_id as separate
members rather than a union (unlike perf_record_mmap2), so build_id
wouldn't be overwritten in memory, though setting mmap2_valid did take
precedence in __dso_id__cmp() and `perf report --mmaps`.

Agreed we should prioritize build-ID over inode. In v13, I've moved the
build-ID checks ahead of the inode checks in __dso_id__cmp(),
dso__needs_improve_id(), and __dso__improve_id(), returning early when
the existing DSO already has a defined build-ID so inode metadata isn't
added on top of it.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > +}
> > +
> >  void __dso__improve_id(struct dso *dso, const struct dso_id *id)
> >  {
> >       struct dsos *dsos = dso__dsos(dso);
> > @@ -1692,6 +1704,8 @@ void __dso__improve_id(struct dso *dso, const struct dso_id *id)
> >       bool changed = false;
> >
> >       /* dsos write lock held by caller. */
> > +     if (!id)
> > +             return;
> >
> >       if (id->mmap2_valid && !dso_id->mmap2_valid) {
> >               dso_id->maj = id->maj;
> > @@ -1715,7 +1729,7 @@ void __dso__improve_id(struct dso *dso, const struct dso_id *id)
> >
> >  int dso_id__cmp(const struct dso_id *a, const struct dso_id *b)
> >  {
> > -     if (a == &dso_id_empty || b == &dso_id_empty) {
> > +     if (!a || !b || a == &dso_id_empty || b == &dso_id_empty) {
> >               /* There is no valid data to compare so the comparison always returns identical. */
> >               return 0;
> >       }
> > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > index 9e38b3b69e9b..df1748763d95 100644
> > --- a/tools/perf/util/dso.h
> > +++ b/tools/perf/util/dso.h
> > @@ -802,6 +802,7 @@ void dso__delete(struct dso *dso);
> >  int dso__cmp_id(struct dso *a, struct dso *b);
> >  void dso__set_short_name(struct dso *dso, const char *name, bool name_allocated);
> >  void dso__set_long_name(struct dso *dso, const char *name, bool name_allocated);
> > +bool dso__needs_improve_id(const struct dso *dso, const struct dso_id *id);
> >  void __dso__improve_id(struct dso *dso, const struct dso_id *id);
> >
> >  int dso__name_len(const struct dso *dso);
> > diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> > index fbbf98c4ccae..1b3f8b84ed30 100644
> > --- a/tools/perf/util/dsos.c
> > +++ b/tools/perf/util/dsos.c
> > @@ -349,6 +349,16 @@ static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, const
> >  struct dso *dsos__findnew_id(struct dsos *dsos, const char *name, const struct dso_id *id)
> >  {
> >       struct dso *dso;
> > +
> > +     down_read(&dsos->lock);
> > +     dso = __dsos__find_id(dsos, name, id, false, /*write_locked=*/false);
> > +     if (dso && !dso__needs_improve_id(dso, id)) {
> > +             up_read(&dsos->lock);
> > +             return dso;
> > +     }
> > +     up_read(&dsos->lock);
> > +     dso__put(dso);
> > +
> >       down_write(&dsos->lock);
> >       dso = __dsos__findnew_id(dsos, name, id);
> >       up_write(&dsos->lock);
> > --
> > 2.56.0.542.g3ef89b7020-goog
> >