Re: [PATCH v5 07/11] perf tools: Factor out dso__find_binary_type()

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Tue, Oct 13, 2026 at 10:21:51AM -0700, Namhyung Kim wrote:
> Separate out the logic to detect binary type of a DSO.  It checks if
> the candidate file contains ".text" section.  There could be data-only
> DSOs but as our use cases focus on the code (perf annotate or intel
> PT), we can ignore them for now.
 
> But it also can fail due to open file limit.  Handle the case by
> retrying with _dso__data_open_lock.
 
> Introduce bintype_searched flag and it accessors to prevent repeated
> (failed) search.

This one, when compiled with clang, catch this in all my build
containers:

  LD      /tmp/build/perf-tools-next/tests/perf-test-in.o
util/dso.c:942:7: error: mutex '_dso__data_open_lock' is not held on every path through here [-Werror,-Wthread-safety-analysis]
  942 |                 if (dso__data_open_cnt) {
      |                     ^
util/dso.c:940:4: note: mutex acquired here
  940 |                         mutex_lock(dso__data_open_lock());
      |                         ^
util/dso.c:942:7: error: reading variable 'dso__data_open_cnt' requires holding mutex '_dso__data_open_lock'   CC      /tmp/build/perf-tools-next/util/maps.o
[-Werror,-Wthread-safety-analysis]
  942 |                 if (dso__data_open_cnt) {
      |                     ^
util/dso.c:943:4: error: calling function 'close_first_dso' requires holding mutex '_dso__data_open_lock' exclusively [-Werror,-Wthread-safety-analysis]
  943 |                         close_first_dso();
      |                         ^
util/dso.c:949:3: error: releasing mutex '_dso__data_open_lock' that was not held [-Werror,-Wthread-safety-analysis]
  949 |                 mutex_unlock(dso__data_open_lock());
      |                 ^
4 errors generated.
make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:95: /tmp/build/perf-tools-next/util/dso.o] Error 1
  CC	  /tmp/build/perf-tools-next/util/thread.o

perf adds -Wthread-safety for clang builds (tools/perf/Makefile.config),
so it is -Werror there.

This is because __dso__find_binary_type() takes _dso__data_open_lock only
on the first retry, under needs_open_lock, and then reads
dso__data_open_cnt and calls close_first_dso(); with the lock taken
conditionally the analyzer can't see it held on every path.  And for the
try_to_open_dso() caller, which already holds it, the requirement was
only implicit, the helper had no annotation for clang to check.

I've folded the following into the patch, see if it is acceptable:

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 47241077ad26..8f274b8b1030 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -925,7 +925,8 @@ static int dso__find_binary_type_locked(struct dso *dso)
 }
 
 #define FIND_MAX_RETRY  3
-static int __dso__find_binary_type(struct dso *dso, bool needs_open_lock)
+static int __dso__find_binary_type(struct dso *dso)
+	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
 {
 	int ret;
 	int retry = 0;
@@ -936,18 +937,12 @@ static int __dso__find_binary_type(struct dso *dso, bool needs_open_lock)
 	mutex_unlock(dso__lock(dso));
 
 	if (ret == -1 && retry++ < FIND_MAX_RETRY) {
-		if (needs_open_lock && retry == 1)
-			mutex_lock(dso__data_open_lock());
-
 		if (dso__data_open_cnt) {
 			close_first_dso();
 			goto again;
 		}
 	}
 
-	if (retry > 0 && needs_open_lock)
-		mutex_unlock(dso__data_open_lock());
-
 	if (ret == -1) {
 		mutex_lock(dso__lock(dso));
 		ret = dso__binary_type(dso);
@@ -961,7 +956,13 @@ static int __dso__find_binary_type(struct dso *dso, bool needs_open_lock)
 
 enum dso_binary_type dso__find_binary_type(struct dso *dso)
 {
-	return __dso__find_binary_type(dso, /*needs_open_lock=*/true);
+	int ret;
+
+	mutex_lock(dso__data_open_lock());
+	ret = __dso__find_binary_type(dso);
+	mutex_unlock(dso__data_open_lock());
+
+	return ret;
 }
 
 static void try_to_open_dso(struct dso *dso, struct machine *machine)
@@ -972,7 +973,7 @@ static void try_to_open_dso(struct dso *dso, struct machine *machine)
 	if (dso_data->fd >= 0)
 		return;
 
-	__dso__find_binary_type(dso, /*needs_open_lock=*/false);
+	__dso__find_binary_type(dso);
 	dso_data->fd = open_dso(dso, machine);
 
 	if (dso_data->fd >= 0)

__dso__find_binary_type() now requires _dso__data_open_lock, the public
dso__find_binary_type() takes it around the call, and the needs_open_lock
parameter goes away.  try_to_open_dso() stays as is: it is annotated
EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock), its four call sites take
the lock first, and now clang checks the whole chain.

The patches I took (v5 07/11 onward) build with clang 22.1.8 here and
'perf test dso' passes (read, cache, reopen), so please fold something
like this if you respin, or let me know if you'd rather have it done
differently.

Regards,

- Arnaldo