Re: [PATCH 02/11] sched: Add helper to block retained proxy donors

[Andrea Righi <arighi@nvidia.com>]

Hi Prateek,

On Thu, Jul 16, 2026 at 11:17:53PM +0530, K Prateek Nayak wrote:
> Hello Andrea,
> 
> On 7/16/2026 2:24 AM, Andrea Righi wrote:
> > +/*
> > + * Remove a retained proxy donor before changing its scheduler ownership.
> > + * The caller holds p->pi_lock, so p cannot wake and migrate after block_task()
> > + * drops it from the runqueue.
> > + *
> > + * Unlike the regular schedule() path, this must leave @p fully dequeued.
> > + * DELAY_DEQUEUE may keep a blocked fair task queued with sched_delayed set,
> > + * which would let the following sched_change preserve and re-enqueue it under
> > + * the new scheduler. Complete an existing or newly-created delayed dequeue
> > + * before returning.
> > + */
> > +void sched_proxy_block_task(struct rq *rq, struct task_struct *p)
> > +{
> > +	unsigned long state = READ_ONCE(p->__state);
> > +
> > +	lockdep_assert_held(&p->pi_lock);
> > +	lockdep_assert_rq_held(rq);
> > +
> > +	if (!p->is_blocked || !task_on_rq_queued(p))
> > +		return;
> > +	if (WARN_ON_ONCE(state == TASK_RUNNING))
> > +		return;
> > +
> > +	if (task_current_donor(rq, p)) {
> > +		proxy_resched_idle(rq);
> 
> This should be proxy_reset_donor() else the current may try to
> do a sched_yield() with idle as donor and you'll try calling
> idle class' yield which will crash.

Good catch! I'll fix this in the next version.

> 
> > +		/* Kick the execution context if @rq is remote. */
> > +		resched_curr(rq);
> > +	}
> > +
> > +	if (!p->se.sched_delayed)
> > +		block_task(rq, p, state);
> > +	if (p->se.sched_delayed)
> > +		dequeue_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_DELAYED |
> > +			     DEQUEUE_NOCLOCK);
> 
> nit.
> 
> This case gets dealt by the sched_change_begin() which happens right
> after scx_prepare_setscheduler(). Do we really need it here?

You're right. All callers immediately follow this with sched_change_begin(). On
a FAIR-to-EXT transition, switching_from_fair() completes the delayed dequeue
before sched_change_begin() snapshots the queued state. And EXT-to-EXT changes
can't have sched_delayed set. I'll drop the redundant dequeue and update the
comment accordingly.

> 
> > +
> > +	WARN_ON_ONCE(task_on_rq_queued(p));
> > +	WARN_ON_ONCE(p->se.sched_delayed);
> 
> Very defensive :-) I personally think on_rq check should be
> sufficient. Delayed but not on_rq would scream at wakeup.

Right, sched_delayed implies on_rq, so the second check was redundant.

With the explicit dequeue_task() removed per your previous comment, a FAIR task
may remain queued with sched_delayed until the immediately following
sched_change_begin() calls switching_from_fair(), so we should probably check
WARN_ON_ONCE(task_on_rq_queued(p) && !p->se.sched_delayed) here.

> 
> > +}
> > +
> >  static inline void proxy_release_rq_lock(struct rq *rq, struct rq_flags *rf)
> >  	__releases(__rq_lockp(rq))
> >  {
> 
> Also kudos with the sched_ext + proxy demo. Very impressive to see
> that latency reduction.

Ah that was a bit of a synthetic setup, but I needed something to show that the
numbers move in the right direction with this whole thing. :)

Thanks for taking a look at this!
-Andrea