Re: [PATCH 1/1] sched: move update_curr() in check_preempt_wakeup() to avoid redundant call

[jupyung lee <jupyung@gmail.com>]

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