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

[Namhyung Kim <namhyung@kernel.org>]

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@google.com>
> ---
>  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?

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
>