Re: [PATCH 03/18] sched: Make NOHZ CFS bandwidth checks follow proxy donor

[Andrea Righi <arighi@nvidia.com>]

Hi Peter,

thanks for looking at this and sorry for my late response!
Comments below.

On Thu, Sep 10, 2026 at 11:54:48AM +0200, Peter Zijlstra wrote:
> On Mon, Aug 31, 2026 at 03:42:13PM +0200, Andrea Righi wrote:
> > Proxy execution separates the scheduling context in rq->donor from the
> > physical execution context in rq->curr. sched_can_stop_tick() checks the
> > latter for CFS bandwidth constraints and only does so when nr_running is
> > one.
> > 
> > A retained proxy donor keeps both the donor and mutex owner queued. The
> > check therefore misses a constrained FAIR donor and may stop the tick
> > while its runtime still needs to be enforced.
> > 
> > Check the selected donor instead and remove the nr_running restriction.
> > The donor being a queued FAIR task is sufficient to require bandwidth
> > accounting regardless of other runnable tasks.
> > 
> > Fixes: af0c8b2bf67b ("sched: Split scheduler and execution contexts")
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Link: https://lore.kernel.org/r/20260713164807.E5ED21F00A3A@smtp.kernel.org
> > Acked-by: John Stultz <jstultz@google.com>
> > Signed-off-by: Andrea Righi <arighi@nvidia.com>
> > ---
> >  kernel/sched/core.c | 35 ++++++++++++++++++-----------------
> >  kernel/sched/fair.c | 12 +++++++-----
> >  2 files changed, 25 insertions(+), 22 deletions(-)
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 237d216382f46..14d0d5c884393 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1419,11 +1419,8 @@ static void nohz_csd_func(void *info)
> >  #endif /* CONFIG_NO_HZ_COMMON */
> >  
> >  #ifdef CONFIG_NO_HZ_FULL
> > -static inline bool __need_bw_check(struct rq *rq, struct task_struct *p)
> > +static inline bool __need_bw_check(struct task_struct *p)
> >  {
> > -	if (rq->nr_running != 1)
> > -		return false;
> > -
> >  	if (p->sched_class != &fair_sched_class)
> >  		return false;
> >  
> > @@ -1441,6 +1438,14 @@ bool sched_can_stop_tick(struct rq *rq)
> >  	if (rq->dl.dl_nr_running)
> >  		return false;
> >  
> > +	/*
> > +	 * The selected scheduling context can be a constrained FAIR donor even
> > +	 * when rq->curr is an RT task. Check it before the RT fast paths below,
> > +	 * which may report that the tick can stop for a throttled RT context.
> > +	 */
> 
> I am most confused... how can we ever have rq->donor be FAIR and
> rq->curr be RT? That makes no sense. If an RT task is runnable, pick
> should just straight up pick that.

Yeah, the RT example is bogus.

The relevant case is a FAIR donor belonging to a quota-controlled cgroup which
still has runtime available, proxy-executing a lower-class mutex owner
(specifically an EXT owner with the other patches in the series).

The owner's execution is charged to the donor's scheduling context. The retained
donor and runnable owner make rq->nr_running greater than one, while rq->curr
identifies the EXT owner rather than the quota-controlled FAIR context. There is
also only one queued FAIR task, so the normal CFS multiplicity check does not
retain the tick.

The bandwidth check must therefore follow rq->donor so that the donor's
remaining CFS quota is enforced.

> 
> > +	if (__need_bw_check(rq->donor) && cfs_task_bw_constrained(rq->donor))
> > +		return false;
> > +
> 
> 
> > @@ -7114,7 +7107,7 @@ find_proxy_task(struct rq *rq, struct task_struct *donor, struct rq_flags *rf)
> >   */
> >  static void __sched notrace __schedule(int sched_mode)
> >  {
> > -	struct task_struct *prev, *next;
> > +	struct task_struct *prev, *next, *tick_donor;
> >  	/*
> >  	 * On PREEMPT_RT kernel, SM_RTLOCK_WAIT is noted
> >  	 * as a preemption by schedule_debug() and RCU.
> > @@ -7168,6 +7161,7 @@ static void __sched notrace __schedule(int sched_mode)
> >  	rq->clock_update_flags <<= 1;
> >  	update_rq_clock(rq);
> >  	rq->clock_update_flags = RQCF_UPDATED;
> > +	tick_donor = rq->donor;
> >  
> >  	switch_count = &prev->nivcsw;
> >  
> > @@ -7243,6 +7237,13 @@ static void __sched notrace __schedule(int sched_mode)
> >  	clear_tsk_need_resched(prev);
> >  	clear_preempt_need_resched();
> >  keep_resched:
> > +	/*
> > +	 * Enqueue and dequeue updates can evaluate the outgoing donor. Refresh
> > +	 * the dependency after selecting a different scheduling context.
> > +	 */
> > +	if (rq->donor != tick_donor)
> > +		sched_update_tick_dependency(rq);
> 
> Please keep all the proxy specific bits inside the one
> sched_proxy_exec() branch above.

Sure, I'll move the donor-change tracking and tick dependency update into the
sched_proxy_exec() branch.

Thanks,
-Andrea