Re: [PATCH v3]PM/Sleep: Timer quiesce in freeze state

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 26 Jan 2015, Li, Aubrey wrote:
> On 2015/1/22 18:15, Thomas Gleixner wrote:
> > Can we please stop adding more crap to that notifier thing? I rather
> > see that go away than being expanded.
> 
> Are you referring to FREEZE_PREPARE or remove all of FREEZE staff at all?
> 
> What's the disadvantage of adding more notifier?

clockevents_notify() is not a notifier. Its a multiplex call and I
want to get rid of it and replace it with explicit functions.

> >> +		/*
> >> +		 * cpuidle_enter will return with interrupt enabled
> >> +		 */
> >> +		cpuidle_enter(drv, dev, next_state);
> > 
> > How is that supposed to work?
> > 
> > If timekeeping is not yet unfrozen, then any interrupt handling code
> > which calls anything time related is going to hit lala land.
> > 
> > You must guarantee that timekeeping is unfrozen before any interrupt
> > is handled. If you cannot guarantee that, you cannot freeze
> > timekeeping ever.
> > 
> > The cpu local tick device is less critical, but it happens to work by
> > chance, not by design.
> 
> There are two way to guarantee this: the first way is, disable interrupt
> before timekeeping frozen and enable interrupt after timekeeping is
> unfrozen. However, we need to handle wakeup handler before unfreeze
> timekeeping to wake freeze task up from wait queue.
> 
> So we have to go the other way, the other way is, we ignore time related
> calls during freeze, like what I added in irq_enter below.

Groan. You just do not call in irq_enter/exit(), but what prevents any
interrupt handler or whatever to call into the time/timer code after
interrupts got reenabled?

Nothing. 

> Or, we need to re-implement freeze wait and wake up mechanism?

You need to make sure in the low level idle implementation that this
cannot happen.

tick_freeze()
{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()
{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()
{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be
a horrible mess of bandaids and duct tape.

So that does not need any of the irq_enter/exit conditionals, it does
not need the real_handler hack. It just works.

The only remaining issue might be a NMI calling into
ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a
non issue on x86/tsc, but it might be a problem on other platforms
which turn off devices, clocks, It's not rocket science to prevent
that.

> It looks like a notifier is unpopular, I was trying to avoid a global
> variable across different kernel modules. But yes, I can change. May I
> know the disadvantage of notifier callback?

You do not need a global variable at all. See above.

> The V2 patch was using stop machine mechanism according to Peter's
> suggestion. This V3 patch, IIRC, clockevents_lock was your suggestion.

A suggestion does not mean that you should follow it blindly. If you
see that the result is horrible and not feasible then you should
notice yourself, think about different approaches and discuss that.

I'm about to post a series which gets rid of clockevents_notify() and
distangles the stuff so the above becomes possible.

Thanks,

	tglx