Re: [PATCH v3] perf stat: Include PMU name and split uncore events per PMU in metric-only JSON output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Flawed line-split transition logic causes spurious empty JSON objects and incorrect PMU attribution for tool events.
--

--- Patch [1]: [PATCH v3] perf stat: Include PMU name and split uncore events per PMU in metric-only JSON output ---
commit 20e3cd08fd5e415b458343262ee76bc1f5f6ce85
Author: Chun-Tse Shao <ctshao@google.com>

perf stat: Include PMU name and split uncore events per PMU in metric-only JSON output

This commit modifies perf stat JSON output in --metric-only and -A mode to
group metrics by PMU. It achieves this by adding a "pmu" key to the JSON
output and starting a new JSON line when the underlying PMU instance changes.

> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index b337cc23f4131..6785e2c442bd7 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c

[ ... ]

> @@ -1146,13 +1149,34 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>  				if (config->aggr_map->map[aggr_idx].cpu.cpu == cpu.cpu)
>  					break;
>  			}
> +			if (aggr_idx >= config->aggr_map->nr)
> +				continue;
>  
>  			os->evsel = counter;
>  			os->id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
> -			if (first) {
> -				print_metric_begin(config, evlist, os, aggr_idx);
> -				first = false;
> +
> +			if (config->metric_only) {
> +				struct perf_pmu *pmu = counter->pmu;
> +
> +				if (!evsel__is_tool(counter)) {
> +					if (config->json_output && line_open &&
> +					    (!last_pmu || pmu != last_pmu)) {

[Severity: Medium]
Does this line-split transition logic handle tool events correctly depending
on their order in the event list?

If a tool event is processed first, line_open becomes true, but last_pmu
remains NULL. When the next event (a PMU event) is processed, the condition
(!last_pmu || pmu != last_pmu) evaluates to true. This prematurely closes
the line and creates a spurious empty JSON object for the tool event.

Conversely, if a tool event comes after a PMU event, the else-if block below
is taken because !evsel__is_tool(counter) evaluates to false. This bypasses
the line-splitting check entirely, causing the tool event to silently merge
into the preceding PMU's JSON object and incorrectly inherit its PMU
attribution.

> +						print_metric_end(config, os);
> +						line_open = false;
> +					}
> +					if (!line_open) {
> +						print_metric_begin(config, evlist, os,
> +								   aggr_idx, counter);
> +						line_open = true;
> +					}
> +					last_pmu = pmu;
> +				} else if (!line_open) {
> +					print_metric_begin(config, evlist, os,
> +							   aggr_idx, counter);
> +					line_open = true;
> +				}
>  			}

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