Re: [PATCH v6 1/2] perf timechart: Add a --live mode to the TUI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf timechart: unescaped quotes in syscall filter in
  LiveRecorder.start()
- [Low] perf timechart: unstarted control_thread.join() in
  LiveRecorder._stop()
--

commit 5178221692a3b293f71ade63d89dfc1258413569
Author: Alice Rogers <alice.mei.rogers@gmail.com>

perf timechart: Add a --live mode to the TUI

This commit adds a --live option to perf timechart and ttimechart.py that
runs perf record to stream scheduler, power, or I/O syscall events over a
pipe and displays a rolling window of recent activity in the TUI.

> diff --git a/tools/perf/python/ttimechart.py b/tools/perf/python/ttimechart.py
> index d7ba0276a9fb4..c71afedb453ed 100755
> --- a/tools/perf/python/ttimechart.py
> +++ b/tools/perf/python/ttimechart.py

[ ... ]

> @@ -1726,52 +2210,630 @@ class TimechartApp(App):
>          self.goto_task(waker, wake_time)
>  
>  
> +class PerfPipeTracker:

[ ... ]

> +class LiveRecorder:

[ ... ]

> +    def start(self) -> None:
> +        """Start perf record, the data is then read from data_fd."""
> +        output = open(self.output, "wb") if self.output else None
> +        # perf record's ends of the control pipes, closed once it is started.
> +        child_fds: List[int] = []
> +        try:
> +            ctl_r, self.ctl_fd = os.pipe()
> +            child_fds.append(ctl_r)
> +            self.ack_fd, ack_w = os.pipe()
> +            child_fds.append(ack_w)
> +            self.wake_r, self.wake_w = os.pipe()
> +            os.set_blocking(self.wake_r, False)
> +            os.set_blocking(self.wake_w, False)
> +            ev_names = self.events()
> +            if any(name.startswith("syscalls:") for name in ev_names):
> +                own_comm = self.set_own_comm()
> +                # common_pid is a thread ID; matching comm excludes all threads
> +                # of this process, and $$ excludes the perf record process that
> +                # replaces sh via exec.
> +                filt = (f'comm != "{own_comm}" && common_pid != $$' if own_comm else
> +                        f"common_pid != {os.getpid()} && common_pid != $$")
> +                ev_args = " ".join(
> +                    f'-e {shlex.quote(name)} --filter "{filt}"'
> +                    if name.startswith("syscalls:") else f"-e {shlex.quote(name)}"
> +                    for name in ev_names
> +                )
> +                script = ('perf_exe=$1; ctl=$2; shift 2; '
> +                          'exec "$perf_exe" record -q -o - -R -c 1 -k mono '
> +                          f'--synth=task --control "$ctl" {ev_args} -a "$@"')
> +                cmd = ["/bin/sh", "-c", script, "sh", self.perf_exe, f"fd:{ctl_r},{ack_w}"]

[Severity: Medium]
Does /bin/sh strip the inner double quotes around own_comm when expanding
--filter "{filt}"?

When set_own_comm() succeeds, interpolating filt into --filter "{filt}"
produces --filter "comm != "ttimechart:..." && common_pid != $$", so the
shell removes the quotes around the comm string before execing perf
record. The kernel tracepoint filter parser requires string operands for
comm to be quoted and rejects the filter with -EINVAL, causing perf
timechart --live -I to fail on startup.

> +                if self.workload:
> +                    cmd += ["--", *self.workload]
> +            else:
> +                # Quiet so that only errors are written to stderr, see error_text().
> +                cmd = [self.perf_exe, "record", "-q", "-o", "-", "-R", "-c", "1", "-k", "mono",
> +                       "--synth=task", "--control", f"fd:{ctl_r},{ack_w}"]
> +                for name in ev_names:
> +                    cmd += ["-e", name]
> +                # Record system wide, as timechart__record() does. Switches into a
> +                # task and idle states aren't seen by events following tasks. With
> +                # a workload, recording stops when it exits, otherwise when stopped.
> +                cmd.append("-a")
> +                if self.workload:
> +                    cmd += ["--", *self.workload]
> +            self.origin = monotonic_ns()
> +            # A new session so that signals for the terminal aren't received.
> +            self.proc = subprocess.Popen(cmd, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE,
> +                                         stderr=self.stderr, pass_fds=(ctl_r, ack_w),
> +                                         start_new_session=True)
> +            assert self.proc.stdout
> +            proc_fd = self.proc.stdout.fileno()
> +            self.grow_pipe(proc_fd)
> +            self.tee_r, tee_w = os.pipe()
> +            self.pump_thread = threading.Thread(target=self.pump,
> +                                                args=(proc_fd, tee_w, output),
> +                                                daemon=True)
> +            try:
> +                self.pump_thread.start()
> +            except BaseException:
> +                self.pump_thread = None
> +                os.close(tee_w)
> +                raise
> +            # Closed by the pump thread.
> +            output = None
> +            self.data_fd = self.tee_r
> +            self.grow_pipe(self.data_fd)
> +            self.control_thread = threading.Thread(target=self.control_loop, daemon=True)
> +            self.control_thread.start()
> +        except BaseException:
> +            self.stop()

[ ... ]

> +    def _stop(self) -> None:
> +        """Stop perf record and the threads, stop_lock must be held."""
> +        proc = self.proc
> +        if proc is not None and proc.poll() is None:
> +            try:
> +                proc.send_signal(signal.SIGINT)
> +                proc.wait(timeout=2)
> +            except subprocess.TimeoutExpired:
> +                # Kill the whole session so a workload isn't left running.
> +                try:
> +                    os.killpg(proc.pid, signal.SIGKILL)
> +                except OSError:
> +                    pass
> +                proc.kill()
> +                proc.wait()
> +        if self.pump_thread and self.pump_thread is not threading.current_thread():
> +            self.pump_thread.join(timeout=1)
> +        if self.control_thread and self.control_thread is not threading.current_thread():
> +            self.control_thread.join(timeout=1)
> +        self.close_control_fds()
> +        self.close_wake_fds()
> +        self.close_data_fds()
> +        self.restore_own_comm()

[Severity: Low]
If self.control_thread.start() fails in LiveRecorder.start(), can
self.control_thread.join() here raise RuntimeError and skip the cleanup
that follows?

Unlike self.pump_thread, self.control_thread is not reset to None if
starting the thread raises an exception, so self.stop() -> self._stop()
attempts to join an unstarted thread and aborts before closing the pipe
file descriptors and restoring /proc/self/comm.

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