Re: [PATCH] sched: Folding nohz load accounting more accurate

[Peter Zijlstra <peterz@infradead.org>]

On Tue, 2012-06-19 at 14:08 +0800, Yong Zhang wrote:

> > ---
> >  kernel/sched/core.c      |  290 ++++++++++++++++++++++++++++++++++------------
> >  kernel/sched/idle_task.c |    1 -
> >  kernel/sched/sched.h     |    2 -
> >  kernel/time/tick-sched.c |    2 +
> >  4 files changed, 220 insertions(+), 75 deletions(-)
> > 
> > + *  - When we go NO_HZ idle during the window, we can negate our sample
> > + *    contribution, causing under-accounting.
> > + *
> > + *    We avoid this by keeping two idle-delta counters and flipping them
> > + *    when the window starts, thus separating old and new NO_HZ load.
> > + *
> > + *    The only trick is the slight shift in index flip for read vs write.
> > + *
> > + *       0             5             10            15
> > + *         +10           +10           +10           +10
> > + *       |-|-----------|-|-----------|-|-----------|-|
> > + *    r:001           110           001           110
> > + *    w:011           100           011           100
> 
> I'm confused by this comments, looking at your code, index is increased by
> 1 for each samaple window.

Also looking at the code you'll find we only ever use idx & 1.

> > + *
> > + *    This ensures we'll fold the old idle contribution in this window while
> > + *    accumlating the new one.
> > + *
> > + *  - When we wake up from NO_HZ idle during the window, we push up our
> > + *    contribution, since we effectively move our sample point to a known
> > + *    busy state.
> > + *
> > + *    This is solved by pushing the window forward, and thus skipping the
> > + *    sample, for this cpu (effectively using the idle-delta for this cpu which
> > + *    was in effect at the time the window opened). This also solves the issue
> > + *    of having to deal with a cpu having been in NOHZ idle for multiple
> > + *    LOAD_FREQ intervals.
> >   *
> >   * When making the ILB scale, we should try to pull this in as well.
> >   */
> > +void calc_load_exit_idle(void)
> >  {
> > +	struct rq *this_rq = this_rq();
> >  
> >  	/*
> > +	 * If we're still outside the sample window, we're done.
> >  	 */
> > +	if (time_before(jiffies, this_rq->calc_load_update))
> > +		return;

> 	else if (time_before(jiffies, calc_load_update + 10)
> 		this_rq->calc_load_update = calc_load_update + LOAD_FREQ;
> 	else
> 		this_rq->calc_load_update = calc_load_update;
> 
> Otherwise if you woke after the sample window, we loose on sample?
> And maybe we need local variable to cache calc_load_update.

Ah indeed, although I'd write it like:

	this_rq->calc_load_update = calc_load_update;
	if (time_before(jiffies, this_rq->calc_load_update + 10)
		this_rq->calc_load_update += LOAD_FREQ;

Thanks!