[PATCH v3 0/2] sched/fair: Optimize some active balance logic

[PATCH v3 0/2] sched/fair: Optimize some active balance logic

[Xin Zhao]

Active balancing needs the help by migration threads which will interrupt
task on src_rq. It has a certain impact on overall performance. Active
balancing often fails, there is a check to determine whether the current
task(say it 'curr') on src_rq can run on dst_rq. We have observed that
even that, if curr is a CFS task and on_rq is 0, the failure rate of
active balancing is very high. Below are the test data from a certain
fillback task scenario executed on a platform with 18 CPUs over 300
seconds:

total: the total count of cases that match
cpumask_test_cpu(this_cpu, busiest->curr->cpus_ptr) &&
busiest->curr->sched_class == &fair_sched_class &&
!busiest->curr->on_rq
succ/fail: the active balance success/fail cases that match
cpumask_......->on_rq

                      total    succ    fail
cpu0  domain0 00003       0       0       0
cpu0  domain1 3ffff      32       0      32
cpu1  domain0 00003       0       0       0
cpu1  domain1 3ffff      40       0      40
cpu2  domain0 0003c       3       0       3
cpu2  domain1 3ffff       6       0       6
cpu3  domain0 0003c       3       1       2
cpu3  domain1 3ffff       3       0       3
cpu4  domain0 0003c       3       0       3
cpu4  domain1 3ffff       4       0       4
cpu5  domain0 0003c       1       0       1
cpu5  domain1 3ffff       6       0       6
cpu6  domain0 003c0      39       0      39
cpu6  domain1 3ffff      36       0      36
cpu7  domain0 003c0     213       4     209
cpu7  domain1 3ffff      24       2      22
cpu8  domain0 003c0     242      16     226
cpu8  domain1 3ffff      16       0      16
cpu9  domain0 003c0       0       0       0
cpu9  domain1 3ffff       6       1       5
cpu10 domain0 03c00      58       1      57
cpu10 domain1 3ffff       0       0       0
cpu11 domain0 03c00      54       4      50
cpu11 domain1 3ffff       1       0       1
cpu12 domain0 03c00      66       1      65
cpu12 domain1 3ffff       0       0       0
cpu13 domain0 03c00      66       1      65
cpu13 domain1 3ffff       0       0       0
cpu14 domain0 3c000       0       0       0
cpu14 domain1 3ffff      57       5      52
cpu15 domain0 3c000      15       0      15
cpu15 domain1 3ffff      35       0      35
cpu16 domain0 3c000     148       3     145
cpu16 domain1 3ffff     109       1     108
cpu17 domain0 3c000     182       2     180
cpu17 domain1 3ffff      78       1      77

In __schedule(), before setting curr to next, during the execution of
pick_next_task(), sched_balance_rq() is called. It will unlock and then
re-lock the rq, creating "holes" during which other CPUs may see zero
rq->curr->on_rq. try_to_block_task() sets curr->on_rq to 0, and during the
rq lock "hole" in pick_next_task(), rq->curr has not yet been assigned to
next, resulting in curr->on_rq being seen as 0.

We do not need to perform active balancing when src_rq->curr is CFS task
but on_rq is 0, as other CFS tasks have been already checked just before.
For cases where src_rq->curr is a non-CFS task, we retain the affinity
check for dst_rq to trigger active balancing because such task is likely
to wake-up or woken-by src_rq CFS task which has similar affinity
characteristics to migrate.

Two reasons why not check sched_class and on_rq of busiest->curr with the
cpumask_test_cpu() check:
1. Let the PATCH not introduce new cases that skip logic for resetting
balance_interval to min_interval.
2. The check of whether busiest cpu has been just triggered active balance
filters a bit more cases than the check of sched_class and on_rq.

Additionally, in sched_balance_rq(), we unconditionally reset the
balance_interval to min_interval. The difference is that original logic
does not reset the balance_interval when dst_cpu softirq handler is
preempted while src_cpu successfully run the just-dispatched active
balancing, during the gaps between two need_active_balance() checks. It
seems that we haven't observed any substantial benefits from reducing the
opportunities for balance under such fluctuating conditions. So simplify
the need_active_balance() checks logic.

---
Changes in v3:
- Consider the cost by sched_class and on_rq check,
  as suggested by Aiqun(Maira) Yu.
  Move the check after the check of whether busiest cpu has been just
  triggered active balance.
- Separate the revise of balance_interval reset part to an independent
  patch, as suggested by Aiqun(Maira) Yu.
  Add more details about the independent patch.

Change in v2:
- Add reason in the commit log why we can see zero rq->curr->on_rq when we
  hold rq lock,
  as suggested by Valentin Schneider.
- Link to v2: https://lore.kernel.org/all/20260613073228.1951105-1-jackzxcui1989@163.com/

v1:
- Link to v1: https://lore.kernel.org/all/20260603125938.1938115-1-jackzxcui1989@163.com/

Xin Zhao (2):
  sched/fair: Don't trigger active lb if src_rq->curr is CFS and not
    on_rq
  sched/fair: Simplify balance_interval reset logic in
    sched_balance_rq()

 kernel/sched/fair.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] sched/fair: Don't trigger active lb if src_rq->curr is CFS and not on_rq

[Xin Zhao]

Active balancing needs the help by migration threads which will interrupt
task on src_rq. It has a certain impact on overall performance. Active
balancing often fails, there is a check to determine whether the current
task(say it 'curr') on src_rq can run on dst_rq. We have observed that
even that, if curr is a CFS task and on_rq is 0, the failure rate of
active balancing is very high. Below are the test data from a certain
fillback task scenario executed on a platform with 18 CPUs over 300
seconds:

total: the total count of cases that match
cpumask_test_cpu(this_cpu, busiest->curr->cpus_ptr) &&
busiest->curr->sched_class == &fair_sched_class &&
!busiest->curr->on_rq
succ/fail: the active balance success/fail cases that match
cpumask_......->on_rq

                      total    succ    fail
cpu0  domain0 00003       0       0       0
cpu0  domain1 3ffff      32       0      32
cpu1  domain0 00003       0       0       0
cpu1  domain1 3ffff      40       0      40
cpu2  domain0 0003c       3       0       3
cpu2  domain1 3ffff       6       0       6
cpu3  domain0 0003c       3       1       2
cpu3  domain1 3ffff       3       0       3
cpu4  domain0 0003c       3       0       3
cpu4  domain1 3ffff       4       0       4
cpu5  domain0 0003c       1       0       1
cpu5  domain1 3ffff       6       0       6
cpu6  domain0 003c0      39       0      39
cpu6  domain1 3ffff      36       0      36
cpu7  domain0 003c0     213       4     209
cpu7  domain1 3ffff      24       2      22
cpu8  domain0 003c0     242      16     226
cpu8  domain1 3ffff      16       0      16
cpu9  domain0 003c0       0       0       0
cpu9  domain1 3ffff       6       1       5
cpu10 domain0 03c00      58       1      57
cpu10 domain1 3ffff       0       0       0
cpu11 domain0 03c00      54       4      50
cpu11 domain1 3ffff       1       0       1
cpu12 domain0 03c00      66       1      65
cpu12 domain1 3ffff       0       0       0
cpu13 domain0 03c00      66       1      65
cpu13 domain1 3ffff       0       0       0
cpu14 domain0 3c000       0       0       0
cpu14 domain1 3ffff      57       5      52
cpu15 domain0 3c000      15       0      15
cpu15 domain1 3ffff      35       0      35
cpu16 domain0 3c000     148       3     145
cpu16 domain1 3ffff     109       1     108
cpu17 domain0 3c000     182       2     180
cpu17 domain1 3ffff      78       1      77

In __schedule(), before setting curr to next, during the execution of
pick_next_task(), sched_balance_rq() is called. It will unlock and then
re-lock the rq, creating "holes" during which other CPUs may see zero
rq->curr->on_rq. try_to_block_task() sets curr->on_rq to 0, and during the
rq lock "hole" in pick_next_task(), rq->curr has not yet been assigned to
next, resulting in curr->on_rq being seen as 0.

We do not need to perform active balancing when src_rq->curr is CFS task
but on_rq is 0, as other CFS tasks have been already checked just before.
For cases where src_rq->curr is a non-CFS task, we retain the affinity
check for dst_rq to trigger active balancing because such task is likely
to wake-up or woken-by src_rq CFS task which has similar affinity
characteristics to migrate.

Two reasons why not check sched_class and on_rq of busiest->curr with the
cpumask_test_cpu() check:
1. Let the PATCH not introduce new cases that skip logic for resetting
balance_interval to min_interval.
2. The check of whether busiest cpu has been just triggered active balance
filters a bit more cases than the check of sched_class and on_rq.

Signed-off-by: Xin Zhao <jackzxcui1989@163.com>
---

Change in v3:
- Consider the cost by sched_class and on_rq check,
  as suggested by Aiqun(Maira) Yu.
  Move the check after the check of whether busiest cpu has been just
  triggered active balance.

Change in v2:
- Add reason in the commit log why we can see zero rq->curr->on_rq when we
  hold rq lock,
  as suggested by Valentin Schneider.
- Link to v2: https://lore.kernel.org/all/20260613073228.1951105-1-jackzxcui1989@163.com/

v1:
- Link to v1: https://lore.kernel.org/all/20260603125938.1938115-1-jackzxcui1989@163.com/
---
 kernel/sched/fair.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b5819c489..1c5043629 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -13436,7 +13436,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 			 * ->active_balance_work.  Once set, it's cleared
 			 * only after active load balance is finished.
 			 */
-			if (!busiest->active_balance) {
+			if (!busiest->active_balance &&
+			    !(busiest->curr->sched_class == &fair_sched_class &&
+			      !busiest->curr->on_rq)) {
 				busiest->active_balance = 1;
 				busiest->push_cpu = this_cpu;
 				active_balance = 1;
-- 
2.34.1

[PATCH v3 2/2] sched/fair: Simplify balance_interval reset logic in sched_balance_rq()

[Xin Zhao]

In sched_balance_rq(), it is possible to call need_active_balance() twice
in quick succession, which is not appropriate. There are two conditions in
sched_balance_rq() that reset balance_interval to min_interval, one is
when the local variable active_balance is 0, and the other is when
need_active_balance() returns a non-zero value. The local variable
active_balance is initialized to 0. Therefore, the only situation in which
balance_interval can be reset to min_interval is if need_active_balance()
has been executed once, marking the local variable active_balance as 1,
and then the second call to need_active_balance() returns 0. In other
words, the case is that during the interval between two close calls to
need_active_balance(), busiest rq completes the recently dispatched active
balance stop work, which is quite rare.

There are mainly two scenarios that lead to reaching sched_balance_rq():
one is the newly idle balance triggered by __schedule(), and the other is
the periodic balance logic controlled by sd->balance_interval or
nohz.next_balance, which ultimately executes in the softirq context. The
vast majority of cases executing sched_balance_rq() in the system fall
under the first scenario. During the execution of __schedule(), preemption
is disabled, so the interval between two checks of need_active_balance()
in this case will not be long. Thus, before applying this patch,
balance_interval would only be unlikely to reset to min_interval during
the periodic balance logic, which is relatively much more less frequent
than newly balance. In the case which is in softirq context, the execution
of the two need_active_balance() checks can indeed be preempted by other
tasks, leading to a longer interval between the two checks. However, there
is no evidence to suggest that not resetting min_interval in these
low-probability cases caused by scheduling preemption offers any
significant benefits. It would be better to simplify this complex reset
logic for balance_interval to an unconditional reset.

Signed-off-by: Xin Zhao <jackzxcui1989@163.com>
---
 kernel/sched/fair.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1c5043629..b562a610b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -13457,10 +13457,8 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 		sd->nr_balance_failed = 0;
 	}
 
-	if (likely(!active_balance) || need_active_balance(&env)) {
-		/* We were unbalanced, so reset the balancing interval */
-		sd->balance_interval = sd->min_interval;
-	}
+	/* We were unbalanced, so reset the balancing interval */
+	sd->balance_interval = sd->min_interval;
 
 	goto out;
 
-- 
2.34.1

Re: [PATCH v3 1/2] sched/fair: Don't trigger active lb if src_rq->curr is CFS and not on_rq

[Valentin Schneider]

On 15/06/26 13:38, Xin Zhao wrote:
> Active balancing needs the help by migration threads which will interrupt
> task on src_rq. It has a certain impact on overall performance. Active
> balancing often fails, there is a check to determine whether the current
> task(say it 'curr') on src_rq can run on dst_rq. We have observed that
> even that, if curr is a CFS task and on_rq is 0, the failure rate of
> active balancing is very high. Below are the test data from a certain
> fillback task scenario executed on a platform with 18 CPUs over 300
> seconds:
>
> total: the total count of cases that match
> cpumask_test_cpu(this_cpu, busiest->curr->cpus_ptr) &&
> busiest->curr->sched_class == &fair_sched_class &&
> !busiest->curr->on_rq
> succ/fail: the active balance success/fail cases that match
> cpumask_......->on_rq
>
>                       total    succ    fail
> cpu0  domain0 00003       0       0       0
> cpu0  domain1 3ffff      32       0      32
> cpu1  domain0 00003       0       0       0
> cpu1  domain1 3ffff      40       0      40
> cpu2  domain0 0003c       3       0       3
> cpu2  domain1 3ffff       6       0       6
> cpu3  domain0 0003c       3       1       2
> cpu3  domain1 3ffff       3       0       3
> cpu4  domain0 0003c       3       0       3
> cpu4  domain1 3ffff       4       0       4
> cpu5  domain0 0003c       1       0       1
> cpu5  domain1 3ffff       6       0       6
> cpu6  domain0 003c0      39       0      39
> cpu6  domain1 3ffff      36       0      36
> cpu7  domain0 003c0     213       4     209
> cpu7  domain1 3ffff      24       2      22
> cpu8  domain0 003c0     242      16     226
> cpu8  domain1 3ffff      16       0      16
> cpu9  domain0 003c0       0       0       0
> cpu9  domain1 3ffff       6       1       5
> cpu10 domain0 03c00      58       1      57
> cpu10 domain1 3ffff       0       0       0
> cpu11 domain0 03c00      54       4      50
> cpu11 domain1 3ffff       1       0       1
> cpu12 domain0 03c00      66       1      65
> cpu12 domain1 3ffff       0       0       0
> cpu13 domain0 03c00      66       1      65
> cpu13 domain1 3ffff       0       0       0
> cpu14 domain0 3c000       0       0       0
> cpu14 domain1 3ffff      57       5      52
> cpu15 domain0 3c000      15       0      15
> cpu15 domain1 3ffff      35       0      35
> cpu16 domain0 3c000     148       3     145
> cpu16 domain1 3ffff     109       1     108
> cpu17 domain0 3c000     182       2     180
> cpu17 domain1 3ffff      78       1      77
>
> In __schedule(), before setting curr to next, during the execution of
> pick_next_task(), sched_balance_rq() is called. It will unlock and then
> re-lock the rq, creating "holes" during which other CPUs may see zero
> rq->curr->on_rq. try_to_block_task() sets curr->on_rq to 0, and during the
> rq lock "hole" in pick_next_task(), rq->curr has not yet been assigned to
> next, resulting in curr->on_rq being seen as 0.
>

Aaah, it's a load balance shaped hole... Urgh, okay I see it now, thanks.

> We do not need to perform active balancing when src_rq->curr is CFS task
> but on_rq is 0, as other CFS tasks have been already checked just before.
> For cases where src_rq->curr is a non-CFS task, we retain the affinity
> check for dst_rq to trigger active balancing because such task is likely
> to wake-up or woken-by src_rq CFS task which has similar affinity
> characteristics to migrate.
>
> Two reasons why not check sched_class and on_rq of busiest->curr with the
> cpumask_test_cpu() check:
> 1. Let the PATCH not introduce new cases that skip logic for resetting
> balance_interval to min_interval.
> 2. The check of whether busiest cpu has been just triggered active balance
> filters a bit more cases than the check of sched_class and on_rq.
>
> Signed-off-by: Xin Zhao <jackzxcui1989@163.com>
> ---
>
> Change in v3:
> - Consider the cost by sched_class and on_rq check,
>   as suggested by Aiqun(Maira) Yu.
>   Move the check after the check of whether busiest cpu has been just
>   triggered active balance.
>
> Change in v2:
> - Add reason in the commit log why we can see zero rq->curr->on_rq when we
>   hold rq lock,
>   as suggested by Valentin Schneider.
> - Link to v2: https://lore.kernel.org/all/20260613073228.1951105-1-jackzxcui1989@163.com/
>
> v1:
> - Link to v1: https://lore.kernel.org/all/20260603125938.1938115-1-jackzxcui1989@163.com/
> ---
>  kernel/sched/fair.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index b5819c489..1c5043629 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13436,7 +13436,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
>                        * ->active_balance_work.  Once set, it's cleared
>                        * only after active load balance is finished.
>                        */
> -			if (!busiest->active_balance) {
> +			if (!busiest->active_balance &&
> +			    !(busiest->curr->sched_class == &fair_sched_class &&
> +			      !busiest->curr->on_rq)) {

This should be commented; I restructured this a little, it's one more label
but it reads better to me, what do you think?
---
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d78467ec6ee13..a0ac31a3be988 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -13482,12 +13482,22 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 			 * ->active_balance_work.  Once set, it's cleared
 			 * only after active load balance is finished.
 			 */
-			if (!busiest->active_balance) {
-				busiest->active_balance = 1;
-				busiest->push_cpu = this_cpu;
-				active_balance = 1;
-			}
+			if (busiest->active_balance)
+				goto no_active_balance;
 
+			/*
+			 * @busiest dropped its rq_lock in the middle of
+			 * scheduling out its ->curr task (->on_rq := 0), no
+			 * need to forcefully punt it away with active balance.
+			 */
+			if ((busiest->curr->sched_class == &fair_sched_class) &&
+			    !busiest->curr->on_rq)
+				goto no_active_balance;
+
+			busiest->active_balance = 1;
+			busiest->push_cpu = this_cpu;
+			active_balance = 1;
+no_active_balance:
 			preempt_disable();
 			raw_spin_rq_unlock_irqrestore(busiest, flags);
 			if (active_balance) {

Re: [PATCH v3 1/2] sched/fair: Don't trigger active lb if src_rq->curr is CFS and not on_rq

[Phil Auld]

Hi,

On Mon, Jun 15, 2026 at 03:39:22PM +0200 Valentin Schneider wrote:
> On 15/06/26 13:38, Xin Zhao wrote:
> > Active balancing needs the help by migration threads which will interrupt
> > task on src_rq. It has a certain impact on overall performance. Active
> > balancing often fails, there is a check to determine whether the current
> > task(say it 'curr') on src_rq can run on dst_rq. We have observed that
> > even that, if curr is a CFS task and on_rq is 0, the failure rate of
> > active balancing is very high. Below are the test data from a certain
> > fillback task scenario executed on a platform with 18 CPUs over 300
> > seconds:
> >
> > total: the total count of cases that match
> > cpumask_test_cpu(this_cpu, busiest->curr->cpus_ptr) &&
> > busiest->curr->sched_class == &fair_sched_class &&
> > !busiest->curr->on_rq
> > succ/fail: the active balance success/fail cases that match
> > cpumask_......->on_rq
> >
> >                       total    succ    fail
> > cpu0  domain0 00003       0       0       0
> > cpu0  domain1 3ffff      32       0      32
> > cpu1  domain0 00003       0       0       0
> > cpu1  domain1 3ffff      40       0      40
> > cpu2  domain0 0003c       3       0       3
> > cpu2  domain1 3ffff       6       0       6
> > cpu3  domain0 0003c       3       1       2
> > cpu3  domain1 3ffff       3       0       3
> > cpu4  domain0 0003c       3       0       3
> > cpu4  domain1 3ffff       4       0       4
> > cpu5  domain0 0003c       1       0       1
> > cpu5  domain1 3ffff       6       0       6
> > cpu6  domain0 003c0      39       0      39
> > cpu6  domain1 3ffff      36       0      36
> > cpu7  domain0 003c0     213       4     209
> > cpu7  domain1 3ffff      24       2      22
> > cpu8  domain0 003c0     242      16     226
> > cpu8  domain1 3ffff      16       0      16
> > cpu9  domain0 003c0       0       0       0
> > cpu9  domain1 3ffff       6       1       5
> > cpu10 domain0 03c00      58       1      57
> > cpu10 domain1 3ffff       0       0       0
> > cpu11 domain0 03c00      54       4      50
> > cpu11 domain1 3ffff       1       0       1
> > cpu12 domain0 03c00      66       1      65
> > cpu12 domain1 3ffff       0       0       0
> > cpu13 domain0 03c00      66       1      65
> > cpu13 domain1 3ffff       0       0       0
> > cpu14 domain0 3c000       0       0       0
> > cpu14 domain1 3ffff      57       5      52
> > cpu15 domain0 3c000      15       0      15
> > cpu15 domain1 3ffff      35       0      35
> > cpu16 domain0 3c000     148       3     145
> > cpu16 domain1 3ffff     109       1     108
> > cpu17 domain0 3c000     182       2     180
> > cpu17 domain1 3ffff      78       1      77
> >
> > In __schedule(), before setting curr to next, during the execution of
> > pick_next_task(), sched_balance_rq() is called. It will unlock and then
> > re-lock the rq, creating "holes" during which other CPUs may see zero
> > rq->curr->on_rq. try_to_block_task() sets curr->on_rq to 0, and during the
> > rq lock "hole" in pick_next_task(), rq->curr has not yet been assigned to
> > next, resulting in curr->on_rq being seen as 0.
> >
> 
> Aaah, it's a load balance shaped hole... Urgh, okay I see it now, thanks.
> 
> > We do not need to perform active balancing when src_rq->curr is CFS task
> > but on_rq is 0, as other CFS tasks have been already checked just before.
> > For cases where src_rq->curr is a non-CFS task, we retain the affinity
> > check for dst_rq to trigger active balancing because such task is likely
> > to wake-up or woken-by src_rq CFS task which has similar affinity
> > characteristics to migrate.
> >
> > Two reasons why not check sched_class and on_rq of busiest->curr with the
> > cpumask_test_cpu() check:
> > 1. Let the PATCH not introduce new cases that skip logic for resetting
> > balance_interval to min_interval.
> > 2. The check of whether busiest cpu has been just triggered active balance
> > filters a bit more cases than the check of sched_class and on_rq.
> >
> > Signed-off-by: Xin Zhao <jackzxcui1989@163.com>
> > ---
> >
> > Change in v3:
> > - Consider the cost by sched_class and on_rq check,
> >   as suggested by Aiqun(Maira) Yu.
> >   Move the check after the check of whether busiest cpu has been just
> >   triggered active balance.
> >
> > Change in v2:
> > - Add reason in the commit log why we can see zero rq->curr->on_rq when we
> >   hold rq lock,
> >   as suggested by Valentin Schneider.
> > - Link to v2: https://lore.kernel.org/all/20260613073228.1951105-1-jackzxcui1989@163.com/
> >
> > v1:
> > - Link to v1: https://lore.kernel.org/all/20260603125938.1938115-1-jackzxcui1989@163.com/
> > ---
> >  kernel/sched/fair.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index b5819c489..1c5043629 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -13436,7 +13436,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
> >                        * ->active_balance_work.  Once set, it's cleared
> >                        * only after active load balance is finished.
> >                        */
> > -			if (!busiest->active_balance) {
> > +			if (!busiest->active_balance &&
> > +			    !(busiest->curr->sched_class == &fair_sched_class &&
> > +			      !busiest->curr->on_rq)) {
> 
> This should be commented; I restructured this a little, it's one more label
> but it reads better to me, what do you think?

This below does look better to me as well, fwiw.


Cheers,
Phil

> ---
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d78467ec6ee13..a0ac31a3be988 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13482,12 +13482,22 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
>  			 * ->active_balance_work.  Once set, it's cleared
>  			 * only after active load balance is finished.
>  			 */
> -			if (!busiest->active_balance) {
> -				busiest->active_balance = 1;
> -				busiest->push_cpu = this_cpu;
> -				active_balance = 1;
> -			}
> +			if (busiest->active_balance)
> +				goto no_active_balance;
>  
> +			/*
> +			 * @busiest dropped its rq_lock in the middle of
> +			 * scheduling out its ->curr task (->on_rq := 0), no
> +			 * need to forcefully punt it away with active balance.
> +			 */
> +			if ((busiest->curr->sched_class == &fair_sched_class) &&
> +			    !busiest->curr->on_rq)
> +				goto no_active_balance;
> +
> +			busiest->active_balance = 1;
> +			busiest->push_cpu = this_cpu;
> +			active_balance = 1;
> +no_active_balance:
>  			preempt_disable();
>  			raw_spin_rq_unlock_irqrestore(busiest, flags);
>  			if (active_balance) {
> 
> 

-- 

Re: [PATCH v3 1/2] sched/fair: Don't trigger active lb if src_rq->curr is CFS and not on_rq

[Xin Zhao]

On Mon, 15 Jun 2026 15:39:22 +0200 Valentin Schneider <vschneid@redhat.com> wrote:

> > @@ -13436,7 +13436,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
> >                        * ->active_balance_work.  Once set, it's cleared
> >                        * only after active load balance is finished.
> >                        */
> > -			if (!busiest->active_balance) {
> > +			if (!busiest->active_balance &&
> > +			    !(busiest->curr->sched_class == &fair_sched_class &&
> > +			      !busiest->curr->on_rq)) {
> 
> This should be commented; I restructured this a little, it's one more label
> but it reads better to me, what do you think?

:) Thank you for your changes; it looks much better now, and the necessary
comments have been added.

> ---
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d78467ec6ee13..a0ac31a3be988 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13482,12 +13482,22 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
>  			 * ->active_balance_work.  Once set, it's cleared
>  			 * only after active load balance is finished.
>  			 */
> -			if (!busiest->active_balance) {
> -				busiest->active_balance = 1;
> -				busiest->push_cpu = this_cpu;
> -				active_balance = 1;
> -			}
> +			if (busiest->active_balance)
> +				goto no_active_balance;
>  
> +			/*
> +			 * @busiest dropped its rq_lock in the middle of
> +			 * scheduling out its ->curr task (->on_rq := 0), no
> +			 * need to forcefully punt it away with active balance.
> +			 */
> +			if ((busiest->curr->sched_class == &fair_sched_class) &&
> +			    !busiest->curr->on_rq)
> +				goto no_active_balance;
> +
> +			busiest->active_balance = 1;
> +			busiest->push_cpu = this_cpu;
> +			active_balance = 1;
> +no_active_balance:
>  			preempt_disable();
>  			raw_spin_rq_unlock_irqrestore(busiest, flags);
>  			if (active_balance) {

Thanks
Xin Zhao

Re: [PATCH v3 1/2] sched/fair: Don't trigger active lb if src_rq->curr is CFS and not on_rq

[Aiqun(Maria) Yu]

On 6/15/2026 10:09 PM, Xin Zhao wrote:
> On Mon, 15 Jun 2026 15:39:22 +0200 Valentin Schneider <vschneid@redhat.com> wrote:
> 
>>> @@ -13436,7 +13436,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
>>>                        * ->active_balance_work.  Once set, it's cleared
>>>                        * only after active load balance is finished.
>>>                        */
>>> -			if (!busiest->active_balance) {
>>> +			if (!busiest->active_balance &&
>>> +			    !(busiest->curr->sched_class == &fair_sched_class &&
>>> +			      !busiest->curr->on_rq)) {
>>
>> This should be commented; I restructured this a little, it's one more label
>> but it reads better to me, what do you think?
> 
> :) Thank you for your changes; it looks much better now, and the necessary
> comments have been added.

This format looks better.

> 
>> ---
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index d78467ec6ee13..a0ac31a3be988 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -13482,12 +13482,22 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
>>  			 * ->active_balance_work.  Once set, it's cleared
>>  			 * only after active load balance is finished.
>>  			 */
>> -			if (!busiest->active_balance) {
>> -				busiest->active_balance = 1;
>> -				busiest->push_cpu = this_cpu;
>> -				active_balance = 1;
>> -			}
>> +			if (busiest->active_balance)
>> +				goto no_active_balance;
>>  
>> +			/*
>> +			 * @busiest dropped its rq_lock in the middle of
>> +			 * scheduling out its ->curr task (->on_rq := 0), no
>> +			 * need to forcefully punt it away with active balance.
>> +			 */
>> +			if ((busiest->curr->sched_class == &fair_sched_class) &&
>> +			    !busiest->curr->on_rq)

Still have the doubt why need to do active load balance when the current
is not CFS. And busiest->curr->on_rq==0 case only.

Is it should be like:
if ((busiest->curr->sched_class == &fair_sched_class) ||
unlikely(!task_on_rq_queued(busiest->curr)));

if busiest->curr->on_rq==2 (TASK_ON_RQ_MIGRATING), it is no need to do
active_balance as well.

>> +				goto no_active_balance;
>> +
>> +			busiest->active_balance = 1;
>> +			busiest->push_cpu = this_cpu;
>> +			active_balance = 1;
>> +no_active_balance:
>>  			preempt_disable();
>>  			raw_spin_rq_unlock_irqrestore(busiest, flags);
>>  			if (active_balance) {
> 
> Thanks
> Xin Zhao
> 


-- 
Thx and BRs,
Aiqun(Maria) Yu

Re: [PATCH v3 1/2] sched/fair: Don't trigger active lb if src_rq->curr is CFS and not on_rq

[Xin Zhao]

On Tue, 16 Jun 2026 11:06:54 +0800 "Aiqun(Maria) Yu" <aiqun.yu@oss.qualcomm.com> wrote:

> >> +			 * need to forcefully punt it away with active balance.
> >> +			 */
> >> +			if ((busiest->curr->sched_class == &fair_sched_class) &&
> >> +			    !busiest->curr->on_rq)
> 
> Still have the doubt why need to do active load balance when the current
> is not CFS. And busiest->curr->on_rq==0 case only.
> 
> Is it should be like:
> if ((busiest->curr->sched_class == &fair_sched_class) ||
> unlikely(!task_on_rq_queued(busiest->curr)));

The platform I am using is quite close to actual usage, and the tasks
bound to different CPUs and different scheduling domains vary
significantly. From my data, there isn't a single CPU with an active
balance success rate below 50% under such scenarios. As for your earlier
comment that "just do detach_task and attach_task and the success rate
will be 100%", I believe you should take a closer look at the
implementation of active_load_balance_cpu_stop. That should make it clear
where the statistical code I previously sent you is located, instead of
simply questioning my approach of performing an ineffective "detach_task
and attach_task" operation to artificially inflate the success rate during
testing.

Regarding why the success rate of active balance is so high in this case,
I have my own reasonable explanations, and I prefer not to repeat them
over and over.

> if busiest->curr->on_rq==2 (TASK_ON_RQ_MIGRATING), it is no need to do
> active_balance as well.

There is a slight difference for the compiler between comparing == 1 and
comparing != 0. From my current understanding, comparing to a non-zero
value is generally faster than comparing to a specific number in most of
arch. I have previously tested whether on_rq would ever be equal to 2 and,
over a long period of time, I did not encounter a single instance where
on_rq was 2. This at least indicates that the probability is extremely
low, and it may even be theoretically impossible for such a situation to
occur.

Thanks
Xin Zhao