Re: [RFC PATCH 2/5] sched_ext: Emit ops.cpu_acquire/release() on actual CPU transitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched_ext: late cpu_avail init in scx_root_enable_workfn() misses release
--

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

sched_ext: Emit ops.cpu_acquire/release() on actual CPU transitions

This commit replaces switch_class() and the speculative acquire in
dispatch_one() with an edge-triggered state machine in
__scx_update_cpu_avail() invoked from the ext and idle classes'
put_prev_task() and set_next_task() callbacks. It also initializes CPU
availability state on scheduler enablement and adds SCX_RQ_OPS_ONLINE to
gate callbacks across hotplug.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index f644382902188..4d02d2d6e6d0d 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -7991,6 +8055,30 @@ static void scx_root_enable_workfn(struct kthread_work *work)
>  		goto err_disable;
>  	}
>  
> +	/*
> +	 * Every CPU starts released. Once the hooks are live, acquire the
> +	 * available CPUs. CPUs which don't context switch (e.g. idle ones)
> +	 * wouldn't get there otherwise. scx_root_disable() turns the key off
> +	 * under scx_enable_mutex, so it can't race this.
> +	 */
> +	if (sch->ops.flags & SCX_OPS_HAS_CPU_PREEMPT) {
> +		for_each_possible_cpu(cpu) {
> +			struct rq *rq = cpu_rq(cpu);
> +
> +			guard(rq_lock_irqsave)(rq);
> +			rq->scx.cpu_released = true;
> +		}
> +
> +		static_branch_enable(&__scx_cpu_avail_enabled);
> +
> +		for_each_possible_cpu(cpu) {
> +			struct rq *rq = cpu_rq(cpu);
> +
> +			guard(rq_lock_irqsave)(rq);
> +			__scx_update_cpu_avail(rq, rq->donor);
> +		}
> +	}
> +

[Severity: Medium]
Can this initialization ordering in scx_root_enable_workfn() miss
ops.cpu_release() and leave SCX tasks stranded on a local DSQ?

Earlier in scx_root_enable_workfn(), tasks are switched to ext_sched_class
and scx_bypass(sch, false) is called before __scx_cpu_avail_enabled is
turned on and before the initial ops.cpu_acquire() callbacks are emitted:

scx_root_enable_workfn()
  scoped_guard(sched_change, p, queue_flags)
    p->sched_class = new_class;
    sched_change_end()
      set_next_task_scx() -> ops.running()
  scx_bypass(sch, false)
    sched_change -> ops.enqueue() / ops.running()
    resched_curr(rq)

Because bypass mode is already lifted at this point, ops.running(),
ops.enqueue(), and ops.dispatch() can run on CPUs and populate
rq->scx.local_dsq before those CPUs are acquired.

If an RT, DL, or stop task preempts a CPU after scx_bypass(sch, false) and
before the second for_each_possible_cpu() loop runs __scx_update_cpu_avail()
for that CPU, put_prev_task_scx() puts the preempted SCX task back on
rq->scx.local_dsq and calls scx_update_cpu_avail(rq, next).

If that preemption happens before static_branch_enable(&__scx_cpu_avail_enabled),
scx_update_cpu_avail() is a no-op. If it happens between
static_branch_enable(&__scx_cpu_avail_enabled) and the second loop,
scx_cpu_release() sees rq->scx.cpu_released == true from the first loop:

	if (rq->scx.cpu_released || !(rq->scx.flags & SCX_RQ_OPS_ONLINE))
		return;

and returns without calling ops.cpu_release(). When the second loop then
calls __scx_update_cpu_avail(rq, rq->donor), rq->donor is already the higher
class task, so scx_cpu_release() again sees rq->scx.cpu_released == true and
does nothing. As a result, ops.cpu_release() is never called and tasks in
rq->scx.local_dsq remain stranded while the higher class task runs.

Similarly, if an idle CPU wakes up to run an SCX task between
static_branch_enable(&__scx_cpu_avail_enabled) and the second loop reaching
that CPU, put_prev_task_idle() calls scx_update_idle(rq, false, true) before
scx_update_cpu_avail(rq, next) emits the initial ops.cpu_acquire().

Should rq->scx.cpu_released initialization and the initial
__scx_update_cpu_avail() pass happen before tasks are switched into SCX and
scx_bypass(sch, false) is called?

>  	if (!(ops->flags & SCX_OPS_SWITCH_PARTIAL))
>  		static_branch_enable(&__scx_switched_all);

[ ... ]

> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index eb73b65ce6c47..89313f6971244 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -484,12 +484,16 @@ static void put_prev_task_idle(struct rq *rq, struct task_struct *prev, struct t
>  {
>  	update_curr_idle(rq);
>  	scx_update_idle(rq, false, true);
> +	/* idle -> stop/DL/RT/fair releases the CPU from the BPF scheduler */
> +	scx_update_cpu_avail(rq, next);
>  	update_rq_avg_idle(rq);
>  }

[ ... ]

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