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

sashiko-bot
2026-04-24T17:29:23Z

Sashiko AI review found 2 potential issue(s):
- [Medium] Live mode produces mathematically incorrect 'delta of deltas' because it subtracts previous interval counts even though `evsel.read()` already returns interval deltas.
- [Medium] Loose substring matching for event categorization (`"cycles" in name`) misidentifies unrelated events and overwrites their counts, corrupting metrics.
--

commit c55f90f4c4dcd6e961de218774905ee84760602d
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, calculating
and displaying CPI per interval per CPU/thread.

> diff --git a/tools/perf/python/stat-cpi.py b/tools/perf/python/stat-cpi.py
> new file mode 100755
> index 00000000000000..4b1f1f69c94a63
> --- /dev/null
> +++ b/tools/perf/python/stat-cpi.py
> @@ -0,0 +1,151 @@
[ ... ]
> + 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"

Are there potential issues with this broad substring match? If a perf.data
file contains events like bus-cycles or stalled-cycles-frontend, won't
they also match cycles here and overwrite the true core cycles measurements
in self.data?

> + else:
> + return
> + self.store(event_name, event.cpu, event.thread, (event.val, event.ena, event.run))
[ ... ]
> + def read_counters(self, evlist: Any) -> None:
> + """Read counters live."""
> + for evsel in evlist:
> + name = str(evsel)
> + if "cycles" in name:
> + event_name = "cycles"
> + elif "instructions" in name:
> + event_name = "instructions"
> + else:
> + continue
> +
> + for cpu in evsel.cpus():
> + for thread in evsel.threads():
> + try:
> + counts = evsel.read(cpu, thread)
> + self.store(event_name, cpu, thread,
> + (counts.val, counts.ena, counts.run))

Does this result in an incorrect delta of deltas calculation?

The underlying C implementation for the Python binding (pyrf_evsel__read in
tools/perf/util/python.c) explicitly subtracts the previous counter values,
so evsel.read() already returns the delta for the current interval.

If store() is then called, which computes cur_val = val - prev_val, won't
this subtract the previous interval's delta from the current interval's
delta and result in incorrect CPI metrics for all intervals after the first?

> + except OSError:
> + pass
[ ... ]

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