Re: [PATCH] clocksource: Allow toggling between runtime and persistent clocksource for idle

[Tony Lindgren <tony@atomide.com>]

* Thomas Gleixner <tglx@linutronix.de> [150706 08:48]:
> On Mon, 6 Jul 2015, Tony Lindgren wrote:
> > * Thomas Gleixner <tglx@linutronix.de> [150706 07:20]:
> > > On Mon, 6 Jul 2015, Tony Lindgren wrote:
> > The timekeeping accuracy issue certainly needs some thinking, and
> > also the resolution between the clocksources can be different.. In the
> > test case I have the slow timer is always on and of a lower resolution
> > than the ARM global timer being used during runtime.
> > 
> > Got some handy timer test in mind you want me to run to provide data
> > on the accuracy?
> 
> John Stultz might have something.
>   
> > > > +/**
> > > > + * clocksource_pm_enter - change to a persistent clocksource before idle
> > > > + *
> > > > + * Changes system to use a persistent clocksource for idle. Intended to
> > > > + * be called from CPUidle from the last active CPU.
> > > > + */
> > > > +int clocksource_pm_enter(void)
> > > > +{
> > > > +	bool oneshot = tick_oneshot_mode_active();
> > > > +	struct clocksource *best;
> > > > +
> > > > +	if (WARN_ONCE(!mutex_trylock(&clocksource_mutex),
> > > > +		      "Unable to get clocksource_mutex"))
> > > > +		return -EINTR;
> > > 
> > > This trylock serves which purpose?
> > 
> > Well we don't want to start changing clocksource if something is
> > running like you pointed out.
> 
> Well yes, but ....
>  
> > > I really cannot see how this is proper serialized.
> > 
> > We need to allow this only from the last cpu before hitting idle.
> 
> And I cannot see anything which does so.
> 
> cpu0  	     	 	  	cpu1
> 				is_idle	
> go_idle()
>   clocksource_pm_enter()
>     lock(cs_mutex);
> 				wakeup()
> 				clocksource_pm_exit()
> 				  trylock fails ....
> 
>     ...  
>     unlock(cs_mutex);
>   
> --> Crap!

OK you're right, this only works with cpuidle and using
drivers/cpuidle/coupled.c.
 
> > > > @@ -1086,7 +1086,18 @@ int timekeeping_notify(struct clocksource *clock)
> > > >  
> > > >  	if (tk->tkr_mono.clock == clock)
> > > >  		return 0;
> > > > -	stop_machine(change_clocksource, clock, NULL);
> > > > +
> > > > +	/*
> > > > +	 * We may want to toggle between a fast and a persistent
> > > > +	 * clocksource from CPUidle on the last active CPU and can't
> > > > +	 * use stop_machine at that point.
> > > > +	 */
> > > > +	if (cpumask_test_cpu(smp_processor_id(), cpu_online_mask) &&
> > > 
> > > Can you please explain how this code gets called from an offline cpu?
> > 
> > Last cpu getting idled..
> 
> That does not make any sense at all. How is idle related to the online
> mask? An idle cpu is still online.

Oops yeah that's a bogus test, cpu off != offlined.
 
> > > > +	    !rcu_is_watching())
> > > 
> > > So pick some random combination of conditions and define that it is
> > > correct, right? How on earth does !rcu_watching() tell that this is
> > > the last running cpu.
> > 
> > We have called rcu_idle_enter() from cpuidle_idle_call(). Do you have
> > some better test in mind when the last cpu is about hit idle?
> 
> The cpuidle code should know that. And if it does not know, it better
> should keep track of that information and based on it provide the
> proper serialization, so the call into the timekeeping code is not a
> subject to guess work and race conditions.

OK I agree. Based on your comments this clearly needs to be limited to
cpuidle. And thanks for your comments.

Regards,

Tony