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

On Fri, Jul 31, 2026 at 4:14=E2=80=AFPM Namhyung Kim <[email protected]> =
wrote:
>
> On Wed, Jul 29, 2026 at 05:17:41PM +0000, [email protected] wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential is=
sue(s) to consider:
> > - [Medium] Flawed line-split transition logic causes spurious empty JSO=
N 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 <[email protected]>
> >
> > perf stat: Include PMU name and split uncore events per PMU in metric-o=
nly 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 JSO=
N
> > output and starting a new JSON line when the underlying PMU instance ch=
anges.
> >
> > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-di=
splay.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.c=
pu =3D=3D cpu.cpu)
> > >                                     break;
> > >                     }
> > > +                   if (aggr_idx >=3D config->aggr_map->nr)
> > > +                           continue;
> > >
> > >                     os->evsel =3D counter;
> > >                     os->id =3D aggr_cpu_id__cpu(cpu, /*data=3D*/NULL)=
;
> > > -                   if (first) {
> > > -                           print_metric_begin(config, evlist, os, ag=
gr_idx);
> > > -                           first =3D false;
> > > +
> > > +                   if (config->metric_only) {
> > > +                           struct perf_pmu *pmu =3D counter->pmu;
> > > +
> > > +                           if (!evsel__is_tool(counter)) {
> > > +                                   if (config->json_output && line_o=
pen &&
> > > +                                       (!last_pmu || pmu !=3D last_p=
mu)) {
> >
> > [Severity: Medium]
> > Does this line-split transition logic handle tool events correctly depe=
nding
> > on their order in the event list?
> >
> > If a tool event is processed first, line_open becomes true, but last_pm=
u
> > remains NULL. When the next event (a PMU event) is processed, the condi=
tion
> > (!last_pmu || pmu !=3D last_pmu) evaluates to true. This prematurely cl=
oses
> > 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

Thanks for the test, I sent the v4 patch for fixing this problem:
lore.kernel.org/[email protected]

Test:
  $ perf stat -aA -e user_time -M insn_per_cycle -j --metric-only true
  {"cpu" : "0", "instructions  insn_per_cycle" : "0.9"}
  {"cpu" : "1", "instructions  insn_per_cycle" : "0.6"}
  {"cpu" : "2", "instructions  insn_per_cycle" : "0.2"}
  {"cpu" : "3", "instructions  insn_per_cycle" : "0.1"}
  {"cpu" : "4", "instructions  insn_per_cycle" : "0.2"}
  {"cpu" : "5", "instructions  insn_per_cycle" : "0.1"}
  {"cpu" : "6", "instructions  insn_per_cycle" : "0.1"}
  {"cpu" : "7", "instructions  insn_per_cycle" : "0.1"}
  {"cpu" : "8", "instructions  insn_per_cycle" : "0.1"}
  {"cpu" : "9", "instructions  insn_per_cycle" : "0.1"}
  ...

-CT
>
> >
> > 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 bypa=
sses
> > the line-splitting check entirely, causing the tool event to silently m=
erge
> > into the preceding PMU's JSON object and incorrectly inherit its PMU
> > attribution.
> >
> > > +                                           print_metric_end(config, =
os);
> > > +                                           line_open =3D false;
> > > +                                   }
> > > +                                   if (!line_open) {
> > > +                                           print_metric_begin(config=
, evlist, os,
> > > +                                                              aggr_i=
dx, counter);
> > > +                                           line_open =3D true;
> > > +                                   }
> > > +                                   last_pmu =3D pmu;
> > > +                           } else if (!line_open) {
> > > +                                   print_metric_begin(config, evlist=
, os,
> > > +                                                      aggr_idx, coun=
ter);
> > > +                                   line_open =3D true;
> > > +                           }
> > >                     }
> >
> > --
> > Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260729170416.=
[email protected]?part=3D1