Peter Zijlstra peterz@infradead.org writes:
On Fri, Sep 16, 2016 at 04:23:16PM +0200, Vincent Guittot wrote:
On 16 September 2016 at 14:16, Peter Zijlstra peterz@infradead.org 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.