Re: [PATCH 14/17] sched_ext: Delegate proxy donor admission to BPF schedulers

[Andrea Righi <arighi@nvidia.com>]

Hi Tejun,

On Sun, Aug 16, 2026 at 05:02:03PM -1000, Tejun Heo wrote:
> On Sun, Aug 16, 2026 at 07:35:12PM +0200, Andrea Righi wrote:
> >  bool scx_allow_proxy_exec(const struct task_struct *p)
> >  {
> > -	return p->sched_class != &ext_sched_class;
> > +	struct scx_sched *sch;
> > +
> > +	if (p->sched_class != &ext_sched_class)
> > +		return true;
> > +
> > +	/*
> > +	 * scx_enabled() may change while __schedule() holds only @p's rq lock.
> > +	 * Once @p is associated with a scheduler, use that scheduler's policy
> > +	 * even while the global enable state is transitioning.
> > +	 */
> 
> I'm not sure this comment is necessary. scx_task_sched() is stable while
> holding the task's rq lock.

That's right, the comment is unnecessary, I'll remove it.

> 
> > @@ -2059,19 +2086,25 @@ void scx_do_enqueue_task(struct rq *rq, struct task_struct *p, u64 enq_flags,
> >  	if (p->scx.ddsp_dsq_id != SCX_DSQ_INVALID)
> >  		goto direct;
> >  
> > +	enq_blocked = (sch->ops.flags & SCX_OPS_ENQ_BLOCKED) &&
> > +		      p->is_blocked && !(enq_flags & SCX_ENQ_WAKEUP);
> 
> Why not just test directly in the if statement? It's not like the test
> result is used anywhere else. Is the intention giving the test result an
> intuitive name? I guess is_blocked && WAKEUP is the condition is the donor
> gaining execution back? Might be worthwhile to add a comment.

Right, the local var was only intended to give the condition a name. I'll inline
it and add a comment explaining the WAKEUP exclusion.

> 
> > +	if (enq_blocked) {
> > +		enq_flags |= SCX_ENQ_BLOCKED;
> > +	} else {
> > +		/* see %SCX_OPS_ENQ_EXITING */
> > +		if (!(sch->ops.flags & SCX_OPS_ENQ_EXITING) &&
> > +		    unlikely(p->flags & PF_EXITING)) {
> 
> While at it, can you swap the order? This is ordered this way because OPS
> testing used to be static_key but now that these are regular tests, it makes
> more sense to test the unlikely one first, or maybe that belongs in a
> separate patch.

Ack. I'll test PF_EXITING first. Since this patch already moves that condition
to handle blocked-donor admission ahead of it, maybe we can fold this into the
same patch.

> 
> > @@ -2183,8 +2216,17 @@ static void enqueue_task_scx(struct rq *rq, struct task_struct *p, int core_enq_
> >  	int sticky_cpu = p->scx.sticky_cpu;
> >  	u64 enq_flags = core_enq_flags | rq->scx.remote_activate_enq_flags;
> >  
> > -	if (enq_flags & ENQUEUE_WAKEUP)
> > +	/*
> > +	 * p->is_blocked is cleared after wakeup_preempt(), so remember whether
> > +	 * this is a full wakeup activation. If wakeup_preempt_scx() isn't called,
> > +	 * set_next_task_scx() or a subsequent non-wakeup enqueue clears the flag.
> > +	 */
> 
> I can't make heads or tails of this comment. This doesn't seem to explain
> what TASK_ENQ_WAKEUP is used for but just goes into how it's managed.

The distinction it was trying to carry is between a retained on-rq donor wakeup,
which reaches wakeup_preempt_scx() through ttwu_runnable() without another
ops.enqueue() and a full wakeup activation, which has already enqueued the task.

We can remove SCX_TASK_ENQ_WAKEUP, pass WF_ON_RQ directly from ttwu_runnable()
and replace the old lifecycle comment with one next to the check explaining that
WF_ON_RQ identifies the no-enqueue path, which needs resched_curr(), so that BPF
can reconsider the task after is_blocked is cleared.

> 
> > +	if (enq_flags & ENQUEUE_WAKEUP) {
> >  		rq->scx.flags |= SCX_RQ_IN_WAKEUP;
> > +		p->scx.flags |= SCX_TASK_ENQ_WAKEUP;
> > +	} else {
> > +		p->scx.flags &= ~SCX_TASK_ENQ_WAKEUP;
> > +	}
> >  
> >  	/*
> >  	 * Restoring the current scheduling context will be immediately followed
> > @@ -2399,10 +2441,30 @@ static void wakeup_preempt_scx(struct rq *rq, struct task_struct *p, int wake_fl
> >  	/*
> >  	 * Preemption between SCX tasks is implemented by resetting the victim
> >  	 * task's slice to 0 and triggering reschedule on the target CPU.
> > -	 * Nothing to do.
> > +	 *
> > +	 * A mutex waiter can remain on-rq as a proxy donor while logically
> > +	 * blocked. If it wakes without having been proxy-migrated,
> > +	 * ttwu_runnable() calls here without another enqueue_task_scx(). Request
> > +	 * rescheduling so that ops.dispatch() can reconsider the task after
> > +	 * ttwu_runnable() clears is_blocked.
> > +	 *
> > +	 * A proxy-migrated donor instead returns through the full activation
> > +	 * path, which calls enqueue_task_scx() before arriving here.
> > +	 * SCX_TASK_ENQ_WAKEUP records that the enqueue already happened and an
> > +	 * additional reschedule isn't needed.
> >  	 */
> > -	if (p->sched_class == &ext_sched_class)
> > +	if (p->sched_class == &ext_sched_class) {
> > +		bool enq_wakeup = p->scx.flags & SCX_TASK_ENQ_WAKEUP;
> 
> Ditto with bouncing test result.
> 
> > +
> > +		p->scx.flags &= ~SCX_TASK_ENQ_WAKEUP;
> 
> I'm not a big fan of SCX_TASK_ENQ_WAKEUP. This is a roundabout way to detect
> owner -> donor case, right? Doesn't the caller already know? If so, can't it
> just pass in that as a wake_flag?

Ok, as mentioned earlier we can pass WF_ON_RQ to wakeup_preempt_scx() to
distinguish the two cases.

> 
> > +		if (!enq_wakeup && p->is_blocked) {
> > +			struct scx_sched *sch = scx_task_sched(p);
> > +
> > +			if (sch && (sch->ops.flags & SCX_OPS_ENQ_BLOCKED))
> > +				resched_curr(rq);
> > +		}
> 
> It'd nice if we can gate the above behind proxy enabled.

Ack.

> 
> > @@ -2517,6 +2579,20 @@ static bool task_can_run_on_remote_rq(struct scx_sched *sch,
> >  
> >  	WARN_ON_ONCE(task_cpu(p) == cpu);
> >  
> > +	/*
> > +	 * A blocked donor may be moved normally to select a new callback rq.
> 
> What's "callback" rq?

Poor terminology... I meant the rq associated with the donor before proxy
execution moved its scheduling context. I'll rephrase this.

> 
> > +	 * set_task_cpu() updates wake_cpu and makes the destination rq its new
> > +	 * callback home.
> > +	 *
> > +	 * proxy_set_task_cpu() instead preserves wake_cpu when moving a donor to
> > +	 * its lock owner's CPU. Keep such a donor on the proxy rq until it wakes;
> > +	 * otherwise normal BPF placement may repeatedly pull it back to its
> > +	 * callback rq only for proxy execution to move it to the owner again.
> > +	 */
> > +	if (sched_proxy_exec() && p->is_blocked &&
> > +	    task_cpu(p) != p->wake_cpu)
> 
> No need for line break. Can you elaborate the scenario this scenario is
> needed for? Is this the exact condition? Let's say a donor is running the
> owner on the same CPU, so task_cpu(p) == p->wake_cpu. Wouldn't you still
> want to block scx from moving it to another CPU? What am I missing?

The condition is intended to identify a completed proxy migration, not all
blocked donors.

For example, suppose BPF places donor D on CPU0 while its mutex owner O runs on
CPU1, proxy-exec moves D to CPU1 (its scheduling context) using
proxy_set_task_cpu(), which preserves D->wake_cpu == CPU0.

Then we have:
 - task_cpu(D) = CPU1
 - wake_cpu(D) = CPU0

If D is subsequently put on a shared DSQ, then it could be consumed by CPU0 and
move it back, only for proxy execution to move it to CPU1 again. The check
prevents that ping-pong while D remains blocked.

In the same-CPU case, while D is actively proxy-running O on CPU0, D is
rq->donor and has already been removed from its DSQ, so another CPU cannot
consume and move it. A remote-transfer race is also rejected by
task_proxy_move_active (aka task_proxy_running_or_donating() after the rename).

If D is later preempted and re-enqueued while still blocked, it is no longer
active and BPF may place it elsewhere. Suppose BPF moves it to CPU1. The normal
migration updates both task_cpu(D) and wake_cpu(D) to CPU1. When CPU1 selects D,
proxy execution finds O on CPU0 and moves D's scheduling context back to CPU0
while preserving wake_cpu(D) == CPU1. The resulting mismatch then prevents
further BPF-directed moves while D remains blocked.

So there are 3 cases:
 - task_proxy_move_active() prevents moving a donor that is currently running or
   donating,
 - task_cpu(D) != D->wake_cpu prevents moving an inactive donor whose scheduling
   context has already been moved to its owner's CPU by proxy exec,
 - an inactive donor that has not yet been proxy-migrated remains under BPF
   placement control.

Thanks,
-Andrea