Re: [PATCH v4 04/10] perf stat-shadow: Read tool events directly

[Ian Rogers <irogers@google.com>]

On Mon, Nov 17, 2025 at 6:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Nov 13, 2025 at 10:05:10AM -0800, Ian Rogers wrote:
> > When reading time values for metrics don't use the globals updated in
> > builtin-stat, just read the events as regular events. The only
> > exception is for time events where nanoseconds need converting to
> > seconds as metrics assume time metrics are in seconds.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/stat-shadow.c | 149 +++++++++++++++-------------------
> >  1 file changed, 66 insertions(+), 83 deletions(-)
> >
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index b3b482e1808f..6c1ad78604e1 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -26,7 +26,32 @@ void perf_stat__reset_shadow_stats(void)
> >       memset(&ru_stats, 0, sizeof(ru_stats));
> >  }
> >
> > -static int prepare_metric(const struct metric_expr *mexp,
> > +static bool tool_pmu__is_time_event(const struct perf_stat_config *config,
> > +                                const struct evsel *evsel, int *tool_aggr_idx)
> > +{
> > +     enum tool_pmu_event event = evsel__tool_event(evsel);
> > +     int aggr_idx;
> > +
> > +     if (event != TOOL_PMU__EVENT_DURATION_TIME &&
> > +         event != TOOL_PMU__EVENT_USER_TIME &&
> > +         event != TOOL_PMU__EVENT_SYSTEM_TIME)
> > +             return false;
> > +
> > +     if (config) {
> > +             cpu_aggr_map__for_each_idx(aggr_idx, config->aggr_map) {
> > +                     if (config->aggr_map->map[aggr_idx].cpu.cpu == 0) {
> > +                             *tool_aggr_idx = aggr_idx;
> > +                             return true;
> > +                     }
> > +             }
> > +             pr_debug("Unexpected CPU0 missing in aggregation for tool event.\n");
> > +     }
> > +     *tool_aggr_idx = 0; /* Assume the first aggregation index works. */
> > +     return true;
> > +}
> > +
> > +static int prepare_metric(struct perf_stat_config *config,
> > +                       const struct metric_expr *mexp,
> >                         const struct evsel *evsel,
> >                         struct expr_parse_ctx *pctx,
> >                         int aggr_idx)
> > @@ -36,93 +61,51 @@ static int prepare_metric(const struct metric_expr *mexp,
> >       int i;
> >
> >       for (i = 0; metric_events[i]; i++) {
> > +             int source_count = 0, tool_aggr_idx;
> > +             bool is_tool_time =
> > +                     tool_pmu__is_time_event(config, metric_events[i], &tool_aggr_idx);
> > +             struct perf_stat_evsel *ps = metric_events[i]->stats;
> > +             struct perf_stat_aggr *aggr;
> >               char *n;
> >               double val;
> > -             int source_count = 0;
> >
> > -             if (evsel__is_tool(metric_events[i])) {
> > -                     struct stats *stats;
> > -                     double scale;
> > -
> > -                     switch (evsel__tool_event(metric_events[i])) {
> > -                     case TOOL_PMU__EVENT_DURATION_TIME:
> > -                             stats = &walltime_nsecs_stats;
> > -                             scale = 1e-9;
> > -                             break;
> > -                     case TOOL_PMU__EVENT_USER_TIME:
> > -                             stats = &ru_stats.ru_utime_usec_stat;
> > -                             scale = 1e-6;
> > +             /*
> > +              * If there are multiple uncore PMUs and we're not reading the
> > +              * leader's stats, determine the stats for the appropriate
> > +              * uncore PMU.
> > +              */
> > +             if (evsel && evsel->metric_leader &&
> > +                 evsel->pmu != evsel->metric_leader->pmu &&
> > +                 mexp->metric_events[i]->pmu == evsel->metric_leader->pmu) {
> > +                     struct evsel *pos;
> > +
> > +                     evlist__for_each_entry(evsel->evlist, pos) {
> > +                             if (pos->pmu != evsel->pmu)
> > +                                     continue;
> > +                             if (pos->metric_leader != mexp->metric_events[i])
> > +                                     continue;
> > +                             ps = pos->stats;
> > +                             source_count = 1;
> >                               break;
> > -                     case TOOL_PMU__EVENT_SYSTEM_TIME:
> > -                             stats = &ru_stats.ru_stime_usec_stat;
> > -                             scale = 1e-6;
> > -                             break;
>
> I think this was broken as it seems to be converted to nsec in the
> update_rusage_stats().

The global utime/stime is in usecs as in the name. In the tool_pmu it
is converted to nanoseconds, for consistency with duration_time. In
the new code the counters are all in nanoseconds and so the same
scaling factor can be applied.

> > -                     case TOOL_PMU__EVENT_NONE:
> > -                             pr_err("Invalid tool event 'none'");
> > -                             abort();
> > -                     case TOOL_PMU__EVENT_MAX:
> > -                             pr_err("Invalid tool event 'max'");
> > -                             abort();
> > -                     case TOOL_PMU__EVENT_HAS_PMEM:
> > -                     case TOOL_PMU__EVENT_NUM_CORES:
> > -                     case TOOL_PMU__EVENT_NUM_CPUS:
> > -                     case TOOL_PMU__EVENT_NUM_CPUS_ONLINE:
> > -                     case TOOL_PMU__EVENT_NUM_DIES:
> > -                     case TOOL_PMU__EVENT_NUM_PACKAGES:
> > -                     case TOOL_PMU__EVENT_SLOTS:
> > -                     case TOOL_PMU__EVENT_SMT_ON:
> > -                     case TOOL_PMU__EVENT_SYSTEM_TSC_FREQ:
> > -                     case TOOL_PMU__EVENT_CORE_WIDE:
> > -                     case TOOL_PMU__EVENT_TARGET_CPU:
> > -                     default:
> > -                             pr_err("Unexpected tool event '%s'", evsel__name(metric_events[i]));
> > -                             abort();
> >                       }
> > -                     val = avg_stats(stats) * scale;
> > -                     source_count = 1;
> > -             } else {
> > -                     struct perf_stat_evsel *ps = metric_events[i]->stats;
> > -                     struct perf_stat_aggr *aggr;
> > -
> > +             }
> > +             /* Time events are always on CPU0, the first aggregation index. */
> > +             aggr = &ps->aggr[is_tool_time ? tool_aggr_idx : aggr_idx];
> > +             if (!aggr || !metric_events[i]->supported) {
> >                       /*
> > -                      * If there are multiple uncore PMUs and we're not
> > -                      * reading the leader's stats, determine the stats for
> > -                      * the appropriate uncore PMU.
> > +                      * Not supported events will have a count of 0, which
> > +                      * can be confusing in a metric. Explicitly set the
> > +                      * value to NAN. Not counted events (enable time of 0)
> > +                      * are read as 0.
> >                        */
> > -                     if (evsel && evsel->metric_leader &&
> > -                         evsel->pmu != evsel->metric_leader->pmu &&
> > -                         mexp->metric_events[i]->pmu == evsel->metric_leader->pmu) {
> > -                             struct evsel *pos;
> > -
> > -                             evlist__for_each_entry(evsel->evlist, pos) {
> > -                                     if (pos->pmu != evsel->pmu)
> > -                                             continue;
> > -                                     if (pos->metric_leader != mexp->metric_events[i])
> > -                                             continue;
> > -                                     ps = pos->stats;
> > -                                     source_count = 1;
> > -                                     break;
> > -                             }
> > -                     }
> > -                     aggr = &ps->aggr[aggr_idx];
> > -                     if (!aggr)
> > -                             break;
> > -
> > -                     if (!metric_events[i]->supported) {
> > -                             /*
> > -                              * Not supported events will have a count of 0,
> > -                              * which can be confusing in a
> > -                              * metric. Explicitly set the value to NAN. Not
> > -                              * counted events (enable time of 0) are read as
> > -                              * 0.
> > -                              */
> > -                             val = NAN;
> > -                             source_count = 0;
> > -                     } else {
> > -                             val = aggr->counts.val;
> > -                             if (!source_count)
> > -                                     source_count = evsel__source_count(metric_events[i]);
> > -                     }
> > +                     val = NAN;
> > +                     source_count = 0;
> > +             } else {
> > +                     val = aggr->counts.val;
> > +                     if (is_tool_time)
> > +                             val *= 1e-9; /* Convert time event nanoseconds to seconds. */
>
> And this code treats all time events are in nsec now.

That's correct: user_time, system_time and duration_time are all
counters that give a count in nanoseconds. However, in the metrics, I
guess as the counts are doubles, the time has always been in whole
seconds.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > +                     if (!source_count)
> > +                             source_count = evsel__source_count(metric_events[i]);
> >               }
> >               n = strdup(evsel__metric_id(metric_events[i]));
> >               if (!n)
> > @@ -168,7 +151,7 @@ static void generic_metric(struct perf_stat_config *config,
> >               pctx->sctx.user_requested_cpu_list = strdup(config->user_requested_cpu_list);
> >       pctx->sctx.runtime = runtime;
> >       pctx->sctx.system_wide = config->system_wide;
> > -     i = prepare_metric(mexp, evsel, pctx, aggr_idx);
> > +     i = prepare_metric(config, mexp, evsel, pctx, aggr_idx);
> >       if (i < 0) {
> >               expr__ctx_free(pctx);
> >               return;
> > @@ -229,7 +212,7 @@ double test_generic_metric(struct metric_expr *mexp, int aggr_idx)
> >       if (!pctx)
> >               return NAN;
> >
> > -     if (prepare_metric(mexp, /*evsel=*/NULL, pctx, aggr_idx) < 0)
> > +     if (prepare_metric(/*config=*/NULL, mexp, /*evsel=*/NULL, pctx, aggr_idx) < 0)
> >               goto out;
> >
> >       if (expr__parse(&ratio, pctx, mexp->metric_expr))
> > --
> > 2.51.2.1041.gc1ab5b90ca-goog
> >