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@xxxxxxx>
> ---
>
> 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@xxxxxxx/
>
> v1:
> - Link to v1: https://lore.kernel.org/all/20260603125938.1938115-1-jackzxcui1989@xxxxxxx/
> ---
>  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) {