Re: [PATCH v6 38/59] perf failed-syscalls-by-pid: Port failed-syscalls-by-pid to use python module

sashiko-bot
2026-04-25T18:05:59Z

Sashiko AI review found 3 potential issue(s):
- [High] The script aborts event processing entirely when an unknown pid is encountered because `find_thread()` raises an unhandled `TypeError`.
- [Medium] Output formatting and sorting logic regression: the script flattens the statistics dictionary, breaking the original script's grouping and frequency sorting.
- [Low] Dead state: `self.unhandled` dictionary is populated but never utilized.
--

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

perf failed-syscalls-by-pid: Port failed-syscalls-by-pid to use python module

This commit ports the failed-syscalls-by-pid script to the newer perf
Python module API. It updates syscall name resolution, error string
lookups, and adds command-line filtering for COMM or PID.

> - Used standard collections.defaultdict for nested statistics
> aggregation.

The commit message states the script uses nested statistics aggregation, but
the implemented code defines a flat dictionary. Should this be a nested
dictionary, or should the commit message be updated?

> diff --git a/tools/perf/python/failed-syscalls-by-pid.py b/tools/perf/python/failed-syscalls-by-pid.py
> new file mode 100755
> index 000000000000..f57b13c5d34f
> --- /dev/null
> +++ b/tools/perf/python/failed-syscalls-by-pid.py
[ ... ]
> +class SyscallAnalyzer:
> + """Analyzes failed syscalls and aggregates counts."""
> +
> + def __init__(self, for_comm: Optional[str] = None, for_pid: Optional[int] = None):
> + self.for_comm = for_comm
> + self.for_pid = for_pid
> + self.session: Optional[perf.session] = None
> + self.syscalls: dict[tuple[str, int, int, int], int] = defaultdict(int)
> + self.unhandled: dict[str, int] = defaultdict(int)

Does the unhandled dictionary get used anywhere else? It appears to be
populated during event processing but is never printed or accessed in the
summary output.

[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
> + """Process a single sample event."""
> + event_name = str(sample.evsel)
> + if "sys_exit" not in event_name:
> + return
> +
> + pid = sample.sample_pid
> + if hasattr(self, 'session') and self.session:
> + comm = self.session.find_thread(pid).comm()

Can this raise a TypeError if the thread is not found? The perf Python
module's C implementation raises a TypeError when a thread lookup fails,
rather than returning None.

If an unmapped thread is encountered, will this terminate the script
prematurely?

[ ... ]
> + def print_summary(self) -> None:
[ ... ]
> + sorted_keys = sorted(self.syscalls.keys(), key=lambda k: (k[0], k[1], k[2]))
> + current_comm_pid = None
> + for comm, pid, syscall_id, ret in sorted_keys:
> + if current_comm_pid != (comm, pid):
> + print(f"\n{comm} [{pid}]")
> + current_comm_pid = (comm, pid)
> + try:
> + name = perf.syscall_name(syscall_id) or str(syscall_id)
> + except AttributeError:
> + name = str(syscall_id)
> + print(f" syscall: {name:<16}")
> + err_str = strerror(ret)
> + count = self.syscalls[(comm, pid, syscall_id, ret)]
> + print(f" err = {err_str:<20} {count:10d}")

Since current_syscall_id is not tracked during this iteration, will the script
print the syscall name header multiple times if a single syscall returns
several different error codes?

Also, is it intended that the output is now sorted alphabetically by the keys
rather than in descending order by occurrence count? The original script
sorted by frequency to highlight the most common failures.

[ ... ]
> +if __name__ == "__main__":
[ ... ]
> + try:
> + print("Press control+C to stop and show the summary")
> + session = perf.session(perf.data(args.input), sample=analyzer.process_event)
> + analyzer.session = session
> + session.process_events()
> + analyzer.print_summary()
> + except KeyboardInterrupt:
> + analyzer.print_summary()
> + except Exception as e:
> + print(f"Error processing events: {e}")

If process_events terminates early due to an exception, like the TypeError
mentioned above, will the script exit without showing any of the accumulated
metrics?

Would it be helpful to call analyzer.print_summary() in this exception
handler as well so partial results are not lost?

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