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

sashiko-bot
2026-04-24T17:30:02Z

Sashiko AI review found 4 potential issue(s):
- [High] Guaranteed crash in file mode due to invalid attribute access on perf.thread
- [High] Unbounded memory leak in live mode due to forever-growing TID dictionary
- [Medium] Incomplete implementation of summary features causing functional regression
- [Medium] Unhandled KeyError on invalid color name crashes the script
--

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

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

This commit ports the task-analyzer tool to use the perf python module and
refactors it to use a class-based architecture. It also introduces support
for both file mode and live mode analysis.

> diff --git a/tools/perf/python/task-analyzer.py b/tools/perf/python/task-analyzer.py
> new file mode 100755
> index 00000000000000..5894ed0b5a402c
> --- /dev/null
> +++ b/tools/perf/python/task-analyzer.py
[ ... ]
> + def _print_task_finish(self, task: Task) -> None:
[ ... ]
> + if str(task.tid) in self.args.highlight_tasks_map:
> + c_row_set = TaskAnalyzer._COLORS[self.args.highlight_tasks_map[str(task.tid)]]
> + c_row_reset = TaskAnalyzer._COLORS["reset"]
> + if task.comm in self.args.highlight_tasks_map:
> + c_row_set = TaskAnalyzer._COLORS[self.args.highlight_tasks_map[task.comm]]
> + c_row_reset = TaskAnalyzer._COLORS["reset"]

What happens if a user passes an invalid color name via the highlight-tasks
argument? It looks like this dictionary access could raise a KeyError and
abort the script when the targeted task context switches.

[ ... ]
> + 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])

Will self.db["tid"] grow indefinitely in live mode?

While _record_cleanup caps the list size for each active TID, it appears
inactive TIDs are never removed from the dictionary itself. This could leak
memory continuously over long monitoring sessions as new transient processes
are created and destroyed.

[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
[ ... ]
> + # Task finish for previous task
> + if self.session:
> + prev_tgid = self.session.process(prev_pid).pid # type: ignore
> + else:
> + prev_tgid = prev_pid # Fallback
> + self._handle_task_finish(prev_pid, common_cpu, time_ns, prev_tgid)

Is the pid attribute available on the thread object returned by
self.session.process?

The python perf bindings for threads generally only expose a comm method. If
pid is missing, this could raise an AttributeError on the first event.
Additionally, might self.session.process raise a TypeError if the thread
cannot be found, causing the event processing loop to unexpectedly exit?

[ ... ]
> + def print_summary(self) -> None:
[ ... ]
> + # 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 ...

Does this leave the summary features functionally incomplete? Users passing
arguments like --summary or --summary-extended might be expecting the fully
calculated metric tables rather than just the printed headers.

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