Re: [PATCH] rtla: add tests for tracefs instance cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] rtla/tests: missing prerequisite checks and swallowed SKIP in cleanup.t
- [Medium] rtla/tests: SIGINT race with set_signals() and leaked process in check-cleanup.sh
- [Medium] rtla/tests: missing exit code checks and flawed action-failure test in check-cleanup.sh
- [Low] rtla/tests: unused TIMEOUT, skipped reset_osnoise(), and dead code in cleanup test
--

commit 5c3de0149491a2031a1d6a6944918bec1fa8f6ee
Author: Chaosheng Qu <chaosheng.qu.linux@gmail.com>

rtla: add tests for tracefs instance cleanup

This commit adds cleanup.t and a helper script check-cleanup.sh to verify
that rtla removes its tracefs instances on SIGINT, timeout, action failure,
and repeated interrupted runs.

> diff --git a/tools/tracing/rtla/tests/cleanup.t b/tools/tracing/rtla/tests/cleanup.t
> new file mode 100755
> index 0000000000000..46e69c1917924
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/cleanup.t
> @@ -0,0 +1,55 @@
[ ... ]
> +source tests/engine.sh
> +test_begin
> +
> +set_timeout 2m
> +
> +#
> +# These cases need to run rtla in the background and signal it, which
> +# engine.sh's check() (one foreground command) cannot express.
> +#
> +run_cleanup_cases() {
> +	local helper="$testdir/scripts/check-cleanup.sh"
> +	local out rc
> +
> +	ctr=$((ctr + 1))
> +	[ -z "$TEST_COUNT" ] && return
> +
> +	if [ ! -x "$helper" ]; then
> +		echo "not ok $ctr - cleanup helper is not executable"
> +		return
> +	fi
> +
> +	out=$("$helper" "$RTLA" 2>&1)

[Severity: Low]
Calling set_timeout 2m sets the TIMEOUT variable in tests/engine.sh, and
engine.sh's check() normally runs reset_osnoise before executing each test
under $TIMEOUT.

Because run_cleanup_cases() bypasses check() and invokes $helper directly,
$TIMEOUT is never applied (allowing wait "$pid" in check-cleanup.sh to hang
indefinitely if rtla does not exit) and global osnoise settings and
tracing_thresh are not reset before the test runs. Should run_cleanup_cases()
call reset_osnoise and prefix the helper invocation with $TIMEOUT?

> +	rc=$?
> +	if [ $rc -eq 0 ]; then
> +		# Surface known issues as TAP comments so they stay visible
> +		# even when the case passes.
> +		printf '%s\n' "$out" | grep '^TODO: ' | while read -r line; do
> +			echo "# ${line#TODO: }"
> +		done
> +		echo "ok $ctr - instances cleaned up on SIGINT, timeout and action failure"

[Severity: Medium]
When check-cleanup.sh prints "SKIP: ..." and exits with 0, run_cleanup_cases()
only greps for '^TODO: ' and emits a plain "ok $ctr" line instead of a TAP
"# SKIP" directive. Should run_cleanup_cases() detect '^SKIP: ' in $out and
emit "ok $ctr - ... # SKIP ..."?

> +	else
> +		echo "not ok $ctr - instances cleaned up on SIGINT, timeout and action failure"
[ ... ]
> diff --git a/tools/tracing/rtla/tests/scripts/check-cleanup.sh b/tools/tracing/rtla/tests/scripts/check-cleanup.sh
> new file mode 100755
> index 0000000000000..1ead5ce3c5177
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/scripts/check-cleanup.sh
> @@ -0,0 +1,187 @@
[ ... ]
> +check_instances_absent() {
> +	_desc=$1
> +	_left=""
> +	for i in osnoise_top osnoise_hist timerlat_top timerlat_hist; do
> +		[ -d "$INSTANCES/$i" ] && _left="$_left $i"
> +	done
> +	if [ -z "$_left" ]; then
> +		pass "$_desc: no leftover instance"
> +	else
> +		fail "$_desc: leftover instance(s):$_left"
> +	fi
> +}

[Severity: Low]
check_instances_absent() is defined here, but it is never called anywhere in
check-cleanup.sh. Should this unused function be removed or used in the test
cases below?

> +
> +if [ ! -d "$INSTANCES" ]; then
> +	log "SKIP: tracefs not mounted at $TRACEFS"
> +	exit 0
> +fi

[Severity: Medium]
The commit message states:

  The test needs root, tracefs and CONFIG_OSNOISE_TRACER, and skips cleanly
  when tracefs is not mounted writable.

However, this check only tests whether $INSTANCES is a directory. If the test
runs without root privileges, with a read-only tracefs mount, or on a kernel
without CONFIG_OSNOISE_TRACER ($TRACEFS/osnoise), [ ! -d "$INSTANCES" ] can
still be false, causing all test cases to run and fail instead of skipping.
Should this also verify [ -w "$INSTANCES" ] and [ -d "$TRACEFS/osnoise" ]?

> +
> +# ---------------------------------------------------------------------------
> +# 1. SIGINT during a long run
> +# ---------------------------------------------------------------------------
> +cleanup_all
> +$RTLA osnoise top -q -d 30s -P F:1 >/dev/null 2>&1 &
> +pid=$!
> +wait_present "$INSTANCES/osnoise_top" 10000
> +sleep 5
> +
> +if [ ! -d "$INSTANCES/osnoise_top" ]; then
> +	fail "SIGINT: instance was never created (rtla did not start?)"

[Severity: Medium]
If $INSTANCES/osnoise_top is not present here, the failure branch logs a
failure without killing or waiting on $pid (unlike Case 2 below). Can this
leave the background rtla process running while the remaining test cases
execute?

> +else
> +	pass "SIGINT: instance created while running"
[ ... ]
> +# ---------------------------------------------------------------------------
> +# 3. rtla's own -d timeout path
> +# ---------------------------------------------------------------------------
> +cleanup_all
> +$RTLA osnoise top -q -d 2s -P F:1 >/dev/null 2>&1
> +rc=$?
> +if wait_gone "$INSTANCES/osnoise_top"; then
> +	pass "self-timeout: instance removed (exit=$rc)"
> +else
> +	fail "self-timeout: instance still present (exit=$rc)"
> +fi
> +
> +# ---------------------------------------------------------------------------
> +# 4. shell action failure (-a / --on-threshold shell,command=...)
> +# ---------------------------------------------------------------------------
> +cleanup_all
> +$RTLA osnoise top -q -d 5s -P F:1 -r 1000 -S 1 \
> +	--on-threshold 'shell,command="/bin/false"' >/dev/null 2>&1
> +rc=$?
> +if wait_gone "$INSTANCES/osnoise_top"; then
> +	pass "action-failure: instance removed (exit=$rc)"
> +else
> +	fail "action-failure: instance still present (exit=$rc)"
> +fi

[Severity: Medium]
Can Cases 3 and 4 pass without exercising their intended cleanup paths?

First, neither Case 3 nor Case 4 verifies $rc or checks that osnoise_top was
actually created. Because cleanup_all removes $INSTANCES/osnoise_top before
running $RTLA, if $RTLA fails during startup before creating the instance,
wait_gone "$INSTANCES/osnoise_top" immediately succeeds and reports a pass.

Second, in Case 4, -r 1000 sets a 1ms runtime per 1s period (0.1% duty cycle)
with SCHED_FIFO:1. On an idle system, 5 periods can finish without
accumulating 1us of noise, so rtla exits via the 5s duration timeout (rc=0)
rather than hitting the stop-tracing threshold (rc=2) and never runs the
shell action.

Third, in tools/tracing/rtla/src/common.c:common_threshold_handler():

	actions_perform(&tool->params->threshold_actions);

	if (!should_continue_tracing(tool->params))
		/* continue flag not set, break */
		return 0;

common_threshold_handler() ignores the return value of actions_perform() and
stops tracing whenever continue_flag is not set, which is only set if a
subsequent "--on-threshold continue" action is reached after the shell action
succeeds. Without "--on-threshold continue" and checking [ "$rc" -eq 2 ],
Case 4 does not distinguish a failing shell action from a succeeding one or
from a normal timeout.

> +
> +# ---------------------------------------------------------------------------
> +# 5. repeated interrupted runs must not poison the next run
> +# ---------------------------------------------------------------------------
> +cleanup_all
> +for n in 1 2 3; do
> +	$RTLA osnoise top -q -d 20s -P F:1 >/dev/null 2>&1 &
> +	pid=$!
> +	sleep 1
> +	kill -INT "$pid" 2>/dev/null
> +	wait "$pid" 2>/dev/null
> +done

[Severity: Medium]
In tools/tracing/rtla/src/common.c:run_tool(), the tracefs instance is
created at the beginning of ops->init_tool(params) -> trace_instance_init(),
before tracefs_local_events(NULL), ops->apply_config(tool),
enable_tracer_by_name(), set_comm_sched_attr(), and ops->enable(tool), while
set_signals(params) only installs the SIGINT handler afterward:

	tool = ops->init_tool(params);
	...
	retval = ops->enable(tool);
	if (retval)
		goto out_trace;

	tool->start_time = time(NULL);
	set_signals(params);

In Case 5, each background invocation of $RTLA is sent SIGINT after a fixed
"sleep 1" without wait_present or waiting for set_signals() to complete:

1. Under non-interactive POSIX /bin/sh, background jobs started with '&'
   inherit SIG_IGN for SIGINT until set_signals() overrides it. If rtla is
   still initializing after 1 second (for example while parsing tracefs event
   formats or setting per-CPU osnoise kthreads to SCHED_FIFO:1), kill -INT is
   ignored and wait "$pid" blocks for the full 20s duration timer without
   testing SIGINT interruption.
2. If SIGINT is not ignored in the caller's environment and arrives after
   create_instance() but before set_signals(), rtla is terminated by the
   default SIGINT disposition before stop_trace() is installed, leaking the
   instance.

Should this loop wait for $INSTANCES/osnoise_top to be present and for
set_signals() to be active before sending SIGINT, and verify wait_gone on
each iteration?

> +
> +$RTLA osnoise top -q -d 2s -P F:1 >/dev/null 2>&1
[ ... ]

 base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
-- 
2.51.0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017122359.402696-1-chaosheng.qu.linux@gmail.com?part=1