Re: [PATCH sched_ext v2 2/2] selftests/sched_ext: check that a drained DSQ iterator stays NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] selftests/sched_ext: unconditional overwrite of
  next_after_drain in dequeue_iter_dequeue()
- [Medium] selftests/sched_ext: empty TEST_DSQ_ID in
  dequeue_iter_dequeue() leaves iter_count at 0
--

commit 2125e4ff3cb11bd1e11aff810c77c47f61782533
Author: Kang Chen <k.chen@smail.nju.edu.cn>

selftests/sched_ext: check that a drained DSQ iterator stays NULL

This commit extends the dequeue_iter selftest to verify that calling
bpf_iter_scx_dsq_next() again after a DSQ iterator is drained continues
to return NULL.

> diff --git a/tools/testing/selftests/sched_ext/dequeue_iter.bpf.c b/tools/testing/selftests/sched_ext/dequeue_iter.bpf.c
> index 76c2c71a90b61..38eb2df54a6b5 100644
> --- a/tools/testing/selftests/sched_ext/dequeue_iter.bpf.c
> +++ b/tools/testing/selftests/sched_ext/dequeue_iter.bpf.c
> @@ -18,6 +18,8 @@ UEI_DEFINE(uei);
>  #define TEST_DSQ_ID 1000
>  
>  u64 dq_count;
> +u64 iter_count;
> +u64 next_after_drain = -1;
>  
>  s32 BPF_STRUCT_OPS_SLEEPABLE(dequeue_iter_init)
>  {
> @@ -47,7 +49,10 @@ void BPF_STRUCT_OPS(dequeue_iter_dequeue, struct task_struct *p, u64 deq_flags)
>  
>  	if (!bpf_iter_scx_dsq_new(&it, TEST_DSQ_ID, 0)) {
>  		while ((t = bpf_iter_scx_dsq_next(&it)))
> -			;
> +			iter_count++;
> +
> +		/* NULL must stay final after the iterator is drained. */
> +		next_after_drain = (__u64)bpf_iter_scx_dsq_next(&it);
>  	}
>  	bpf_iter_scx_dsq_destroy(&it);

[Severity: Medium]
Can this assignment overwrite a non-NULL next_after_drain captured by an
earlier dequeue and cause a false pass on an unfixed kernel?

scx_consume_dispatch_q() unlinks the consumed task from TEST_DSQ_ID
before calling ops.dequeue().  If an earlier dequeue saw another task in
TEST_DSQ_ID and recorded a non-NULL task pointer in next_after_drain, any
subsequent dequeue that runs when TEST_DSQ_ID is empty will unconditionally
overwrite next_after_drain with 0.  In addition, iter_count and
next_after_drain are updated with plain stores even though ops.dequeue()
can run concurrently on multiple CPUs after dsq->lock is dropped.

> diff --git a/tools/testing/selftests/sched_ext/dequeue_iter.c b/tools/testing/selftests/sched_ext/dequeue_iter.c
> index f60711bf3b395..48e8c94b7bf14 100644
> --- a/tools/testing/selftests/sched_ext/dequeue_iter.c
> +++ b/tools/testing/selftests/sched_ext/dequeue_iter.c
> @@ -59,6 +59,10 @@ static enum scx_test_status run(void *ctx)
>  		return SCX_TEST_FAIL;
>  	}
>  
> +	SCX_FAIL_IF(!skel->bss->iter_count,
> +		    "DSQ iterator drained no tasks");

[Severity: Medium]
Will skel->bss->iter_count stay at 0 on an idle or single-CPU system?

Because scx_consume_dispatch_q() unlinks the task being consumed from
TEST_DSQ_ID and clears SCX_TASK_IN_CUSTODY before invoking
dequeue_iter_dequeue(), bpf_iter_scx_dsq_next() only yields a task if a
second task happens to be queued in TEST_DSQ_ID at the same instant.
run() only drives a single thread calling usleep(100) until dq_count
reaches DQ_TARGET, so if no other task is concurrently queued in
TEST_DSQ_ID during those 10 dequeues, the test fails with "DSQ iterator
drained no tasks".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113024505.922854-1-k.chen@smail.nju.edu.cn?part=2