Re: [RFC-PATCH] clocksource: update lpj if clocksource has been changed.

[john stultz <johnstul@us.ibm.com>]

On Thu, 2010-11-11 at 17:36 +0900, MyungJoo Ham wrote:
> With a clocksource change, loops_per_jiffy may have been changed; thus,
> the loops_per_jiffy in each cpu should be updated. Especially after some
> of the cpus were turned off and on, their loops_per_jiffy values are
> updated while the cpus kept on are not. Therefore, in order to make them
> "normalized equally", we need to let the loops_per_jiffy values of
> different cpus be based on the same clocksource.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

First, Thanks for reporting the issue and submitting the patch!

So the premise is that read_current_timer -> get_cycles ->
clocksource_read on some arches. And then when we select a different
clocksource for timekeeping, this also changes the get_cycles source
breaking delay loops.

The clocksource selected for timekeeping and the counter being used for
get_cycles really shouldn't be explicitly bound. On most systems I don't
think that is the case, so this patch would force needless recalibration
calls on clocksource changes.

Which arch specifically are you seeing the issue on? I suspect there is
be a better way to fix this.

thanks
-john


> ---
>  kernel/time/clocksource.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index c18d7ef..a9d7935 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -30,6 +30,9 @@
>  #include <linux/sched.h> /* for spin_unlock_irq() using preempt_count() m68k */
>  #include <linux/tick.h>
>  #include <linux/kthread.h>
> +#include <linux/delay.h>
> 
>  void timecounter_init(struct timecounter *tc,
>  		      const struct cyclecounter *cc,
> @@ -592,6 +593,9 @@ static inline void clocksource_select(void) { }
>   */
>  static int __init clocksource_done_booting(void)
>  {
> +#ifdef CONFIG_SMP
> +	int cpu;
> +#endif
>  	mutex_lock(&clocksource_mutex);
>  	curr_clocksource = clocksource_default_clock();
>  	mutex_unlock(&clocksource_mutex);
> @@ -606,6 +610,13 @@ static int __init clocksource_done_booting(void)
>  	mutex_lock(&clocksource_mutex);
>  	clocksource_select();
>  	mutex_unlock(&clocksource_mutex);
> +
> +	calibrate_delay();
> +#ifdef CONFIG_SMP
> +	/* loops_per_jiffy may have been changed. */
> +	for_each_online_cpu(cpu)
> +		smp_store_cpu_info(cpu);
> +#endif
>  	return 0;
>  }
>  fs_initcall(clocksource_done_booting);