Re: [PATCH v2 2/2] selftests/sched_ext: Add dispatch_cookie test

On Wed, May 06, 2026 at 08:52:34PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Failing an assertion in the run() function leaks the bpf_link and leaves the BPF scheduler permanently attached.
> - [Medium] The BPF program incorrectly drops the kernel-provided enq_flags when calling scx_bpf_dsq_insert_with_cookie().
> - [Medium] The return value of fork() is not checked for errors, leading to incorrect waitpid() behavior.
> --
> 
> commit de7f9ee9893bdc6cb049e25475e095b028e1ed26
> Author: Cheng-Yang Chou <[email protected]>
> selftests/sched_ext: Add dispatch_cookie test
> 
> This commit adds a new sched_ext selftest to verify the behavior of
> scx_bpf_task_get_cookie() and scx_bpf_dsq_insert_with_cookie(). It captures
> a cookie before dispatch and verifies that userspace accurately reflects
> the cookie-based dispatch activity.
> 
> > diff --git a/tools/testing/selftests/sched_ext/dispatch_cookie.bpf.c b/tools/testing/selftests/sched_ext/dispatch_cookie.bpf.c
> > new file mode 100644
> > index 0000000000000..cb198a5f50e89
> > --- /dev/null
> > +++ b/tools/testing/selftests/sched_ext/dispatch_cookie.bpf.c
> [ ... ]
> > +void BPF_STRUCT_OPS(dispatch_cookie_enqueue, struct task_struct *p,
> > +		    u64 enq_flags)
> > +{
> > +	u64 cookie = scx_bpf_task_get_cookie(p);
> > +
> > +	if (scx_bpf_dsq_insert_with_cookie(p, SCX_DSQ_GLOBAL, 0, cookie))
> 
> Does this code unintentionally drop the enq_flags provided by the kernel?
> 
> The fallback branch correctly passes enq_flags to scx_bpf_dsq_insert(),
> but hardcoding 0 here might alter task placement context by dropping
> directives like SCX_ENQ_WAKEUP.
> 
> > +		__sync_fetch_and_add(&nr_cookie_dispatched, 1);
> > +	else
> > +		scx_bpf_dsq_insert(p, SCX_DSQ_GLOBAL, SCX_SLICE_DFL, enq_flags);
> > +}
> [ ... ]
> > diff --git a/tools/testing/selftests/sched_ext/dispatch_cookie.c b/tools/testing/selftests/sched_ext/dispatch_cookie.c
> > new file mode 100644
> > index 0000000000000..f799b5828f9d6
> > --- /dev/null
> > +++ b/tools/testing/selftests/sched_ext/dispatch_cookie.c
> [ ... ]
> > +static enum scx_test_status run(void *ctx)
> > +{
> > +	struct dispatch_cookie *skel = ctx;
> > +	struct bpf_link *link;
> > +	pid_t pids[NUM_CHILDREN];
> > +	int i, status;
> > +
> > +	link = bpf_map__attach_struct_ops(skel->maps.dispatch_cookie_ops);
> > +	SCX_FAIL_IF(!link, "Failed to attach scheduler");
> > +
> > +	for (i = 0; i < NUM_CHILDREN; i++) {
> > +		pids[i] = fork();
> 
> Should the return value of fork() be checked here?
> 
> If fork() fails and returns -1, waitpid() in the subsequent loop will be
> called with -1, causing it to wait for any child process rather than the
> specific process. This could lead to out-of-order reaping and confusing
> test failures.
> 
> > +		if (pids[i] == 0)
> > +			exit(0);
> > +	}
> > +
> > +	for (i = 0; i < NUM_CHILDREN; i++) {
> > +		SCX_EQ(waitpid(pids[i], &status, 0), pids[i]);
> > +		SCX_EQ(status, 0);
> 
> Can this cause a resource leak that affects other tests?
> 
> SCX_EQ() forces an immediate return upon failure. If a check fails here,
> the function will return before reaching bpf_link__destroy(link).
> 
> Leaving the BPF scheduler attached could cause subsequent tests in the
> runner to fail with -EBUSY, since sched_ext only allows one active
> scheduler globally.

This is a valid issue, which can be fixed by moving bpf_link__destory()
before the assertion loop.

Will address this in v3, but would like to wait for more reviews before
sending it. Thanks!

> 
> > +	}
> > +
> > +	bpf_link__destroy(link);
> > +
> > +	SCX_GT(skel->bss->nr_cookie_dispatched, 0);
> > +
> > +	return SCX_TEST_PASS;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
Cheers,
Cheng-Yang