The dequeue_patch function in kernel/sched.c is complicated by
including a sleep parameter. This parameter is always zero
except in one instance. This patch clarifies the task of
dequeue_patch by removing the sleep parameter and moving the
code that handles non-zero sleep to that one place where it is
needed.
* David Newall <[email protected]> wrote:
> The dequeue_patch function in kernel/sched.c is complicated by
> including a sleep parameter. This parameter is always zero
> except in one instance. This patch clarifies the task of
> dequeue_patch by removing the sleep parameter and moving the
> code that handles non-zero sleep to that one place where it is
> needed.
>
> --- kernel/sched.c 2009-03-12 18:41:41.000000000 +1030
> +++ kernel/sched.c.dn 2009-03-12 18:45:18.000000000 +1030
> @@ -1791,21 +1791,10 @@
> p->se.on_rq = 1;
> }
>
> -static void dequeue_task(struct rq *rq, struct task_struct *p, int sleep)
> +static void dequeue_task(struct rq *rq, struct task_struct *p)
> {
> - if (sleep) {
> - if (p->se.last_wakeup) {
> - update_avg(&p->se.avg_overlap,
> - p->se.sum_exec_runtime - p->se.last_wakeup);
> - p->se.last_wakeup = 0;
> - } else {
> - update_avg(&p->se.avg_wakeup,
> - sysctl_sched_wakeup_granularity);
> - }
> - }
> -
> sched_info_dequeued(p);
> - p->sched_class->dequeue_task(rq, p, sleep);
> + p->sched_class->dequeue_task(rq, p, 0);
> p->se.on_rq = 0;
> }
>
> @@ -1875,7 +1864,22 @@
> if (task_contributes_to_load(p))
> rq->nr_uninterruptible++;
>
> - dequeue_task(rq, p, sleep);
> + if (sleep) {
> + if (p->se.last_wakeup) {
> + update_avg(&p->se.avg_overlap,
> + p->se.sum_exec_runtime - p->se.last_wakeup);
> + p->se.last_wakeup = 0;
> + } else {
> + update_avg(&p->se.avg_wakeup,
> + sysctl_sched_wakeup_granularity);
> + }
> + }
> +
> + /*dequeue_task(rq, p, sleep);*/
> + sched_info_dequeued(p);
> + p->sched_class->dequeue_task(rq, p, sleep);
> + p->se.on_rq = 0;
> +
> dec_nr_running(rq);
> }
>
> @@ -5323,7 +5327,7 @@
> on_rq = p->se.on_rq;
> running = task_current(rq, p);
> if (on_rq)
> - dequeue_task(rq, p, 0);
> + dequeue_task(rq, p);
> if (running)
> p->sched_class->put_prev_task(rq, p);
>
> @@ -5372,7 +5376,7 @@
> }
> on_rq = p->se.on_rq;
> if (on_rq)
> - dequeue_task(rq, p, 0);
> + dequeue_task(rq, p);
>
> p->static_prio = NICE_TO_PRIO(nice);
> set_load_weight(p);
> @@ -9189,7 +9193,7 @@
> on_rq = tsk->se.on_rq;
>
> if (on_rq)
> - dequeue_task(rq, tsk, 0);
> + dequeue_task(rq, tsk);
> if (unlikely(running))
> tsk->sched_class->put_prev_task(rq, tsk);
It would be cleaner to achieve this by introducing a
__dequeue_task() inline function that does not have the sleep
parameter and is a thin wrapper over
p->sched_class->dequeue_task, and keep dequeue_task() with the
sleep parameter - but update it to use __dequeue_task() and mark
it an inline function.
[ Btw., could you please submit future patches in the regular
-p1 format? See Documentation/SubmittingPatches. And please
Cc: me to future scheduler patches. Thanks! ]
Ingo
On Thu, 2009-03-12 at 10:55 +0100, Ingo Molnar wrote:
> * David Newall <[email protected]> wrote:
>
> > The dequeue_patch function in kernel/sched.c is complicated by
^^^^^^^^^^^^^
>
> > including a sleep parameter. This parameter is always zero
> > except in one instance. This patch clarifies the task of
> > dequeue_patch by removing the sleep parameter and moving the
> > code that handles non-zero sleep to that one place where it is
> > needed.
> >
>
> > --- kernel/sched.c 2009-03-12 18:41:41.000000000 +1030
> > +++ kernel/sched.c.dn 2009-03-12 18:45:18.000000000 +1030
> > @@ -1791,21 +1791,10 @@
> > p->se.on_rq = 1;
> > }
> >
> > -static void dequeue_task(struct rq *rq, struct task_struct *p, int sleep)
> > +static void dequeue_task(struct rq *rq, struct task_struct *p)
> > {
> > - if (sleep) {
> > - if (p->se.last_wakeup) {
> > - update_avg(&p->se.avg_overlap,
> > - p->se.sum_exec_runtime - p->se.last_wakeup);
> > - p->se.last_wakeup = 0;
> > - } else {
> > - update_avg(&p->se.avg_wakeup,
> > - sysctl_sched_wakeup_granularity);
> > - }
> > - }
> > -
> > sched_info_dequeued(p);
> > - p->sched_class->dequeue_task(rq, p, sleep);
> > + p->sched_class->dequeue_task(rq, p, 0);
> > p->se.on_rq = 0;
> > }
> >
> > @@ -1875,7 +1864,22 @@
> > if (task_contributes_to_load(p))
> > rq->nr_uninterruptible++;
> >
> > - dequeue_task(rq, p, sleep);
> > + if (sleep) {
> > + if (p->se.last_wakeup) {
> > + update_avg(&p->se.avg_overlap,
> > + p->se.sum_exec_runtime - p->se.last_wakeup);
> > + p->se.last_wakeup = 0;
> > + } else {
> > + update_avg(&p->se.avg_wakeup,
> > + sysctl_sched_wakeup_granularity);
> > + }
> > + }
> > +
> > + /*dequeue_task(rq, p, sleep);*/
> > + sched_info_dequeued(p);
> > + p->sched_class->dequeue_task(rq, p, sleep);
> > + p->se.on_rq = 0;
> > +
> > dec_nr_running(rq);
> > }
> >
> > @@ -5323,7 +5327,7 @@
> > on_rq = p->se.on_rq;
> > running = task_current(rq, p);
> > if (on_rq)
> > - dequeue_task(rq, p, 0);
> > + dequeue_task(rq, p);
> > if (running)
> > p->sched_class->put_prev_task(rq, p);
> >
> > @@ -5372,7 +5376,7 @@
> > }
> > on_rq = p->se.on_rq;
> > if (on_rq)
> > - dequeue_task(rq, p, 0);
> > + dequeue_task(rq, p);
> >
> > p->static_prio = NICE_TO_PRIO(nice);
> > set_load_weight(p);
> > @@ -9189,7 +9193,7 @@
> > on_rq = tsk->se.on_rq;
> >
> > if (on_rq)
> > - dequeue_task(rq, tsk, 0);
> > + dequeue_task(rq, tsk);
> > if (unlikely(running))
> > tsk->sched_class->put_prev_task(rq, tsk);
>
> It would be cleaner to achieve this by introducing a
> __dequeue_task() inline function that does not have the sleep
> parameter and is a thin wrapper over
> p->sched_class->dequeue_task, and keep dequeue_task() with the
> sleep parameter - but update it to use __dequeue_task() and mark
> it an inline function.
Is there any real gain from doing this? It messes up the symmetry of
enqueue/dequeue.
-Mike
Mike Galbraith wrote:
> Is there any real gain from doing this? It messes up the symmetry of
> enqueue/dequeue.
No. On further consideration, I don't like my patch.
* David Newall <[email protected]> wrote:
> Mike Galbraith wrote:
> > Is there any real gain from doing this? It messes up the symmetry of
> > enqueue/dequeue.
>
>
> No. On further consideration, I don't like my patch.
With the __dequeue_task()+inline suggestion i made i think it
would be a micro-optimization and would not break symmetry in a
significant way.
Ingo
Ingo Molnar wrote:
> * David Newall <[email protected]> wrote:
>
>> No. On further consideration, I don't like my patch.
>>
>
> With the __dequeue_task()+inline suggestion i made i think it
> would be a micro-optimization and would not break symmetry in a
> significant way.
What I realised, upon closer examination, is that enqueue_task has the
same issue (with wakeup); and that the one places where sleep and wakeup
are set are in a single call for each, to (de|en)activate_task. If it
made sense to do what I suggested, then it should be done all the way,
that is, move the sleep and wakeup code to those two places, and
replicate the (admittedly smaller) *queue_task and *activate_task at
those points. This smacks of premature optimisation.