Re: [PATCH v2 49/58] perf rw-by-pid: Port rw-by-pid to use python module

sashiko-bot
2026-04-23T04:14:45Z

Sashiko AI review found 5 potential issue(s):
- [Medium] The commit message describes a refactoring that is not present in the code.
- [Medium] The script incorrectly tracks the requested write size instead of the actual bytes written.
- [Medium] End-of-file (return value 0) is incorrectly classified as an error for read operations.
- [Low] The script attempts to match non-existent event names for the `raw_syscalls` subsystem.
- [Low] Missing process name (`comm`) assignment for processes traced mid-syscall.
--

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

perf rw-by-pid: Port rw-by-pid to use python module

This patch ports the legacy rw-by-pid Perl script to a Python script. It
uses a class-based architecture to track read and write activity by PID for
all processes.

> perf rw-by-pid: Port rw-by-pid to use python module
>
> Port the legacy Perl script rw-by-pid.pl to a python script using the
> perf module in tools/perf/python.
>
> The new script uses a class-based architecture and leverages the
> perf.session API for event processing.
>
> It tracks read and write activity by PID for all processes,
> aggregating bytes requested, bytes read, total reads, and errors.
>
> Complications:
> - Refactored process_event to extract helper methods
> (_handle_sys_enter_read, etc.) to reduce the number of branches and
> satisfy pylint.

Is this description accurate? Looking at the diff below, the process_event
function is a single large method that uses pylint: disable=too-many-branches
and no helper methods were extracted.

[ ... ]

> diff --git a/tools/perf/python/rw-by-pid.py b/tools/perf/python/rw-by-pid.py
> new file mode 100755
> index 0000000000000..53e16b373111a
> --- /dev/null
> +++ b/tools/perf/python/rw-by-pid.py

[ ... ]

> + def process_event(self, sample: perf.sample_event) -> None: # pylint: disable=too-many-branches
> + """Process events."""
> + event_name = sample.evsel.name # type: ignore
> + pid = sample.sample_pid
> +
> + assert self.session is not None
> + try:
> + comm = self.session.process(pid).comm()
> + except Exception: # pylint: disable=broad-except
> + comm = "unknown"
> +
> + if event_name in ("syscalls:sys_enter_read", "raw_syscalls:sys_enter_read"):

Do event names like raw_syscalls:sys_enter_read exist? The raw_syscalls
subsystem typically provides generic tracepoints like raw_syscalls:sys_enter
rather than per-syscall tracepoints.

> + try:
> + count = sample.count
> + self.reads[pid]["bytes_requested"] += count
> + self.reads[pid]["total_reads"] += 1
> + self.reads[pid]["comm"] = comm
> + except AttributeError:
> + self.unhandled[event_name] += 1
> + elif event_name in ("syscalls:sys_exit_read", "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 classify a return value of 0 as an error? A return value of 0 from
read operations indicates end-of-file.

Also, if tracing starts while a process is already inside a read syscall, is
the comm field left empty since it is only populated during the sys_enter_read
event?

> + except AttributeError:
> + self.unhandled[event_name] += 1
> + elif event_name in ("syscalls:sys_enter_write", "raw_syscalls:sys_enter_write"):
> + try:
> + count = sample.count
> + self.writes[pid]["bytes_written"] += count

Does this record the requested write size instead of the actual bytes written?

> + self.writes[pid]["total_writes"] += 1
> + self.writes[pid]["comm"] = comm
> + except AttributeError:
> + self.unhandled[event_name] += 1
> + elif event_name in ("syscalls:sys_exit_write", "raw_syscalls:sys_exit_write"):
> + try:
> + ret = sample.ret
> + if ret <= 0:
> + self.writes[pid]["errors"][ret] += 1

Since successful writes where ret > 0 are ignored here, does this miss
recording the actual bytes written during the sys_exit_write event?

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