Re: [PATCH v1 19/41] perf metricgroup: Improve matching of metrics to recorded events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] perf metricgroup: duplicate data_evsel in metric_expr__dup_for_data()
- [Medium] perf metricgroup: metric_no_group=true inverts group score in matching
- [Medium] perf metricgroup: all-PMU check and partial mutation break PMU matching
- [Medium] perf metricgroup: wildcard PMU match in counter_key__equal() when pmu is NULL
--

commit 0dda1611167f07ce9d5277e31daf8894a0250f02
Author: Ian Rogers <irogers@google.com>

perf metricgroup: Improve matching of metrics to recorded events

This commit improves how metrics are matched to recorded events in perf.data
by comparing events using a counter key (PMU name, counter encoding, and
exclude modifiers), converting legacy hardware events to sysfs encodings,
duplicating metric expressions per PMU, and scoring candidate events.

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index e87da791fd1e4..90d22c67bfaeb 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c

[ ... ]

> @@ -471,151 +483,581 @@ int metricgroup__for_each_metric(struct perf_pmus *pmus, const struct pmu_metric
> +static void counter_key__init_attr(struct counter_key *key, const char *pmu_name,
> +				   const struct perf_event_attr *attr)
> +{
> +	key->pmu_name = pmu_name;
> +	key->type = attr->type;
> +	key->config = attr->config;
> +	key->config1 = attr->config1;
> +	key->config2 = attr->config2;
> +	key->exclude = (attr->exclude_user ? COUNTER_KEY_EXCLUDE_USER : 0) |
> +		       (attr->exclude_kernel ? COUNTER_KEY_EXCLUDE_KERNEL : 0) |
> +		       (attr->exclude_hv ? COUNTER_KEY_EXCLUDE_HV : 0) |
> +		       (attr->exclude_host ? COUNTER_KEY_EXCLUDE_HOST : 0) |
> +		       (attr->exclude_guest ? COUNTER_KEY_EXCLUDE_GUEST : 0);
> +	if (attr->type == PERF_TYPE_HARDWARE || attr->type == PERF_TYPE_HW_CACHE)
> +		key->config &= PERF_HW_EVENT_MASK;
> +	else if (attr->type >= PERF_TYPE_MAX)
> +		key->type = PERF_TYPE_RAW;
> +}

[ ... ]

> +static bool counter_key__equal(const struct counter_key *a, const struct counter_key *b)
> +{

[ ... ]

> +	if (a->pmu_name && b->pmu_name)
> +		return !strcmp(a->pmu_name, b->pmu_name);
> +
> +	return true;
> +}

[Severity: Medium]
Can an unresolved dynamic PMU or tool PMU event act as a wildcard that
matches metric events from any PMU with the same config values?

When evsel__find_pmu(data_evsel) returns NULL for a recorded event with
attr.type >= PERF_TYPE_MAX (such as PERF_PMU_TYPE_TOOL or a dynamic PMU type
when HEADER_PMU_MAPPINGS in perf.data is absent or incomplete while
env->pmus.read_pmu_types == PERF_TOOL_PMU_TYPE_ALL_MASK),
counter_key__init_attr() sets key->pmu_name = NULL and overwrites key->type
with PERF_TYPE_RAW.

When counter_key__equal() then compares this recorded event against a parsed
metric event from any PMU (which also has key->type == PERF_TYPE_RAW and a
non-NULL pmu_name), the pmu_name comparison is skipped because one pmu_name
is NULL, returning true whenever config, config1, and config2 happen to match.

[ ... ]

> +static struct evsel *map_metric_evsel_to_evlist(struct find_matching_metrics_args *args,
>  						struct evsel *metric_evsel)
>  {
> -	struct evsel *data_evsel;
> +	struct evsel *data_evsel, *best = NULL;
> +	struct counter_key metric_key;
> +	bool metric_grouped = evsel__leader(metric_evsel)->core.nr_members > 1;
> +	int best_score = -1;
> +
> +	evsel__counter_key(args, metric_evsel, &metric_key);
> +	evlist__for_each_entry(args->evlist, data_evsel) {
> +		bool data_grouped;
> +		int score;
>  
> -	evlist__for_each_entry(data_evlist, data_evsel) {
> -		/* Skip if perf_event_attr differ. */
> -		if (metric_evsel->core.attr.type != data_evsel->core.attr.type)
> +		if (!data_evsel->supported)
>  			continue;
> -		if (metric_evsel->core.attr.config != data_evsel->core.attr.config)
> +
> +		/* Skip if the evsels don't program the same counter. */
> +		if (data_evsel->core.idx >= args->nr_keys ||
> +		    !counter_key__equal(&metric_key, &args->keys[data_evsel->core.idx]))
>  			continue;
> -		return data_evsel;
> +
> +		data_grouped = evsel__leader(data_evsel)->core.nr_members > 1 ||
> +			       (data_evsel->core.attr.read_format & PERF_FORMAT_GROUP);
> +		score = (!strcmp(evsel__name(metric_evsel), evsel__name(data_evsel)) ? 16 : 0) +
> +			(metric_key.exclude == args->keys[data_evsel->core.idx].exclude ? 8 : 0) +
> +			evsel__count_score(data_evsel) * 2 +
> +			(metric_grouped == data_grouped ? 1 : 0);
> +		if (score > best_score) {
> +			best = data_evsel;
> +			best_score = score;
> +		}
>  	}
> +	return best;
> +}

[Severity: Medium]
Does passing metric_no_group = true in find_matching_metrics_cb() invert the
(metric_grouped == data_grouped) scoring heuristic here?

In find_matching_metrics_cb(), metricgroup__parse_groups() is called with
metric_no_group set to true, which causes metric__new() to set
m->group_events = false. As a result, the parsed metric_evsel entries in
metric_evlist are not placed in groups, so core.nr_members is 1 and
metric_grouped evaluates to false.

When map_metric_evsel_to_evlist() then scores candidate events,
(metric_grouped == data_grouped ? 1 : 0) evaluates to (!data_grouped ? 1 : 0),
which penalizes grouped data_evsel entries and favors ungrouped ones even for
metrics that normally group their events when recorded.

[ ... ]

> +static struct metric_expr *metric_expr__dup_for_data(struct find_matching_metrics_args *args,
> +						     struct evlist *metric_evlist,
> +						     const struct metric_expr *expr,
> +						     const struct perf_pmu *pmu)
> +{

[ ... ]

> +	while (expr->metric_events[nr])
> +		nr++;
> +	copy->metric_events = calloc(nr + 1, sizeof(*copy->metric_events));
> +	if (!copy->metric_events)
> +		goto err;
> +
> +	for (int i = 0; i < nr; i++) {
> +		struct evsel *ev = metric_evsel_on_pmu(metric_evlist, expr->metric_events[i], pmu);
> +
> +		if (!ev)
> +			goto err;
> +		copy->metric_events[i] = map_metric_evsel_to_evlist(args, ev);
> +		if (!copy->metric_events[i])
> +			goto err;
> +		if (evsel__copy_metric_ids(copy->metric_events[i], expr->metric_events[i]))
> +			goto err;
> +		/*
> +		 * As in setup_metric_events(), a matched event without a metric
> +		 * leader leads itself so that evsel__source_count() counts it.
> +		 */
> +		if (!copy->metric_events[i]->metric_leader)
> +			copy->metric_events[i]->metric_leader = copy->metric_events[i];
> +	}
> +	return copy;
> +err:
> +	metric_expr__delete(copy);
>  	return NULL;
>  }

[Severity: Medium]
What happens when two entries in expr->metric_events[] map to the same
recorded data_evsel in copy->metric_events[]?

For example, in tma_info_system_mux
(cpu_core@CPU_CLK_UNHALTED.THREAD_P@ / cpu_core@CPU_CLK_UNHALTED.THREAD@),
or when a metric references both an unqualified event and a :u event while
the recorded event had exclude_kernel/exclude_hv set by evsel__fallback(),
multiple expr->metric_events[i] resolve to the same data_evsel.

Because copy->metric_events[] is not deduplicated while all metric_ids are
copied onto that shared data_evsel via evsel__copy_metric_ids(),
prepare_metric() in tools/perf/util/stat-shadow.c visits the same data_evsel
multiple times:

tools/perf/util/stat-shadow.c:prepare_metric() {
    ...
		args.val = val;
		args.source_count = source_count;
		args.aggr_nr = aggr_nr;
		ret = evsel__for_each_metric_id(metric_events[i], add_metric_id_val_cb, &args);
    ...
}

On each repeat visit, expr__add_id_val_source_count_aggr_nr() finds old_data
for every metric_id on that data_evsel and accumulates val, source_count, and
aggr_nr again:

tools/perf/util/expr.c:expr__add_id_val_source_count_aggr_nr() {
    ...
	} else if (old_data) {
		data_ptr->val.val += old_data->val.val;
		data_ptr->val.source_count += old_data->val.source_count;
		data_ptr->val.aggr_nr += old_data->val.aggr_nr;
	}
    ...
}

Won't this multiply the event counts, source_count, and aggr_nr by the number
of times data_evsel appears in copy->metric_events[], as well as match
multi-counter comparison metrics against a single recorded counter?

[ ... ]

> @@ -523,99 +965,97 @@ static int find_matching_metrics_cb(const struct pmu_metric *pm,
>  				    const struct pmu_metrics_table *table __maybe_unused,
>  				    void *data)
>  {
>  	struct find_matching_metrics_args *args = data;
> -	struct evlist *data_evlist = args->evlist;
>  	struct evlist *metric_evlist = evlist__new();
>  	struct evsel *metric_evsel;
> -	int ret = metricgroup__parse_groups(metric_evlist,
> +	int ret;
> +
> +	if (!metric_evlist)
> +		return -ENOMEM;
> +
> +	evlist__set_session(metric_evlist, evlist__session(args->evlist));
> +	/*
> +	 * Event grouping doesn't affect the matching and avoids warnings about
> +	 * grouping constraints.
> +	 */
> +	ret = metricgroup__parse_groups(metric_evlist,
>  					/*pmu=*/"all",
>  					/*cputype_filter=*/false,
>  					pm->metric_name,
> -					/*metric_no_group=*/false,
> +					/*metric_no_group=*/true,
>  					/*metric_no_merge=*/false,
>  					/*metric_no_threshold=*/true,
>  					/*user_requested_cpu_list=*/NULL,
>  					args->system_wide,
>  					/*hardware_aware_grouping=*/false);
> -
>  	if (ret) {
>  		/* Metric parsing failed but continue the search. */
> +		ret = 0;
>  		goto out;
>  	}
>  
> -	/*
> -	 * Check the data_evlist has an entry for each metric_evlist entry. If
> -	 * the data evsel was already set up avoid changing data that may
> -	 * break it.
> -	 */
> +	/* Check the data_evlist has an entry for each metric_evlist entry. */
>  	evlist__for_each_entry(metric_evlist, metric_evsel) {
> -		struct evsel *data_evsel =
> -			map_metric_evsel_to_evlist(data_evlist, metric_evsel);
> -		struct evsel *new_metric_leader;
> -
> -		if (!data_evsel) {
> +		if (!map_metric_evsel_to_evlist(args, metric_evsel)) {
>  			pr_debug("Skipping metric '%s' as evsel '%s' is missing\n",
> -				pm->metric_name, evsel__name(metric_evsel));
> -			goto out;
> -		}
> -
> -		if (data_evsel->metric_leader == NULL)
> -			continue;
> -
> -		if (metric_evsel->metric_leader == metric_evsel) {
> -			new_metric_leader = data_evsel;
> -		} else {
> -			new_metric_leader =
> -				map_metric_evsel_to_evlist(data_evlist,
> -								 metric_evsel->metric_leader);
> -		}
> -		/* Mismatching evsel leaders. */
> -		if (data_evsel->metric_leader != new_metric_leader) {
> -			pr_debug("Skipping metric '%s' due to mismatching evsel metric leaders '%s' vs '%s'\n",
> -				pm->metric_name, evsel__name(metric_evsel),
> -				evsel__name(data_evsel));
> +				 pm->metric_name, evsel__name(metric_evsel));
>  			goto out;
>  		}
>  	}
> -	/*
> -	 * Metric events match those in the data evlist, copy metric evsel
> -	 * data into the data evlist.
> -	 */
> -	evlist__for_each_entry(metric_evlist, metric_evsel) {
> -		struct evsel *data_evsel =
> -			map_metric_evsel_to_evlist(data_evlist, metric_evsel);
> -		struct metric_event *metric_me =
> -			metricgroup__lookup(evlist__metric_events(metric_evlist),
> -					    metric_evsel,
> -					    /*create=*/false);
>  
> -		if (evsel__copy_metric_ids(data_evsel, metric_evsel))
> -			goto out;
> +	ret = add_matching_metric_evlist(args, metric_evlist,
> +					 /*default_metricgroup=*/false);

[Severity: Medium]
On a hybrid or multi-PMU system where events were recorded on only one PMU
(for example, only cpu_core on Alder Lake), won't this check skip valid
per-PMU metrics?

Because metricgroup__parse_groups() is called with pmu set to "all" rather
than pm->pmu, default_core metrics (like insn_per_cycle) and metrics sharing
the same name across PMUs (like tma_retiring) expand into evsels for both
cpu_core and cpu_atom in metric_evlist. Requiring every metric_evsel in
metric_evlist to match before calling add_matching_metric_evlist() causes the
entire metric to be skipped when cpu_atom events were not recorded, even
though all cpu_core events for the metric are present.

Also, in metric_expr__dup_for_data(), evsel__copy_metric_ids() and
copy->metric_events[i]->metric_leader are updated inside the loop before
verifying that all nr events in the expression resolve on pmu and match in
args->evlist. If an earlier event in the expression matches and a later event
fails (taking goto err), won't the earlier matched data_evsel be left with
partially mutated metric_ids and metric_leader?

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