Re: [PATCH v1 2/3] perf stat-shadow: Read tool events directly

[Namhyung Kim <namhyung@kernel.org>]

Hi Ian,

On Mon, Nov 03, 2025 at 09:34:48PM -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 | 123 +++++++++++++---------------------
>  1 file changed, 45 insertions(+), 78 deletions(-)
> 
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index abaf6b579bfc..9fae3d32a519 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -371,6 +371,18 @@ static void print_nsecs(struct perf_stat_config *config,
>  	}
>  }
>  
> +static double tool_pmu__scale_for_metric(const struct evsel *evsel)
> +{
> +	enum tool_pmu_event event = evsel__tool_event(evsel);
> +
> +	if (event == TOOL_PMU__EVENT_DURATION_TIME ||
> +	    event == TOOL_PMU__EVENT_USER_TIME ||
> +	    event == TOOL_PMU__EVENT_SYSTEM_TIME)
> +		return 1e-9; /* Scale nanoseconds to seconds. */
> +
> +	return 1.0;
> +}
> +
>  static int prepare_metric(const struct metric_expr *mexp,
>  			  const struct evsel *evsel,
>  			  struct expr_parse_ctx *pctx,
> @@ -382,90 +394,45 @@ static int prepare_metric(const struct metric_expr *mexp,
>  
>  	for (i = 0; metric_events[i]; i++) {
>  		char *n;
> -		double val;
> +		double val, scale = tool_pmu__scale_for_metric(metric_events[i]);
>  		int source_count = 0;
> +		struct perf_stat_evsel *ps = metric_events[i]->stats;
> +		struct perf_stat_aggr *aggr;
>  
> -		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;
> -				break;
> -			case TOOL_PMU__EVENT_SYSTEM_TIME:
> -				stats = &ru_stats.ru_stime_usec_stat;
> -				scale = 1e-6;

Do {USER,SYSTEM}_TIME become nanosecond now?

Thanks,
Namhyung

> +		/*
> +		 * 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_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:
> -			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;
> -
> +		}
> +		aggr = &ps->aggr[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 * scale;
> +			if (!source_count)
> +				source_count = evsel__source_count(metric_events[i]);
>  		}
>  		n = strdup(evsel__metric_id(metric_events[i]));
>  		if (!n)
> -- 
> 2.51.2.1006.ga50a493c49-goog
>