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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Bypassing line initialization and termination for tool events in '--metric-only' mode corrupts JSON, CSV, and standard formatting when those events evaluate to metrics.
--

--- Patch [1]: [PATCH v5] perf stat: Include PMU name and split uncore events per PMU in metric-only JSON output ---
commit f2dfadb52a27c2891ce164d2b06f57924949dd1d
Author: Chun-Tse Shao <[email protected]>

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

This commit updates perf stat JSON output to include the PMU name for uncore
events in metric-only mode and split output lines when transitioning between
PMUs. It bypasses line initialization for tool events to avoid creating
spurious empty JSON objects.

> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c

[ ... ]

> @@ -1146,12 +1151,42 @@ 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;
> +				bool is_tool = evsel__is_tool(counter);
> +
> +				if (!is_tool && pmu) {

[Severity: Medium]
Does bypassing line initialization for tool events corrupt the output
formatting if the tool event evaluates to a metric?

If a tool event has an associated metric (e.g., from a custom metric using
duration_time), it will fail the !is_tool check and skip calling
print_metric_begin().

The event still proceeds to printout() down below:

        uval = val * counter->scale;
        printout(config, os, uval, run, ena, 1.0, aggr_idx);

If printout() outputs the metric directly, won't this lack the necessary JSON
or CSV preamble (such as { "interval"... ) because print_metric_begin() was
skipped?

Furthermore, since line_open remains false, print_metric_end() is also
bypassed at the end of the loop, leaving the JSON object unclosed.

> +					bool pmu_changed = false;
> +
> +					if (line_open && last_pmu) {
> +						bool non_core = !pmu->is_core ||
> +								evsel__is_hybrid(counter) ||
> +								!last_pmu->is_core ||
> +								evsel__is_hybrid(last_evsel);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1