Re: PELT initial task load and wake_up_new_task()

[Yuyang Du <yuyang.du@intel.com>]

On Tue, Dec 15, 2015 at 10:45:53AM -0800, Steve Muckle wrote:
> On 12/14/2015 06:24 PM, Yuyang Du wrote:
> >>> On Fri, Dec 11, 2015 at 06:01:45PM -0800, Steve Muckle wrote:
> >>>> 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.
> >>
> >> 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.
> 
> 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.
 
Indeed it is. Sorry that I did not look at this carefully before.

I think it should still be regarded as migration. It looks better as such.

Hope the following patch should work.

---
Subject: [PATCH] sched: Fix new task's load avg removed from source CPU in
 wake_up_new_task()

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.

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 normal migration, the last_update_time is
set to 0 after remove_entity_load_avg().

Reported-by: Steve Muckle <steve.muckle@linaro.org>
Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/fair.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index e3266eb..4676988 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2909,6 +2909,12 @@ 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 should not be removed from the source CPU before migration
+	 */
+	if (se->avg.last_update_time == 0)
+		return;
+
 #ifndef CONFIG_64BIT
 	u64 last_update_time_copy;
 
--