Re: [PATCH bpf-next v2 3/3] selftests/bpf: libarena: parallel test harness and spmc parallel selftest

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Worker threads can spin indefinitely without checking `test_abort`, causing a soft lockup.
- [Medium] Missing BPF FD Validation after manual program lookup.
- [Medium] Thread synchronization loops use a spin limit that is too low, causing flakiness.
- [Low] Returning a boolean flag as an error code.
--

commit 4565a6a5313dffad1dda425572fe86afb75dd583
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: parallel test harness and spmc parallel selftest
    
Add a parallel test for the SPMC Lev-Chase workstealing queue. The queue
is built to be wait-free even when there are multiple consumers, and
the parallel selftest provides a signal on whether the queue behaves
correctly when stress tested.

To support the test, this patch includes a test harness for parallel
selftests. The spmc selftest acts as an example of the naming and other
conventions expected by the harness.

> diff --git a/tools/testing/selftests/bpf/libarena/selftests/test_parallel_spmc.bpf.c b/tools/testing/selftests/bpf/libarena/selftests/test_parallel_spmc.bpf.c
> new file mode 100644
> index 0000000000000..3d9fe3f579c87
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/libarena/selftests/test_parallel_spmc.bpf.c
> @@ -0,0 +1,659 @@
[ ... ]
> +/*
> + * The test requires the stealers/owners to sometimes quiesce
> + * before continuing the benchmark. Normally we'd use something
> + * like a condition variable, but since the benchmark is short-lived
> + * and operations are wait-free we just spin around the quiescence
> + * point instead. If we time out, we just fail the benchmark.
> + */
> +#define TEST_SPMC_SYNC_SPINS (1U << 18)

[Severity: Medium]
Is this iteration limit sufficient for thread synchronization here?

Since a simple read loop of this size takes a fraction of a millisecond
in kernel space, if the OS delays scheduling the worker threads, could
this loop time out prematurely and incorrectly fail the test with -ETIMEDOUT?

[ ... ]
> +static int spmc_resize_stealer(void)
> +{
> +	u64 val;
> +	int ret;
> +
> +	arena_subprog_init();
> +
> +	ret = spmc_quiesce_on_owner(1);
> +	if (ret)
> +		return ret;
> +
> +	__sync_fetch_and_add(&stealers_started, 1);
> +
> +	while (can_loop) {
> +		spmc_waste_time();
> +
> +		ret = spmc_steal(spmc, &val);

[Severity: High]
If the owner thread fails and sets test_abort = true, could this loop spin
indefinitely because it doesn't check test_abort?

> +		if (!ret) {
> +			ret = spmc_update_stats(val, false);
> +			if (ret)
> +				return ret;
> +			continue;
> +		}
[ ... ]
> +static int spmc_wait_for_round_steals(u64 target)
> +{
> +	u64 i;
> +
> +	arena_subprog_init();
> +
> +	bpf_for(i, 0, TEST_SPMC_SYNC_SPINS) {
> +		if (test_abort)
> +			return test_abort;

[Severity: Low]
Does this unintentionally return 1 instead of a negative error code like
-EINTR?

> +		if (round_steals >= target)
> +			return 0;
> +	}
[ ... ]
> +static int spmc_burst_stealer(void)
> +{
> +	u64 round, val, active_epoch;
> +	int ret;
> +
> +	arena_subprog_init();
> +
> +	for (round = zero; round < TEST_SPMC_BURST_ROUNDS && can_loop; round++) {
> +		active_epoch = round * 2 + 1;
> +
> +		/*
> +		 * Wait till the owner prefills the queue then
> +		 * start stealing.
> +		 */
> +		ret = spmc_quiesce_on_owner(active_epoch);
> +		if (ret)
> +			return ret;
> +
> +		while (owner_epoch == active_epoch && can_loop) {

[Severity: High]
Could this loop also hang indefinitely if the owner thread fails and sets
test_abort = true without updating the epoch?

> +			ret = spmc_steal(spmc, &val);
> +			if (!ret) {
> +				ret = spmc_update_stats(val, false);
[ ... ]
> diff --git a/tools/testing/selftests/bpf/prog_tests/libarena.c b/tools/testing/selftests/bpf/prog_tests/libarena.c
> index 81bdb084c2715..a42c1935eaf6e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/libarena.c
> +++ b/tools/testing/selftests/bpf/prog_tests/libarena.c
> @@ -27,6 +27,147 @@ static void run_libarena_test(struct libarena *skel, struct bpf_program *prog,
>  
>  }
>  
> +static void *run_libarena_parallel_prog(void *arg)
> +{
> +	struct bpf_program *prog = arg;
> +
> +	return (void *)(long)libarena_run_prog(bpf_program__fd(prog));

[Severity: Medium]
Should the file descriptor from bpf_program__fd() be validated here before
being passed to libarena_run_prog()?

Since run_libarena_parallel_test_workers() finds tdprog via a manual lookup
(bpf_object__find_program_by_name), it doesn't have the same guarantees as
skeleton fields. Could we use ASSERT_GE(fd, 0) to ensure we don't pass an
invalid FD?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603182727.3922-1-emil@etsalapatis.com?part=3