Re: [PATCH v1 4/4] libperf evlist: Recompute CPU map when an evsel goes system wide

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Oct 27, 2026 at 05:15:19PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] libperf evlist: stale CPUs left in evlist->all_cpus by perf_evlist__go_system_wide()
> --
> 
> commit 03b9925431ed73d3588b37d9c3ef20e7a5b73d9c
> Author: Ian Rogers <irogers@google.com>
> 
> libperf evlist: Recompute CPU map when an evsel goes system wide
> 
> This patch drops the existing evsel->cpus map in
> perf_evlist__go_system_wide() before calling __perf_evlist__propagate_maps()
> so that an evsel added after map creation gets its PMU's CPUs instead of
> retaining the user-requested CPUs.
> 
> > diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> > index 1f210dadd666f..c48425d4933dc 100644
> > --- a/tools/lib/perf/evlist.c
> > +++ b/tools/lib/perf/evlist.c
> > @@ -834,7 +834,10 @@ void perf_evlist__go_system_wide(struct perf_evlist *evlist, struct perf_evsel *
> >  {
> >  	if (!evsel->system_wide) {
> >  		evsel->system_wide = true;
> > -		if (evlist->needs_map_propagation)
> > +		if (evlist->needs_map_propagation) {
> > +			perf_cpu_map__put(evsel->cpus);
> > +			evsel->cpus = NULL;
> >  			__perf_evlist__propagate_maps(evlist, evsel);
> > +		}
> 
> [Severity: Medium]
> Does calling __perf_evlist__propagate_maps() on only this evsel leave stale
> CPUs in evlist->all_cpus?
> 
> When an evsel is added via perf_evlist__add() after the evlist's maps were
> created in per-thread or workload mode, __perf_evlist__propagate_maps() sets
> evsel->cpus to the user-requested {-1} map and merges {-1} into
> evlist->all_cpus:
> 
> tools/lib/perf/evlist.c:__perf_evlist__propagate_maps() {
> 	...
> 	perf_cpu_map__merge(&evlist->all_cpus, evsel->cpus);
> }
> 
> When perf_evlist__go_system_wide() subsequently drops evsel->cpus and calls
> __perf_evlist__propagate_maps(evlist, evsel) again, the PMU CPUs are merged
> into evlist->all_cpus, but the old -1 entry is never removed. In contrast,
> perf_evlist__propagate_maps() clears and rebuilds evlist->all_cpus across all
> evsels:
> 
> tools/lib/perf/evlist.c:perf_evlist__propagate_maps() {
> 	evlist->needs_map_propagation = true;
> 
> 	/* Clear the all_cpus set which will be merged into during propagation. */
> 	perf_cpu_map__put(evlist->all_cpus);
> 	evlist->all_cpus = NULL;
> 	...
> }
> 
> If no remaining evsel in the evlist uses CPU -1 (for instance, this evsel is
> the only event, or all other events are system-wide or have requires_cpu set),
> evlist->all_cpus retains the stale -1 entry. When perf_evlist__mmap_ops()
> runs, perf_cpu_map__has_any_cpu_or_is_empty(cpus) then evaluates to true and
> selects mmap_per_thread() instead of mmap_per_cpu():
> 
> tools/lib/perf/evlist.c:perf_evlist__mmap_ops() {
> 	...
> 	if (perf_cpu_map__has_any_cpu_or_is_empty(cpus))
> 		return mmap_per_thread(evlist, ops, mp);
> 
> 	return mmap_per_cpu(evlist, ops, mp);
> }
> 
> In mmap_per_thread(), cpu_idx 0 (for CPU -1 in evlist->all_cpus) matches no
> evsel->cpus, so idx 0 is never mmapped, perf_evlist__set_mmap_first() is
> skipped, and nr_mmaps is miscounted.
> 
> Should perf_evlist__go_system_wide() call perf_evlist__propagate_maps(evlist)
> to rebuild evlist->all_cpus instead of only re-propagating the single evsel?

Hi Ian, what do you think about this?

I've applied the first 3 patches to perf-tools-next already.

Thanks,
Namhyung