Re: [PATCH v2 33/58] perf check-perf-trace: Port check-perf-trace to use python module

sashiko-bot
2026-04-23T04:18:35Z

Sashiko AI review found 4 potential issue(s):
- [Medium] The ported script hardcodes tracepoint string resolution, removing its intended test coverage.
- [Medium] The script uses `sample_pid` instead of `sample_tid`, causing incorrect thread reporting.
- [Medium] An unhandled `TypeError` when looking up untracked threads will crash the script.
- [Medium] The PEP-585 type hint causes a runtime crash on Python 3.8 and older.
--

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

perf check-perf-trace: Port check-perf-trace to use python module

This commit adds a port of the check-perf-trace script that uses the perf
python module directly, aiming for faster execution by avoiding intermediate
dictionaries.

> diff --git a/tools/perf/python/check-perf-trace.py b/tools/perf/python/check-perf-trace.py
> new file mode 100755
> index 0000000000000..4c05540bdc057
> --- /dev/null
> +++ b/tools/perf/python/check-perf-trace.py
> @@ -0,0 +1,113 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +"""
> +Basic test of Python scripting support for perf.
> +Ported from tools/perf/scripts/python/check-perf-trace.py
> +"""
> +
> +import argparse
> +import collections
> +import perf
> +
> +unhandled: collections.defaultdict[str, int] = collections.defaultdict(int)

Will this type hint cause a runtime crash on older Python versions?

Using subscripted standard collections like collections.defaultdict[str, int]
was introduced in PEP 585 for Python 3.9. Without a future import, Python
3.8 and earlier will evaluate this at runtime and raise a TypeError.

> +session = None
> +
> +softirq_vecs = {
> + 0: "HI_SOFTIRQ",
> + 1: "TIMER_SOFTIRQ",
> + 2: "NET_TX_SOFTIRQ",

[ ... ]

> + 8: "HRTIMER_SOFTIRQ",
> + 9: "RCU_SOFTIRQ",
> +}

[ ... ]

> +def symbol_str(event_name: str, field_name: str, value: int) -> str:
> + """Resolves symbol values to strings."""
> + if event_name == "irq__softirq_entry" and field_name == "vec":
> + return softirq_vecs.get(value, str(value))
> + return str(value)

Does hardcoding the tracepoint string resolution here bypass the original
intent of the test?

The original script delegated trace field resolution to the perf scripting
engine to verify its correctness, but using a hardcoded dictionary drops
that test coverage for the Python scripting API.

[ ... ]

> +def print_header(event_name: str, sample: perf.sample_event) -> None:
> + """Prints common header for events."""
> + secs = sample.sample_time // 1000000000
> + nsecs = sample.sample_time % 1000000000
> + comm = session.process(sample.sample_pid).comm() if session else "[unknown]"

Does using sample.sample_pid instead of sample.sample_tid cause incorrect
thread reporting?

In the perf subsystem, the pid field corresponds to the process group leader
(TGID) while the tid field corresponds to the actual thread ID, which might
output the group leader's name instead of the specific thread generating
the event.

Also, can session.process() raise an unhandled TypeError here?

When machine__find_thread() cannot find the thread (which is common in
truncated traces), the C extension raises a TypeError. Without a try-except
block, this could crash the script.

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