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

[Arnaldo Melo]

On October 15, 2026 2:09:44 PM GMT-03:00, Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>On Thu, Oct 15, 2026 at 10:18:17AM -0300, Arnaldo Carvalho de Melo wrote:
>> 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.
>
>I think it'd grab the dso open lock unnecessarily, but it'd be simpler.
>So I'm fine with the patch and thanks for the fix!
>
>Do you want me to send the whole series again?  Or can you apply the fix
>on top?


No need, I'll apply them fix myself. 
 
- Arnaldo
>
>Thanks,
>Namhyung
>

- Arnaldo