On Mon, Mar 27, 2017 at 04:56:51PM +0200, Luca Abeni wrote: > > > +u64 grub_reclaim(u64 delta, struct rq *rq, u64 u) > > > { > > > + u64 u_act; > > > + > > > + if (rq->dl.this_bw - rq->dl.running_bw > (1 << 20) - u) > > > + u_act = u; > > > + else > > > + u_act = (1 << 20) - rq->dl.this_bw + > > > rq->dl.running_bw; + > > > + return (delta * u_act) >> 20; > > > > But that's not what is done here I think, something like this instead: > > > > Uinact = Utot - Uact > > > > -t_u dt ; Uinact > (1 - t_u) > > dq = { > > -(1 - Uinact) dt > > > > > > And nowhere do we have an explanation for that. > > Sorry about this confusion... The accounting should be > dq = -(1 - Uinact)dt > but if (1 - Uinact) is too large (larger than the task's utilization) > then we use the task's utilization instead (otherwise, we end up > reclaiming other runqueues' time). I realized that this check was > needed after writing the comments, and I forgot to update the comments > when I fixed the code :( > > > Now, I suspect we can write that like: dq = -max{ t_u, (1 - Uinact) } > > dt, which would suggest this is a sanity check on Utot, which I > > suspect can be over 1. Is this what is happening? > > Right... I'll fix the code and comments according to your suggestion.
But doesn't that suggest there is now another corner case where we 'always' select t_u because of Utot overload? My intuition suggests we'd reclaim insufficient time in that case, but I've not thought much about it. I feel we want a few words explaining the trade-offs made here and the corner cases explored. Does that make sense?