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

[sashiko-bot@kernel.org]

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027170415.2815859-1-irogers@google.com?part=4