Re: [PATCH v5 34/58] perf compaction-times: Port compaction-times to use python module

sashiko-bot
2026-04-24T17:15:22Z

Sashiko AI review found 3 potential issue(s):
- [Medium] Unhandled TypeError when retrieving missing thread metadata aborts event processing.
- [Medium] Directly accessing dynamic tracepoint fields on `sample` objects can cause an unhandled AttributeError.
- [Medium] Filtering specifically for PID 0 behaves incorrectly and instead includes all processes.
--

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

perf compaction-times: Port compaction-times to use python module

This commit adds a port of the compaction-times script that uses the perf
python module directly. This approach is significantly faster than using
perf script callbacks as it avoids creating intermediate dictionaries.

> diff --git a/tools/perf/python/compaction-times.py b/tools/perf/python/compaction-times.py
> new file mode 100755
> index 00000000000000..153b47930e3ccf
> --- /dev/null
> +++ b/tools/perf/python/compaction-times.py

[ ... ]

> +def get_pid_filter(low_str: str, high_str: str) -> Callable[[int, str], bool]:
> + """Returns a filter function based on PID range."""
> + low = 0 if low_str == "" else int(low_str)
> + high = 0 if high_str == "" else int(high_str)
> +
> + def filter_func(pid: int, _comm: str) -> bool:
> + return not (pid >= low and (high == 0 or pid <= high))
> + return filter_func

Does this filter behave correctly when specifically filtering for PID 0?

If a user explicitly requests to filter only PID 0 by passing "0" or "0-0",
high_str evaluates to "0", making high = 0.

Since high == 0 is used as a sentinel condition for no upper limit, the
condition (high == 0) evaluates to True. This simplifies the logic to
not (pid >= 0).

Since all PIDs are >= 0, the expression returns False for every PID,
bypassing the filter entirely and including all processes.

[ ... ]

> +def process_event(sample: perf.sample_event) -> None:
> + """Callback for processing events."""
> + event_name = str(sample.evsel)
> + pid = sample.sample_pid
> + comm = session.process(pid).comm() if session else "[unknown]"

Will this code safely handle cases where the thread metadata is missing?

In the underlying C API wrapper pyrf_session__process(), if the requested
thread is not found in the session's machine thread lists, it explicitly sets
a TypeError.

Since this call isn't wrapped in a try/except block, the exception propagates
back to the C caller. The C callback prints the exception traceback and
returns -1, which abruptly terminates the entire process_events() loop.

> + secs = sample.sample_time // 1000000000
> + nsecs = sample.sample_time % 1000000000
> +
> + if "evsel(compaction:mm_compaction_begin)" in event_name:
> + Chead.create_pending(pid, comm, secs, nsecs)
> + elif "evsel(compaction:mm_compaction_end)" in event_name:
> + Chead.complete_pending(pid, secs, nsecs)
> + elif "evsel(compaction:mm_compaction_migratepages)" in event_name:
> + Chead.increment_pending(pid, Pair(sample.nr_migrated, sample.nr_failed), None, None)

Can accessing these tracepoint fields directly on the sample object
cause an unhandled AttributeError?

These properties are resolved dynamically by pyrf_sample_event__getattro
using libtraceevent. If libtraceevent fails to resolve the field, it sets an
AttributeError.

Similar to the thread lookup issue, this exception would be uncaught and
propagate back to C, causing the loop to abort prematurely. Could this be
updated to use getattr() to safely retrieve the fields with a fallback?

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