Re: [PATCH 5/5] selftests/sched_ext: Bound exit waits and report reload_loop failures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/sched_ext: leak of BPF link and skeleton on timeout in exit.c
- [Low] selftests/sched_ext: dead code in reload_loop failure reporting
--

commit b49c5aa1d20c7dfda172f19617a72ddbef7da918
Author: Tao Cui <cuitao@kylinos.cn>

selftests/sched_ext: Bound exit waits and report reload_loop failures

This commit replaces unbounded waits for scheduler exit in multiple tests
with an iteration-bound timeout using SCX_TIMED_WAIT_UEI(). It also updates
reload_loop to track and report attach failures, failing the test on
unexpected errors or if every attach attempt fails.

> diff --git a/tools/testing/selftests/sched_ext/exit.c b/tools/testing/selftests/sched_ext/exit.c
> index 01b17092d5c8f..9270d8e79eb63 100644
> --- a/tools/testing/selftests/sched_ext/exit.c
> +++ b/tools/testing/selftests/sched_ext/exit.c
> @@ -43,8 +43,8 @@ static enum scx_test_status run(void *ctx)
>  		}
>  
>  		/* Assumes uei.kind is written last */
> -		while (skel->data->uei.kind == EXIT_KIND(SCX_EXIT_NONE))
> -			sched_yield();
> +		SCX_FAIL_IF(!SCX_TIMED_WAIT_UEI(skel, uei, 10000),
> +			    "Timed out waiting for scheduler to exit");
>  

[Severity: Medium]
Does this SCX_FAIL_IF() check leak the BPF link and skeleton on timeout?

In run(), skel and link are allocated locally inside the loop. If the wait
times out, SCX_FAIL_IF() will immediately return SCX_TEST_FAIL, bypassing
the bpf_link__destroy() and exit__destroy() calls at the end of the loop block.

Failing to destroy the BPF link leaves the scheduler attached to the kernel,
which breaks test isolation. Subsequent tests in the runner might then fail
with -EBUSY because only one sched_ext scheduler can be attached at a time.

>  		SCX_EQ(skel->data->uei.kind, EXIT_KIND(SCX_EXIT_UNREG_BPF));
>  		SCX_EQ(skel->data->uei.exit_code, tc);

[ ... ]

> diff --git a/tools/testing/selftests/sched_ext/reload_loop.c b/tools/testing/selftests/sched_ext/reload_loop.c
> index ecff86f59e6c7..4ae6fb43b6c5c 100644
> --- a/tools/testing/selftests/sched_ext/reload_loop.c
> +++ b/tools/testing/selftests/sched_ext/reload_loop.c
[ ... ]
> @@ -60,7 +78,24 @@ static enum scx_test_status run(void *ctx)
>  	}
>  
>  	SCX_FAIL_IF(pthread_join(threads[0], &ret), "thread 0 failed");
> +	failures += (unsigned long)ret;
> +
>  	SCX_FAIL_IF(pthread_join(threads[1], &ret), "thread 1 failed");
> +	failures += (unsigned long)ret;
> +
> +	SCX_FAIL_IF(unexpected_errno, "unexpected attach errno %d",
> +		    unexpected_errno);
> +
> +	if (failures) {
> +		printf("reload_loop: %lu attach failures%s\n",
> +		       (unsigned long)failures,
> +		       unexpected_errno ? ", unexpected errno" :
> +					  " (EBUSY: scheduler slot busy)");

[Severity: Low]
Is the true branch of this ternary condition dead code?

The SCX_FAIL_IF(unexpected_errno, ...) check slightly above will immediately
return from the function if unexpected_errno is non-zero. As a result, when
execution reaches this printf statement, unexpected_errno must evaluate to
false.

The true branch (", unexpected errno") can never be executed, preventing the
unexpected errno from being reported in the summary as intended.

> +		fflush(stdout);
> +	}
> +
> +	SCX_FAIL_IF(failures == RELOAD_ITERS * RELOAD_THREADS,
> +		    "every attach failed");
>  
>  	return SCX_TEST_PASS;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009134104.296891-1-cui.tao@linux.dev?part=5