Re: [PATCH v2 23/30] perf metricgroup: Share metric events by their counter encoding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] perf metricgroup: counter_key__equal() ignores evsel CPU mask
--

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

perf metricgroup: Share metric events by their counter encoding

This commit replaces identifier-name metric event sharing with an evsel
matcher that compares counter encodings, modifiers, and cgroups so metrics
can share equivalent events across expressions and with events already in
the evlist.

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index f25709750c3e1..f4a2b280091cd 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c

[ ... ]

> @@ -471,6 +479,512 @@ int metricgroup__for_each_metric(struct perf_pmus *pmus, const struct pmu_metric
>  	return pmu_for_each_sys_metric(metricgroup__sys_event_iter, &sys_data);
>  }
>  
> +/*
> + * A key identifying the counter an evsel programs, used to find evsels counting
> + * the same thing, such as when metrics share events.
> + */
> +struct counter_key {
> +	/** @pmu_name: Name of the evsel's PMU, or NULL if unknown. */
> +	const char *pmu_name;
> +	/** @cgrp: The cgroup the evsel counts. */
> +	const struct cgroup *cgrp;

[ ... ]

> +static void evsel_matcher__key(struct evsel_matcher *matcher, struct evsel *evsel,
> +			       struct counter_key *key)
> +{
> +	struct perf_pmu *pmu = evsel__find_pmu(evsel);
> +
> +	counter_key__init_attr(key, pmu ? pmu->name : NULL, &evsel->core.attr);
> +	key->cgrp = evsel->cgrp;
> +	if (evsel->percore)
> +		key->modifiers |= COUNTER_KEY_MOD_PERCORE;
> +	if (evsel__is_retire_lat(evsel))
> +		key->modifiers |= COUNTER_KEY_MOD_RETIRE_LAT;
> +	if (evsel->precise_max)
> +		key->modifiers |= COUNTER_KEY_MOD_PRECISE_MAX;
> +	if (evsel->bpf_counter)
> +		key->modifiers |= COUNTER_KEY_MOD_BPF;
> +	if (key->type == PERF_TYPE_HARDWARE && pmu && pmu->is_core)
> +		legacy_hw_key(matcher, pmu->name, key->config, key);
> +}
> +
> +/* Do @a and @b count the same thing? */
> +static bool counter_key__equal(const struct counter_key *a, const struct counter_key *b)
> +{
> +	if (a->type != b->type || a->config != b->config ||
> +	    a->config1 != b->config1 || a->config2 != b->config2 ||
> +	    a->config3 != b->config3 || a->config4 != b->config4)
> +		return false;
> +
> +	if ((a->type == PERF_TYPE_RAW || a->type >= PERF_TYPE_MAX) &&
> +	    !a->pmu_name != !b->pmu_name)
> +		return false;
> +
> +	if (a->pmu_name && b->pmu_name && strcmp(a->pmu_name, b->pmu_name))
> +		return false;
> +
> +	return a->exclude == b->exclude && a->modifiers == b->modifiers &&
> +	       a->precise_ip == b->precise_ip && a->cgrp == b->cgrp;
> +}

[Severity: Medium]
Should counter_key also compare evsel->core.cpus (for example with
perf_cpu_map__equal())?

On Intel hybrid platforms, JSON metrics specify per-PMU CPU restrictions
on shared PMUs, such as msr/tsc,cpu=cpu_core/, msr/tsc,cpu=cpu_atom/, and
unrestricted msr/tsc/.  Because parse_events_add_pmu() stores the cpu=
term in evsel->core.cpus without changing perf_event_attr or pmu_name,
counter_key__equal() treats these events as identical and
metric__adopt_events() merges them into whichever evsel was added first,
causing the second metric to read counts from the wrong CPU subset.

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