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

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

[Zhan Xusheng]

Reading alb_break_llc() I cannot tell which set of tasks the equality is
meant to cover:

	/* kernel/sched/fair.c:10756 */
	if (env->src_rq->nr_pref_llc_running &&
	    env->src_rq->nr_pref_llc_running == env->src_rq->cfs.h_nr_runnable) {

The two counters track different sets.  nr_pref_llc_running is maintained
from account_entity_enqueue() and account_entity_dequeue() at fair.c:4522
and 4538, beside cfs_rq->nr_queued++/-- at 4525 and 4541, so it follows
queued tasks.  h_nr_runnable leaves out delay-dequeued entities:
set_delayed() decrements it at 6398 while the entity stays queued and
clear_delayed() restores it at 6418, neither going through
account_entity_dequeue().

So with DELAY_DEQUEUE a task that has just gone to sleep holds
nr_pref_llc_running above h_nr_runnable until it is dequeued for real, the
equality cannot hold, and alb_break_llc() returns false, which stops
active load balance from honouring the LLC preference.  The counter is a
superset of the other, so the error is one-sided: the check can fail to
protect but never protects wrongly.

Comparing against cfs.h_nr_queued would make both sides agree and read as
"every queued fair task prefers this LLC".  Keeping runnable semantics
would instead mean maintaining nr_pref_llc_running from set_delayed() and
clear_delayed(), which may be the better fit, since only runnable tasks
are candidates for active load balance and a task on its way to sleep is
not one.  Which did you have in mind?

This is from reading the code rather than an observed failure, and I could
not exercise the path in a two-socket qemu guest either: with cache aware
scheduling on and one process of 6 busy and 10 sleeping threads,
p->preferred_llc was never assigned, so nr_pref_llc_running stayed 0
throughout while alb_break_llc() itself ran 21 times.  Nearly every
account_mm_sched() call returned before the mm->sc_stat check, which makes
me suspect update_se() bails on delta_exec <= 0 under emulation before the
accounting is reached.  If a guest cannot exercise this, that is worth
knowing on its own, since it is also how a mismatch like this stays
hidden.

Thanks,
Zhan Xusheng

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

[Tim Chen]

On Thu, 2026-08-27 at 21:50 +0800, Zhan Xusheng wrote:
> Reading alb_break_llc() I cannot tell which set of tasks the equality is
> meant to cover:
> 
> 	/* kernel/sched/fair.c:10756 */
> 	if (env->src_rq->nr_pref_llc_running &&
> 	    env->src_rq->nr_pref_llc_running == env->src_rq->cfs.h_nr_runnable) {
> 
> The two counters track different sets.  nr_pref_llc_running is maintained
> from account_entity_enqueue() and account_entity_dequeue() at fair.c:4522
> and 4538, beside cfs_rq->nr_queued++/-- at 4525 and 4541, so it follows
> queued tasks.  h_nr_runnable leaves out delay-dequeued entities:
> set_delayed() decrements it at 6398 while the entity stays queued and
> clear_delayed() restores it at 6418, neither going through
> account_entity_dequeue().
> 
> So with DELAY_DEQUEUE a task that has just gone to sleep holds
> nr_pref_llc_running above h_nr_runnable until it is dequeued for real, the
> equality cannot hold, and alb_break_llc() returns false, which stops
> active load balance from honouring the LLC preference.  The counter is a
> superset of the other, so the error is one-sided: the check can fail to
> protect but never protects wrongly.

Thank you for your review of this code.  You have a valid point that DELAY_DEQUEUE
could cause problem with the check in question. The intention of the check is to take
care of the situation where all running tasks prefer the source LLC, and
we should try not to do active balance that will break LLC locality.
And delayed dequeue of a task preferring source LLC could break the check
unintentionally, even though the rest of running tasks still prefer source LLC.

> 
> Comparing against cfs.h_nr_queued would make both sides agree and read as
> "every queued fair task prefers this LLC".  Keeping runnable semantics
> would instead mean maintaining nr_pref_llc_running from set_delayed() and
> clear_delayed(), which may be the better fit, since only runnable tasks
> are candidates for active load balance and a task on its way to sleep is
> not one.  Which did you have in mind?

I think the proper thing to do is to make sure the nr_pref_llc_running accounts correctly
the number of running tasks that prefer the source LLC.  And
exclude those that are delayed dequeued.  How about the following fix:

Tim

---
From de4acf2ec43a57d84aa301b50c4ab51965ac1f23 Mon Sep 17 00:00:00 2001
From: Tim Chen <tim.c.chen@linux.intel.com>
Date: Thu, 27 Aug 2026 11:20:31 -0700
Subject: [PATCH] sched/cache: Keep nr_pref_llc_running in the runnable domain

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.

Say we start off with all running tasks preferring source LLC.
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().

Fix it on the counter side. Adjust nr_pref_llc_running in set_delayed()
and clear_delayed(), the same places that adjust h_nr_runnable, so both
exclude delayed tasks.

This needs care to not count a task twice. set_delayed() removes the task
from nr_pref_llc_running when it sleeps. Later, when the task is really
dequeued, dequeue_entity() calls account_llc_dequeue() and then
clear_delayed(). Left as is, account_llc_dequeue() would remove the task a
second time and clear_delayed() would add it back. So account_llc_dequeue()
skips its decrement while the task is still delayed, and clears
pref_llc_queued so clear_delayed() also leaves the count alone.
nr_llc_running and sd->llc_counts are not touched and stay on queued
semantics.

Reported-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 kernel/sched/fair.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d78467ec6ee1..1673b17273c5 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1544,7 +1544,14 @@ 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--;
+		/*
+		 * If the task is being finally dequeued while still delayed,
+		 * set_delayed() already removed it from nr_pref_llc_running;
+		 * skip here to avoid underflow. Clearing pref_llc_queued also
+		 * stops the subsequent clear_delayed() from re-adding it.
+		 */
+		if (!p->se.sched_delayed)
+			rq->nr_pref_llc_running--;
 		/*
 		 * Update the status in case
 		 * other logic might query
@@ -1572,6 +1579,24 @@ static void account_llc_dequeue(struct rq *rq, struct task_struct *p)
 	}
 }
 
+/*
+ * A task becoming delay-dequeued leaves the runnable set while staying
+ * queued. Keep nr_pref_llc_running in the runnable domain (like
+ * h_nr_runnable) so alb_break_llc() can compare the two directly.
+ */
+static void account_llc_delayed(struct rq *rq, struct task_struct *p)
+{
+	if (p->pref_llc_queued)
+		rq->nr_pref_llc_running--;
+}
+
+/* A delay-dequeued task becoming runnable again rejoins the count. */
+static void account_llc_requeue_delayed(struct rq *rq, struct task_struct *p)
+{
+	if (p->pref_llc_queued)
+		rq->nr_pref_llc_running++;
+}
+
 void mm_init_sched(struct mm_struct *mm,
 		   struct sched_cache_time __percpu *_pcpu_sched)
 {
@@ -1969,6 +1994,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 account_llc_delayed(struct rq *rq, struct task_struct *p) {}
+
+static void account_llc_requeue_delayed(struct rq *rq, struct task_struct *p) {}
+
 #endif /* CONFIG_SCHED_CACHE */
 
 /*
@@ -6203,6 +6232,13 @@ static void set_delayed(struct sched_entity *se)
 	if (!entity_is_task(se))
 		return;
 
+	/*
+	 * A delayed task is queued but no longer runnable. Drop it from
+	 * nr_pref_llc_running so that counter keeps runnable semantics and
+	 * stays comparable with h_nr_runnable in alb_break_llc().
+	 */
+	account_llc_delayed(rq_of(cfs_rq_of(se)), task_of(se));
+
 	for_each_sched_entity(se) {
 		struct cfs_rq *cfs_rq = cfs_rq_of(se);
 
@@ -6223,6 +6259,13 @@ static void clear_delayed(struct sched_entity *se)
 	if (!entity_is_task(se))
 		return;
 
+	/*
+	 * Re-add on wake (requeue_delayed_entity). On the final delayed
+	 * dequeue, account_llc_dequeue() has already cleared pref_llc_queued,
+	 * so this correctly does nothing.
+	 */
+	account_llc_requeue_delayed(rq_of(cfs_rq_of(se)), task_of(se));
+
 	for_each_sched_entity(se) {
 		struct cfs_rq *cfs_rq = cfs_rq_of(se);
 

base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
-- 
2.53.0

Re: [PATCH] sched/cache: Keep nr_pref_llc_running in the runnable domain

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

The ordering the skip depends on is not visible in the diff, so for the
record: dequeue_hierarchy() at fair.c:8148 reaches account_llc_dequeue()
through dequeue_entity(), and clear_delayed() runs only afterwards at
8164, so se->sched_delayed is still set when account_llc_dequeue() tests
it.

The sequences balance:

  enqueue, dequeue                   +1, -1
  enqueue, sleep, wake, dequeue      +1, -1 set_delayed,
                                     +1 clear_delayed, -1
  enqueue, sleep, real dequeue       +1, -1, skip, no re-add
  load balance moves a delayed task  source unchanged, destination +1
                                     with sched_delayed already clear

requeue_delayed_entity() does not reach account_entity_enqueue(), doing
only __enqueue_entity() and clear_delayed(), so the wake path cannot
double count.  account_mm_sched() cannot see a delayed task either, since
task_running_on_cpu() at fair.c:12245 requires task_on_rq_queued() on the
current cpu and the caller is update_curr().

sd->llc_counts is right to stay on queued semantics: both readers compare
it against another llc_counts value, at fair.c:11895 and 13228, so delayed
tasks shift both sides alike.

Reviewed-by: Zhan Xusheng <zhanxusheng@xiaomi.com>

Thanks,
Zhan Xusheng

Re: [PATCH] sched/cache: Keep nr_pref_llc_running in the runnable domain

[Tim Chen]

On Fri, 2026-08-28 at 10:20 +0800, Zhan Xusheng wrote:
> From: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 
> The ordering the skip depends on is not visible in the diff, so for the
> record: dequeue_hierarchy() at fair.c:8148 reaches account_llc_dequeue()
> through dequeue_entity(), and clear_delayed() runs only afterwards at
> 8164, so se->sched_delayed is still set when account_llc_dequeue() tests
> it.
> 
> The sequences balance:
> 
>   enqueue, dequeue                   +1, -1
>   enqueue, sleep, wake, dequeue      +1, -1 set_delayed,
>                                      +1 clear_delayed, -1
>   enqueue, sleep, real dequeue       +1, -1, skip, no re-add
>   load balance moves a delayed task  source unchanged, destination +1
>                                      with sched_delayed already clear
> 
> requeue_delayed_entity() does not reach account_entity_enqueue(), doing
> only __enqueue_entity() and clear_delayed(), so the wake path cannot
> double count.  account_mm_sched() cannot see a delayed task either, since
> task_running_on_cpu() at fair.c:12245 requires task_on_rq_queued() on the
> current cpu and the caller is update_curr().

Thanks for reviewing the patch proposed to fix the problem you pointed out,
and validating that the nr_pref_llc_running accounting under the
various scenarios are sane.

> 
> sd->llc_counts is right to stay on queued semantics: both readers compare
> it against another llc_counts value, at fair.c:11895 and 13228, so delayed
> tasks shift both sides alike.
> 

Yes, sd->llc_counts do not need to change as you pointed out.

> Reviewed-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 

Tim

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

[Chen Yu]

On Thu, Aug 27, 2026 at 01:57:33PM -0700, Tim Chen wrote:
> On Thu, 2026-08-27 at 21:50 +0800, Zhan Xusheng wrote:
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  kernel/sched/fair.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d78467ec6ee1..1673b17273c5 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1544,7 +1544,14 @@ 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--;
> +		/*
> +		 * If the task is being finally dequeued while still delayed,
> +		 * set_delayed() already removed it from nr_pref_llc_running;
> +		 * skip here to avoid underflow. Clearing pref_llc_queued also
> +		 * stops the subsequent clear_delayed() from re-adding it.
> +		 */
> +		if (!p->se.sched_delayed)
> +			rq->nr_pref_llc_running--;

Should we also do similar check in account_llc_enqueue()? It is possible during
load balance migration, a migrate_load allows the task to be migrated across
CPUs. And load balance leverages detach_tasks()/attach_tasks() to move tasks.
During this stage the p->se.sched_delayed remained unchanged:
In enqueue_task_fair(), only when if (se->on_rq && se->sched_delayed) is true,
p->se.sched_delayed will be cleared by requeue_delayed_entity() - during migration,
se->on_rq is false.

As a result, attach_tasks -> enqueue_hierarchy -> enqueue_entity -> account_llc_enqueue(rq, p)
incorrectly increase rq->nr_pref_llc_running - even that task is in delayed state, and
not runnable.

>  		/*
>  		 * Update the status in case
>  		 * other logic might query
> @@ -1572,6 +1579,24 @@ static void account_llc_dequeue(struct rq *rq, struct task_struct *p)
>  	}
>  }
>  
> +/*
> + * A task becoming delay-dequeued leaves the runnable set while staying
> + * queued. Keep nr_pref_llc_running in the runnable domain (like
> + * h_nr_runnable) so alb_break_llc() can compare the two directly.
> + */
>

Regarding alb_break_llc(), since we have compared nr_pref_llc_running vs 
nr_runnable, I wonder if we should also change the code?
	if (env->src_rq->nr_pref_llc_running &&
 	    env->src_rq->nr_pref_llc_running == env->src_rq->cfs.h_nr_runnable)
		unsigned long util = 0;
		struct task_struct *cur;
 
                if (env->src_rq->cfs.h_nr_runnable <= 1)
                        return true;

thanks,
Chenyu