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

sashiko-bot
2026-04-25T18:07:54Z

Sashiko AI review found 1 potential issue(s):
- [High] In live mode, `store()` incorrectly subtracts previous values from `counts` that are already computed as deltas, resulting in invalid (often negative) counter and multiplexing scaling values.
--

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

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

This patch ports stat-cpi.py from the legacy framework to a standalone script
that supports both file processing and live mode. It accumulates cycles and
instructions to calculate and display Cycles Per Instruction per interval per
CPU or thread.

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

[ ... ]

> + def store(self, event: str, cpu: int, thread: int, counts: tuple[int, int, int]) -> None:
> + """Store counter values, computing difference from previous absolute values."""
> + self.store_key(cpu, thread)
> + key = self.get_key(event, cpu, thread)
> +
> + val, ena, run = counts
> + if key in self.prev_data:
> + prev_val, prev_ena, prev_run = self.prev_data[key]
> + cur_val = val - prev_val
> + cur_ena = ena - prev_ena
> + cur_run = run - prev_run

Does this subtraction work correctly for both file and live modes?

In file mode, PERF_RECORD_STAT events provide cumulative, absolute values, so
taking the difference here extracts the interval delta correctly.

However, in live mode, read_counters() calls evsel.read(). The Python perf
extension (pyrf_evsel__read in tools/perf/util/python.c) already computes
and returns the delta since the last read.

If counts already contains the interval delta in live mode, won't subtracting
prev_val here effectively calculate current_delta - previous_delta, leading to
invalid or negative values?

Also, since the run delta is typically constant across intervals, wouldn't
cur_run become near zero and break the multiplexing scaling math in get()?

> + else:
> + cur_val = val
> + cur_ena = ena
> + cur_run = run
> +
> + self.data[key] = (cur_val, cur_ena, cur_run)
> + self.prev_data[key] = counts # Store absolute value for next time

[ ... ]

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