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

sashiko-bot
2026-04-25T23:05:04Z

Sashiko AI review found 5 potential issue(s):
- [Medium] The commit message contradicts the submitted code regarding the refactoring of `process_event`.
- [Medium] The script incorrectly uses the requested byte count instead of the actual written byte count for write statistics.
- [Medium] Normal End-of-File (EOF) events are incorrectly classified as read errors.
- [Low] The script checks for non-existent kernel tracepoints like `raw_syscalls:sys_enter_read`.
- [Low] The script uses brittle string slicing to extract the event name from `evsel`.
--

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

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

This commit ports the legacy Perl rw-by-pid script to a Python script using
the perf module. It leverages the perf.session API to track read and write
activity by PID.

> Complications:
> - Refactored process_event to extract helper methods
> (_handle_sys_enter_read, etc.) to reduce the number of branches and
> satisfy pylint.

This isn't a bug, but the commit message states that helper methods were
extracted to satisfy pylint, whereas the submitted code provides a single
process_event method and uses a pylint disable comment instead. Could the
commit message be updated to reflect the final code?

> diff --git a/tools/perf/python/rw-by-pid.py b/tools/perf/python/rw-by-pid.py
> new file mode 100755
> index 000000000000..b206d2a575cd
> --- /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 = str(sample.evsel)[6:-1]

Is this string slicing robust? If the internal string formatting convention
for evsel changes in the future, could this fail and silently drop events
into the unhandled category?

> + pid = sample.sample_pid
> +
> + assert self.session is not None
> + try:
> + comm = self.session.find_thread(pid).comm()
> + except Exception: # pylint: disable=broad-except
> + comm = "unknown"
> +
> + if event_name in ("syscalls:sys_enter_read", "raw_syscalls:sys_enter_read"):

Are there tracepoints named raw_syscalls:sys_enter_read? The kernel provides
generic raw tracepoints like raw_syscalls:sys_enter and specific ones like
syscalls:sys_enter_read. Does checking for the raw_syscalls variation here
represent dead code?

> + 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 incorrectly classify an End-of-File as an error? For the read
system call, returning 0 indicates EOF, which is an expected condition.
Treating 0 as an error might artificially inflate the failed reads summary.

> + 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 over-report the actual bytes written? Using the requested size
from sample.count might be inaccurate if there are partial or failed
writes. Should the actual bytes successfully written be accumulated from
sample.ret in sys_exit_write instead, similar to how reads are handled?

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