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

sashiko-bot
2026-04-23T04:26:22Z

Sashiko AI review found 5 potential issue(s):
- [Medium] Meaningful code and commit message mismatch regarding terminal clearing.
- [Medium] The fallback logic for LiveSession uses invalid kernel tracepoints.
- [Medium] 0-byte completions (EOF) for read() and write() are incorrectly classified as errors.
- [Medium] Reference cycle with C extension type causes a memory and file descriptor leak.
- [Low] Unhandled events tally is omitted when terminating Live Mode.
--

commit bcee736641b044aae62f7cca526d939e320836b7
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. It uses a class-based architecture to periodically display
system-wide r/w call activity broken down by PID.

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

The commit message mentions using ANSI escape codes to clear the terminal,
but looking at the print_totals() function in the diff, these escape
sequences don't appear to be used. This means the script will continuously
scroll its output instead of refreshing in place.

> diff --git a/tools/perf/python/rwtop.py b/tools/perf/python/rwtop.py
> new file mode 100755
> index 0000000000000..952d178e99c5a
> --- /dev/null
> +++ b/tools/perf/python/rwtop.py

[ ... ]

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

Are 0-byte completions (EOF) for read() incorrectly classified as errors here?

If a system call returns 0, the code falls into the else block and increments
errors[0]. A return value of 0 for read() signifies End-Of-File (EOF), which
is a normal successful condition, not a system call failure.

[ ... ]

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

Does this have the same issue with classifying 0-byte completions for
write() as errors instead of a successful EOF condition?

[ ... ]

> + def run(self, input_file: str) -> None:
> + """Run the session."""
> + self.session = perf.session(perf.data(input_file), sample=self.process_event)

Does this create a reference cycle that leaks the session and file
descriptor?

By passing self.process_event to perf.session and storing the session in
self.session, a cycle is formed. The perf.session C extension type lacks
garbage collection support, so Python's cyclic GC cannot break it. This
could lead to a resource leak if the class is used programmatically to
process multiple files in the same process.

[ ... ]

> + 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 tracepoints valid?

The raw_syscalls subsystem only exports generic sys_enter and sys_exit
tracepoints. Per-syscall tracepoints like raw_syscalls:sys_enter_read do
not exist, meaning this fallback will invariably raise an OSError and
terminate the script.

[ ... ]

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

Should the unhandled events tally be printed here as well?

When processing an offline file, the run() method prints the contents of the
unhandled dictionary. Omitting this in the KeyboardInterrupt handler creates
an inconsistency where dropped or malformed events aren't shown when
terminating live mode.

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