From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933284AbcITQ7O (ORCPT ); Tue, 20 Sep 2016 12:59:14 -0400 Received: from mail-pf0-f182.google.com ([209.85.192.182]:36287 "EHLO mail-pf0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932692AbcITQ7L (ORCPT ); Tue, 20 Sep 2016 12:59:11 -0400 From: bsegall@google.com To: Peter Zijlstra Cc: Vincent Guittot , Ingo Molnar , linux-kernel , Yuyang Du , Morten Rasmussen , Linaro Kernel Mailman List , Dietmar Eggemann , Paul Turner Subject: Re: [PATCH 7/7 v3] sched: fix wrong utilization accounting when switching to fair class References: <1473666472-13749-1-git-send-email-vincent.guittot@linaro.org> <1473666472-13749-8-git-send-email-vincent.guittot@linaro.org> <20160915131807.GS5008@twins.programming.kicks-ass.net> <20160916121626.GN5012@twins.programming.kicks-ass.net> <20160920115458.GX5016@twins.programming.kicks-ass.net> Date: Tue, 20 Sep 2016 09:59:08 -0700 In-Reply-To: <20160920115458.GX5016@twins.programming.kicks-ass.net> (Peter Zijlstra's message of "Tue, 20 Sep 2016 13:54:58 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Peter Zijlstra writes: > On Fri, Sep 16, 2016 at 04:23:16PM +0200, Vincent Guittot wrote: >> On 16 September 2016 at 14:16, Peter Zijlstra wrote: > >> >> > 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 ! >> >> yes but does it really change the cfs_rq->min_vruntime in this case ? > > So let me see; it does: > > vruntime = cfs_rq->min_vruntime; > > if (curr) // true > vruntime = curr->vruntime; // == vruntime - min_vruntime > > if (leftmost) // possible > if (curr) // true > vruntime = min_vruntime(vruntime, se->vruntime); > if (se->vruntime - (curr->vruntime - min_vruntime)) < 0 // false > > min_vruntime = max_vruntime(min_vruntime, vruntime); > if ((curr->vruntime - min_vruntime) - min_vruntime) > 0) > > > The problem is that double subtraction of min_vruntime can wrap. > The thing is, min_vruntime is the 0-point in our modular space, it > normalizes vruntime (ideally min_vruntime would be our 0-lag point, > resulting in vruntime - min_vruntime being the lag). > > The moment min_vruntime grows past S64_MAX/2 -2*min_vruntime wraps into > positive space again and the test above becomes true and we'll select > the normalized @curr vruntime as new min_vruntime and weird stuff will > happen. > > > Also, even it things magically worked out, its still very icky to mix > the normalized vruntime into things. > >> > 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. > > OK, so not sure we can actually remove it, we do want it to move > min_vruntime forward (sometimes). We just don't want it to do so when > DEQUEUE_SAVE -- we want to get back where we left off, nor do we want to > muck about with touching normalized values. > > Another fun corner case is DEQUEUE_SLEEP; in that case we do not > normalize, but we still want advance min_vruntime if this was the one > holding it back. > > I ended up with the below, but I'm not sure I like it much. Let me prod > a wee bit more to see if there's not something else we can do. > > Google has this patch-set replacing min_vruntime with an actual global > 0-lag, which greatly simplifies things. If only they'd post it sometime > :/ /me prods pjt and ben with a sharp stick :-) > No, we don't have any patches like that. I wish, we've screwed up vruntime a couple of times too.