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

sashiko-bot
2026-04-25T23:22:11Z

Sashiko AI review found 3 potential issue(s):
- [Medium] Missing exception handling around session.find_thread() will cause the script to crash on unknown threads.
- [Medium] Use of sample_pid instead of sample_tid alters aggregation semantics, causing data corruption for multi-threaded processes.
- [Medium] The PID filter logic completely breaks for exact PID 0 matching, silently allowing all PIDs.
--

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

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

Add 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 000000000000..7f17c251ded7
> --- /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))

Does this code handle filtering for PID 0 correctly?

If a user explicitly filters for PID 0, high will be assigned 0. The
condition then evaluates high == 0, which will return True for all valid
PIDs >= low (0), effectively disabling the filter instead of restricting it
to PID 0.

[ ... ]

> +def process_event(sample: perf.sample_event) -> None:
> + """Callback for processing events."""
> + event_name = str(sample.evsel)
> + pid = sample.sample_pid

Does this aggregation cause data corruption for multi-threaded processes?

In the perf_sample Python bindings, sample.sample_pid provides the process ID
(TGID), while sample.sample_tid provides the thread ID (TID). The original
script used common_pid, which corresponds to the TID.

If multiple threads in the same process run compaction concurrently, they will
share the same TGID, which could cause them to clobber each other's pending
state when mark_pending() overwrites self.pending.

> + comm = session.find_thread(pid).comm() if session else "[unknown]"

Will this script crash if the thread cannot be found?

The C-extension function pyrf_session__find_thread() explicitly returns NULL
and raises a Python TypeError if the thread is missing from the machine's
thread map. Without a try...except TypeError block around this call,
encountering an unknown thread will abort trace processing entirely.

> + secs = sample.sample_time // 1000000000

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