Re: [PATCH 1/1] sched/fair: Update blocked averages on tick
From: Pierre Gondois
Date: Mon Oct 21 2024 - 05:47:48 EST
Hello Vincent,
On 10/15/24 14:44, Vincent Guittot wrote:
On Fri, 11 Oct 2024 at 14:32, Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
The Energy Aware Scheduler (EAS) relies on CPU/tasks utilization
signals. On an idle CPU, the blocked load is updated during
load balancing.
sd->balance_interval increases with the number of CPUs in the domain.
On an Arm DynamIQ system, sched domains containing CPUs with the same
capacity do not exist. On a Pixel6 with 4 little, 2 mid, 2 big CPUs:
- sd->min_interval = 8
- sd->min_interval = 16
The balance interval is doubled if the system is balanced, meaning
that a balanced system will likely update blocked load every 16ms.
The real max boundary is LOAD_AVG_PERIOD that is used to update
nohz.next_blocked. This is the max between 2 updates of blocked load.
The other ones are opportunistics updates when a normal load balance
is triggered.
I wanted to mean that on an idle CPU with ticks still on, the cfs_rq load
is only updated through this path:
sched_balance_trigger() {
if (time_after_eq(jiffies, rq->next_balance))
raise_softirq(SCHED_SOFTIRQ);
}
...
sched_balance_softirq()
\-sched_balance_update_blocked_averages()
If the next_balance happens every 16ms, this means feec() might operate
its task placement using an (up to) 16ms old util signal. The CPU might
thus look busier than what it actually is.
The find_energy_efficient_cpu() function might thus relies on outdated
util signals to place tasks, leading to bad energy placement.
Moving from 8ms to 16 ms is what makes the difference for you ?
With this patch, the cfs_rq signal of an idle CPU is updated every tick,
so every 4ms.
The LOAD_AVG_PERIOD mas period has been used as a default value but if
it's too long, we could consider changing the max period between 2
updates
Update blocked load on sched tick if:
- the rq is idle
- the load balancer will not be triggered.
Signed-off-by: Pierre Gondois <pierre.gondois@xxxxxxx>
---
kernel/sched/fair.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 225b31aaee55..2f03bd10ac7a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9841,15 +9841,12 @@ static unsigned long task_h_load(struct task_struct *p)
}
#endif
-static void sched_balance_update_blocked_averages(int cpu)
+static void update_blocked_averages(struct rq *rq)
{
bool decayed = false, done = true;
- struct rq *rq = cpu_rq(cpu);
- struct rq_flags rf;
- rq_lock_irqsave(rq, &rf);
- update_blocked_load_tick(rq);
update_rq_clock(rq);
+ update_blocked_load_tick(rq);
decayed |= __update_blocked_others(rq, &done);
decayed |= __update_blocked_fair(rq, &done);
@@ -9857,6 +9854,18 @@ static void sched_balance_update_blocked_averages(int cpu)
update_blocked_load_status(rq, !done);
if (decayed)
cpufreq_update_util(rq, 0);
+}
+
+static void sched_balance_update_blocked_averages(int cpu)
+{
+ struct rq *rq = cpu_rq(cpu);
+ struct cfs_rq *cfs_rq;
+ struct rq_flags rf;
+
+ cfs_rq = &rq->cfs;
+
+ rq_lock_irqsave(rq, &rf);
+ update_blocked_averages(rq);
rq_unlock_irqrestore(rq, &rf);
}
@@ -12877,6 +12886,8 @@ void sched_balance_trigger(struct rq *rq)
if (time_after_eq(jiffies, rq->next_balance))
raise_softirq(SCHED_SOFTIRQ);
+ else if (idle_cpu(rq->cpu))
+ update_blocked_averages(rq);
would be good to explain why you don't need rq lock here
This is a mistake, the lock is indeed required.
There is no rate limit so we can do this every tick (possibly 1ms)
when staying in shallowest state
I'm not sure we understood each other as this patch should no be related
to NOHZ CPUs. So please let me know if I used a wrong path as you said,
or if a rate limit would be needed.
So it's looks better to update the period between 2 update of blocked
load instead of adding a new path
nohz_balancer_kick(rq);
}
--
2.25.1
Regards,
Pierre