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 <[email protected]>
Signed-off-by: Kyungmin Park <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Kyungmin Park <[email protected]>
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);
On Fri, Nov 12, 2010 at 5:02 AM, john stultz <[email protected]> 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 <[email protected]>
>> Signed-off-by: Kyungmin Park <[email protected]>
>
> 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
On Fri, 2010-11-12 at 08:58 +0900, MyungJoo Ham wrote:
> On Fri, Nov 12, 2010 at 5:02 AM, john stultz <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Kyungmin Park <[email protected]>
> >
> > 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
On Fri, 2010-11-12 at 09:23 AM, john stultz wrote:
> On Fri, 2010-11-12 at 08:58 +0900, MyungJoo Ham wrote:
> > On Fri, Nov 12, 2010 at 5:02 AM, john stultz 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
> > >> Signed-off-by: Kyungmin Park
> > >
> > > 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?
>
Ah.. I'm not concerned with delay functions in this clocksource recalibration issue. The delay function has been working just fine. The issue affects the consistency of "BogoMIPS" values when we try "# cat /proc/cpuinfo", which is based on loops_per_jiffy stroed at cpu_data of each core. The cpu-on/off function recalibrates loops_per_jiffy for the cpu that was turned off and on. In a situation where part of cpus were turned off and on after the clocksource has been changed, the bogomips values have inconsistency between cpus. So, I'm not concerned with the delay function, which has been working fine before and after the patch, but with the consistency of loops_per_jiffy values in different cpus/cores.
>
> > 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
>
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Fri, 2010-11-12 at 01:31 +0000, MyungJoo Ham wrote:
> On Fri, 2010-11-12 at 09:23 AM, john stultz wrote:
> > 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?
> >
>
> Ah.. I'm not concerned with delay functions in this clocksource
> recalibration issue. The delay function has been working just fine.
> The issue affects the consistency of "BogoMIPS" values when we try "#
> cat /proc/cpuinfo", which is based on loops_per_jiffy stroed at
> cpu_data of each core. The cpu-on/off function recalibrates
> loops_per_jiffy for the cpu that was turned off and on.
So I'm focused on __delay(), because it is used to generate the
loops_per_jiffy value in calibrate_delay(). Or is loops_per_jiffy
calculated by some other method on your board?
And if the issue is between the two cpus, it could be something
interfering with calibrate_delay when startup up the second cpu.
Is it that the first one is faster and the second one slower? Or the
other way around? Or is one right and the other just wrong?
> In a situation where part of cpus were turned off and on after the
> clocksource has been changed, the bogomips values have inconsistency
> between cpus. So, I'm not concerned with the delay function, which has
> been working fine before and after the patch, but with the consistency
> of loops_per_jiffy values in different cpus/cores.
So have you isolated as to why the bogomips value changes when the
clocksource changes? They *should* be independent.
I suspect your patch works, because its causing the calibration to
happen again at a later time to correct for whatever interruption
occurred during the initial calibration. So if the calibration were run
again from an device_initcall() hook instead of the clocksource_select
point it would do the exact same thing.
I'd suggest focusing on why the calibration was incorrect the first
time, rather then just running it again to fix it.
thanks
-john