Re: [PATCH v1 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 2 potential issue(s) to consider:
- [Medium] The `.name` field in `pmu_metrics_table` is erroneously initialized to the string literal "NULL" instead of a null pointer for CPUs without metrics.
- [Medium] The `pmu_metrics_table__for_each_table()` API yields duplicate metric tables when multiple CPU IDs share the same architecture.
--

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

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

This commit adds a name field to struct pmu_metrics_table and populates it
in generated tables. It introduces functions to retrieve the table name and
to iterate over all known metric tables.

> diff --git a/tools/perf/pmu-events/empty-pmu-events.c b/tools/perf/pmu-events/empty-pmu-events.c
> index 322e6c2d58cb0..f51062ef25d3f 100644
> --- a/tools/perf/pmu-events/empty-pmu-events.c
> +++ b/tools/perf/pmu-events/empty-pmu-events.c

[ ... ]

> @@ -5992,6 +5995,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);

Are duplicate metric tables yielded here when multiple CPU IDs share the same
architecture?

In architectures like ARM64 and x86, multiple CPU mappings can point to the
exact same metric definitions. Because struct pmu_metrics_table is embedded
directly into each element of the pmu_events_map array, each duplicate CPU
mapping yields a unique memory address (&pmu_events_map[i].metric_table)
while containing identical .pmus pointers.

As this makes trivial pointer deduplication impossible for callers, consumers
of this API could generate redundant operations for identical tables.

[ ... ]

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 7d325604f04db..a74c5e828bd15 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py

[ ... ]

> @@ -796,6 +799,7 @@ static const struct pmu_events_map pmu_events_map[] = {
>  		.num_pmus = {event_size}
>  	}},
>  	.metric_table = {{
> +		.name = "{metric_tblname.replace('pmu_metrics__', '')}",
>  		.pmus = {metric_tblname},
>  		.num_pmus = {metric_size}
>  	}}

If metric_tblname is the string 'NULL' for architectures that have mapped
events but lack an associated metric table, will this evaluate to
.name = "NULL"?

This writes a 4-character string literal into the generated C code instead
of a true null pointer. Callers attempting to perform null checks like
if (table->name) would then erroneously evaluate to true.

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