Re: [PATCH v5 50/58] perf rwtop: Port rwtop to use python module

sashiko-bot
2026-04-24T17:14:47Z

Sashiko AI review found 5 potential issue(s):
- [Medium] The commit message claims to use ANSI escape codes to clear the terminal, but the code does not implement this.
- [Medium] The fallback events for `raw_syscalls` use invalid tracepoint names.
- [Medium] Command name (comm) resolution is completely broken in live mode.
- [Medium] Read and write system calls returning 0 (such as EOF) are incorrectly tracked as errors.
- [Medium] Process names (`comm`) will be missing from the output for cross-interval events.
--

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

perf rwtop: Port rwtop to use python module

This commit ports the legacy rwtop.pl Perl script to a Python script
using the perf module. It leverages the perf.session API for event
processing to display system-wide read and write call activity periodically.

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

Is this description accurate?

Looking at print_totals() in the code, there do not appear to be any ANSI
escape sequences emitted. This results in the terminal scrolling continuously
instead of clearing.

> diff --git a/tools/perf/python/rwtop.py b/tools/perf/python/rwtop.py
> new file mode 100755
> index 00000000000000..952d178e99c5a1
> --- /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
> +
> + if self.last_print_time == 0:
> + self.last_print_time = sample_time
> +
> + # Check if interval has passed
> + if sample_time - self.last_print_time >= self.interval_ns:
> + self.print_totals()
> + self.last_print_time = sample_time
> +
> + try:
> + comm = f"PID({pid})" if not self.session else self.session.process(pid).comm()
> + except Exception: # pylint: disable=broad-except
> + comm = f"PID({pid})"

Will process names resolve correctly in live mode?

When running with the LiveSession path in main(), self.session is never
initialized. It appears this will always fall back to printing the PID instead
of the command name.

[ ... ]
> + 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 incorrectly classify EOF as an error?

A return value of 0 indicates End-Of-File for reads, which is a normal
operational state, whereas actual syscall errors return negative values.

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

Similarly for writes, a return value of 0 indicates 0 bytes written.

Should this be checking for ret >= 0 instead, or strictly tracking negative
values as errors?

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

Will this result in missing process names for syscalls that span across
interval boundaries?

Because the dictionaries are cleared completely, if a sys_exit event arrives
in the new interval without a preceding sys_enter, the defaultdict will
recreate the entry with an empty string for comm. The summary table might
then display a blank command name for that pid.

[ ... ]
> +def main() -> None:
[ ... ]
> + 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"
> + )

Are these fallback tracepoint names valid?

The raw_syscalls subsystem only exposes generic sys_enter and sys_exit
tracepoints. It does not append the syscall name, so attempting to attach
to raw_syscalls:sys_enter_read will fail.

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