Re: [PATCH v2 00/10] tools/perf: Adopt kernel compiler-context-analysis.h annotations

[Bart Van Assche <bvanassche@acm.org>]

On 10/13/26 3:26 PM, Ian Rogers wrote:
> The perf tool previously added Clang thread-safety annotations in
> tools/perf/util/mutex.h (GUARDED_BY, LOCKABLE, EXCLUSIVE_LOCK_FUNCTION,
> UNLOCK_FUNCTION, etc.). Since then, the Linux kernel has adopted a
> unified set of static context analysis macros and attributes in
> include/linux/compiler-context-analysis.h (context_lock_struct,
> __guarded_by, __acquires, __releases, __must_hold, etc.).

New versions of a patch series should be posted as a new e-mail thread
instead of as a reply to an existing email thread. Additionally, posting
a new version of a patch series only four hours after the previous
version is way too soon.

The LLM that I asked to take a look at this patch series came up with
the feedback below. Please take a look at this feedback.

Thanks,

Bart.

--------------------------------------------------------------------------

Verdict: Needs Substantive Revision

While the overall goal and majority of the annotations are 
well-executed, several patches bundle distinct bug fixes with 
refactoring and annotation churn (violating the "one logical change per 
patch" rule).
Additionally, there are concurrency and lifetime subtleties in the newly 
introduced dsos__findnew_id() read-lock fast path and vdso.c, an 
unsynchronized benign data race in maps.c, and missing Fixes: 
attribution tags.

2. Critical & Concurrency Findings

A. Patch 07/10: Lock drop-and-reacquire race and unneeded 
re-invalidations in dsos__findnew_id()

In tools/perf/util/dsos.c:

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);
	down_write(&dsos->lock);
	if (dso)
		__dso__improve_id(dso, id);
	else
		dso = __dsos__findnew_id(dsos, name, id);
	up_write(&dsos->lock);
	return dso;
}
Issue: If dso is found under down_read but needs ID improvement 
(dso__needs_improve_id(dso, id) == true), up_read() releases the lock. 
Before down_write() is acquired, another thread may have acquired 
down_write() and already improved the DSO's ID with the identical or 
better info.
Consequence: When down_write() is acquired, __dso__improve_id(dso, id) 
is executed unconditionally without re-checking dso__needs_improve_id(). 
While updating fields is mostly idempotent, __dso__improve_id() sets 
dsos->sorted = false;, unnecessarily degrading subsequent read lookups 
to qsort().
Fix: Re-check dso__needs_improve_id(dso, id) under down_write(). If no 
improvement is needed, avoid calling __dso__improve_id().
B. Patch 07/10: Unnecessary lock churn and array sort invalidation in vdso.c

In tools/perf/util/vdso.c:

static struct dso *__machine__addnew_vdso(struct machine *machine, const 
char *short_name, const char *long_name)
{
	struct dso *dso;
	dso = dso__new(short_name);
	if (dso != NULL) {
		dsos__add(&machine->dsos, dso);
		dso__set_long_name(dso, long_name, false);
	}
Issue: Calling dsos__add() followed by dso__set_long_name() causes two 
separate down_write(&machine->dsos.lock) acquisitions in immediate 
succession. Worse, dsos__add() inserts the DSO into the array ordered by 
its current short_name. Immediately afterwards, dso__set_long_name() 
mutates the name and explicitly sets dsos->sorted = false; (lines 1528 
in dso.c).
Fix: Set the long name before adding the DSO to dsos (matching the 
pattern used in dsos__findnew_module_dso() in util/dsos.c):

	dso = dso__new(short_name);
	if (dso != NULL) {
		dso__set_long_name(dso, long_name, false);
		dsos__add(&machine->dsos, dso);
	}

This requires only a single write lock acquisition in dsos__add() and 
keeps dsos->sorted = true.

C. Patch 10/10: Unsynchronized concurrent write in maps__find_by_name()

In tools/perf/util/maps.c:

struct map *maps__find_by_name(struct maps *maps, const char *name)
{
...
	while (!done) {
		down_read(maps__lock(maps));
...
		/* Second search sorted array. */
		if (!done && maps__maps_by_name_sorted(maps)) {
			struct map **mapp =
				bsearch(name, maps__maps_by_name(maps), __maps__nr_maps(maps),
					sizeof(*mapp), map__strcmp_name);
			if (mapp) {
				result = map__get(*mapp);
				i = mapp - maps__maps_by_name(maps);
				RC_CHK_ACCESS(maps)->last_search_by_name_idx = i;
			}
			done = true;
		}
		up_read(maps__lock(maps));
Issue: last_search_by_name_idx is written while holding only 
down_read(maps__lock(maps)). Because multiple reader threads execute 
maps__find_by_name() concurrently, this is a concurrent write race 
(which is why last_search_by_name_idx could not be annotated with 
__guarded_by(&lock)).
Consequence: Under TSAN or strict memory models, unsynchronized 
conflicting writes cause data races.
Suggestion: Use WRITE_ONCE()/data_race() on the assignment, or annotate 
last_search_by_name_idx with a brief comment explaining that this is an 
intentional, benign unlocked cache update.

3. Patch Architecture & Organization Findings

Per Documentation/process/submitting-patches.rst ("Separate each logical 
change into a separate patch"):

Patch 07/10 mixes 4 distinct logical changes:

Bug fix: Fixing the unlocked __dsos__add() call in __machine__addnew_vdso().
Performance optimization: Adding the read-lock fast path in 
dsos__findnew_id().
Refactoring: Replacing _dso__data_open_lock and pthread_once with 
DEFINE_MUTEX(dso__data_open_lock).
Annotations: Adding static context analysis macros across dso.c, dso.h, 
dsos.c, dsos.h.
Recommendation: Split this patch into at least 3 patches:
Standalone bug fix for vdso.c with a Fixes: tag (see metadata below).
Standalone performance optimization for dsos__findnew_id().
DSO/DSOs lock annotations and mutex cleanup.

Patch 09/10 bundles three disparate subsystems plus a bug fix:

Combines perf config, intel-tpebs, and perf_env.
Includes a real bug fix: fixing an unlocked read of tpebs_reader_thread 
after releasing tpebs_mtx in tpebs_stop().
Recommendation: Split the intel-tpebs race fix into its own commit with 
a Fixes: tag. Split the remaining annotations into separate commits per 
subsystem (perf config, perf intel-tpebs, and perf env).