[PATCH] sched/fair: Fix task utilization accountability in cpu_util_next()

[vincent . donnefort]

From: Vincent Donnefort <vincent.donnefort@xxxxxxx>

Currently, cpu_util_next() estimates the CPU utilization as follows:

  max(cpu_util + task_util,
      cpu_util_est + task_util_est)

This is an issue when making a comparison between CPUs, as the task
contribution can be either:

  (1) task_util_est, on a mostly idle CPU, where cpu_util is close to 0
      and task_util_est > cpu_util.
  (2) task_util, on a mostly busy CPU, where cpu_util > task_util_est.

This gives an unfair advantage to some CPUs, when comparing energy deltas
in the task waking placement, where task_util is always smaller than
task_util_est. The energy delta is therefore, likely to be bigger on
a mostly idle CPU (1) than a mostly busy CPU (2).

This issue is, moreover, not sporadic. By starving idle CPUs, it keeps
their cpu_util < task_util_est (1) while others will maintain cpu_util >
task_util_est (2).

The new approach uses (if UTIL_EST is enabled) task_util_est() as task
contribution, which ensures that all CPUs will use the same value:

  max(cpu_util + max(task_util, task_util_est),
      cpu_util_est + max(task_util, task_util_est))

This patch doesn't modify the !UTIL_EST behaviour.

Also, replace sub_positive with lsub_positive which saves one explicit
load-store.

Signed-off-by: Vincent Donnefort <vincent.donnefort@xxxxxxx>

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index fb9f10d4312b..dd143aafaf97 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6516,32 +6516,42 @@ static unsigned long cpu_util_without(int cpu, struct task_struct *p)
 static unsigned long cpu_util_next(int cpu, struct task_struct *p, int dst_cpu)
 {
 	struct cfs_rq *cfs_rq = &cpu_rq(cpu)->cfs;
-	unsigned long util_est, util = READ_ONCE(cfs_rq->avg.util_avg);
+	unsigned long util = READ_ONCE(cfs_rq->avg.util_avg);
 
 	/*
-	 * If @p migrates from @cpu to another, remove its contribution. Or,
-	 * if @p migrates from another CPU to @cpu, add its contribution. In
-	 * the other cases, @cpu is not impacted by the migration, so the
-	 * util_avg should already be correct.
+	 * UTIL_EST case: hide the task_util contribution from util.
+	 * During wake-up, the task isn't enqueued yet and doesn't
+	 * appear in the util_est of any CPU. No contribution has
+	 * therefore to be removed from util_est.
+	 *
+	 * If @p migrates to this CPU, add its contribution to util and
+	 * util_est.
 	 */
-	if (task_cpu(p) == cpu && dst_cpu != cpu)
-		sub_positive(&util, task_util(p));
-	else if (task_cpu(p) != cpu && dst_cpu == cpu)
-		util += task_util(p);
-
 	if (sched_feat(UTIL_EST)) {
+		unsigned long util_est;
+
 		util_est = READ_ONCE(cfs_rq->avg.util_est.enqueued);
 
-		/*
-		 * During wake-up, the task isn't enqueued yet and doesn't
-		 * appear in the cfs_rq->avg.util_est.enqueued of any rq,
-		 * so just add it (if needed) to "simulate" what will be
-		 * cpu_util() after the task has been enqueued.
-		 */
-		if (dst_cpu == cpu)
-			util_est += _task_util_est(p);
+		if (task_cpu(p) == cpu)
+			lsub_positive(&util, task_util(p));
+
+		if (dst_cpu == cpu) {
+			unsigned long task_util = task_util_est(p);
+
+			util += task_util;
+			util_est += task_util;
+		}
 
 		util = max(util, util_est);
+	/*
+	 * !UTIL_EST case: If @p migrates from @cpu to another, remove its
+	 * contribution. If @p migrates to @cpu, add it.
+	 */
+	} else {
+		if (task_cpu(p) == cpu && dst_cpu != cpu)
+			lsub_positive(&util, task_util(p));
+		else if (task_cpu(p) != cpu && dst_cpu == cpu)
+			util += task_util(p);
 	}
 
 	return min(util, arch_scale_cpu_capacity(cpu));
-- 
2.25.1