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

[Ian Rogers]

On Tue, Oct 13, 2026 at 3:53 PM Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> 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.

Hi Bart,

Which mailing list is objecting to the frequent emails? I'll remove it
and yourself from follow ups as the intended recipients are on
linux-perf-users except for patch 1, which fixes a build issue in a
file copied into the tools directory. The LLM feedback included was
flawed and introduces feature creep.

Thanks,
Ian

> 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).
>
>
>