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@xxxxxxxxxx> 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@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

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
--
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/