Re: [PATCH v2 5/5] selftests/perf_events: add AUX kernel API selftest script

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] selftests: bash syntax error due to missing line continuation
- [Medium] selftests: manual C compiler invocation bypasses Makefile and CFLAGS
- [Low] selftests: incorrect translation of KSFT_SKIP exit code to failure
- [Low] selftests: git dependency for source tree path resolution fails in tarballs
- [Low] selftests: untracked TEST_GEN_PROGS binary missing from .gitignore
- [Low] selftests: brittle source code grep tests create maintenance burden
--

commit b62175593d8e1e9cdad05106f3f506af47e718eb
Author: Kunwu Chan <kunwu.chan@gmail.com>

selftests/perf_events: add AUX kernel API selftest script

This commit adds a shell script to run the perf_aux_kernel KUnit suite, userspace
regression program, hardware tests, and static checks. It is designed to work
both in-tree and from an installed selftest location, with results written to a
temporary directory.

> diff --git a/tools/testing/selftests/perf_events/Makefile b/tools/testing/selftests/perf_events/Makefile
> index fbafbd43f097e..9b82c7ee1238a 100644
> --- a/tools/testing/selftests/perf_events/Makefile
> +++ b/tools/testing/selftests/perf_events/Makefile
> @@ -4,4 +4,5 @@ LDFLAGS += -lpthread
>  
>  TEST_GEN_PROGS := sigtrap_threads remove_on_exec watermark_signal mmap
>  TEST_GEN_PROGS += aux_kernel_usermode

[Severity: Low]
Is aux_kernel_usermode missing from tools/testing/selftests/perf_events/.gitignore?

Building the selftests will leave this untracked binary in the working tree,
polluting the repository status.

> +TEST_PROGS += aux_kernel.sh
>  include ../lib.mk

[ ... ]

> diff --git a/tools/testing/selftests/perf_events/aux_kernel.sh b/tools/testing/selftests/perf_events/aux_kernel.sh
> new file mode 100755
> index 0000000000000..af7f2e8d5d094
> --- /dev/null
> +++ b/tools/testing/selftests/perf_events/aux_kernel.sh

[ ... ]

> +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
> +ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel 2>/dev/null || true)"

[Severity: Low]
Will this git dependency fail when executed from a release tarball?

When testing from a kernel source tarball, the .git directory is absent and
the git rev-parse command will fail. This causes $ROOT to be empty,
silently skipping the source-level analysis despite the full source tree
being available. Traversing parent directories via relative paths is a more
robust approach.

[ ... ]

> +	echo "  Building aux_kernel_usermode with ${CC}..."
> +	if "$CC" -o "$UMODE_BIN" "$UMODE_SRC" -Wall -Wextra -O2; then

[Severity: Medium]
Does this manual compilation bypass the kselftest build infrastructure?

By avoiding make, variables such as cross-compilation CFLAGS,
KHDR_INCLUDES, and LDFLAGS are discarded. For instance, LDFLAGS += -lpthread
is explicitly set in the subsystem Makefile but is omitted here. Test
dependencies should rely entirely on the TEST_GEN_PROGS infrastructure.

[ ... ]

> +	if [[ "$UMODE_RET" -ne 0 ]]; then
> +		fail "Userspace API regression: C program failed"

[Severity: Low]
Does this check treat standard test skips as fatal failures?

In the kselftest framework, a test program returns 4 (KSFT_SKIP) when
prerequisites are unmet (such as missing permissions or unsupported
features). Evaluating any non-zero exit code as a failure improperly fails
the overall test suite. Should this explicitly check for exit code 4 and
translate it into a skip?

> +	else
> +		pass "Userspace API regression: $UMODE_PASS passed, $UMODE_FAIL failed,"
> +			" $UMODE_SKIP skipped"

[Severity: Medium]
Is there a missing line continuation backslash here?

Because set -e is active at the top of the script, bash parses the second
line (" $UMODE_SKIP skipped") as an independent command, causing a fatal
command not found error. This immediately terminates the test script,
preventing subsequent hardware AUX tests, DAMON checks, and the final
summary from executing.

[ ... ]

> +# ---- Section 6: Memory ordering and concurrency checks ----
> +echo ""
> +echo "--- 6. Memory Ordering & Concurrency ---"
> +
> +if [[ "$HAVE_SRC" -ne 1 ]]; then
> +	skip "Memory ordering checks" "kernel source not available (installed run)"
> +else
> +
> +# Verify smp_rmb() in perf_event_aux_head()
> +if grep -q 'smp_rmb' "$SRC_EVENTS/ring_buffer.c" 2>/dev/null; then
> +	pass "perf_event_aux_head(): smp_rmb() barrier present"

[Severity: Low]
Do these brittle source code grep tests create a maintenance burden?

This tightly couples the selftest to an exact textual representation rather
than verifying behavioral API contracts. Routine kernel refactoring (such as
renaming variables, extracting logic to helpers, or changing barrier
primitives) will falsely fail these tests. Source-level constraints should
be enforced by static analysis tools (like sparse or smatch) rather than
regex matches in the selftest pipeline.

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