find_energy_efficient_cpu() integrates a margin to protect tasks from
bouncing back and forth from a CPU to another. This margin is set as being
6% of the total current energy estimated on the system. This however does
not work for two reasons:
1. The energy estimation is not a good absolute value:
The function, compute_energy() used in feec() is a good estimation for
task placement as it allows to compare the energy with and without a task.
The computed delta will give a good overview of the cost for a certain
task placement. It, however, doesn't work as an absolute estimation for
the total energy of the system. First it adds the contribution to idle
CPUs into the energy, second it mixes util_avg with util_est values.
util_avg represents integrates the near history for a CPU usage,
it doesn't tell at all what the current utilization is. A system that has
been quite busy in the near past will hold a very high energy and then a
high margin preventing any task migration to a lower capacity CPU, wasting
energy. It even creates a negative feedback loop: by holding the tasks on
a less efficient CPU, the margin contributes in keeping the energy high.
2. The margin handicaps small tasks:
On a system where the workload is composed mostly of small tasks (which is
often the case on Android), the overall energy will be high enough to
create a margin none of those tasks can cross. e.g. On a Pixel4, a small
utilization of 5% on all the CPUs creates a global estimated energy of 140
joules, as per the Energy Model declaration of that same device. This
means, after applying the 6% margin that any migration must save more than
8 joules to happen. No task with a utilization lower than 40 would then be
able to migrate away from the biggest CPU of the system.
The 6% of the overall system energy was brought by the following patch:
(eb92692b2544 sched/fair: Speed-up energy-aware wake-ups)
It was previously 6% of the prev_cpu energy. Also, the following one
made this margin value conditional on the clusters where the task fits:
(8d4c97c105ca sched/fair: Only compute base_energy_pd if necessary)
We could simply revert that margin change to what it was, but the original
version didn't have strong grounds neither and as demonstrated in (1.) the
estimated energy isn't a good absolute value. Instead, removing it
completely. It is indeed, made possible by recent changes that improved
energy estimation comparison fairness (sched/fair: Remove task_util from
effective utilization in feec()) (PM: EM: Increase energy calculation
precision) and task utilization stabilization (sched/fair: Decay task
util_avg during migration)
Without a margin, we could have feared bouncing between CPUs. But running
LISA's eas_behaviour test coverage on three different platforms (Hikey960,
RB-5 and DB-845) showed no issue and even fixed previously known failures.
Removing the energy margin enables more energy-optimized placements for a
more energy efficient system.
Signed-off-by: Vincent Donnefort <[email protected]>
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b48ba181c8ec..05518a6150e5 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6736,7 +6736,6 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
struct root_domain *rd = cpu_rq(smp_processor_id())->rd;
int cpu, best_energy_cpu = prev_cpu, target = -1;
unsigned long cpu_cap, cpu_thermal_cap, util;
- unsigned long base_energy = 0;
struct sched_domain *sd;
struct perf_domain *pd;
struct energy_env eenv;
@@ -6767,8 +6766,8 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
for (; pd; pd = pd->next) {
unsigned long cur_delta, spare_cap, max_spare_cap = 0;
bool compute_prev_delta = false;
- unsigned long base_energy_pd;
int max_spare_cap_cpu = -1;
+ unsigned long base_energy;
cpumask_and(cpus, perf_domain_span(pd), cpu_online_mask);
@@ -6823,16 +6822,15 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
/* Compute the 'base' energy of the pd, without @p */
eenv_pd_busy_time(&eenv, cpus, p);
- base_energy_pd = compute_energy(&eenv, pd, cpus, p, -1);
- base_energy += base_energy_pd;
+ base_energy = compute_energy(&eenv, pd, cpus, p, -1);
/* Evaluate the energy impact of using prev_cpu. */
if (compute_prev_delta) {
prev_delta = compute_energy(&eenv, pd, cpus, p,
prev_cpu);
- if (prev_delta < base_energy_pd)
+ if (prev_delta < base_energy)
goto unlock;
- prev_delta -= base_energy_pd;
+ prev_delta -= base_energy;
best_delta = min(best_delta, prev_delta);
}
@@ -6840,9 +6838,9 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
if (max_spare_cap_cpu >= 0) {
cur_delta = compute_energy(&eenv, pd, cpus, p,
max_spare_cap_cpu);
- if (cur_delta < base_energy_pd)
+ if (cur_delta < base_energy)
goto unlock;
- cur_delta -= base_energy_pd;
+ cur_delta -= base_energy;
if (cur_delta < best_delta) {
best_delta = cur_delta;
best_energy_cpu = max_spare_cap_cpu;
@@ -6851,12 +6849,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
}
rcu_read_unlock();
- /*
- * Pick the best CPU if prev_cpu cannot be used, or if it saves at
- * least 6% of the energy used by prev_cpu.
- */
- if ((prev_delta == ULONG_MAX) ||
- (prev_delta - best_delta) > ((prev_delta + base_energy) >> 4))
+ if (best_delta < prev_delta)
target = best_energy_cpu;
return target;
--
2.25.1
On 08/03/2022 19:19, Vincent Donnefort wrote:
[...]
> 1. The energy estimation is not a good absolute value:
>
> The function, compute_energy() used in feec() is a good estimation for
s/The function, compute_energy()/compute_energy() ... shorter
[...]
> util_avg represents integrates the near history for a CPU usage,
s/util_avg represents integrates/util_avg contains ?
[...]
> 2. The margin handicaps small tasks:
>
> On a system where the workload is composed mostly of small tasks (which is
> often the case on Android), the overall energy will be high enough to
> create a margin none of those tasks can cross. e.g. On a Pixel4, a small
s/e.g.// ?
[...]
> Without a margin, we could have feared bouncing between CPUs. But running
> LISA's eas_behaviour test coverage on three different platforms (Hikey960,
> RB-5 and DB-845) showed no issue and even fixed previously known failures.
Can you be more specific what those fixes are? I think you mention (some
of) them in the cover letter. It's just that when somebody will read
this patch, once it's in, this information is not there.
You could also add
https://developer.arm.com/tools-and-software/open-source-software/linux-kernel/energy-aware-scheduling/eas-mainline-development
to the cover letter so people can get more information about those EAS
behaviour tests.
[...]
> @@ -6736,7 +6736,6 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
> struct root_domain *rd = cpu_rq(smp_processor_id())->rd;
> int cpu, best_energy_cpu = prev_cpu, target = -1;
Nit-picking: IMHO, best_energy_cpu doesn't have to be initialized
anymore since we only use it now when `best_delta < prev_delta`,
hence best_energy_cpu has to be set at least once.
[...]
> @@ -6851,12 +6849,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
> }
> rcu_read_unlock();
>
> - /*
> - * Pick the best CPU if prev_cpu cannot be used, or if it saves at
> - * least 6% of the energy used by prev_cpu.
> - */
> - if ((prev_delta == ULONG_MAX) ||
> - (prev_delta - best_delta) > ((prev_delta + base_energy) >> 4))
> + if (best_delta < prev_delta)
> target = best_energy_cpu;
>
> return target;
Can we now move the `unlock:` before the first rcu_read_unlock()?
All error case have best_delta and prev_delta = ULONG_MAX so we return
`target = -1` or `target = prev_cpu` correctly.
@@ -6960,17 +6960,13 @@ static int find_energy_efficient_cpu(struct
task_struct *p, int prev_cpu)
}
}
}
+unlock:
rcu_read_unlock();
if (best_delta < prev_delta)
target = best_energy_cpu;
return target;
-
-unlock:
- rcu_read_unlock();
-
- return target;