Re: [PATCH] rtla/tests: cover a CPU hotplug cycle in user-space timerlat mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [Medium] rtla/tests: check-hotplug.sh greps full dmesg for wrong
  WARNING: at pattern
- [Medium] rtla/tests: timerlat.t treats HOTPLUG_SKIP output as a test
  failure
- [Medium] rtla/tests: timerlat.t hotplug_cpu loop can select offline or
  rejected CPU
- [Medium] rtla/tests: check-hotplug.sh does not verify rtla is running
  before hotplug
- [Low] rtla/tests: check-hotplug.sh triggers crashing teardown and has
  wrong comments
- [Low] rtla/tests: timerlat.t hotplug block ignores SUBTESTS, TIMEOUT,
  and STOPONFAIL
--

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

rtla/tests: cover a CPU hotplug cycle in user-space timerlat mode

This commit adds a test script and a timerlat.t test case that offline and
online a CPU during a user-space timerlat run to verify that no kernel
workload thread is spawned on hotplug.

> diff --git a/tools/tracing/rtla/tests/scripts/check-hotplug.sh b/tools/tracing/rtla/tests/scripts/check-hotplug.sh
> new file mode 100755
> index 0000000000000..5287034cd665e
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/scripts/check-hotplug.sh
> @@ -0,0 +1,97 @@

[ ... ]

> +# The stray kthread shows up within a fraction of a second of the CPU coming
> +# back, well before the teardown that crashes, so asserting on it gives a
> +# clean failure instead of taking the kernel down.

[ ... ]

> +"$RTLA" timerlat top -q -d 20s -c 0,"$CPU" -P F:1 >/dev/null 2>&1 &
> +pid=$!
> +
> +# Let rtla create its instance and settle into user-space sampling.
> +sleep 3

[Severity: Medium]
Can check-hotplug.sh report HOTPLUG_OK even when rtla fails to start?

Stdout and stderr are discarded, -u is not passed to require user-space
mode, and the script never checks that $pid is still alive (or that
timerlatu/$CPU exists) after sleep 3 before toggling the CPU.  If rtla
exits at startup, the hotplug cycle runs with no active tracer, finds no
timerlat/* kthreads, and passes.

> +
> +if ! echo 0 >"$ONLINE"; then
> +	echo "HOTPLUG_SKIP: could not offline cpu$CPU"
> +	kill -INT $pid 2>/dev/null; wait $pid 2>/dev/null; exit 0
> +fi
> +if ! echo 1 >"$ONLINE"; then
> +	echo "HOTPLUG_SKIP: could not online cpu$CPU"
> +	kill -INT $pid 2>/dev/null; wait $pid 2>/dev/null; exit 0
> +fi
> +
> +# The hotplug work is queued on the CPU that just came up, and the damage is
> +# done as part of bringing it up: stop_kthread() runs before this point and
> +# the log is already tainted.  Check both, so that a build where the stray
> +# kthread is gone but the teardown still misbehaves does not slip through.
> +stray=''
> +i=0
> +while [ $i -lt 20 ]; do
> +	stray=$(timerlat_threads)
> +	[ -n "$stray" ] && break
> +	sleep 0.2
> +	i=$((i + 1))
> +done
> +
> +# Give the run time to reach its own stop path, which is where the kthread
> +# gets torn down, and then let it finish.
> +sleep 2
> +kill -INT $pid 2>/dev/null
> +wait $pid 2>/dev/null
> +sleep 1

[Severity: Low]
Does stopping rtla here still trigger the crashing teardown when a stray
kthread is detected, and is the comment above the loop accurate about when
stop_kthread() runs?

Bringing the CPU online runs osnoise_hotplug_workfn() -> start_kthread(),
not stop_kthread().  Although the header comment notes that asserting on
the stray kthread avoids taking the kernel down during teardown, the script
still sends SIGINT to rtla while OSN_WORKLOAD is cleared, which runs
stop_kthread() down the user-thread path anyway unless OSNOISE_WORKLOAD is
re-enabled before stopping rtla.

> +
> +warn=$(dmesg 2>/dev/null | grep -c 'WARNING: at')
> +fault=$(dmesg 2>/dev/null | grep -c 'BUG: kernel NULL pointer')
> +
> +if [ -n "$stray" ]; then
> +	echo "stray workload kthread(s): $(echo $stray)"
> +	echo "HOTPLUG_FAIL: cpu$CPU came back online and a timerlat" \
> +		"workload kthread was started in user-space mode"
> +	exit 1
> +fi
> +
> +if [ "$fault" -gt 0 ] || [ "$warn" -gt 0 ]; then
> +	dmesg | grep -A12 'WARNING: at' | tail -30
> +	echo "HOTPLUG_FAIL: the kernel warned ($warn) or faulted ($fault) during the hotplug run"
> +	exit 1
> +fi

[Severity: Medium]
Does grepping dmesg for 'WARNING: at' miss warnings when
CONFIG_DEBUG_BUGVERBOSE=y?

kernel/panic.c:__warn() {
	...
	if (file) {
		pr_warn("WARNING: %s:%d at %pS, CPU#%d: %s/%d\n",
			file, line, caller,
			raw_smp_processor_id(), current->comm, current->pid);
	} else {
		pr_warn("WARNING: at %pS, CPU#%d: %s/%d\n",
			caller,
			raw_smp_processor_id(), current->comm, current->pid);
	}
	...
}

When CONFIG_DEBUG_BUGVERBOSE=y (the default), the file and line number sit
between "WARNING:" and "at", so grep -c 'WARNING: at' always returns 0.
In addition, querying the full dmesg buffer without a pre-test baseline
fails the test if any unrelated warning or fault occurred earlier since
boot, and only grepping for 'WARNING: at' when printing context outputs
nothing when $fault is non-zero and $warn is 0.

> diff --git a/tools/tracing/rtla/tests/timerlat.t b/tools/tracing/rtla/tests/timerlat.t
> index 1d317c851a0fa..e521fc7183c56 100644
> --- a/tools/tracing/rtla/tests/timerlat.t
> +++ b/tools/tracing/rtla/tests/timerlat.t
> @@ -6,6 +6,18 @@ test_begin
>  set_timeout 2m
>  timerlat_sample_event='/sys/kernel/tracing/events/osnoise/timerlat_sample'
>  
> +# Pick a CPU that can be offlined without taking cpu0 down.  A hotplug test
> +# needs one whose "online" file is writable, plus at least one other CPU so
> +# the tool can still run; otherwise skip the case.
> +hotplug_cpu=''
> +for c in /sys/devices/system/cpu/cpu[1-9]*/online; do
> +	[ -w "$c" ] || continue
> +	hotplug_cpu=${c#/sys/devices/system/cpu/cpu}
> +	hotplug_cpu=${hotplug_cpu%/online}
> +	[ "$(cat /sys/devices/system/cpu/online)" != "$hotplug_cpu" ] || continue
> +	break
> +done

[Severity: Medium]
Can this loop pick a CPU that is already offline, or keep a rejected value
in hotplug_cpu?

An offline CPU still has a writable sysfs online file (containing 0).  If
that CPU is selected, rtla's user-space thread on $hotplug_cpu fails
sched_setaffinity() at startup, and check-hotplug.sh leaves the previously
offline CPU online when it finishes.  Also, because hotplug_cpu is assigned
before the continue check, a rejection on the last iteration leaves
hotplug_cpu non-empty after the loop ends.

[ ... ]

> @@ -155,4 +167,34 @@ else
>  fi
>  done
>  
> +# Hotplug test: timerlat in user-space mode samples from the user thread and
> +# has no workload kthread.  Onlining a CPU mid-run must not install one; if
> +# it does, stopping the trace tears that kthread down through the
> +# user-thread path, which warns and then faults.
> +#
> +# The helper drives rtla itself, because the kthread is only torn down after
> +# rtla exits, so there is no "rtla arguments" string to hand to check().
> +if [ -n "$hotplug_cpu" ]; then
> +	ctr=$((ctr + 1))
> +	if [ -n "$TEST_COUNT" ]; then
> +		[ "$NO_RESET_OSNOISE" == 1 ] || reset_osnoise
> +		tmpdir=$(mktemp -d)
> +		pushd "$tmpdir" >/dev/null
> +
> +		result=$("$testdir"/scripts/check-hotplug.sh "$RTLA" "$hotplug_cpu" 2>&1)
> +		exitcode=$?

[Severity: Low]
Does open-coding this test block bypass SUBTESTS, TIMEOUT, and STOPONFAIL
from engine.sh?

Unlike check() in tools/tracing/rtla/tests/engine.sh, this block only tests
[ -n "$TEST_COUNT" ], so running timerlat.t with SUBTESTS set to select an
unrelated test still runs the hotplug test, check-hotplug.sh is not
bounded by $TIMEOUT, and $STOPONFAIL is ignored on failure.

> +
> +		if [ "$exitcode" -eq 0 ] && grep -q '^HOTPLUG_OK' <<< "$result"; then
> +			echo "ok $ctr - top survives a CPU hotplug cycle"
> +		else
> +			echo "not ok $ctr - top survives a CPU hotplug cycle"
> +			echo "$result" | while read -r line; do echo "# $line"; done
> +			printf "#\n# exit code %s\n" "$exitcode"
> +		fi

[Severity: Medium]
Does this treat HOTPLUG_SKIP from check-hotplug.sh as a test failure?

When check-hotplug.sh cannot offline or online the CPU at runtime, it
prints "HOTPLUG_SKIP: ..." and exits with 0.  Because only ^HOTPLUG_OK is
matched here, a skip falls into the else branch and emits "not ok".

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