Re: [PATCH v2 3/3] sched/fair: break out of newidle balancing if an RT task appears
From: Peter Zijlstra <peterz@infradead.org>
Date: 2021-05-07 11:05:03
Also in:
lkml
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:
quoted hunk ↗ jump to hunk
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 <redacted> Reported-by: Clark Williams <redacted> Signed-off-by: Clark Williams <redacted> [swood: Limit change to newidle] Signed-off-by: Scott Wood <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
/* 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;