Re: [PATCH v2] rtla: clean up tracefs instances on SIGTERM, and test the abnormal exits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] rtla: late set_signals() in run_tool() leaks instances on
  startup signals
- [Medium] rtla/tests: false passes and leaked child processes in
  check-cleanup.sh
- [Low] rtla/tests: run_cleanup_cases() relies on reset_osnoise() return
  status
- [Low] rtla: inaccurate commit log and test comments, and wrong
  instance names
--

commit 7feddf6b326eb1b197836a5ea9675a50618c3123
Author: Chaosheng Qu <chaosheng.qu.linux@gmail.com>

> rtla: clean up tracefs instances on SIGTERM, and test the abnormal exits
>
> rtla: clean up tracefs instances on SIGTERM, and test the abnormal exits
>
> rtla creates a tracefs instance for each run and removes it on the normal
> exit path. If the process is interrupted, killed by its own -d timer, or if
> an --on-threshold shell action fails, the instance is left behind and the
> next invocation finds it in use.
>
> Install a SIGTERM handler next to the existing SIGINT one, so the same
> cleanup runs for both, and add a test that walks each of those paths and
> checks that the instance is gone afterwards.
>
> The test harness had two bugs, both reported by sashiko:
>
>   - The availability probe ran "$RTLA" --version. rtla has no --version:
>     main() only understands -h/--help and the three subcommands, and
>     anything else falls through to rtla_usage(), which exits 129. The probe
>     therefore always failed and the whole suite reported SKIP, so none of
>     these cases ever ran. Probe --help instead, require exit 129 and the
>     version banner, which is what the other rtla tests already do.
>
>   - The TAP wrapper treated any line starting with "SKIP: " from the helper
>     as a group-level skip. A subtest that could not cross the threshold on
>     an idle machine printed one of those, which marked the entire test as
>     skipped and left the loop below it unreachable. Per-case skips now say
>     "note:", and only real group-level skips keep the SKIP: prefix.
>
> Assisted-by: LLM
> Signed-off-by: Chaosheng Qu <chaosheng.qu.linux@gmail.com>

[Severity: Low]
Does the commit message accurately describe the problem being fixed?

The subject line is repeated at the top of the commit body, and the first
paragraph states that SIGINT, the -d timer, and failing --on-threshold shell
actions previously left the tracefs instance behind.  Prior to this patch,
set_signals() already handled SIGINT and SIGALRM (-d), and failing
--on-threshold shell actions already returned through common_threshold_handler()
to the out_free cleanup path in run_tool(); only SIGTERM lacked a handler.
Should this also include:

  Fixes: 2f3172f9dd58 ("tools/rtla: Consolidate code between osnoise/timerlat and hist/top")

In addition, the "The test harness had two bugs" section in the commit message
and the matching --version comment in check-cleanup.sh describe fixes to an
earlier unmerged draft of this patch rather than pre-existing repository code
(which belongs below the --- separator).  In check-cleanup.sh, the header
comment also refers to the script as rtla_cleanup_test.sh, the --help comment
says main() knows -usage (main() only checks -h and --help), the action-failure
comment says -S yields exit 2 when the action fails (run_tool() exits with 2
whenever -S stops tracing regardless of whether actions_perform() succeeded),
and cleanup_all() checks hwnoise_top (hwnoise uses osnoise_top) while omitting
osnoise_trace and timerlat_aa.

> diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c
> index ac4172bcc4444..e91d718c728b6 100644
> --- a/tools/tracing/rtla/src/common.c
> +++ b/tools/tracing/rtla/src/common.c
> @@ -38,6 +38,7 @@ static void stop_trace(int sig)
>  static void set_signals(struct common_params *params)
>  {
>  	signal(SIGINT, stop_trace);
> +	signal(SIGTERM, stop_trace);
>  	if (params->duration) {
>  		signal(SIGALRM, stop_trace);
>  		alarm(params->duration);

[Severity: Medium]
Can a SIGTERM or SIGINT delivered during startup still terminate rtla before
set_signals() is installed and leak the tracefs instances?

In run_tool(), ops->init_tool() creates the primary tracefs instance before
tracefs_local_events() parses tracefs events, ops->apply_config() modifies
global osnoise settings, enable_tracer_by_name() spawns per-CPU kernel
threads, set_comm_sched_attr() scans /proc, osnoise_init_trace_tool() creates
osnoise_trace, and ops->enable() creates timerlat_aa and sleeps for
params->common.warmup seconds:

tools/tracing/rtla/src/common.c:run_tool() {
	tool = ops->init_tool(params);
	...
	retval = ops->enable(tool);
	if (retval)
		goto out_trace;

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

Because set_signals() is only called after ops->enable() returns, any SIGTERM
or SIGINT during initialization or the warmup sleep (where osnoise_enable()
and timerlat_enable() check stop_tracing) hits the default signal disposition
and exits without running out_free cleanup.

In check-cleanup.sh, wait_running() only waits for $INSTANCES/osnoise_top to
appear and sleeps 1 second before signaling rtla.  Because the instance
directory is created at the start of trace_instance_init() before event
parsing and kthread startup finish, on slower or loaded systems kill -TERM can
arrive before set_signals() and leak osnoise_top, while in a non-interactive
shell background jobs inherit SIG_IGN for SIGINT until set_signals() runs, so
an early kill -INT is ignored until the -d timer expires.

[ ... ]

> diff --git a/tools/tracing/rtla/tests/cleanup.t b/tools/tracing/rtla/tests/cleanup.t
> new file mode 100644
> index 0000000000000..176b1a96f87d5
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/cleanup.t
> @@ -0,0 +1,70 @@

[ ... ]

> +	# check() normally does this before every test. Do the same here, both to
> +	# leave a clean tracer state and to make a hung helper terminate.
> +	reset_osnoise || {
> +		echo "ok $ctr - instances cleaned up on SIGINT, timeout and action failure # SKIP tracefs is not writable"
> +		return
> +	}

[Severity: Low]
Does reset_osnoise return a non-zero status when tracefs is not writable or
when CONFIG_OSNOISE_TRACER is disabled?

In tests/engine.sh, reset_osnoise() only fails if pushd /sys/kernel/tracing
fails:

tools/tracing/rtla/tests/engine.sh:reset_osnoise() {
	pushd /sys/kernel/tracing >/dev/null || return 1
	...
	cd ../osnoise
	echo all > cpus
	...
	popd >/dev/null
}

When /sys/kernel/tracing is searchable by an unprivileged user or
/sys/kernel/tracing/osnoise is absent, the writes and cd ../osnoise fail with
stderr errors (and redirect into /sys/kernel/tracing/instances if cd fails),
while popd still returns 0.

> +	timeout_cmd=${TIMEOUT:-timeout -v -k 15s 2m}
> +
> +	out=$($timeout_cmd "$helper" "$RTLA" 2>&1)
> +	rc=$?

[ ... ]

> 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..c27db96cd556a
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/scripts/check-cleanup.sh
> @@ -0,0 +1,211 @@

[ ... ]

> +if [ ! -x "$RTLA" ]; then
> +	log "SKIP: $RTLA is not executable"
> +	exit 0
> +fi
> +help_out=$("$RTLA" --help 2>&1)
> +rtla_rc=$?
> +if [ "$rtla_rc" -ne 129 ] ||
> +   ! printf '%s\n' "$help_out" | grep -q 'rtla version'; then
> +	log "SKIP: $RTLA is not usable (--help exited $rtla_rc, no version banner)"
> +	exit 0
> +fi
> +
> +# Kill a background rtla if a case fails before reaping it.
> +stop_pid() {
> +	[ -n "$1" ] || return
> +	kill -KILL "$1" 2>/dev/null
> +	wait "$1" 2>/dev/null
> +}

[ ... ]

> +	kill -INT "$pid" 2>/dev/null
> +	wait "$pid" 2>/dev/null
> +	if wait_gone "$INSTANCES/osnoise_top"; then
> +		pass "SIGINT: instance removed after interrupt"
> +	else
> +		fail "SIGINT: instance still present after interrupt"
> +	fi

[Severity: Medium]
Can check-cleanup.sh report false passes or leave background rtla processes
running after a timeout?

First, when $RTLA is missing, not executable, or fails the --help check,
check-cleanup.sh logs SKIP: and exits 0, which cleanup.t turns into a skipped
test rather than a failure when the binary under test is broken.

Second, the SIGINT, SIGTERM, and repeated-interrupt cases ignore the exit
status of wait "$pid", so if rtla exits with an error (which also removes the
instance via out_free) instead of cleanly handling the signal and exiting 0,
wait_gone still reports PASS.

Third, check-cleanup.sh installs no EXIT/INT/TERM trap to kill a live
background $pid and run cleanup_all, and cleanup.t does not print $out when
timeout exits with 124 or 137; if the helper times out, the diagnostic output
is lost and the orphaned rtla child and its tracefs instance remain active
during subsequent tests.

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