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

[sashiko-bot@kernel.org]

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