Messages in this thread | | | Date | Fri, 20 Nov 2009 17:26:30 +0900 | Subject | Re: [PATCH 1/1] sched: move update_curr() in check_preempt_wakeup() to avoid redundant call | From | jupyung lee <> |
| |
Hi Peter,
On Nov 20, 8:00 am, Peter Zijlstra <pet...@infradead.org> wrote: > On Tue, 2009-11-17 at 18:51 +0900, Jupyung Lee wrote: > > Impact: micro-optimization > > > If a RT task is woken up while a non-RT task is running, > > check_preempt_wakeup() is called to check whether the new task can > > preempt the old task. The function returns quickly without going deeper > > because it is apparent that a RT task can always preempt a non-RT task. > > > In this situation, check_preempt_wakeup() always calls update_curr() to > > update vruntime value of the currently running task. However, the function > > call is unnecessary and redundant at that moment because (1) a non-RT task can > > always be preempted by a RT task regardless of its vruntime value, and > > (2) update_curr() will be called shortly when the context switch between two occurs. > > > By moving update_curr() in check_preempt_wakeup(), we can avoid redundant > > call to update_curr(), slightly reducing the time taken to wake up RT tasks. > > So far so good, but then you let me figure out why you placed it where > you did. >
Currently, in check_preempt_wakeup(), update_curr(cfs_rq) is located as follows:
-----------------------------------------------------------------------------------------
update_curr(cfs_rq);
if (unlikely(rt_prio(p->prio))) { .... /* A */ resched_task(curr); return; }
if (unlikely(p->sched_class != &fair_sched_class)) .... /* B */ return;
if (unlikely(se == pse)) .... /* C */ return;
if (sched_feat(NEXT_BUDDY) && scale && !(wake_flags & WF_FORK)) .... /* D */ set_next_buddy(pse);
if (test_tsk_need_resched(curr)) .... /* E */ return;
if (unlikely(p->policy != SCHED_NORMAL)) .... /* F */ return;
if (unlikely(curr->policy == SCHED_IDLE)) { .... /* G */ resched_task(curr); return; }
if ((sched_feat(WAKEUP_SYNC) && sync) || .... /* H */ (sched_feat(WAKEUP_OVERLAP) && (se->avg_overlap < sysctl_sched_migration_cost && pse->avg_overlap < sysctl_sched_migration_cost))) { resched_task(curr); return; }
if (sched_feat(WAKEUP_RUNNING)) { .... /* I */ if (pse->avg_running < se->avg_running) { set_next_buddy(pse); resched_task(curr); return; } }
if (!sched_feat(WAKEUP_PREEMPT)) .... /* J */ return;
find_matching_se(&se, &pse);
BUG_ON(!pse);
if (wakeup_preempt_entity(se, pse) == 1) { .... /* K */ resched_task(curr);
-----------------------------------------------------------------------------------------
Among operations A to L, only 'K' is dependent on update_curr(), which updates curr->vruntime; wakeup_preempt_entity() compares curr->vruntime with se->vruntime. Thus, it is straightforward to move update_curr() just above 'K'. It allows us to avoid unnecessary function call to update_curr() when we do not go up to 'K'. As I said above, calling the function update_curr() here is unnecessary and redundant except the case 'K' because it will be called shortly when the context switch between two occurs.
> Anyway, I think the patch is good and took the patch, thanks! >
Thanks.
Jupyung
| |