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

quoted text
>
>
>> ---
>>  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
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/