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

From: john stultz
Date: Thu Nov 11 2010 - 15:03:00 EST


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@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>

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);


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