Message-ID: <alpine.LFD.2.02.1207111539060.32033@ionos>
Date:	Wed, 11 Jul 2012 17:18:15 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
cc:	Prarit Bhargava <prarit@...hat.com>,
	John Stultz <johnstul@...ibm.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>, stable@...r.kernel.org
Subject: Re: [PATCH 1/6] hrtimer: Provide clock_was_set_delayed()

On Wed, 11 Jul 2012, Peter Zijlstra wrote:
> On Wed, 2012-07-11 at 14:45 +0200, Thomas Gleixner wrote:
> > On Wed, 11 Jul 2012, Prarit Bhargava wrote:
> > > On 07/10/2012 06:43 PM, John Stultz wrote:
> > > > clock_was_set() cannot be called from hard interrupt context because
> > > > it calls on_each_cpu(). For fixing the widely reported leap seconds
> > > > issue it's necessary to call it from the timer interrupt context.
> > > > 
> > > > Provide a new function which denotes it in the hrtimer cpu base
> > > > structure of the cpu on which it is called and raising the timer
> > > > softirq.
> > > > 
> > > > We then execute the clock_was_set() notificiation in the timer softirq
> > > > context in hrtimer_run_pending().
> > > 
> > > I wish there was a nicer way to do this ... but looking at the code I can't
> > > figure out a better way.  (no offense John, it's just the way the code is ;) )
> > 
> > Yeah, I had the same discussion with Peter earlier today. There is
> > only a rather limited set of options.
> > 
> > 1) Retrigger the timer interrupt vectors on all CPUs - except the one
> >    we are running on, but we have no interface for that at the moment
> > 
> > 2) Do the nasty __smp_call_function_single() hack
> > 
> >    Preallocate call_single_data for all cpus and do a
> >    __smp_call_function_single() on all online cpus.
> > 
> >    This can be called from hard interrupt context or irq disabled
> >    regions.
> > 
> >    That would allow to get rid of the whole delay magic all
> >    together.
> > 
> > Thoughts?
> 
> The __smp_call_function_single() thing isn't particularly pretty either
> and a lot more code to boot.. 
> 
> static DEFINE_PER_CPU(struct call_single_data, cws_csd);
> 
> void clock_was_set(void)
> {
> 	int cpu;
> 
> 	for_each_online_cpu(cpu) {
> 		struct call_single_data *csd = &per_cpu(cws_csd, cpu);
> 
> 		if (csd->flags & CSD_FLAG_LOCK)
> 			continue; /* a pending request is good enough */
> 
> 		csd->func = retrigger_next_event;
> 
> 		__smp_call_function_single(cpu, csd, 0);
> 	}
> 
> 	timerfd_clock_was_set();
> }
> 
> It also is a for_each_cpu loop with preemption disabled, not pretty :/

Right. I think with the atomic update of the offset in the timer
interrupt we are on the safe side. The main problem of timers expiring
early forever is covered by this.

Thinking more about it.

If time goes backwards, then the IPI is pointless. The already armed
clockevent device will fire too early, hrtimer_interrupt will update
and just rearm it. That's one "spurious" event.

So we only need it in the case of time going forward. 

Though with the leap second the maximum observable delay is 1 second
on a completely idle core. Surely nothing to worry about for an event
which happens rarely. So we could safely avoid the whole delayed
business and just do the timerfd notification, though I wonder if even
that is necessary in the leap second case.

On NOHZ=n systems the IPI is pointless as well. The maximum lateness
will be 10ms for HZ=100. Nothing we should worry about.

That leaves NOHZ enabled systems and there we might be clever and
avoid the IPIs to those cores which are not idle and let the tick
interrupt deal with it. And we can make the calls async and just let
them raise the hrtimer softirq on those cores, which will run the
hrtimer interrupt code and take care of everything.

Thoughts?

	tglx




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists