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

sashiko-bot
2026-04-20T00:46:09Z

Sashiko AI review found 7 potential issue(s):
- [High] Incomplete implementation of the summary feature
- [High] Unbounded memory growth in live mode with summary enabled
- [Medium] Process ID (TGID) is incorrectly tracked as Thread ID (TID)
- [Medium] ANSI color escape codes are injected into CSV output
- [Medium] Conflicting headers and delimiters corrupt the CSV output
- [Medium] Mutually exclusive filter arguments (limit-to-tasks is ignored)
- [Medium] Task history lists are erroneously truncated for extended/only summary options
--

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

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

This commit ports task-analyzer.py to the perf.session python module
and refactors it into a class-based architecture. It adds support for live
mode and file mode while keeping previous command-line options.

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

[ ... ]

> + def _check_color(self) -> None:
> + """Check if color should be enabled."""
> + if sys.stdout.isatty() and self.args.stdio_color != "never":
> + return
> + TaskAnalyzer._COLORS = {k: "" for k in TaskAnalyzer._COLORS}

Does this inject ANSI color escape codes into the CSV output?

When using --csv, sys.stdout might still be attached to a terminal even
though the tabular data is written to a file. This would cause the check to
pass and corrupt the CSV fields with color codes.

[ ... ]

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

Should this check include self.args.summary_extended and
self.args.summary_only?

If those flags are used without --summary, the arrays are prematurely
truncated, which might break calculations that rely on the full history.

Could this also cause unbounded memory growth in live mode?

If self.args.summary is set, _record_cleanup() doesn't truncate the lists.
Since _run_live() runs an infinite loop, it looks like the self.db arrays
will grow forever and eventually exhaust memory.

[ ... ]

> + def _limit_filtered(self, tid: int, pid: int, comm: str) -> bool:
> + """Filter tasks based on CLI arguments."""
> + if self.args.filter_tasks:
> + if (str(tid) in self.args.filter_tasks or
> + str(pid) in self.args.filter_tasks or
> + comm in self.args.filter_tasks):
> + return True
> + return False
> + if self.args.limit_to_tasks:
> + if (str(tid) in self.args.limit_to_tasks or
> + str(pid) in self.args.limit_to_tasks or
> + comm in self.args.limit_to_tasks):
> + return False
> + return True
> + return False

Are --filter-tasks and --limit-to-tasks meant to be mutually exclusive?

If self.args.filter_tasks is specified but no match is found, the function
returns False early, meaning the limit_to_tasks check is unreachable.

[ ... ]

> + 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
> +
> + next_comm = self._filter_non_printable(next_comm)
> +
> + # Task finish for previous task
> + self._handle_task_finish(prev_pid, common_cpu, time_ns, prev_pid)

Are we incorrectly tracking the Thread ID (TID) as the Process ID (TGID) here?

sample.prev_pid corresponds to the kernel's thread ID, but it's passed as
both the tid and pid arguments to _handle_task_finish(). This loses the
actual Process ID information and breaks the PID column output.

[ ... ]

> + def print_summary(self) -> None:
> + """Calculate and print summary."""
> + if not (self.args.summary or self.args.summary_extended or self.args.summary_only):
> + 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 ...

Does this intentionally omit the full summary logic?

The --summary flags seem to be broken compared to the legacy version because
the actual calculation and output are missing.

[ ... ]

> + def run(self) -> None:
> + """Run the session."""
> + with self.open_output(self.args.csv, sys.stdout) as fd_task:
> + with self.open_output(self.args.csv_summary, sys.stdout) as fd_sum:
> + self.fd_task = fd_task
> + self.fd_sum = fd_sum
> + if self.args.csv:
> + fd_task.write("Comm;Out-Out;\n")
> + if self.args.csv_summary:
> + fd_sum.write("Comm;Out-Out;\n")
> +
> + if not os.path.exists(self.args.input) and self.args.input == "perf.data":
> + self._run_live()
> + else:
> + self._run_file()

Does this corrupt the CSV file by writing two conflicting headers?

This writes a hardcoded header with a semicolon delimiter, but immediately
after, _run_file() calls _print_header() which writes comma-separated
headers. This leaves the file with multiple contradictory headers.

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