* [PATCH] (latest tip) make dequeue_task less confusing
@ 2009-03-12 8:56 David Newall
2009-03-12 9:55 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: David Newall @ 2009-03-12 8:56 UTC (permalink / raw)
To: Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 319 bytes --]
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.
[-- Attachment #2: sched.patch --]
[-- Type: text/x-patch, Size: 1782 bytes --]
--- 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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] (latest tip) make dequeue_task less confusing
2009-03-12 8:56 [PATCH] (latest tip) make dequeue_task less confusing David Newall
@ 2009-03-12 9:55 ` Ingo Molnar
2009-03-12 13:36 ` Mike Galbraith
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2009-03-12 9:55 UTC (permalink / raw)
To: David Newall; +Cc: Linux Kernel Mailing List, Peter Zijlstra
* David Newall <davidn@davidnewall.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] (latest tip) make dequeue_task less confusing
2009-03-12 9:55 ` Ingo Molnar
@ 2009-03-12 13:36 ` Mike Galbraith
2009-03-12 18:02 ` David Newall
0 siblings, 1 reply; 6+ messages in thread
From: Mike Galbraith @ 2009-03-12 13:36 UTC (permalink / raw)
To: Ingo Molnar; +Cc: David Newall, Linux Kernel Mailing List, Peter Zijlstra
On Thu, 2009-03-12 at 10:55 +0100, Ingo Molnar wrote:
> * David Newall <davidn@davidnewall.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] (latest tip) make dequeue_task less confusing
2009-03-12 13:36 ` Mike Galbraith
@ 2009-03-12 18:02 ` David Newall
2009-03-13 0:45 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: David Newall @ 2009-03-12 18:02 UTC (permalink / raw)
To: Mike Galbraith; +Cc: Ingo Molnar, Linux Kernel Mailing List, Peter Zijlstra
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] (latest tip) make dequeue_task less confusing
2009-03-12 18:02 ` David Newall
@ 2009-03-13 0:45 ` Ingo Molnar
2009-03-13 3:58 ` David Newall
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2009-03-13 0:45 UTC (permalink / raw)
To: David Newall; +Cc: Mike Galbraith, Linux Kernel Mailing List, Peter Zijlstra
* David Newall <davidn@davidnewall.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] (latest tip) make dequeue_task less confusing
2009-03-13 0:45 ` Ingo Molnar
@ 2009-03-13 3:58 ` David Newall
0 siblings, 0 replies; 6+ messages in thread
From: David Newall @ 2009-03-13 3:58 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Mike Galbraith, Linux Kernel Mailing List, Peter Zijlstra
Ingo Molnar wrote:
> * David Newall <davidn@davidnewall.com> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-13 3:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-12 8:56 [PATCH] (latest tip) make dequeue_task less confusing David Newall
2009-03-12 9:55 ` Ingo Molnar
2009-03-12 13:36 ` Mike Galbraith
2009-03-12 18:02 ` David Newall
2009-03-13 0:45 ` Ingo Molnar
2009-03-13 3:58 ` David Newall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.