Re: [patch] Re: scheduler oddity [bug?]

[David Newall]

Ingo Molnar wrote:
> * Mike Galbraith <efault@xxxxxx> wrote:
>   


...
>> OK, I've not seen any problem indications yet, so find patchlet below.
>>
>> However! Balazs has stated that this problem is _not_ present in .git,
>> and that..
>>
>> 	commit 38736f475071b80b66be28af7b44c854073699cc
>> 	Author: Gautham R Shenoy <ego@xxxxxxxxxx>
>> 	Date:   Sat Sep 6 14:50:23 2008 +0530
>>
>> ..is what fixed it.  Willy Tarreau verified this as being the case on
>> his HW as well.  It is present in .git with my HW.
>>
>> I see it as a problem, but it's your call.  Dunno if I'd apply it or
>> hold back, given these conflicting reports.
>>     
>
> I think we still want it - as the purpose of the overlap metric 
> is to measure reality. If preemption causes overlap in execution 
> we should not ignore that.
>   

I'm sure it's wrong. The only call to dequeue with a non-zero sleep
value is in deactivate_task. All the rest have zero sleep. The section
of code identified by Mike in his patchlet should be moved for purpose
of clarity. It also hilights the symmetry between queue_task and
dequeue_task:

--- sched.c     2009-02-21 09:09:34.000000000 +1030
+++ sched.c.dn  2009-03-09 20:13:51.000000000 +1030
@@ -1647,12 +1647,6 @@
 
 static void dequeue_task(struct rq *rq, struct task_struct *p, int sleep)
 {
-       if (sleep && p->se.last_wakeup) {
-               update_avg(&p->se.avg_overlap,
-                          p->se.sum_exec_runtime - p->se.last_wakeup);
-               p->se.last_wakeup = 0;
-       }
-
        sched_info_dequeued(p);
        p->sched_class->dequeue_task(rq, p, sleep);
        p->se.on_rq = 0;
@@ -1724,6 +1718,12 @@
        if (task_contributes_to_load(p))
                rq->nr_uninterruptible++;
 
+       if (sleep && p->se.last_wakeup) {
+               update_avg(&p->se.avg_overlap,
+                          p->se.sum_exec_runtime - p->se.last_wakeup);
+               p->se.last_wakeup = 0;
+       }
+
        dequeue_task(rq, p, sleep);
        dec_nr_running(rq);
 }



Having done that, it makes sense to entirely remove dequeue_task 's
sleep parameter, and replicate all three lines in deactivate_task:

--- sched.c.dn  2009-03-09 20:41:13.000000000 +1030
+++ sched.c.dn2 2009-03-09 20:41:30.000000000 +1030
@@ -1645,10 +1645,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)
 {
        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;
 }
 
@@ -1724,7 +1724,11 @@
                p->se.last_wakeup = 0;
        }
 
-       dequeue_task(rq, p, sleep);
+       /*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);
 }
 
@@ -4848,7 +4852,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);
 
@@ -4897,7 +4901,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);
@@ -8637,7 +8641,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);
 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/