Re: [PATCH v7 50/59] perf rwtop: Port rwtop to use python module

sashiko-bot
2026-04-25T23:06:56Z

Sashiko AI review found 5 potential issue(s):
- [High] Invalid fallback tracepoint names will cause an unconditional crash.
- [Medium] Commit message discrepancy regarding ANSI escape codes.
- [Medium] Legitimate syscall returns of 0 are misclassified as errors.
- [Medium] Command names are lost for syscalls spanning across reporting intervals.
- [Medium] Process name resolution is functionally disabled in live mode.
--

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

perf rwtop: Port rwtop to use python module

Port the legacy Perl script rwtop.pl to a python script using the perf
module in tools/perf/python.

> - Used ANSI escape codes (\x1b[H\x1b[2J) to clear the terminal.

This isn't a bug, but the commit message states ANSI escape codes are used
to clear the terminal. Looking at print_totals(), these escape sequences
do not seem to be emitted anywhere in the script.

> diff --git a/tools/perf/python/rwtop.py b/tools/perf/python/rwtop.py
> new file mode 100755
> --- /dev/null
> +++ b/tools/perf/python/rwtop.py
[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None: # pylint: disable=too-many-branches
> + """Process events."""
> + event_name = str(sample.evsel)
> + pid = sample.sample_pid
> + sample_time = sample.sample_time
[ ... ]
> + try:
> + comm = f"PID({pid})" if not self.session else self.session.find_thread(pid).comm()
> + except Exception: # pylint: disable=broad-except
> + comm = f"PID({pid})"

Does this prevent process name resolution in live mode?

In main(), live_session is used and analyzer.session is never initialized
during live mode. Will this cause the script to always fall back to
f"PID({pid})" during live tracing?

[ ... ]
> + elif event_name in ("evsel(syscalls:sys_exit_read)", "evsel(raw_syscalls:sys_exit_read)"):
> + try:
> + ret = sample.ret
> + if ret > 0:
> + self.reads[pid]["bytes_read"] += ret
> + else:
> + self.reads[pid]["errors"][ret] += 1

Does this misclassify legitimate syscalls as errors?

A return value of 0 indicates EOF for read() or 0 bytes transferred for
write(), but this logic will fall into the else block and increment the
error counter. Should this check for ret >= 0 instead, since syscall
errors are represented by negative integers?

[ ... ]
> + def print_totals(self) -> None:
> + """Print summary tables."""
[ ... ]
> + # Reset counts
> + self.reads.clear()
> + self.writes.clear()

Does clearing these dictionaries cause us to lose the command name for
syscalls spanning across intervals?

If an interval elapses between a process's sys_enter and sys_exit events,
the dictionary entry is cleared. When the sys_exit handler recreates it
via the defaultdict factory, the comm string will be empty, which might
result in blank command names in the output table.

[ ... ]
> +def main() -> None:
> + """Main function."""
[ ... ]
> + if not os.path.exists(args.input) and args.input == "perf.data":
> + # Live mode
> + events = (
> + "syscalls:sys_enter_read,syscalls:sys_exit_read,"
> + "syscalls:sys_enter_write,syscalls:sys_exit_write"
> + )
> + try:
> + live_session = LiveSession(events, sample_callback=analyzer.process_event)
> + except OSError:
> + events = (
> + "raw_syscalls:sys_enter_read,raw_syscalls:sys_exit_read,"
> + "raw_syscalls:sys_enter_write,raw_syscalls:sys_exit_write"
> + )
> + live_session = LiveSession(events, sample_callback=analyzer.process_event)

Will this fallback path crash?

The raw_syscalls subsystem exposes generic sys_enter and sys_exit
tracepoints, but it does not have per-syscall variants like
raw_syscalls:sys_enter_read. If the primary session fails, will this
fallback unconditionally raise another OSError?

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