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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] perf dso: broken bsearch() ordering in __dso_id__cmp() with
  build_id
--

commit c6037e2ef92de325486412d51231ca52f297ef40
Author: Ian Rogers <irogers@google.com>

perf dsos: Use read lock fast path in dsos__findnew_id()

This commit adds a read-lock fast path to dsos__findnew_id() and
prioritizes build IDs over inode fields when comparing and improving DSO
IDs.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index f0e02062c09c8..f736a94a53a97 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1649,6 +1649,11 @@ static void dso__set_long_name_id(struct dso *dso, const char *name, bool name_a
>  
>  static int __dso_id__cmp(const struct dso_id *a, const struct dso_id *b)
>  {
> +	if (build_id__is_defined(&a->build_id) && build_id__is_defined(&b->build_id)) {
> +		if (a->build_id.size != b->build_id.size)
> +			return a->build_id.size < b->build_id.size ? -1 : 1;
> +		return memcmp(a->build_id.data, b->build_id.data, a->build_id.size);
> +	}
>  	if (a->mmap2_valid && b->mmap2_valid) {
>  		if (a->maj > b->maj) return -1;
>  		if (a->maj < b->maj) return 1;
> @@ -1663,11 +1668,6 @@ static int __dso_id__cmp(const struct dso_id *a, const struct dso_id *b)
>  		if (a->ino_generation > b->ino_generation) return -1;
>  		if (a->ino_generation < b->ino_generation) return 1;
>  	}
> -	if (build_id__is_defined(&a->build_id) && build_id__is_defined(&b->build_id)) {
> -		if (a->build_id.size != b->build_id.size)
> -			return a->build_id.size < b->build_id.size ? -1 : 1;
> -		return memcmp(a->build_id.data, b->build_id.data, a->build_id.size);
> -	}
>  	return 0;
>  }

[Severity: Medium]
Can moving the build_id check ahead of mmap2_valid break the bsearch()
ordering in __dsos__find_by_longname_id()?

A DSO created from an MMAP2 event without a build ID has mmap2_valid =
true, and retains mmap2_valid = true when a build_id is later added via
__dso__improve_id() or dso__set_build_id().  If two DSOs with the same
long_name have both build_id and mmap2_valid set (for example, after a
library is replaced on disk), qsort() orders them in dsos->dsos by
build_id.

When dsos__findnew_id() later searches for a lookup key that has only
mmap2_valid set and no build_id, __dso_id__cmp() skips the build_id
comparison and compares (maj, min, ino) against an array sorted by
build_id.  If the build_id order and inode order disagree, bsearch()
searches the wrong half of dsos->dsos, misses the existing DSO, and
inserts a duplicate entry.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113164152.2095566-1-irogers@google.com?part=12