Messages in this thread | | | Date | Wed, 4 Oct 2023 23:46:21 +0800 | From | Chen Yu <> | Subject | Re: [PATCH v2] sched/fair: Preserve PLACE_DEADLINE_INITIAL deadline |
| |
Hi Daniel,
On 2023-10-04 at 09:09:08 -0400, Daniel Jordan wrote: > An entity is supposed to get an earlier deadline with > PLACE_DEADLINE_INITIAL when it's forked, but the deadline gets > overwritten soon after in enqueue_entity() the first time a forked > entity is woken so that PLACE_DEADLINE_INITIAL is effectively a no-op. > > Placing in task_fork_fair() seems unnecessary since none of the values > that get set (slice, vruntime, deadline) are used before they're set > again at enqueue time, so get rid of that (and with it all of > task_fork_fair()) and just pass ENQUEUE_INITIAL to enqueue_entity() via > wake_up_new_task(). > > Fixes: e8f331bcc270 ("sched/smp: Use lag to simplify cross-runqueue placement") > Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com> > --- > > v2 > - place_entity() seems like the only reason for task_fork_fair() to exist > after the recent removal of sysctl_sched_child_runs_first, so take out > the whole function.
At first glance I thought if we remove task_fork_fair(), do we lose one chance to update the parent task's statistic in update_curr()? We might get out-of-date parent task's deadline and make preemption decision based on the stale data in wake_up_new_task() -> wakeup_preempt() -> pick_eevdf(). But after a second thought, I found that wake_up_new_task() -> enqueue_entity() itself would invoke update_curr(), so this should not be a problem.
Then I was wondering why can't we just skip place_entity() in enqueue_entity() if ENQUEUE_WAKEUP is not set, just like the code before e8f331bcc270? In this way the new fork task's deadline will not be overwritten by wake_up_new_task()-> enqueue_entity(). Then I realized that, after e8f331bcc270, the task's vruntime and deadline are all calculated by place_entity() rather than being renormalised to cfs_rq->min_vruntime in enqueue_entity(), so we can not simply skip place_entity() in enqueue_entity().
Per my understanding, this patch looks good,
Reviewed-by: Chen Yu <yu.c.chen@intel.com>
thanks, Chenyu
| |