Re: [PATCH v2 44/58] perf sctop: Port sctop to use python module

sashiko-bot
2026-04-23T04:20:21Z

Sashiko AI review found 3 potential issue(s):
- [High] Live mode process filtering is broken due to non-existent `comm` attribute on sample events.
- [High] Offline mode interval reporting is non-functional due to incorrect timestamp attribute and timer starvation.
- [Low] Outdated hardcoded usage message contradicts standalone script execution.
--

commit abb433706222dd69bc39b0037938568f893314f9
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/, refactoring it into a class-based structure. It also adds
support for live mode using LiveSession when no input file is specified.

> diff --git a/tools/perf/python/sctop.py b/tools/perf/python/sctop.py
> new file mode 100755
> index 0000000000000..d7ac922da510c
> --- /dev/null
> +++ b/tools/perf/python/sctop.py
[ ... ]
> @@ -0,0 +1,174 @@
[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
> + """Collect syscall events."""
> + name = str(sample.evsel)
> + syscall_id = getattr(sample, "id", -1)
> +
> + if syscall_id == -1:
> + return
> +
> + if hasattr(self, 'session') and self.session:
> + comm = self.session.process(sample.sample_pid).comm()
> + else:
> + comm = getattr(sample, "comm", "Unknown")

Are we sure the sample object has a comm attribute in live mode?

Since perf.sample_event objects do not natively possess a comm attribute,
it looks like this will always evaluate to Unknown. If a user specifies a
process to filter via the command line, the filter condition will always drop
the events because Unknown won't match the requested process name.

> +
> + 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 the C extension export the timestamp field as time, or is it sample_time?

If the attribute is actually sample_time, hasattr(sample, "time") will always
be false, preventing offline periodic updates from being printed.

Additionally, this timer update block is placed after the early returns for
unmatched events.

If the monitored process goes idle while other processes continue generating
events, will the offline timer freeze and prevent steady periodic updates?

> + 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]")

This isn't a bug, but since the script was ported to a standalone executable
using argparse, this usage message seems outdated and contradicts the new
execution model.

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