PELT initial task load and wake_up_new_task()

In init_entity_runnable_average() the last_update_time is initialized to zero. The task is given max load and utilization as a pessimistic initial estimate.

But if in wake_up_new_task() the task is placed on a CPU other than where it was created, __update_load_avg() will be called via set_task_cpu() -> migrate_task_rq_fair() -> remove_entity_load_avg().

Since last_update_time is zero the delta will be huge and the task's load will be entirely decayed away before it is enqueued at the destination CPU.

Hi Steve,

Quoted 8 lines · Steve Muckle

In init_entity_runnable_average() the last_update_time is initialized to zero. The task is given max load and utilization as a pessimistic initial estimate.

But if in wake_up_new_task() the task is placed on a CPU other than where it was created, __update_load_avg() will be called via set_task_cpu() -> migrate_task_rq_fair() -> remove_entity_load_avg().

Quoted 3 lines

Since last_update_time is zero the delta will be huge and the task's load will be entirely decayed away before it is enqueued at the destination CPU.

Since the new task's last_update_time is equal to 0, it will not be decayed.

Hi Yuyang,

Quoted 15 lines · Yuyang Du

Hi Steve,

Quoted 11 lines · Steve Muckle

In init_entity_runnable_average() the last_update_time is initialized to zero. The task is given max load and utilization as a pessimistic initial estimate.

But if in wake_up_new_task() the task is placed on a CPU other than where it was created, __update_load_avg() will be called via set_task_cpu() -> migrate_task_rq_fair() -> remove_entity_load_avg().

Since last_update_time is zero the delta will be huge and the task's load will be entirely decayed away before it is enqueued at the destination CPU.

Quoted 1 line

Since the new task's last_update_time is equal to 0, it will not be decayed.

Can you point me to the code for that logic? I don't see anything that prevents the decay when a newly woken task is placed on a different CPU via the call chain I mentioned above. My testing also shows the load being decayed to zero.

Quoted 15 lines

Quoted 14 lines · Steve Muckle

Quoted 11 lines

In init_entity_runnable_average() the last_update_time is initialized to zero. The task is given max load and utilization as a pessimistic initial estimate.

But if in wake_up_new_task() the task is placed on a CPU other than where it was created, __update_load_avg() will be called via set_task_cpu() -> migrate_task_rq_fair() -> remove_entity_load_avg().

Since last_update_time is zero the delta will be huge and the task's load will be entirely decayed away before it is enqueued at the destination CPU.

Since the new task's last_update_time is equal to 0, it will not be decayed.

Quoted 5 lines

Can you point me to the code for that logic? I don't see anything that prevents the decay when a newly woken task is placed on a different CPU via the call chain I mentioned above. My testing also shows the load being decayed to zero.

You may search the last_update_time, and see it would be treated differently if it is 0. Hope this may be helpful.

Quoted 20 lines · Yuyang Du

Quoted 20 lines

Quoted 9 lines · Steve Muckle

Quoted 8 lines

In init_entity_runnable_average() the last_update_time is initialized to zero. The task is given max load and utilization as a pessimistic initial estimate.

But if in wake_up_new_task() the task is placed on a CPU other than where it was created, __update_load_avg() will be called via set_task_cpu() -> migrate_task_rq_fair() -> remove_entity_load_avg().

Quoted 5 lines

Quoted 3 lines

Since last_update_time is zero the delta will be huge and the task's load will be entirely decayed away before it is enqueued at the destination CPU.

Since the new task's last_update_time is equal to 0, it will not be decayed.

Can you point me to the code for that logic? I don't see anything that prevents the decay when a newly woken task is placed on a different CPU via the call chain I mentioned above. My testing also shows the load being decayed to zero.

Quoted 2 lines

You may search the last_update_time, and see it would be treated differently if it is 0. Hope this may be helpful.

Are you referring to the test in enqueue_entity_load_avg()? If so that isn't called until after remove_entity_load_avg() in this scenario, which has no check on last_update_time.

Hope the following patch should work.

Quoted 1 line · Yuyang Du

Hope the following patch should work.

Thanks Yuyang. AFAICS it should work, though I believe the test on last_update_time could instead go at the top of migrate_task_rq_fair()? It'd save the fn call to remove_entity_load_avg() and two unnecessary assignments (as p->se.avg.last_update_time and p->se.exec_start = 0 for newly forked tasks). This worked for me.

kernel/sched/fair.c +2 -1

index e3266eb..3f6a8b3 100644
@@ -2908,10 +2908,18 @@ void remove_entity_load_avg(struct sched_entity *se)
 {
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 	u64 last_update_time;
-
 #ifndef CONFIG_64BIT
 	u64 last_update_time_copy;
+#endif
 
+	/*

Quoted 14 lines · Yuyang Du

kernel/sched/fair.c +2 -1

index e3266eb..3f6a8b3 100644
@@ -2908,10 +2908,18 @@ void remove_entity_load_avg(struct sched_entity *se)
 {
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 	u64 last_update_time;
-
 #ifndef CONFIG_64BIT
 	u64 last_update_time_copy;
+#endif
 
+	/*

kernel/sched/fair.c void remove_entity_load_avg(struct sched_entity *se) · line 2925

Quoted 10 lines

kernel/sched/fair.c +7 -0

+	 * Newly created task or never used group entity should not be removed
+	 * from its (source) cfs_rq
+	 */
+	if (se->avg.last_update_time == 0)
+		return;
+
+#ifndef CONFIG_64BIT
 	do {
 		last_update_time_copy = cfs_rq->load_last_update_time_copy;
 		smp_rmb();

So that ifdef stuff annoyed me a wee bit, so I did the below.

Initially I wanted to do a macro that we could use for both this and the vruntime, but that didn't end up particularly pretty either, so I scrapped that.

---
Subject: sched: Fix new task's load avg removed from source CPU in wake_up_new_task()
From: Yuyang Du <[email protected]>
Date: Thu, 17 Dec 2015 07:34:27 +0800

If a newly created task is selected to go to a different CPU in fork balance when it wakes up the first time, its load averages should not be removed from the source CPU since they are never added to it before. The same is also applicable to a never used group entity.

Fix it in remove_entity_load_avg(): when entity's last_update_time is 0, simply return. This should precisely identify the case in question, because in other migrations, the last_update_time is set to 0 after remove_entity_load_avg().

Reported-by Steve Muckle <[email protected]>

Signed-off-by Yuyang Du <[email protected]>

Cc Patrick Bellasi <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Vincent Guittot <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Dietmar Eggemann <[email protected]>

Cc Morten Rasmussen <[email protected]>

[peterz: cfs_rq_last_update_time]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 kernel/sched/fair.c |   38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

kernel/sched/fair.c +28 -10

@@ -2809,27 +2809,45 @@ dequeue_entity_load_avg(struct cfs_rq *c
 		max_t(s64,  cfs_rq->runnable_load_sum - se->avg.load_sum, 0);
 }
 
-/*
- * Task first catches up with cfs_rq, and then subtract
- * itself from the cfs_rq (task must be off the queue now).
- */
-void remove_entity_load_avg(struct sched_entity *se)
-{
-	struct cfs_rq *cfs_rq = cfs_rq_of(se);
-	u64 last_update_time;
-
 #ifndef CONFIG_64BIT
+static inline u64 cfs_rq_last_update_time(struct cfs_rq *cfs_rq)
+{
 	u64 last_update_time_copy;
+	u64 last_update_time;
 
 	do {
 		last_update_time_copy = cfs_rq->load_last_update_time_copy;
 		smp_rmb();
 		last_update_time = cfs_rq->avg.last_update_time;
 	} while (last_update_time != last_update_time_copy);
+
+	return last_update_time;
+}
 #else
-	last_update_time = cfs_rq->avg.last_update_time;
+static inline u64 cfs_rq_last_update_time(struct cfs_rq *cfs_rq)
+{
+	return cfs_rq->avg.last_update_time;
+}
 #endif
 
+/*
+ * Task first catches up with cfs_rq, and then subtract
+ * itself from the cfs_rq (task must be off the queue now).
+ */
+void remove_entity_load_avg(struct sched_entity *se)
+{
+	struct cfs_rq *cfs_rq = cfs_rq_of(se);
+	u64 last_update_time;
+
+	/*
+	 * Newly created task or never used group entity should not be removed
+	 * from its (source) cfs_rq
+	 */
+	if (se->avg.last_update_time == 0)
+		return;
+
+	last_update_time = cfs_rq_last_update_time(cfs_rq);
+
 	__update_load_avg(last_update_time, cpu_of(rq_of(cfs_rq)), &se->avg, 0, 0, NULL);
 	atomic_long_add(se->avg.load_avg, &cfs_rq->removed_load_avg);
 	atomic_long_add(se->avg.util_avg, &cfs_rq->removed_util_avg);