Re: [PATCH v2 1/2] sched_ext: Don't run ops.dequeue() with a DSQ lock held

[Tejun Heo <tj@kernel.org>]

Hello, Qiurong.

On Tue, Sep 15, 2026 at 09:03:33PM +0800, Qiurong Fang wrote:
> ops.dequeue() is called with the source user DSQ's lock still held on
> the consume and move paths (scx_consume_dispatch_q(),
> move_task_between_dsqs()) and with the terminal global/bypass DSQ's
> lock still held in scx_dispatch_enqueue(). A BPF scheduler that locks
> the same DSQ from ops.dequeue() - e.g. by iterating it with
> bpf_iter_scx_dsq, which takes the DSQ lock on every step -
> self-deadlocks.

The self-deadlock only exists on the two user DSQ paths. ops.dequeue() can
only call the "any" kfuncs and none of them can lock a builtin DSQ, so the
global/bypass path can't deadlock. Moving it out is still right because all
DSQ locks share one lockdep class and iterating any user DSQ from there
trips the recursion check. Please describe it that way.

> Move the invocation after the DSQ unlock on all three paths.
> SCX_TASK_IN_CUSTODY is cleared under the lock so that the callback is
> invoked exactly once; it is not ordered against consumption of the
> task and may run after the task has been moved to, or consumed from,
> a terminal DSQ.

The last sentence and the matching doc addition confuse more than they help.
The only thing that matters is that the task can't start running or be
re-enqueued before ops.dequeue() completes. Please state that invariant
instead or drop it.

That invariant currently has a hole which this patch widens. After the
unlock, the task sits in an unlocked DSQ with ops_state still DISPATCHING
until the final store, now for the whole callback, and the reenq paths don't
wait on DISPATCHING the way ops_dequeue() does. That's from ebf1ccff79c4 and
I'll fix it separately. No need to address it here.

>  		if (dsq->id == SCX_DSQ_GLOBAL || dsq->id == SCX_DSQ_BYPASS)
> -			call_task_dequeue(sch, rq, p, 0);
> +			call_dequeue = task_leave_custody(p);
>  		else
>  			p->scx.flags |= SCX_TASK_IN_CUSTODY;
>
>  		raw_spin_unlock(&dsq->lock);
> +
> +		if (call_dequeue && SCX_HAS_OP(sch, dequeue))
> +			SCX_CALL_OP_TASK(sch, dequeue, rq, p, 0);

This leaves call_task_dequeue() with two callers, both of which are these
two lines without the unlock in between. Please open-code them too and drop
call_task_dequeue(). Each ops.dequeue() invocation being explicit is better
than a helper which may or may not invoke it depending on a flag cleared
elsewhere.

> +static struct scx_dispatch_q *
> +__scx_move_local_task_to_local_dsq(struct scx_sched *sch,
> +				   struct task_struct *p, u64 *enq_flags,
> +				   struct scx_dispatch_q *src_dsq,
> +				   struct rq *dst_rq)

The split isn't necessary. Unlock @src_dsq right after
scx_task_unlink_from_dsq() in the two callers and call
scx_move_local_task_to_local_dsq() as is, dropping its lockdep assert on
@src_dsq. @p is off the DSQ and its rq is locked, so nothing can reach it in
between.

Also, please add Cc: stable # v7.1+.

Thanks.

--
tejun