Re: [PATCH v1 20/41] perf metricgroup: Add metricgroup__add_recorded_metrics()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf metricgroup: metricgroup__add_recorded_metrics() hides unmatched events
- [Low] perf metricgroup: inaccurate kernel-doc for metricgroup__has_metric_or_groups()
--

commit 45c4bebdd16b2b622cdd8dcc5c1f189c3de98cfa
Author: Ian Rogers <irogers@google.com>

perf metricgroup: Add metricgroup__add_recorded_metrics()

This commit adds metricgroup__add_matching_metric_groups() and
metricgroup__add_recorded_metrics() to parse metrics or metricgroups from a
recorded perf stat command line (or Default metricgroups) and match them
against the recorded events in the evlist.

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

[ ... ]

> @@ -1060,6 +1062,315 @@ int metricgroup__add_matching_metrics(struct evlist *evlist, bool system_wide)
>  	return ret;
>  }
>  
> +int metricgroup__add_matching_metric_groups(struct evlist *evlist,

[ ... ]

> +void metricgroup__add_recorded_metrics(struct evlist *evlist,
> +				       const char *metrics,
> +				       const char *pmu_filter,
> +				       bool cputype_filter,
> +				       bool metric_no_group,
> +				       bool metric_no_merge,
> +				       bool metric_no_threshold,
> +				       bool all_metrics,
> +				       bool system_wide,
> +				       bool default_metricgroup,
> +				       bool *metric_only)
> +{

[ ... ]

> +	} else if (!has_events && !had_rec_metrics && !rec_null) {
> +		static const char * const default_names[] = {
> +			"Default", "Default2", "Default3", "Default4",
> +		};
> +		struct evsel *evsel;
> +
> +		if (default_metricgroup) {
> +			evlist__for_each_entry(evlist, evsel) {
> +				if (!evsel__match(evsel, HARDWARE, HW_CPU_CYCLES))
> +					evsel->skippable = true;
> +				if (!all_metrics)
> +					evsel->default_metricgroup = true;
> +			}
> +		}
> +		for (size_t i = 0; !all_metrics && i < ARRAY_SIZE(default_names); i++) {
> +			if ((int)i > rec_detailed)
> +				break;
> +			if (!metricgroup__has_metric_or_groups(pmus, pmu_filter ?: "all",
> +							       default_names[i]))
> +				continue;
> +			metricgroup__add_matching_metric_groups(evlist, pmu_filter,
> +								cputype_filter,
> +								default_names[i],
> +								/*metric_no_group=*/false,
> +								/*metric_no_merge=*/false,
> +								/*metric_no_threshold=*/true,
> +								system_wide,
> +								default_metricgroup);
> +		}
> +	}

[Severity: Medium]
When default_metricgroup is true and !all_metrics, setting
evsel->default_metricgroup = true on every evsel in evlist upfront before
matching the Default metricgroups appears to cause unmatched or unsupported
events to be hidden during reporting.

During reporting, print_counter_aggrdata() checks
perf_stat__skip_metric_event() before should_skip_zero_counter():

tools/perf/util/stat-shadow.c:perf_stat__skip_metric_event() {
	if (!evsel->default_metricgroup)
		return false;

	return !metricgroup__lookup(evlist__metric_events(evsel->evlist), evsel, false);
}

Because map_metric_evsel_to_evlist() skips any evsel with
!data_evsel->supported, no unsupported evsel ever gets an entry in
evlist->metric_events. With evsel->default_metricgroup set to true on all
evsels, perf_stat__skip_metric_event() returns true for every unsupported
counter before should_skip_zero_counter() is reached, which makes the
!evsel__match(evsel, HARDWARE, HW_CPU_CYCLES) non-skippable exception and
verbose reporting of unsupported counters unreachable when !all_metrics.

Similarly, if a supported recorded event in evlist does not have a matched
Default metric attached to it (for instance, task-clock when duration_time
was not recorded so CPUs_utilized fails to match, instructions when
cpu-cycles was unsupported, or when the recorded CPUID has no matching
Default metrics), won't perf_stat__skip_metric_event() suppress those
recorded events as well?

Conversely, metricgroup__add_matching_metric_groups() documents that
default_metricgroup marks matched events with Default metricgroup display
flags, but add_matching_metric_evlist() only sets
data_evsel->default_show_events = true and data_me->is_default = true without
setting default_metricgroup = true on the matched evsels. Should
default_metricgroup be set only on the matched events inside
add_matching_metric_evlist(), or only when a Default metricgroup actually
matches?

> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index 25398eafd6e28..7b371171d315f 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -112,6 +112,67 @@ int metricgroup__for_each_metric(struct perf_pmus *pmus, const struct pmu_metric

[ ... ]

> +/**
> + * metricgroup__has_metric_or_groups() - Check if any metric or metricgroup in
> + *                                       @metric_or_groups exists for @pmus.
> + * @pmus: The PMUs to search.
> + * @pmu: PMU filter name or "all".
> + * @metric_or_groups: Comma-separated list of metric or metricgroup names.
> + *
> + * Return: true if all names in @metric_or_groups match a metric or group.
> + */
>  bool metricgroup__has_metric_or_groups(struct perf_pmus *pmus, const char *pmu,
>  				       const char *metric_or_groups);

[Severity: Low]
This isn't a bug in the code, but is this kernel-doc description for
metricgroup__has_metric_or_groups() accurate?

The summary line says "if any metric or metricgroup in @metric_or_groups
exists" while the Return section says "true if all names in @metric_or_groups
match", and @metric_or_groups is described as a comma-separated list.

Looking at metricgroup__has_metric_or_groups() -> match_pm_metric_or_groups()
-> match_metric_or_groups() in tools/perf/util/metricgroup.c:

	len = strlen(sought);
	if (!strncasecmp(metric_or_groups, sought, len) &&
	    (metric_or_groups[len] == 0 || metric_or_groups[len] == ';'))
		return true;
	m = strchr(metric_or_groups, ';');
	return m && match_metric_or_groups(m + 1, sought);

match_metric_or_groups() splits the metric's semicolon-separated
pm->metric_group list on ';' and compares sought (@metric_or_groups) as a
single metric or metricgroup name rather than a comma-separated list. Could
the comment be updated to reflect that @metric_or_groups is a single metric
or metricgroup name?

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