Re: [PATCH 2/2] selftests/sched_ext: Test SCX_ENQ_CPU_SELECTED with queued wakeups

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/sched_ext: enq_cpu_selected fails to spread pingpong tasks across CPUs
--

commit e10a521c76ecd2ad846ac3d66a5ccc148e633d86
Author: Kuba Piecuch <jpiecuch@google.com>

selftests/sched_ext: Test SCX_ENQ_CPU_SELECTED with queued wakeups

This commit adds a selftest verifying that SCX_ENQ_CPU_SELECTED is set on
a wakeup ops.enqueue() if and only if ops.select_cpu() ran for that wakeup,
including queued wakeups when SCX_OPS_ALLOW_QUEUED_WAKEUP is set.

> diff --git a/tools/testing/selftests/sched_ext/enq_cpu_selected.bpf.c b/tools/testing/selftests/sched_ext/enq_cpu_selected.bpf.c
> new file mode 100644
> index 0000000000000..80e40592332a2
> --- /dev/null
> +++ b/tools/testing/selftests/sched_ext/enq_cpu_selected.bpf.c
> @@ -0,0 +1,107 @@

[ ... ]

> +s32 BPF_STRUCT_OPS(enq_cpu_selected_select_cpu, struct task_struct *p,
> +		   s32 prev_cpu, u64 wake_flags)
> +{
> +	struct task_ctx *tctx;
> +
> +	/* fork enqueues don't carry SCX_ENQ_WAKEUP, only track ttwu */
> +	if (!(wake_flags & SCX_WAKE_TTWU))
> +		return prev_cpu;
> +
> +	tctx = bpf_task_storage_get(&task_ctx_stor, p, 0, 0);
> +	if (tctx) {
> +		tctx->select_cpu_called = true;
> +		tctx->select_cpu_on = bpf_get_smp_processor_id();
> +	}
> +
> +	return prev_cpu;
> +}
> +
> +void BPF_STRUCT_OPS(enq_cpu_selected_enqueue, struct task_struct *p,
> +		    u64 enq_flags)
> +{
> +	struct task_ctx *tctx;
> +
> +	tctx = bpf_task_storage_get(&task_ctx_stor, p, 0, 0);
> +	if (tctx) {
> +		bool called = tctx->select_cpu_called;
> +		bool flagged = enq_flags & SCX_ENQ_CPU_SELECTED;
> +
> +		tctx->select_cpu_called = false;
> +
> +		if (enq_flags & SCX_ENQ_WAKEUP) {
> +			if (called) {
> +				__sync_fetch_and_add(&nr_selected, 1);
> +				if (bpf_get_smp_processor_id() != tctx->select_cpu_on)
> +					__sync_fetch_and_add(&nr_queued, 1);
> +			}
> +			if (called && !flagged)
> +				__sync_fetch_and_add(&nr_missing, 1);
> +			if (!called && flagged)
> +				__sync_fetch_and_add(&nr_spurious, 1);
> +		}
> +	}
> +
> +	scx_bpf_dsq_insert(p, SCX_DSQ_GLOBAL, SCX_SLICE_DFL, enq_flags);
> +}

[Severity: Medium]
Can this scheduler reliably generate cross-CPU queued wakeups so that
nr_queued becomes non-zero after the warmup reset?

In run(), the struct_ops scheduler is attached before spawn_pair() forks the
worker processes:

tools/testing/selftests/sched_ext/enq_cpu_selected.c:run()
    link = bpf_map__attach_struct_ops(skel->maps.enq_cpu_selected_ops);
    ...
    for (i = 0; i < NR_PAIRS; i++)
        spawn_pair(&pids[i * 2]);

During fork(), sched_cgroup_fork() initializes each child's CPU to
smp_processor_id() (the test runner's CPU), and wake_up_new_task() ->
select_task_rq_scx() invokes enq_cpu_selected_select_cpu() instead of
scx_select_cpu_dfl() because ops.select_cpu is defined. Since
enq_cpu_selected_select_cpu() returns prev_cpu unconditionally on both fork
and wakeup, all worker tasks stay assigned to the runner's CPU.

In addition, enq_cpu_selected_enqueue() inserts tasks into SCX_DSQ_GLOBAL
without kicking or selecting an idle CPU, and scx_dispatch_enqueue() does
not kick idle CPUs when inserting into a non-rq-owned DSQ. When all workers
remain on the same CPU, ttwu_queue_cond() sees cpu == this_cpu and never
uses the ttwu wakelist.

Even if another CPU happens to wake up from an unrelated interrupt and
consumes a worker from SCX_DSQ_GLOBAL via scx_consume_global_dsq(),
enq_cpu_selected_enqueue() still puts woken tasks into SCX_DSQ_GLOBAL rather
than SCX_DSQ_LOCAL. In pingpong(), the waker immediately calls read() after
write() and enters __schedule() -> do_pick_task_scx() ->
scx_consume_global_dsq(), which allows the waker's CPU to pull its own wakee
back onto the waker's CPU during the WARMUP_MS window before
skel->bss->nr_queued is reset to 0 in run().

Would it make sense to spread or pin the worker pairs across distinct CPUs
and dispatch to SCX_DSQ_LOCAL (or kick idle CPUs) so that wakers and wakees
consistently run on different CPUs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023144811.2230393-1-jpiecuch@google.com?part=2