Re: [PATCH v7 46/59] perf task-analyzer: Port task-analyzer to use python module

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

Sashiko AI review found 8 potential issue(s):
- [High] Silent failure masking all functionality: 'sample_cpu' raises AttributeError, dropping all events.
- [High] Crash on '.pid' attribute access on 'perf.thread' object.
- [High] Uncaught TypeError when thread is not found.
- [Medium] Argument parsing crash on '--time-limit' without a colon.
- [Medium] Using '--csv-summary' incorrectly alters standard trace output formatting.
- [Medium] Incomplete implementation of '--summary' feature.
- [Medium] Memory leak in live mode for long-running sessions.
- [Low] CSV output format breakage.
--

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

perf task-analyzer: Port task-analyzer to use python module

This commit ports task-analyzer.py to tools/perf/python, transitioning
it to a class-based architecture. It adds support for both file mode
and live mode.

> diff --git a/tools/perf/python/task-analyzer.py b/tools/perf/python/task-analyzer.py
> new file mode 100755
> index 000000000000..08e44946fe6a
> --- /dev/null
> +++ b/tools/perf/python/task-analyzer.py

[ ... ]

> + def _prepare_fmt_sep(self) -> tuple[str, int]:
> + if self.args.csv or self.args.csv_summary:
> + return ",", 0
> + return " ", 1

If a user requests a CSV summary via --csv-summary but wants standard trace
output on stdout, will this cause the primary trace output to unintentionally
use a comma separator as well?

[ ... ]

> + def _record_by_tid(self, task: Task) -> None:
> + tid = task.tid
> + if tid not in self.db["tid"]:
> + self.db["tid"][tid] = []
> + self.db["tid"][tid].append(task)
> + self.db["tid"][tid] = self._record_cleanup(self.db["tid"][tid])

While _record_cleanup() bounds the list length per TID, it doesn't appear
to evict old TIDs from self.db["tid"] when threads terminate. In a continuous
live trace, could this dictionary grow linearly over time and cause unbounded
memory usage?

[ ... ]

> + def _is_within_timelimit(self, time_ns: int) -> bool:
> + if not self.args.time_limit:
> + return True
> + time_s = decimal.Decimal(time_ns) / decimal.Decimal(1e9)
> + lower_bound, upper_bound = self.args.time_limit.split(":")

If a user provides a single-value string without a colon (e.g.,
--time-limit 12345), won't split(":") return a single-element list and
trigger a ValueError?

[ ... ]

> + def process_event(self, sample: perf.sample_event) -> None:
> + """Process sched:sched_switch events."""
> + if "sched:sched_switch" not in str(sample.evsel):
> + return
> +
> + time_ns = sample.sample_time
> + if not self._is_within_timelimit(time_ns):
> + return
> +
> + # Access tracepoint fields directly from sample object
> + try:
> + prev_pid = sample.prev_pid
> + next_pid = sample.next_pid
> + next_comm = sample.next_comm
> + common_cpu = sample.sample_cpu
> + except AttributeError:
> + # Fallback or ignore if fields are not available
> + return

Since the C extension defines this field as sample.cpu rather than
sample.sample_cpu, won't accessing sample.sample_cpu unconditionally
raise an AttributeError?

If so, the generic except AttributeError block will silently ignore the
error and return, causing the script to drop all events.

> +
> + next_comm = self._filter_non_printable(next_comm)
> +
> + # Task finish for previous task
> + if self.session:
> + prev_tgid = self.session.find_thread(prev_pid).pid # type: ignore

Does the perf.thread C extension object expose a .pid attribute? Looking at
pyrf_thread__type in python.c, it seems to only define a .comm() method,
which would cause an AttributeError here.

Additionally, if self.session.find_thread() cannot find the thread, it
raises a TypeError. Because there is no try/except TypeError block here,
could tracing untracked PIDs crash the script?

> + else:
> + prev_tgid = prev_pid # Fallback
> + self._handle_task_finish(prev_pid, common_cpu, time_ns, prev_tgid)
> + # Task start for next task
> + self._handle_task_start(next_pid, common_cpu, next_comm, time_ns)
> +
> + def print_summary(self) -> None:
> + """Calculate and print summary."""
> + need_summary = (self.args.summary or self.args.summary_extended or
> + self.args.summary_only or self.args.csv_summary)
> + if not need_summary:
> + return
> +
> + # Simplified summary logic for brevity, full logic can be ported if needed
> + print("\nSummary (Simplified)", file=self.fd_sum)
> + if self.args.summary_extended:
> + print("Inter Task Times", file=self.fd_sum)
> + # ... port full Summary class logic here ...

It looks like the original comprehensive statistics logic was replaced
with a stub. Are there plans to port the full summary logic, as the
comment suggests?

[ ... ]

> diff --git a/tools/perf/tests/shell/test_task_analyzer.sh b/tools/perf/tests/shell/test_task_analyzer.sh
> index 0314412e63b4..7465298d0384 100755
> --- a/tools/perf/tests/shell/test_task_analyzer.sh
> +++ b/tools/perf/tests/shell/test_task_analyzer.sh

[ ... ]

> test_csv() {
> - perf script report task-analyzer --csv "${csv}" > /dev/null
> - check_exec_0 "perf script report task-analyzer --csv ${csv}"
> - find_str_or_fail "Comm;" "${csv}" "${FUNCNAME[0]}"
> + $PYTHON $perfdir/python/task-analyzer.py -i "${perfdata}" --csv "${csv}" > /dev/null
> + check_exec_0 "$PYTHON $perfdir/python/task-analyzer.py -i ${perfdata} --csv ${csv}"
> + find_str_or_fail "Comm," "${csv}" "${FUNCNAME[0]}"
> }

This isn't a bug, but the CSV delimiter was changed from a semicolon to a
comma. Could this break external parsers that rely on the previous
separator format?

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