Re: [PATCH v2 3/3] sched/fair: break out of newidle balancing if an RT task appears

[Peter Zijlstra]

I'm going to pretend to have never seen the prior two patches. They do
absolutely horrible things for unspecified reasons. You've utterly
failed to explain what exactly is taking that 1ms+.

newidle_balance() already has 'stop, you're spending too much time'
controls; you've failed to explain how those are falling short and why
they cannot be improved.

On Wed, Apr 28, 2021 at 06:28:21PM -0500, Scott Wood wrote:
> The CFS load balancer can take a little while, to the point of it having
> a special LBF_NEED_BREAK flag, when the task moving code takes a
> breather.
> 
> However, at that point it will jump right back in to load balancing,
> without checking whether the CPU has gained any runnable real time
> (or deadline) tasks.
> 
> Break out of load balancing in the CPU_NEWLY_IDLE case, to allow the
> scheduling of the RT task.  Without this, latencies of over 1ms are
> seen on large systems.
> 
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxx>
> Reported-by: Clark Williams <williams@xxxxxxxxxx>
> Signed-off-by: Clark Williams <williams@xxxxxxxxxx>
> [swood: Limit change to newidle]
> Signed-off-by: Scott Wood <swood@xxxxxxxxxx>
> ---
> v2: Only break out of newidle balancing
> 
>  kernel/sched/fair.c  | 24 ++++++++++++++++++++----
>  kernel/sched/sched.h |  6 ++++++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index aa8c87b6aff8..c3500c963af2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9502,10 +9502,21 @@ imbalanced_active_balance(struct lb_env *env)
>  	return 0;
>  }
>  
> -static int need_active_balance(struct lb_env *env)
> +static bool stop_balance_early(struct lb_env *env)
> +{
> +	return env->idle == CPU_NEWLY_IDLE && rq_has_higher_tasks(env->dst_rq);
> +}
> +
> +static int need_active_balance(struct lb_env *env, int *continue_balancing)
>  {
>  	struct sched_domain *sd = env->sd;
>  
> +	/* Run the realtime task now; load balance later. */
> +	if (stop_balance_early(env)) {
> +		*continue_balancing = 0;
> +		return 0;
> +	}

This placement doesn't make any sense. You very much want this to return
true for the sd->balance_interval = sd->min_interval block for example.

And the other callsite already has an if (idle != CPU_NEWLY_IDLE)
condition to use.

Also, I don't think we care about the higher thing here (either);
newidle is about getting *any* work here, if there's something to do, we
don't need to do more.

> +
>  	if (asym_active_balance(env))
>  		return 1;
>  
> @@ -9550,7 +9561,7 @@ static int should_we_balance(struct lb_env *env)
>  	 * to do the newly idle load balance.
>  	 */
>  	if (env->idle == CPU_NEWLY_IDLE)
> -		return 1;
> +		return !rq_has_higher_tasks(env->dst_rq);

has_higher_task makes no sense here, newidle can stop the moment
nr_running != 0.

>  
>  	/* Try to find first idle CPU */
>  	for_each_cpu_and(cpu, group_balance_mask(sg), env->cpus) {
> @@ -9660,6 +9671,11 @@ static int load_balance(int this_cpu, struct rq *this_rq,
>  
>  		local_irq_restore(rf.flags);
>  
> +		if (stop_balance_early(&env)) {
> +			*continue_balancing = 0;
> +			goto out;
> +		}

Same thing.

> +
>  		if (env.flags & LBF_NEED_BREAK) {
>  			env.flags &= ~LBF_NEED_BREAK;
>  			goto more_balance;