Re: [PATCH] perf stat: Fix uncore metric scaling bug across sockets and nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch codifies a mathematical scaling error across all aggregation modes for uncore metrics by changing `source_count` to `aggr->nr` without removing `#num_packages` from the JSON formulas.
--

commit 1c2296ec7fca6ef588c3a63c6bd68377de82335f
Author: Chun-Tse Shao <ctshao@google.com>

perf stat: Fix uncore metric scaling bug across sockets and nodes

This commit changes how source_count is determined for uncore metrics.
Instead of using the static PMU count via evsel__source_count(), it
dynamically assigns aggr->nr from the metric's leader event to handle
various aggregation modes.

> Before the fix:
>   perf stat -M lpm_miss_lat --per-socket --metric-only -a -j -- sleep 1
>   {"socket" : "S0", "counters" : 28, "ns  lpm_miss_lat_rem" : "163.4", "ns  lpm_miss_lat_loc" : "27.5"}
>   {"socket" : "S1", "counters" : 28, "ns  lpm_miss_lat_rem" : "170.5", "ns  lpm_miss_lat_loc" : "25.5"}
>   perf stat -M lpm_miss_lat --metric-only -a -j -- sleep 1
>   {"ns  lpm_miss_lat_rem" : "90.4", "ns  lpm_miss_lat_loc" : "12.6"}
> `lpm_miss_lat` on global aggr is 0.5x the actual value.

Does this misdiagnose the correct baseline? Existing uncore metrics like
lpm_miss_lat are defined in JSON formulas as dividing by
(source_count(TICKS) * #num_packages).

Before this patch, evsel__source_count() returned the static PMU count per
package (e.g., 28 CHAs). In global aggregation, dividing the global sum of 56
CHAs by (28 * 2) = 56 yields the correct average of ~90.4ns.

The --per-socket mode yielded an incorrectly scaled value because it divided
the sum of 28 CHAs by (28 * 2) = 56 instead of 28.

> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c

[ ... ]

> @@ -103,7 +103,7 @@ static int prepare_metric(struct perf_stat_config *config,
>  					val *= 1e-9;
>  				}
>  				if (!source_count)
> -					source_count = evsel__source_count(metric_events[i]);
> +					source_count = aggr->nr;
>  			}
>  		}

Does changing source_count to aggr->nr break global mode calculations without
updating the JSON metric formulas?

In global mode, aggr->nr dynamically evaluates to 56 instead of 28. Because the
JSON formula still multiplies by #num_packages, the metric divides the global
sum of 56 CHAs by (56 * 2) = 112. This scales the true average incorrectly,
causing the global mode to output the doubled latency value of ~174.5ns.

Similarly, in --per-socket mode, aggr->nr evaluates to 28, and the formula
still divides the sum of 28 CHAs by (28 * 2) = 56, retaining the original
scaling error in per-socket mode.

Should the #num_packages multiplier be removed from the JSON formulas when
making source_count dynamically scale with the aggregation mode?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518231150.1252420-1-ctshao@google.com?part=1