Re: [PATCH v2 2/2] perf test: Test perf timechart --live

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/tests: test hang due to missing FakeRecorder cleanup on error paths
--

commit 160f73df934f65800928fbfdebb9b1aabb7e4e95
Author: Alice Rogers <alice.mei.rogers@gmail.com>

perf test: Test perf timechart --live

This commit extends test_ttimechart_python.sh to validate the --live option.
It exercises LiveRecorder's perf record subprocess, control pipe, output file,
and introduces a synthetic live UI test for checking the rolling window,
pruning, and zoom tracking features.

> diff --git a/tools/perf/tests/shell/test_ttimechart_python.sh b/tools/perf/tests/shell/test_ttimechart_python.sh
> index 87b91ed8fc167..495060586cfc2 100755
> --- a/tools/perf/tests/shell/test_ttimechart_python.sh
> +++ b/tools/perf/tests/shell/test_ttimechart_python.sh

[ ... ]

> +test_live_synthetic() {
> +	echo "ttimechart live UI test (synthetic)"
> +	# Exercise live mode's rolling window, pruning, pause/resume and
> +	# following with a fake recorder, which doesn't need tracepoint
> +	# permissions.
> +	if ! "$PYTHON" - > "${output}" 2>&1 <<'EOF'
> +import asyncio
> +import threading

[ ... ]

> +        cur = 0
> +        while not rec.stopped.is_set() and not rec.finish:
> +            now = monotonic_ns()
> +            if rec.paused:
> +                time.sleep(0.01)
> +                continue

[ ... ]

> +    tc.read_events = fake_read
> +    app = tc.TimechartApp("live", False, False, [], live=rec, window=1.0)
> +    async with app.run_test(size=(120, 40)) as pilot:
> +        for _ in range(300):
> +            with app.data.lock:
> +                if app.data.pruned_to > rec.origin + 3000:
> +                    break
> +            await pilot.pause(0.05)
> +        with app.data.lock:
> +            if app.data.pruned_to <= rec.origin + 3000 or 300 in app.data.tasks:
> +                raise RuntimeError("Task outside the window was not pruned")

[Severity: Medium]
Will this cause the test suite to hang if an exception is raised here?

Because fake_read runs in a background thread and its loop relies on
rec.stopped.is_set() or rec.finish, an exception (like RuntimeError
on a failed assertion) will abort the test logic without executing
rec.finish = True or calling rec.stop().

This lack of cleanup leaves the background thread looping indefinitely and
prevents the Python interpreter from exiting. Other tests introduced in this
patch like test_no_events and test_live_record use try...finally blocks
to ensure rec.stop() is always called. 

Should a similar pattern be applied here to guarantee termination?

> +            starts = [t.segs.starts[0] for t in app.data.tasks.values() if len(t.segs)]
> +            if not starts or min(starts) < app.data.pruned_to:
> +                raise RuntimeError(f"Unpruned segments: {starts} < {app.data.pruned_to}")

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