Re: [RFC PATCH v2 3/3] sched: introduce synchronized idle injection

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

On Tue, 10 Nov 2015 14:23:24 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> > +static enum hrtimer_restart idle_inject_timer_fn(struct hrtimer
> > *hrtimer) +{
> > +	struct hrtimer *hrt = this_cpu_ptr(&idle_inject_timer);
> > +	int cpu = smp_processor_id();
> > +	ktime_t now, delta, period;
> > +	bool status;
> > +
> > +	now = hrtimer_cb_get_time(hrt);  
> 
> You're not interested in the current time.
> 
> > +
> > +	status = raw_cpu_read(idle_injected);
> > +	if (status) {
> > +		/*
> > +		 * We were injecting idle in the last phase, let's
> > forward the
> > +		 * timer to the next period
> > +		 *
> > +		 * status: 1             0                1
> > 0
> > +		 * ____          ____________________
> > _______
> > +		 *     |________|                    |_________|
> > +		 *
> > +		 *     |duration|      interval      |
> > +		 *
> > +		 *              ^ we are here
> > +		 *                  forward to here: ^
> > +		 */
> > +		delta = ktime_sub(now, inject_start_time);
> > +		period = ktime_add(ms_to_ktime(duration),
> > +				ms_to_ktime(inject_interval));
> > +		delta = ktime_roundup(delta, period);
> > +		hrtimer_set_expires(hrt, ktime_add(delta,
> > inject_start_time));  
> 
> This doesn't make any sense. Who cares what the current time is.
> 
> > +	} else {
> > +		/*
> > +		 * We were not injecting idle in the last phase,
> > let's forward
> > +		 * timer after forced idle duration
> > +		 * ____          ____________________
> > _______
> > +		 *     |________|                    |_________|
> > +		 *
> > +		 *     |duration|      interval      |
> > +		 *
> > +		 *     ^ we are here
> > +		 *              ^ forward timer to here
> > +		 */
> > +		hrtimer_set_expires(hrt,
> > ktime_add(ms_to_ktime(duration), now));  
> 
> Same here, we don't care about the current time. The timer was at the
> previous start of injection, just forward it a whole period to find
> the next injection slot.
> 
> > +	}  
> 
> It looks like what you want is:
> 
> 	hrtimer_forward(hrt, period);
> 
> unconditionally.
In the ideal world yes. But my thinking was that timers may not be so
accurate to deliver interrupts, over the time the timeout error may
accumulate so that eventually timers will be out of sync. That is why
at the beginning of the period I realign the timer based on a common
start time so that timers will never drift off. I use current time and
the common start time to locate the next absolute timeout not the
relative timeout.
Overall, my idea was to make it more robust and handles runtime
parameters (percentage, duration) smoothly.

I will fix the rest comments in the patch.

Thank you,

Jacob