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;
- 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. */
+ 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-googOn 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().
> - 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.
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
>
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
> >
On Mon, Nov 17, 2025 at 08:36:34PM -0800, Ian Rogers wrote:
> 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>
> > > ---
[SNIP]
> > > - 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.
What do you mean by global utime/stime?
>
> > > - 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.
Yep, I think it's ok.
Thanks,
Namhyung
© 2016 - 2026 Red Hat, Inc.