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

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Jul 29, 2026 at 05:17:41PM +0000, sashiko-bot@kernel.org wrote:
> 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.

Something like this?

  $ sudo ./perf stat -aA -e user_time -M insn_per_cycle -j --metric-only true
  {"cpu" : "0", "pmu" : "tool"}
  {"cpu" : "0", "pmu" : "cpu_core", "instructions  insn_per_cycle" : "0.8"}
  {"cpu" : "1", "pmu" : "tool"}
  {"cpu" : "1", "pmu" : "cpu_core", "instructions  insn_per_cycle" : "0.1"}
  {"cpu" : "2", "pmu" : "tool"}
  {"cpu" : "2", "pmu" : "cpu_core", "instructions  insn_per_cycle" : "0.5"}
  {"cpu" : "3", "pmu" : "tool"}
  {"cpu" : "3", "pmu" : "cpu_core", "instructions  insn_per_cycle" : "0.3"}
  ...

Thanks,
Namhyung

> 
> 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