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

sashiko-bot
2026-04-20T00:42:34Z

Sashiko AI review found 4 potential issue(s):
- [Medium] Loose substring matching for tracepoint names matches unrelated syscalls and desynchronizes operation counts from byte counts.
- [Medium] Hardcoded ANSI clear-screen escapes combined with event-time flushing breaks offline processing.
- [Medium] Writes track requested bytes instead of actual bytes written, leading to incorrect accounting.
- [Low] Error tracking dictionaries are populated but never used or displayed.
--

commit 2f7b010334a549f6b163fe08533ebb5646479da4
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 to
periodically display system-wide r/w call activity, broken down by PID.

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

[ ... ]

> + if "sys_enter_read" in event_name:
> + self._handle_sys_enter_read(sample, pid, comm)
> + elif "sys_exit_read" in event_name:
> + self._handle_sys_exit_read(sample, pid)
> + elif "sys_enter_write" in event_name:
> + self._handle_sys_enter_write(sample, pid, comm)
> + elif "sys_exit_write" in event_name:
> + self._handle_sys_exit_write(sample, pid)
> + else:
> + self.unhandled[event_name] += 1

Will this loose substring match catch unrelated syscalls like
sys_enter_readv?

If sys_enter_readv is caught, it lacks a count field, which raises an
AttributeError in _handle_sys_enter_read() that is silently caught. But
sys_exit_readv has a ret field, so _handle_sys_exit_read() will add to
bytes_read without incrementing the read count.

Does this lead to desynchronized operation counts from byte counts, and
hide these unhandled variants from the unhandled events dictionary?

[ ... ]

> + def _handle_sys_enter_write(self, sample: perf.sample_event, pid: int, comm: str) -> None:
> + try:
> + count = sample.count
> + self.writes[pid]["bytes_written"] += count
> + self.writes[pid]["total_writes"] += 1
> + self.writes[pid]["comm"] = comm
> + except AttributeError:
> + pass
> +
> + def _handle_sys_exit_write(self, sample: perf.sample_event, pid: int) -> None:
> + try:
> + ret = sample.ret
> + if ret <= 0:
> + self.writes[pid]["errors"][ret] += 1
> + except AttributeError:
> + pass

Does this code incorrectly track requested bytes instead of actual bytes
written?

For reads, bytes_requested and bytes_read are separated, but here
bytes_written uses the requested count and ignores sample.ret on successful
writes, which might overcount partial writes.

This isn't a bug, but it looks like the error tracking dictionaries are
populated here and in _handle_sys_exit_read(), but never used or displayed
in print_totals().

[ ... ]

> + def print_totals(self) -> None:
> + """Print summary tables."""
> + # Clear terminal using ANSI escape codes
> + print("\x1b[H\x1b[2J", end="")

The commit message states this is "robust for file-based processing", but
does the combination of event-time flushing and hardcoded ANSI clear-screen
escapes break offline processing?

When parsing an offline perf.data file, the processing happens at CPU speed,
which will instantly clear the terminal for every interval and only leave
the final output visible, or garble the output if piped.

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