Re: sched/fair: which tasks should nr_pref_llc_running be compared against?

[Tim Chen]

On Wed, 2026-09-09 at 19:40 +0800, Chen Yu wrote:
> On Fri, Sep 04, 2026 at 01:53:10PM -0700, Tim Chen wrote:
> > Good idea - the four sites really are one operation ("if the task is
> > queued on its preferred LLC and runnable, move the counter"), and
> > folding the two conditions into one place is what keeps them from
> > drifting apart later. I've adopted it in v3; account_llc_delayed() and
> > account_llc_requeue_delayed() are gone.
> > 
> > I split it slightly differently: a membership predicate
> > 
> >         static bool task_pref_llc_runnable(struct task_struct *p)
> >         {
> >                 return p->pref_llc_queued && !p->se.sched_delayed;
> >         }
> > 
> > with pref_llc_running_inc()/pref_llc_running_dec() wrappers over it, so
> > the call sites read as inc/dec rather than passing a +1/-1 delta.
> > 
> > Two things to note:
> > 
> > 1) I kept the call site comments of pref_llc_running_inc/dec().
> >    The helper name says *what* happens,
> >    but not *why* it is safe across the delay-dequeue transition - that
> >    set_delayed() already did the decrement, so account_llc_dequeue()
> >    must skip it, and that clearing pref_llc_queued there is what
> >    neutralizes the following clear_delayed(). 
> > 
> > 2) The pref_llc_running_dec() placement in set_delayed()
> >    is subtle. It has to be before se->sched_delayed = 1 or
> >    decrement would not happen. That deserves a comment so no
> >    one would move sched_delayed = 1 before the decrement.
> > 
> > 
> > alb_break_llc() decides whether to break LLC preference during active
> > load balance. It does so by testing that every runnable fair task on the
> > source rq prefers its LLC:
> > 
> > 	env->src_rq->nr_pref_llc_running == env->src_rq->cfs.h_nr_runnable
> > 
> > But the two counters cover different sets. nr_pref_llc_running is updated
> > in account_llc_enqueue()/account_llc_dequeue(), next to cfs_rq->nr_queued,
> > so it follows queued tasks. h_nr_runnable is updated in set_delayed()/
> > clear_delayed() and drops delay-dequeued tasks.
> > 
> > So under DELAY_DEQUEUE, a preferring task that goes to sleep stays counted
> > in nr_pref_llc_running while h_nr_runnable falls. The equality then breaks,
> > alb_break_llc() returns false, and active balance is free to pull a task
> > off its preferred LLC. Active balance only moves runnable tasks, and this
> > is the only LLC check it consults: once the stopper runs, LBF_ACTIVE_LB
> > skips the per-task test in can_migrate_task(). The runnable set is the one
> > we want.
> > 
> > Fix it on the counter side. A task should be counted in
> > nr_pref_llc_running exactly while it is both queued on its preferred LLC
> > (pref_llc_queued) and runnable (!sched_delayed). Define that membership
> > once in task_pref_llc_runnable(), and adjust the counter only through
> > pref_llc_running_inc()/pref_llc_running_dec() from the four sites that
> > change either input: account_llc_enqueue(), account_llc_dequeue(),
> > set_delayed() and clear_delayed(). Gating every update on the same
> > predicate keeps the delay, wake and dequeue paths from double-counting
> > or underflowing; see the comments at those sites for the ordering.
> > 
> > nr_llc_running and sd->llc_counts are not touched and stay on queued
> > semantics.
> > 
> > Reported-by: Zhan Xusheng <zhanxusheng@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/lkml/20260827135000.735138-1-zhanxusheng@xxxxxxxxxx/
> > Suggested-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> > Signed-off-by: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
> > ---
> > Based on v7.3-rc1.
> > 
> > Changes in v3:
> > - Route every nr_pref_llc_running adjustment through a single membership
> >   predicate task_pref_llc_runnable(), with pref_llc_running_inc()/
> >   pref_llc_running_dec() wrappers, instead of four open-coded sites
> >   (Chen Yu). Keep the per-site comments that explain the delay-dequeue
> >   interaction, and note that set_delayed() must adjust the counter
> >   before setting se->sched_delayed.
> > 
> > Changes in v2:
> > - Prevent a delay-dequeued task from being counted as running in the
> >   enqueue path (Chen Yu).
> > 
> >  kernel/sched/fair.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 50 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 8dff37059faf..72aae7a50b8b 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -1538,6 +1538,28 @@ static bool invalid_llc_nr(struct mm_struct *mm, struct task_struct *p,
> >  			(scale * per_cpu(sd_llc_size, cpu)));
> >  }
> >  
> > +/*
> > + * A task counts in nr_pref_llc_running while it is queued on its preferred
> > + * LLC (pref_llc_queued) and runnable (!sched_delayed), keeping the counter in
> > + * the runnable domain so alb_break_llc() can compare it with h_nr_runnable.
> > + */
> > +static bool task_pref_llc_runnable(struct task_struct *p)
> > +{
> > +	return p->pref_llc_queued && !p->se.sched_delayed;
> > +}
> > +
> > +static void pref_llc_running_inc(struct rq *rq, struct task_struct *p)
> > +{
> > +	if (task_pref_llc_runnable(p))
> > +		rq->nr_pref_llc_running++;
> > +}
> > +
> > +static void pref_llc_running_dec(struct rq *rq, struct task_struct *p)
> > +{
> > +	if (task_pref_llc_runnable(p))
> > +		rq->nr_pref_llc_running--;
> > +}
> > +
> >  static void account_llc_enqueue(struct rq *rq, struct task_struct *p)
> >  {
> >  	int pref_llc, pref_llc_queued;
> > @@ -1549,7 +1571,6 @@ static void account_llc_enqueue(struct rq *rq, struct task_struct *p)
> >  
> >  	pref_llc_queued = (pref_llc == task_llc(p));
> >  	rq->nr_llc_running++;
> > -	rq->nr_pref_llc_running += pref_llc_queued;
> >  
> >  	/*
> >  	 * Record whether p is enqueued on its preferred
> > @@ -1567,6 +1588,9 @@ static void account_llc_enqueue(struct rq *rq, struct task_struct *p)
> >  	 */
> >  	p->pref_llc_queued = pref_llc_queued;
> >  
> > +	/* Skipped while delayed; clear_delayed() adds it back on wake. */
> > +	pref_llc_running_inc(rq, p);
> > +
> >  	sd = rcu_dereference_all(rq->sd);
> >  	if (sd && (unsigned int)pref_llc < sd->llc_max)
> >  		sd->llc_counts[pref_llc]++;
> > @@ -1583,7 +1607,12 @@ static void account_llc_dequeue(struct rq *rq, struct task_struct *p)
> >  
> >  	rq->nr_llc_running--;
> >  	if (p->pref_llc_queued) {
> > -		rq->nr_pref_llc_running--;
> > +		/*
> > +		 * Skipped if still delayed (set_delayed() already removed it);
> > +		 * clearing pref_llc_queued below also stops clear_delayed()
> > +		 * from re-adding it.
> > +		 */
> > +		pref_llc_running_dec(rq, p);
> >  		/*
> >  		 * Update the status in case
> >  		 * other logic might query
> > @@ -2008,6 +2037,10 @@ static void account_llc_enqueue(struct rq *rq, struct task_struct *p) {}
> >  
> >  static void account_llc_dequeue(struct rq *rq, struct task_struct *p) {}
> >  
> > +static void pref_llc_running_inc(struct rq *rq, struct task_struct *p) {}
> > +
> > +static void pref_llc_running_dec(struct rq *rq, struct task_struct *p) {}
> > +
> >  #endif /* CONFIG_SCHED_CACHE */
> >  
> >  /*
> > @@ -6382,6 +6415,14 @@ static __always_inline void return_cfs_rq_runtime(struct cfs_rq *cfs_rq);
> >  
> >  static void set_delayed(struct sched_entity *se)
> >  {
> > +	/*
> > +	 * Drop a task leaving the runnable set. Must run before sched_delayed
> > +	 * is set, or task_pref_llc_runnable() would already exclude it;
> > +	 * clear_delayed() mirrors this after clearing the flag.
> > +	 */
> > +	if (entity_is_task(se))
> > +		pref_llc_running_dec(rq_of(cfs_rq_of(se)), task_of(se));
> > +
> >  	se->sched_delayed = 1;
> >  
> >  	/*
> > @@ -6412,6 +6453,13 @@ static void clear_delayed(struct sched_entity *se)
> >  	if (!entity_is_task(se))
> >  		return;
> >  
> > +	/*
> > +	 * Re-add on wake, after sched_delayed is cleared. On a final delayed
> > +	 * dequeue account_llc_dequeue() already cleared pref_llc_queued, so
> > +	 * this does nothing.
> > +	 */
> > +	pref_llc_running_inc(rq_of(cfs_rq_of(se)), task_of(se));
> > +
> >  	for_each_sched_entity(se) {
> >  		struct cfs_rq *cfs_rq = cfs_rq_of(se);
> >  
> > -- 
> > 2.32.0
> > 
> > 
> > 
> 
> Yes, I think this version looks good now. While looking back at Xusheng's proposal,
> I noticed there is another option:
> if (env->src_rq->nr_pref_llc_running == env->src_rq->cfs.h_nr_queued) {
> 	...
> }
> May I know why we did not choose this approach, is it because of the following
> scenario?

The reason is that a common condition we are trying to avoid in
alb_break_llc() is the following: We have one task T1 running on cpu preferring
src LLC and another delay queued task T2 not preferring src LLC and delayed queued.

- runnable domain (current fix): h_nr_runnable == 1, nr_pref_llc_running == 1 → equal → alb_break_llc() true → suppress.
  Correct: the only thing actually running here wants to be here; don't rip it away.
- h_nr_queued alternative: h_nr_queued == 2, nr_pref(queued) == 1 → not equal → alb_break_llc() false → proceed to active balance, 
  which would then break T1's locality to relieve an "imbalance"
  that is really just a sleeping T2.


> Suppose there are 3 queued tasks: p1 and p2 prefer the src_rq, while p3 is a delayed
> task that also prefers src_rq. In the current implementation, nr_pref_llc_running is 3
> and h_nr_runnable is 2, so alb_break_llc() might return false. As a result, active load
> balance would be triggered, and p1 or p2 might be migrated away, which is undesirable.
> However, would this still be a problem after Lu Wang's active load balance guard patch
> has been applied?
> https://lore.kernel.org/lkml/20260903020656.3793626-1-wanglu.priv@xxxxxxxxx/

Lu Wang's patch only mitigate the migrate_llc case but not other migration reasons.
We shouldn't have done active balance in the example I gave if we are doing
migration for other non migrate_llc reasons.

Tim

> 
> thanks,
> Chenyu