Re: [PATCH v2 02/14] perf pmu-events: Add API to get metric table name and iterate tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pmu_metrics_table__for_each_table iterates over CPU mappings rather than unique metric tables, resulting in duplicate callback invocations for identical tables.
--

commit 06009cc5aa38d92e8977305257f6dc0825137ac8
Author: Ian Rogers <irogers@google.com>

perf pmu-events: Add API to get metric table name and iterate tables

This patch adds a name field to struct pmu_metrics_table and functions to
retrieve the name and iterate over all known metric tables. This functionality
is intended to break apart slow metric tests per table.

> diff --git a/tools/perf/pmu-events/empty-pmu-events.c b/tools/perf/pmu-events/empty-pmu-events.c
> index 628e488ecc73c..718a541bd940f 100644
> --- a/tools/perf/pmu-events/empty-pmu-events.c
> +++ b/tools/perf/pmu-events/empty-pmu-events.c
[ ... ]
> @@ -5988,6 +5991,35 @@ int pmu_for_each_sys_metric(pmu_metric_iter_fn fn, void *data)
>  	return 0;
>  }
>  
> +const char *pmu_metrics_table__name(const struct pmu_metrics_table *table)
> +{
> +	return table ? table->name : NULL;
> +}
> +
> +int pmu_metrics_table__for_each_table(
> +	int (*fn)(const struct pmu_metrics_table *table, void *data),
> +	void *data)
> +{
> +	size_t i;
> +	int ret;
> +
> +	for (i = 0; pmu_events_map[i].cpuid; i++) {
> +		if (!pmu_events_map[i].metric_table.pmus)
> +			continue;
> +		ret = fn(&pmu_events_map[i].metric_table, data);

[Severity: Medium]
Since multiple CPU IDs can map to the same architecture metrics directory
(for example, multiple ARM cores mapped to cortex-a53), pmu_events_map can
contain multiple elements that share the identical underlying metrics array and
name.

Will this loop invoke the callback redundantly for these identical tables,
potentially causing identical tests to be generated and run multiple times?

> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 356ac1324be05..0a6db29b8f0ff 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
[ ... ]
> @@ -807,12 +813,45 @@ static const struct pmu_events_map pmu_events_map[] = {
>  	.arch = 0,
>  	.cpuid = 0,
>  	.event_table = { 0, 0 },
> -	.metric_table = { 0, 0 },
> +	.metric_table = { 0 },
>  }
>  };
>  """)
>  
>  
> +def print_metric_table_functions() -> None:
> +  _args.output_file.write("""
> +const char *pmu_metrics_table__name(const struct pmu_metrics_table *table)
> +{
> +	return table ? table->name : NULL;
> +}
> +
> +int pmu_metrics_table__for_each_table(
> +	int (*fn)(const struct pmu_metrics_table *table, void *data),
> +	void *data)
> +{
> +	size_t i;
> +	int ret;
> +
> +	for (i = 0; pmu_events_map[i].cpuid; i++) {
> +		if (!pmu_events_map[i].metric_table.pmus)
> +			continue;
> +		ret = fn(&pmu_events_map[i].metric_table, data);

[Severity: Medium]
This python generator creates the same pmu_metrics_table__for_each_table()
implementation shown above. Might it be beneficial to include deduplication
logic here to prevent identical metric tables from being passed to the
callback?

> +		if (ret)
> +			return ret;
> +	}
[ ... ]

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