Re: [PATCH v3 06/18] perf script: Change metric format to use json metrics

[Ian Rogers]

On Mon, Nov 10, 2025 at 10:59 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Mon, Nov 10, 2025 at 08:04:05PM -0800, Ian Rogers wrote:
> > The metric format option isn't properly supported. This change
> > improves that by making the sample events update the counts of an
> > evsel, where the shadow metric code expects to read the values.  To
> > support printing metrics, metrics need to be found. This is done on
> > the first attempt to print a metric. Every metric is parsed and then
> > the evsels in the metric's evlist compared to those in perf script
> > using the perf_event_attr type and config. If the metric matches then
> > it is added for printing. As an event in the perf script's evlist may
> > have >1 metric id, or different leader for aggregation, the first
> > metric matched will be displayed in those cases.
> >
> > An example use is:
> > ```
> > $ perf record -a -e '{instructions,cpu-cycles}:S' -a -- sleep 1
> > $ perf script -F period,metric
> > ...
> >      867817
> >          metric:    0.30  insn per cycle
> >      125394
> >          metric:    0.04  insn per cycle
> >      313516
> >          metric:    0.11  insn per cycle
> >          metric:    1.00  insn per cycle
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> [SNIP]
> > @@ -2150,23 +2296,72 @@ static void perf_sample__fprint_metric(struct perf_script *script,
> >                        },
> >               .force_header = false,
> >       };
> > -     struct evsel *ev2;
> > -     u64 val;
> > +     struct perf_counts_values *count, *old_count;
> > +     int cpu_map_idx, thread_map_idx, aggr_idx;
> > +     struct evsel *pos;
> > +
> > +     if (!init_metrics) {
> > +             /* One time initialization of stat_config and metric data. */
> > +             struct script_find_metrics_args args = {
> > +                     .evlist = evsel->evlist,
> > +                     /* TODO: Determine system-wide based on evlist.. */
> > +                     .system_wide = true,
>
> Probably you can check if the thread_map has an entry for -1.

Thanks, done. In testing this I found that the CPU map index lookup
can fail and so I added the same thread map index workaround of just
aggregating into the first count.

> > +             };
> > +             if (!stat_config.output)
> > +                     stat_config.output = stdout;
> > +
> > +             if (!stat_config.aggr_map) {
> > +                     /* TODO: currently only global aggregation is supported. */
> > +                     assert(stat_config.aggr_mode == AGGR_GLOBAL);
>
> IIUC there's no option or config to set different aggregation mode for
> perf script.

Right. I don't think this patch needs to do everything given the prior
behavior wasn't working. We can add an aggregation mode. It probably
makes more sense to work on this in the context of perf stat report.

Thanks,
Ian