Re: [PATCH v2 8/9] perf evsel: Name a merged uncore counter after all of its PMUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf evsel: evlist__uniquify_evsel_names() renames unmerged counters
--

commit 570949b12b356d4f0d3d0c5c530601e5cc91533f
Author: Besar Wicaksono <bwicaksono@nvidia.com>

perf evsel: Name a merged uncore counter after all of its PMUs

This commit renames merged uncore counters to reflect all of their underlying
PMU instances, either by stripping a numeric suffix or by replacing differing
numbers with a wildcard when counters are aggregated.

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index c3d7847278106..6720d9dc5af74 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -2767,7 +2767,7 @@ void evlist__uniquify_evsel_names(struct evlist *evlist, const struct perf_stat_
>  		struct evsel *pos;
>  
>  		evlist__for_each_entry(evlist, pos)
> -			evsel__uniquify_counter(pos);
> +			evsel__uniquify_counter(pos, config->aggr_mode != AGGR_NONE);
>  	}
>  }

[Severity: Medium]
Can passing config->aggr_mode != AGGR_NONE here cause unmerged uncore counters
to be renamed with the merged PMU name in perf stat --no-aggr, perf record,
and perf top?

In tools/perf/util/config.c, the global stat_config is initialized with
aggr_mode = AGGR_GLOBAL.

In tools/perf/builtin-stat.c, cmd_stat() parses command-line options before
updating stat_config.aggr_mode:

	argc = parse_options_subcommand(argc, argv, stat_options, stat_subcommands,
					(const char **) stat_usage,
					PARSE_OPT_STOP_AT_NON_OPTION);

	stat_config.aggr_mode = opt_aggr_mode_to_aggr_mode(&opt_mode);

When -e parses an event group that requires regrouping across PMUs,
__parse_events() in tools/perf/util/parse-events.c calls
evlist__uniquify_evsel_names() while stat_config.aggr_mode is still
AGGR_GLOBAL:

	if (ret2 > 0 && warn_if_reordered && !parse_state.wild_card_pmus) {
		evlist__uniquify_evsel_names(evlist, &stat_config);
		pr_warning("WARNING: events were regrouped to match PMUs\n");

Even when --no-aggr was passed on the command line, this early call sees
config->aggr_mode != AGGR_NONE, renames the first PMU instance to the merged
PMU name, and sets counter->uniquified_name = true in
evsel__uniquify_counter():

	/* The evsel was already uniquified. */
	if (counter->uniquified_name)
		return;

	/* Avoid checking to uniquify twice. */
	counter->uniquified_name = true;

When evlist__print_counters() later calls evlist__uniquify_evsel_names() with
AGGR_NONE, the first PMU instance returns early and keeps the merged wildcard
or suffix-stripped name while the remaining instances are given their
individual PMU names.

Similarly, __cmd_record() in tools/perf/builtin-record.c and __cmd_top() in
tools/perf/builtin-top.c call evlist__uniquify_evsel_names() with the global
&stat_config:

	/*
	 * Use global stat_config that is zero meaning aggr_mode is AGGR_NONE
	 * and hybrid_merge is false.
	 */
	evlist__uniquify_evsel_names(rec->evlist, &stat_config);

Because stat_config.aggr_mode is actually AGGR_GLOBAL rather than AGGR_NONE,
wouldn't perf record and perf top also rename the first uncore PMU instance as
if its counts were merged?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030174816.1544471-1-bwicaksono@nvidia.com?part=8