Re: [PATCH 1/2] perf stat: Add aggr_nr metric parser support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incorrect `aggr_nr` evaluation when an event fails to run (`aggr->counts.run == 0`).
--

commit 7b8eacc795788823a2f1b5c16d96f2fc07c9fe23
Author: Chun-Tse Shao <ctshao@google.com>

perf stat: Add aggr_nr metric parser support

This commit introduces the aggr_nr function to the metric expression parser
to allow metric formulas to dynamically utilize the number of aggregated
targets. It updates the lexer, parser, and expression context helpers to
store and provide the aggregation count.

> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -53,6 +53,7 @@ static int prepare_metric(struct perf_stat_config *config,
>  
>  	for (i = 0; metric_events[i]; i++) {
>  		int source_count = 0, tool_aggr_idx;
> +		int aggr_nr = 1;
>  		bool is_tool_time =
>  			tool_pmu__is_time_event(config, metric_events[i], &tool_aggr_idx);
>  		struct perf_stat_evsel *ps = metric_events[i]->stats;

[ ... ]

>  		} else {
>  			struct perf_stat_aggr *aggr =
>  				&ps->aggr[is_tool_time ? tool_aggr_idx : aggr_idx];
>  
>  			if (aggr->counts.run == 0) {
>  				val = NAN;
>  				source_count = 0;

[Severity: Medium]
Does this leave aggr_nr at its default initialized value of 1 when an event
fails to run?

When aggr->counts.run == 0, source_count is explicitly set to 0. Since
aggr_nr skips evaluation in this branch, any metric expression utilizing
the aggr_nr function on an event that failed to run will evaluate to 1
instead of the correct aggregation topology size.

Should aggr_nr be set to 0 here to mirror source_count, or unconditionally
populated with aggr->nr?

>  			} else {
>  				val = aggr->counts.val;
>  				if (is_tool_time) {
>  					/* Convert time event nanoseconds to seconds. */
>  					val *= 1e-9;
>  				}
>  				if (!source_count)
>  					source_count = evsel__source_count(metric_events[i]);
> +				aggr_nr = aggr->nr ?: 1;
>  			}
>  		}
>  		n = strdup(evsel__metric_id(metric_events[i]));
>  		if (!n)
>  			return -ENOMEM;
>  
> -		expr__add_id_val_source_count(pctx, n, val, source_count);
> +		expr__add_id_val_source_count_aggr_nr(pctx, n, val, source_count, aggr_nr);

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