Re: [v4 RFC PATCH 4/4] timers: logic to move non pinned timers

[Thomas Gleixner <tglx@linutronix.de>]

Arun,

On Mon, 6 Apr 2009, Arun R Bharadwaj wrote:
> > > -	new_cpu_base = &__get_cpu_var(hrtimer_bases);
> > > +again:
> > > +	new_cpu_base = &per_cpu(hrtimer_bases, cpu);
> > >  	new_base = &new_cpu_base->clock_base[base->index];
> > >  
> > >  	if (base != new_base) {
> > > @@ -220,6 +242,32 @@ switch_hrtimer_base(struct hrtimer *time
> > >  		spin_unlock(&base->cpu_base->lock);
> > >  		spin_lock(&new_base->cpu_base->lock);
> > >  		timer->base = new_base;
> > > +
> > > +		if (cpu == preferred_cpu) {
> > > +			/* Calculate clock monotonic expiry time */
> > > +			ktime_t expires = ktime_sub(hrtimer_get_expires(timer),
> > > +							new_base->offset);
> > > +
> > > +			/*
> > > +			 * Get the next event on target cpu from the
> > > +			 * clock events layer.
> > > +			 * This covers the highres=off nohz=on case as well.
> > > +			 */
> > > +			ktime_t next = clockevents_get_next_event(cpu);
> > > +
> > > +			ktime_t delta = ktime_sub(expires, next);
> > > +
> > > +			/*
> > > +			 * We do not migrate the timer when it is expiring
> > > +			 * before the next event on the target cpu because
> > > +			 * we cannot reprogram the target cpu hardware and
> > > +			 * we would cause it to fire late.
> > > +			 */
> > > +			if (delta.tv64 < 0) {
> > > +				cpu = smp_processor_id();
> > 
> >   You are missing a small but fatal detail here: You hold
> >   new_base->cpu_base->lock. So you need to do:
> >
> 
> I just moved the if block.. if (cpu==preferred_cpu) above the base
> locking part to avoid the extra unlocking.

That's not a good idea. You want to look at the next event with the
base lock of the other cpu held. That prevents that expires the first
pending timer right after you checked next_event and before you queue
your timer, which then might become the first timer to expire but you
can't reprogram the clock event device on the other cpu.

Thanks,

	tglx