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

sashiko-bot
2026-04-25T18:08:27Z

Sashiko AI review found 4 potential issue(s):
- [High] The fallback mechanism uses non-existent tracepoints (`raw_syscalls:sys_enter_read`, etc.), which will cause an unhandled exception and crash.
- [Medium] The script incorrectly classifies a return value of 0 from read and write syscalls as an error.
- [Medium] The commit message claims to use ANSI escape codes to clear the terminal, but they are absent from the implementation.
- [Low] The unhandled events summary is not printed when exiting live mode.
--

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

perf rwtop: Port rwtop to use python module

This commit ports the legacy Perl script rwtop.pl to a Python script using
the perf module in tools/perf/python. It uses a class-based architecture
and leverages the perf.session API to periodically display system-wide read
and write call activity.

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

The commit message states that ANSI escape codes are used to clear the
terminal, but they appear to be absent from the implementation in
print_totals() or elsewhere. Is this intentional, or were the escape
codes accidentally dropped during development?

> diff --git a/tools/perf/python/rwtop.py b/tools/perf/python/rwtop.py
> new file mode 100755
> index 000000000000..895ebab9af10
> --- /dev/null
> +++ b/tools/perf/python/rwtop.py
> @@ -0,0 +1,219 @@

[ ... ]

> + 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 a return value of 0 as an error?

A return value of 0 from read or write typically indicates End-Of-File (EOF)
or a 0-byte transfer, which are standard, non-error conditions. True syscall
errors are usually indicated by negative return values.

This might unnecessarily pollute the error statistics with false positives.

[ ... ]

> + 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, does this incorrectly classify a return value of 0 from a write
syscall as an error?

[ ... ]

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

Can this fallback mechanism cause an unhandled exception and crash?

The raw_syscalls subsystem exposes generic sys_enter and sys_exit tracepoints,
but there are no syscall-specific variants like raw_syscalls:sys_enter_read.

Because of this, the second LiveSession constructor will likely raise another
OSError that is not caught, crashing the script instead of providing a
graceful fallback.

[ ... ]

> + except KeyboardInterrupt:
> + print("\nStopping live mode...", file=sys.stderr)
> + if analyzer.reads or analyzer.writes:
> + analyzer.print_totals()

Should this also print the unhandled events summary?

When exiting live mode via KeyboardInterrupt, only print_totals() is called.
The script fails to print the unhandled events dictionary, which is normally
printed when processing a perf.data file offline in run().

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