Re: [PATCH v7 44/59] perf sctop: Port sctop to use python module

sashiko-bot
2026-04-25T23:12:11Z

Sashiko AI review found 3 potential issue(s):
- [High] Offline mode interval reporting uses the non-existent `sample.time` attribute, silently breaking periodic output.
- [High] Live mode process filtering is fundamentally broken because it relies on a non-existent `comm` attribute.
- [Medium] Zero or negative intervals cause a CPU spin loop and terminal flooding.
--

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

perf sctop: Port sctop to use python module

This commit ports sctop.py from tools/perf/scripts/python/ to
tools/perf/python/ using a class-based structure. It also adds live mode
support using the LiveSession helper with a fallback strategy.

> diff --git a/tools/perf/python/sctop.py b/tools/perf/python/sctop.py
> new file mode 100755
> index 000000000000..b94f66a8307d
> --- /dev/null
> +++ b/tools/perf/python/sctop.py

[ ... ]

> + comm = "Unknown"
> + if hasattr(self, 'session') and self.session:
> + try:
> + proc = self.session.find_thread(sample.sample_pid)
> + if proc:
> + comm = proc.comm()
> + except (TypeError, AttributeError):
> + pass
> + else:
> + comm = getattr(sample, "comm", "Unknown")

Does live mode process filtering work as intended here? The perf.sample_event
object does not natively contain a comm attribute.

Because getattr falls back to "Unknown", the condition comm != self.for_comm
will evaluate to True for any user-specified command name.

Consequently, if a user specifies a target process in live mode, all events
will be silently filtered out and dropped.

> +
> + if name in ("evsel(raw_syscalls:sys_enter)", "evsel(syscalls:sys_enter)"):
> + if self.for_comm is not None and comm != self.for_comm:
> + return
> + with self.lock:
> + self.syscalls[syscall_id] += 1
> +
> + if self.offline and hasattr(sample, "time"):

Does this properly evaluate the event timestamp? The python extension exposes
the event timestamp as sample_time, not time.

Because hasattr(sample, "time") always evaluates to False, the script skips
the periodic interval checking block in offline mode and only dumps a single
accumulated output at the end of processing.

> + interval_ns = self.interval * (10 ** 9)
> + if self.last_print_time is None:
> + self.last_print_time = sample.time
> + elif sample.time - self.last_print_time >= interval_ns:
> + self.print_current_totals()
> + self.last_print_time = sample.time

[ ... ]

> +def main():
> + """Main function."""
> + ap = argparse.ArgumentParser(description="System call top")
> + ap.add_argument("args", nargs="*", help="[comm] [interval] or [interval]")
> + ap.add_argument("-i", "--input", help="Input file name")
> + args = ap.parse_args()
> +
> + for_comm = None
> + default_interval = 3
> + interval = default_interval
> +
> + if len(args.args) > 2:
> + print("Usage: perf script -s sctop.py [comm] [interval]")
> + sys.exit(1)
> +
> + if len(args.args) > 1:
> + for_comm = args.args[0]
> + try:
> + interval = int(args.args[1])
> + except ValueError:
> + print(f"Invalid interval: {args.args[1]}")
> + sys.exit(1)
> + elif len(args.args) > 0:
> + try:
> + interval = int(args.args[0])
> + except ValueError:
> + for_comm = args.args[0]
> + interval = default_interval

What happens if a user provides zero or a negative number for the interval?
The argument parsing does not validate that the interval is strictly positive.

If a zero or negative value is used, self.stop_event.wait(self.interval) in
the background thread returns immediately without sleeping. This causes the
background thread to infinitely spin in a tight loop, constantly clearing
the terminal and maximizing a CPU core.

In offline mode, a negative or zero interval causes the script to print
statistics on every single event, heavily degrading performance.

> +
> + analyzer = SCTopAnalyzer(for_comm, interval, offline=bool(args.input))

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