linux-kernel - Re: [patch] Re: scheduler oddity [bug?]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <49B4EC7B.4080504@davidnewall.com>
Date:	Mon, 09 Mar 2009 20:46:27 +1030
From:	David Newall <davidn@...idnewall.com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Mike Galbraith <efault@....de>,
	Balazs Scheidler <bazsi@...abit.hu>,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Willy Tarreau <w@....eu>
Subject: Re: [patch] Re: scheduler oddity [bug?]

Ingo Molnar wrote:
> * Mike Galbraith <efault@....de> 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@...ibm.com>
>> 	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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/