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

sashiko-bot
2026-04-25T18:18:18Z

Sashiko AI review found 7 potential issue(s):
- [High] The `print_summary` method is implemented as a placeholder stub, removing actual summary processing.
- [High] Script crashes with an `AttributeError` when a task's thread is not found in the perf session.
- [High] Live mode processes events out of chronological order, corrupting inter-task time tracking.
- [Medium] Trace lists are unconditionally truncated to 1000 items, causing inaccurate summaries.
- [Low] Missing CLI argument validation causes unhandled `ValueError` and `KeyError` crashes.
- [Low] The CSV separator character was silently changed from a semicolon (`;`) to a comma (`,`).
- [Low] The `--stdio-color always` argument is ignored when standard output is piped.
--

commit 5196cbf2ad4b6c11d39c0a5d7c84c508146141e7
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 and refactors it
to a class-based architecture. It also adds support for file mode using
perf.session and live mode using evlist.read_on_cpu.

> 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 _check_color(self) -> None:
> + """Check if color should be enabled."""
> + if self.args.csv or self.args.csv_summary:
> + TaskAnalyzer._COLORS = {k: "" for k in TaskAnalyzer._COLORS}
> + return
> + if sys.stdout.isatty() and self.args.stdio_color != "never":
> + return
> + TaskAnalyzer._COLORS = {k: "" for k in TaskAnalyzer._COLORS}

If standard output is piped, sys.stdout.isatty() is false, which causes the
color mappings to be cleared. Does this unintentionally ignore the
--stdio-color always argument?

[ ... ]

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

This isn't a bug, but the CSV separator was changed from a semicolon to a
comma. Was this change intentional?

[ ... ]

> + def _record_cleanup(self, _list: list[Any]) -> list[Any]:
> + need_summary = (self.args.summary or self.args.summary_extended or
> + self.args.summary_only)
> + if not need_summary and len(_list) > 1:
> + return _list[len(_list) - 1:]
> + if len(_list) > 1000:
> + return _list[len(_list) - 1000:]
> + return _list

Does truncating the list to 1000 items silently discard task occurrences
during file-based processing?

It seems this might cause the subsequent summary calculations to only analyze
a small fraction of large traces, leading to inaccurate statistics.

[ ... ]

> + 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(":")

Does this split operation raise an unhandled ValueError if the user provides
malformed input without a colon, such as --time-limit 123?

[ ... ]

> + 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

[ ... ]

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

If a thread is missing due to lost events or tracing starting after process
initialization, find_thread() can return None. Will accessing .pid here
raise an unhandled AttributeError and crash the analysis?

> + 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 ...

The summary processing appears to be replaced with a stub that only prints
placeholders. Does this cause the --summary and --summary-extended arguments
to lose their expected functionality?

[ ... ]

> + def _run_live(self) -> None:
> + if not self.args.summary_only:
> + self._print_header()
> +
> + cpus = perf.cpu_map()
> + threads = perf.thread_map(-1)
> + evlist = perf.parse_events("sched:sched_switch", cpus, threads)
> + evlist.config()
> +
> + evlist.open()
> + evlist.mmap()
> + evlist.enable()
> +
> + print("Live mode started. Press Ctrl+C to stop.", file=sys.stderr)
> + try:
> + while True:
> + evlist.poll(timeout=-1)
> + for cpu in cpus:
> + while True:
> + event = evlist.read_on_cpu(cpu)
> + if not event:
> + break
> + if not isinstance(event, perf.sample_event):
> + continue
> + self.process_event(event)

By draining each CPU's ring buffer entirely before moving to the next, are
the events processed out of chronological order?

The Timespans class expects chronological occurrences, so processing them
sequentially by CPU might corrupt the time tracking and result in negative
inter-task times.

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