Re: [RFC PATCH RESEND 03/10] sched/fair: Clear active_balance at the end of active_load_balance_cpu_stop()

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Xin,

On 9/10/2026 9:59 AM, Xin Zhao wrote:
> The rq->active_balance flag is used to prevent multiple CPUs from
> simultaneously dispatching active balance stop tasks. Since there can only
> ever be one consumer of the stop task, it is not strictly necessary to
> protect the setting of rq->active_balance to 0 with the rq lock in
> active_load_balance_cpu_stop(). Therefore, we can move the action of
> clearing rq->active_balance to the end of active_load_balance_cpu_stop().
> The benefit of this approach is that the task load of dst_rq will change
> due to the execution of attach_one_task(), which helps avoid prematurely
> clearing rq->active_balance before attach_one_task(), thus preventing
> unnecessary dispatch of duplicate active balance stop tasks.

Aren't we moving tasks *out* of the busiest CPU where the stopper is
scheduled?

As soon as we do detach_one_task() within the rq_lock, the load is
reflected correctly. There is no need to wait until we attach task to
a remote target_rq. TASK_ON_RQ_MIGRATING will immediately detach its
PELT signal from busiest.

That last statement seems to be inaccurate.

> 
> Active balance stop task is triggered only when rq->active_balance flag
> changes from 0 to 1, and there can be at most one consumer of active
> balance stop task at any given time. Therefore, we should never see zero
> rq->active_balance in active_load_balance_cpu_stop(), use WARN_ON_ONCE
> instead.
> 
> Signed-off-by: Xin Zhao <jackzxcui1989@163.com>
> ---
>  kernel/sched/fair.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 11c104010b2e..20d03ceed9d7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -13769,8 +13769,7 @@ static int active_load_balance_cpu_stop(void *data)
>  	if (!cpu_active(busiest_cpu) || !cpu_active(target_cpu))
>  		goto out_unlock;
>  
> -	if (unlikely(!busiest_rq->active_balance))
> -		goto out_unlock;
> +	WARN_ON_ONCE(!busiest_rq->active_balance);
>  
>  	/* Is there any task to move? */
>  	if (busiest_rq->nr_running <= 1)
> @@ -13815,13 +13814,13 @@ static int active_load_balance_cpu_stop(void *data)
>  	}
>  	rcu_read_unlock();
>  out_unlock:
> -	busiest_rq->active_balance = 0;
>  	rq_unlock(busiest_rq, &rf);
>  
>  	if (p)
>  		attach_one_task(target_rq, p);
>  
>  	local_irq_enable();

As soon as we enable IRQs, a timer for a remote tick may go off which
may want to push the task from this CPU again but it sees
->active_balance still set.

At the very least, I think this should be done before IRQs are enabled
but I'm not convinced by the justification for this in the commit
message.

> +	busiest_rq->active_balance = 0;
>  
>  	return 0;
>  }

-- 
Thanks and Regards,
Prateek