Re: [PATCH v1 25/58] perf stat-cpi: Port stat-cpi to use python module

sashiko-bot
2026-04-20T00:33:32Z

Sashiko AI review found 3 potential issue(s):
- [Medium] File processing mode computes cumulative CPI instead of interval CPI.
- [Medium] Counter multiplexing scaling is ignored, leading to inaccurate CPI calculations on overcommitted PMUs.
- [Medium] Cartesian product iteration prints incorrect dummy entries for non-existent CPU/thread pairs.
--

commit fd94c65a2036aa00a6bb9e2487402df6f491024c
Author: Ian Rogers <irogers@google.com>

perf stat-cpi: Port stat-cpi to use python module

This commit ports stat-cpi.py from the legacy framework to a standalone
script. It supports both file processing mode and live mode to calculate
and display CPI per interval.

> diff --git a/tools/perf/python/stat-cpi.py b/tools/perf/python/stat-cpi.py
> new file mode 100755
> --- /dev/null
> +++ b/tools/perf/python/stat-cpi.py

[ ... ]

> + def get(self, event: str, cpu: int, thread: int) -> int:
> + """Get counter value."""
> + key = self.get_key(event, cpu, thread)
> + return self.data[key][0] if key in self.data else 0

Since store() saves the tuple (val, ena, run), does this get() method
lead to inaccurate CPI calculations by only extracting the raw count
value?

If the PMU is overcommitted and counters are multiplexed, they might run
for different fractions of the interval. Without scaling the values
(e.g., val * (ena / float(run)) when run > 0), the script appears to
compute CPI using unscaled partial counts.

[ ... ]

> + def process_stat_event(self, event: Any, name: Optional[str] = None) -> None:
> + """Process PERF_RECORD_STAT and PERF_RECORD_STAT_ROUND events."""
> + if event.type == perf.RECORD_STAT:
> + if name:
> + if "cycles" in name:
> + event_name = "cycles"
> + elif "instructions" in name:
> + event_name = "instructions"
> + else:
> + return
> + self.store(event_name, event.cpu, event.thread, (event.val, event.ena, event.run))
> + elif event.type == perf.RECORD_STAT_ROUND:
> + timestamp = getattr(event, "time", 0)
> + self.print_interval(timestamp)
> + self.data.clear()
> + self.cpus.clear()
> + self.threads.clear()

In file processing mode, PERF_RECORD_STAT events contain cumulative,
absolute counter values.

Because self.data.clear() discards the previous interval's state without
subtracting those previous absolute values from the current ones, does this
result in calculating the cumulative CPI from the start of the trace, rather
than the "per interval" CPI described in the commit message?

[ ... ]

> + def print_interval(self, timestamp: int) -> None:
> + """Print CPI for the current interval."""
> + for cpu in self.cpus:
> + for thread in self.threads:
> + cyc = self.get("cycles", cpu, thread)
> + ins = self.get("instructions", cpu, thread)
> + cpi = 0.0
> + if ins != 0:
> + cpi = cyc / float(ins)
> + t_sec = timestamp / 1000000000.0
> + print(f"{t_sec:15f}: cpu {cpu}, thread {thread} -> cpi {cpi:f} ({cyc}/{ins})")

Since self.cpus and self.threads are independently populated as lists of all
unique CPUs and threads seen across the interval, does this nested loop create
a Cartesian product?

If some CPU/thread pairs fail to read (for example, if OSError is caught in
read_counters()), it looks like this will query get() for pairs that were
never recorded, polluting the output with zeroed dummy entries.

Would storing and iterating over recorded (cpu, thread) tuples prevent this?

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