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

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

[MyungJoo Ham]

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

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

[john stultz]

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

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

[MyungJoo Ham]

On Fri, Nov 12, 2010 at 5:02 AM, john stultz <johnstul@us.ibm.com> wrote:
> 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

We are working on ARM/S5PC210 with two cores. Actually, in single core
systems, clocksource changes that affect loops-per-jiffy do not matter
much as in multi-core systems because we do not have something to
compare with in such a system. This patch adds some overheads on
changing system clocksources; however, is happens only once at boot.

Or, would it be better if we add another entry to struct clocksource;
i.e., "bool recalibrate" in struct clocksource? Then, we can put
recalibration routine in clocksource_select() at the end of the
function deciding whether to recalibrate based on the
"base->recalibrate" value. How about this?



Cheers!
- MyungJoo

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



-- 
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

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

[john stultz]

On Fri, 2010-11-12 at 08:58 +0900, MyungJoo Ham wrote:
> On Fri, Nov 12, 2010 at 5:02 AM, john stultz <johnstul@us.ibm.com> wrote:
> > 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
> 
> We are working on ARM/S5PC210 with two cores. Actually, in single core
> systems, clocksource changes that affect loops-per-jiffy do not matter
> much as in multi-core systems because we do not have something to
> compare with in such a system. This patch adds some overheads on
> changing system clocksources; however, is happens only once at boot.

Well, clocksource changes can happen any time a system is running.

Looking through the 2.6.37-rc arm code, I'm not seeing any counter based
delay implementation. I only see the loop based implementation in
arm/lib/delay.S. Additionally, I don't see ARCH_HAS_READ_CURRENT_TIMER
or a get_cycles implementation that uses the clocksource.

Have implemented a non-loop based delay for your platform? Or could you
more clearly explain how the clocksource being used for timekeeping
effects the delay function on your hardware?


> Or, would it be better if we add another entry to struct clocksource;
> i.e., "bool recalibrate" in struct clocksource? Then, we can put
> recalibration routine in clocksource_select() at the end of the
> function deciding whether to recalibrate based on the
> "base->recalibrate" value. How about this?

No, I don't think adding more to the clocksource is the right fix here. 

In my view, the correct solution should be to separate the get_cycles or
read_current_timer implementation (if that is the culprit) so it is not
dependent on the clocksource that the timekeeping code is currently
using.

That way, changes to the time keeping clocksource won't affect the delay
function, and the re-calibration will be unnecessary.

thanks
-john