Message ID | 20150706152201.GJ10705@atomide.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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! > > > @@ -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. > > > + !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. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 6, 2015 at 8:46 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > 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. You can turn on ntp stats in your ntp.conf and chart the loopstats data w/ gnuplot: set terminal png set output "loopstat.png" plot "/var/log/ntpstats/loopstats.<dateoftest>" using 2:3 with linespoints I also have the drift-log.py and graph-log.py scripts I use here: https://github.com/johnstultz-work/timetests But those aren't really ready for mass consumption, as they're probably not very cross-distro compatible. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* John Stultz <john.stultz@linaro.org> [150706 10:53]: > On Mon, Jul 6, 2015 at 8:46 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > 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. > > You can turn on ntp stats in your ntp.conf and chart the loopstats > data w/ gnuplot: > > set terminal png > set output "loopstat.png" > plot "/var/log/ntpstats/loopstats.<dateoftest>" using 2:3 with linespoints > > > I also have the drift-log.py and graph-log.py scripts I use here: > https://github.com/johnstultz-work/timetests > > But those aren't really ready for mass consumption, as they're > probably not very cross-distro compatible. Great thanks, I'll play a bit with those to see how bad the jitter gets. Regrds, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -111,6 +111,7 @@ static int omap_enter_idle_coupled(struct cpuidle_device *dev, (cx->mpu_logic_state == PWRDM_POWER_OFF); tick_broadcast_enter(); + clocksource_pm_enter(); /* * Call idle CPU PM enter notifier chain so that @@ -167,6 +168,7 @@ static int omap_enter_idle_coupled(struct cpuidle_device *dev, if (dev->cpu == 0 && mpuss_can_lose_context) cpu_cluster_pm_exit(); + clocksource_pm_exit(); tick_broadcast_exit(); fail: