linux-kernel - Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class

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: <20160916121626.GN5012@twins.programming.kicks-ass.net>
Date:   Fri, 16 Sep 2016 14:16:26 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Yuyang Du <yuyang.du@...el.com>,
        Morten Rasmussen <Morten.Rasmussen@....com>,
        Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Paul Turner <pjt@...gle.com>,
        Benjamin Segall <bsegall@...gle.com>
Subject: Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when
 switching to fair class

On Thu, Sep 15, 2016 at 05:36:58PM +0200, Vincent Guittot wrote:
> On 15 September 2016 at 15:18, Peter Zijlstra <peterz@...radead.org> wrote:
> > On Mon, Sep 12, 2016 at 09:47:52AM +0200, Vincent Guittot wrote:

> >> Update the sequence to follow the right one:
> >> -dequeue task
> >> -put task
> >> -change the property
> >> -enqueue task
> >> -set task as current task
> >
> > But enqueue_entity depends on cfs_rq->curr, which is set by
> > set_curr_task_fair().
> 
> With this sequence, cfs_rq->curr is null and the cfs_rq is "idle" as
> the entity has been dequeued and put back in the rb tree the time to
> change the properties.
> 
> enqueue_entity use cfs_rq->cur == se for:
> - updating current. With this sequence, current is now null so nothing to do
> - to skip the enqueue of the se in rb tree. With this sequence, se is
> put in the rb tree during the enqueue and take back during the set
> task as current task
> 
> I don't see any functional issue but we are not doing the same step
> with the new sequence

So I think you're right in that it should work.

I also think we can then simplify enqueue_entity() in that it will never
be possible to enqueue current with your change.

But my brain just isn't working today, so who knows.

> > Also, the normalize comment in dequeue_entity() worries me, 'someone'
> > didn't update that when he moved update_min_vruntime() around.

I now worry more, so we do:

	dequeue_task := dequeue_task_fair (p == current)
	  dequeue_entity
	    update_curr()
	      update_min_vruntime()
	    vruntime -= min_vruntime
	    update_min_vruntime()
	      // use cfs_rq->curr, which we just normalized !

	put_prev_task := put_prev_task_fair
	  put_prev_entity
	    cfs_rq->curr = NULL;


Now the point of the latter update_min_vruntime() is to advance
min_vruntime when the task we removed was the one holding it back.

However, it means that if we do dequeue+enqueue, we're further in the
future (ie. we get penalized).

So I'm inclined to simply remove the (2nd) update_min_vruntime() call.
But as said above, my brain isn't co-operating much today.