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

[sashiko-bot@kernel.org]

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 <yphbchou0911@gmail.com>
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.

> +	}
> +
> +	bpf_link__destroy(link);
> +
> +	SCX_GT(skel->bss->nr_cookie_dispatched, 0);
> +
> +	return SCX_TEST_PASS;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260506160412.522199-1-yphbchou0911@gmail.com?part=2