linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fix and optimize clock source update
@ 2006-06-21 12:38 Roman Zippel
  2006-06-21 20:58 ` john stultz
  0 siblings, 1 reply; 6+ messages in thread
From: Roman Zippel @ 2006-06-21 12:38 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, john stultz


This fixes the clock source updates in update_wall_time() to correctly
track the time coming in via current_tick_length(). Optimize the fast
paths to be as short as possible to keep the overhead low.

Signed-off-by: Roman Zippel <zippel@linux-m68k.org>

---

Andrew, please apply this patch, it's essential so that we can finally
get the clock source stuff merged. It keeps the overhead added by all
the patches in a not really great but acceptable range.
Tested with 2.6.17-rc6-mm2, applies and compiles with 2.6.17-mm1



 arch/powerpc/kernel/time.c  |    4 -
 include/linux/clocksource.h |  113 ++---------------------------------
 include/linux/timex.h       |    4 -
 kernel/timer.c              |  142 ++++++++++++++++++++++++++++++--------------
 4 files changed, 113 insertions(+), 150 deletions(-)

Index: linux-2.6-mm/arch/powerpc/kernel/time.c
===================================================================
--- linux-2.6-mm.orig/arch/powerpc/kernel/time.c	2006-06-21 13:59:35.000000000 +0200
+++ linux-2.6-mm/arch/powerpc/kernel/time.c	2006-06-21 14:01:22.000000000 +0200
@@ -102,7 +102,7 @@ EXPORT_SYMBOL(tb_ticks_per_sec);	/* for 
 u64 tb_to_xs;
 unsigned tb_to_us;
 
-#define TICKLEN_SCALE	(SHIFT_SCALE - 10)
+#define TICKLEN_SCALE	TICK_LENGTH_SHIFT
 u64 last_tick_len;	/* units are ns / 2^TICKLEN_SCALE */
 u64 ticklen_to_xs;	/* 0.64 fraction */
 
@@ -534,7 +534,7 @@ static __inline__ void timer_recalc_offs
 
 	if (__USE_RTC())
 		return;
-	tlen = current_tick_length(SHIFT_SCALE - 10);
+	tlen = current_tick_length();
 	offset = cur_tb - do_gtod.varp->tb_orig_stamp;
 	if (tlen == last_tick_len && offset < 0x80000000u)
 		return;
Index: linux-2.6-mm/include/linux/clocksource.h
===================================================================
--- linux-2.6-mm.orig/include/linux/clocksource.h	2006-06-21 14:00:38.000000000 +0200
+++ linux-2.6-mm/include/linux/clocksource.h	2006-06-21 14:01:22.000000000 +0200
@@ -46,8 +46,8 @@ typedef u64 cycle_t;
  * @shift:		cycle to nanosecond divisor (power of two)
  * @update_callback:	called when safe to alter clocksource values
  * @is_continuous:	defines if clocksource is free-running.
- * @interval_cycles:	Used internally by timekeeping core, please ignore.
- * @interval_snsecs:	Used internally by timekeeping core, please ignore.
+ * @cycle_interval:	Used internally by timekeeping core, please ignore.
+ * @xtime_interval:	Used internally by timekeeping core, please ignore.
  */
 struct clocksource {
 	char *name;
@@ -61,8 +61,9 @@ struct clocksource {
 	int is_continuous;
 
 	/* timekeeping specific data, ignore */
-	cycle_t interval_cycles;
-	u64 interval_snsecs;
+	cycle_t cycle_last, cycle_interval;
+	u64 xtime_nsec, xtime_interval;
+	s64 error;
 };
 
 /* simplify initialization of mask field */
@@ -168,107 +169,11 @@ static inline void clocksource_calculate
 	tmp += c->mult/2;
 	do_div(tmp, c->mult);
 
-	c->interval_cycles = (cycle_t)tmp;
-	if(c->interval_cycles == 0)
-		c->interval_cycles = 1;
+	c->cycle_interval = (cycle_t)tmp;
+	if (c->cycle_interval == 0)
+		c->cycle_interval = 1;
 
-	c->interval_snsecs = (u64)c->interval_cycles * c->mult;
-}
-
-
-/**
- * error_aproximation - calculates an error adjustment for a given error
- *
- * @error:	Error value (unsigned)
- * @unit:	Adjustment unit
- *
- * For a given error value, this function takes the adjustment unit
- * and uses binary approximation to return a power of two adjustment value.
- *
- * This function is only for use by the the make_ntp_adj() function
- * and you must hold a write on the xtime_lock when calling.
- */
-static inline int error_aproximation(u64 error, u64 unit)
-{
-	static int saved_adj = 0;
-	u64 adjusted_unit = unit << saved_adj;
-
-	if (error > (adjusted_unit * 2)) {
-		/* large error, so increment the adjustment factor */
-		saved_adj++;
-	} else if (error > adjusted_unit) {
-		/* just right, don't touch it */
-	} else if (saved_adj) {
-		/* small error, so drop the adjustment factor */
-		saved_adj--;
-		return 0;
-	}
-
-	return saved_adj;
-}
-
-
-/**
- * make_ntp_adj - Adjusts the specified clocksource for a given error
- *
- * @clock:		Pointer to clock to be adjusted
- * @cycles_delta:	Current unacounted cycle delta
- * @error:		Pointer to current error value
- *
- * Returns clock shifted nanosecond adjustment to be applied against
- * the accumulated time value (ie: xtime).
- *
- * If the error value is large enough, this function calulates the
- * (power of two) adjustment value, and adjusts the clock's mult and
- * interval_snsecs values accordingly.
- *
- * However, since there may be some unaccumulated cycles, to avoid
- * time inconsistencies we must adjust the accumulation value
- * accordingly.
- *
- * This is not very intuitive, so the following proof should help:
- * The basic timeofday algorithm:  base + cycle * mult
- * Thus:
- *    new_base + cycle * new_mult = old_base + cycle * old_mult
- *    new_base = old_base + cycle * old_mult - cycle * new_mult
- *    new_base = old_base + cycle * (old_mult - new_mult)
- *    new_base - old_base = cycle * (old_mult - new_mult)
- *    base_delta = cycle * (old_mult - new_mult)
- *    base_delta = cycle * (mult_delta)
- *
- * Where mult_delta is the adjustment value made to mult
- *
- */
-static inline s64 make_ntp_adj(struct clocksource *clock,
-				cycles_t cycles_delta, s64* error)
-{
-	s64 ret = 0;
-	if (*error  > ((s64)clock->interval_cycles+1)/2) {
-		/* calculate adjustment value */
-		int adjustment = error_aproximation(*error,
-						clock->interval_cycles);
-		/* adjust clock */
-		clock->mult += 1 << adjustment;
-		clock->interval_snsecs += clock->interval_cycles << adjustment;
-
-		/* adjust the base and error for the adjustment */
-		ret =  -(cycles_delta << adjustment);
-		*error -= clock->interval_cycles << adjustment;
-		/* XXX adj error for cycle_delta offset? */
-	} else if ((-(*error))  > ((s64)clock->interval_cycles+1)/2) {
-		/* calculate adjustment value */
-		int adjustment = error_aproximation(-(*error),
-						clock->interval_cycles);
-		/* adjust clock */
-		clock->mult -= 1 << adjustment;
-		clock->interval_snsecs -= clock->interval_cycles << adjustment;
-
-		/* adjust the base and error for the adjustment */
-		ret =  cycles_delta << adjustment;
-		*error += clock->interval_cycles << adjustment;
-		/* XXX adj error for cycle_delta offset? */
-	}
-	return ret;
+	c->xtime_interval = (u64)c->cycle_interval * c->mult;
 }
 
 
Index: linux-2.6-mm/include/linux/timex.h
===================================================================
--- linux-2.6-mm.orig/include/linux/timex.h	2006-06-21 14:00:41.000000000 +0200
+++ linux-2.6-mm/include/linux/timex.h	2006-06-21 14:01:22.000000000 +0200
@@ -303,8 +303,10 @@ time_interpolator_reset(void)
 
 #endif /* !CONFIG_TIME_INTERPOLATION */
 
+#define TICK_LENGTH_SHIFT	32
+
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
-extern u64 current_tick_length(long);
+extern u64 current_tick_length(void);
 
 extern int do_adjtimex(struct timex *);
 
Index: linux-2.6-mm/kernel/timer.c
===================================================================
--- linux-2.6-mm.orig/kernel/timer.c	2006-06-21 14:00:44.000000000 +0200
+++ linux-2.6-mm/kernel/timer.c	2006-06-21 14:30:47.000000000 +0200
@@ -771,7 +771,7 @@ static void update_ntp_one_tick(void)
  * specified number of bits to the right of the binary point.
  * This function has no side-effects.
  */
-u64 current_tick_length(long shift)
+u64 current_tick_length(void)
 {
 	long delta_nsec;
 	u64 ret;
@@ -780,14 +780,8 @@ u64 current_tick_length(long shift)
 	 *    ie: nanosecond value shifted by (SHIFT_SCALE - 10)
 	 */
 	delta_nsec = tick_nsec + adjtime_adjustment() * 1000;
-	ret = ((u64) delta_nsec << (SHIFT_SCALE - 10)) + time_adj;
-
-	/* convert from (SHIFT_SCALE - 10) to specified shift scale: */
-	shift = shift - (SHIFT_SCALE - 10);
-	if (shift < 0)
-		ret >>= -shift;
-	else
-		ret <<= shift;
+	ret = (u64)delta_nsec << TICK_LENGTH_SHIFT;
+	ret += (s64)time_adj << (TICK_LENGTH_SHIFT - (SHIFT_SCALE - 10));
 
 	return ret;
 }
@@ -795,7 +789,6 @@ u64 current_tick_length(long shift)
 /* XXX - all of this timekeeping code should be later moved to time.c */
 #include <linux/clocksource.h>
 static struct clocksource *clock; /* pointer to current clocksource */
-static cycle_t last_clock_cycle;  /* cycle value at last update_wall_time */
 
 #ifdef CONFIG_GENERIC_TIME
 /**
@@ -814,7 +807,7 @@ static inline s64 __get_nsec_offset(void
 	cycle_now = clocksource_read(clock);
 
 	/* calculate the delta since the last update_wall_time: */
-	cycle_delta = (cycle_now - last_clock_cycle) & clock->mask;
+	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 
 	/* convert to nanoseconds: */
 	ns_offset = cyc2ns(clock, cycle_delta);
@@ -928,7 +921,7 @@ static int change_clocksource(void)
 		timespec_add_ns(&xtime, nsec);
 
 		clock = new;
-		last_clock_cycle = now;
+		clock->cycle_last = now;
 		printk(KERN_INFO "Time: %s clocksource has been installed.\n",
 					clock->name);
 		return 1;
@@ -969,7 +962,7 @@ void __init timekeeping_init(void)
 	write_seqlock_irqsave(&xtime_lock, flags);
 	clock = clocksource_get_next();
 	clocksource_calculate_interval(clock, tick_nsec);
-	last_clock_cycle = clocksource_read(clock);
+	clock->cycle_last = clocksource_read(clock);
 	ntp_clear();
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 }
@@ -989,7 +982,7 @@ static int timekeeping_resume(struct sys
 
 	write_seqlock_irqsave(&xtime_lock, flags);
 	/* restart the last cycle value */
-	last_clock_cycle = clocksource_read(clock);
+	clock->cycle_last = clocksource_read(clock);
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 	return 0;
 }
@@ -1016,60 +1009,123 @@ static int __init timekeeping_init_devic
 device_initcall(timekeeping_init_device);
 
 /*
+ * If the error is already larger, we look ahead another tick,
+ * to compensate for late or lost adjustments.
+ */
+static __always_inline int clocksource_bigadjust(int sign, s64 error, s64 interval, s64 offset)
+{
+	int adj = 0;
+
+	error += current_tick_length() >> (TICK_LENGTH_SHIFT - clock->shift + 1);
+	error -= clock->xtime_interval >> 1;
+
+	while (1) {
+		error >>= 1;
+		if (sign > 0 ? error <= interval : error >= interval) {
+			error = (error << 1) - interval + offset;
+			if (sign > 0 ? error > interval : error < interval)
+				adj++;
+			return adj;
+		}
+		adj++;
+	}
+}
+
+#define clocksource_adjustcheck(sign, error, interval, offset) ({	\
+	int adj = sign;							\
+	error >>= 2;							\
+	if (unlikely(sign > 0 ? error > interval : error < interval)) {	\
+		adj = clocksource_bigadjust(sign, error,		\
+					    interval, offset);		\
+		interval <<= adj;					\
+		offset <<= adj;						\
+		adj = sign << adj;					\
+	}								\
+	adj;								\
+})
+
+/*
+ * adjust the multiplier to reduce the error value,
+ * this is optimized for the most common adjustments of -1,0,1,
+ * for other values we can do a bit more work.
+ */
+static void clocksource_adjust(struct clocksource *clock, s64 offset)
+{
+	s64 error, interval = clock->cycle_interval;
+	int adj;
+
+	error = clock->error >> (TICK_LENGTH_SHIFT - clock->shift - 1);
+	if (error > interval) {
+		adj = clocksource_adjustcheck(1, error, interval, offset);
+	} else if (error < -interval) {
+		interval = -interval;
+		offset = -offset;
+		adj = clocksource_adjustcheck(-1, error, interval, offset);
+	} else
+		goto done;
+
+	clock->mult += adj;
+	clock->xtime_interval += interval;
+	clock->xtime_nsec -= offset;
+	clock->error -= (interval - offset) << (TICK_LENGTH_SHIFT - clock->shift);
+done:
+	/* store full nanoseconds into xtime */
+	xtime.tv_nsec = clock->xtime_nsec >> clock->shift;
+}
+
+/*
  * update_wall_time - Uses the current clocksource to increment the wall time
  *
  * Called from the timer interrupt, must hold a write on xtime_lock.
  */
 static void update_wall_time(void)
 {
-	static s64 remainder_snsecs, error;
-	s64 snsecs_per_sec;
-	cycle_t now, offset;
+	cycle_t offset;
 
-	snsecs_per_sec = (s64)NSEC_PER_SEC << clock->shift;
-	remainder_snsecs += (s64)xtime.tv_nsec << clock->shift;
+	clock->xtime_nsec += (s64)xtime.tv_nsec << clock->shift;
 
-	now = clocksource_read(clock);
-	offset = (now - last_clock_cycle)&clock->mask;
+#ifdef CONFIG_GENERIC_TIME
+	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
+#else
+	offset = clock->cycle_interval;
+#endif
 
 	/* normally this loop will run just once, however in the
 	 * case of lost or late ticks, it will accumulate correctly.
 	 */
-	while (offset > clock->interval_cycles) {
-		/* get the ntp interval in clock shifted nanoseconds */
-		s64 ntp_snsecs	= current_tick_length(clock->shift);
-
+	while (offset > clock->cycle_interval) {
 		/* accumulate one interval */
-		remainder_snsecs += clock->interval_snsecs;
-		last_clock_cycle += clock->interval_cycles;
-		offset -= clock->interval_cycles;
+		clock->xtime_nsec += clock->xtime_interval;
+		clock->cycle_last += clock->cycle_interval;
+		offset -= clock->cycle_interval;
+
+		if (clock->xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
+			clock->xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
+			xtime.tv_sec++;
+			second_overflow();
+		}
 
 		/* interpolator bits */
-		time_interpolator_update(clock->interval_snsecs
+		time_interpolator_update(clock->xtime_interval
 						>> clock->shift);
 		/* increment the NTP state machine */
 		update_ntp_one_tick();
 
 		/* accumulate error between NTP and clock interval */
-		error += (ntp_snsecs - (s64)clock->interval_snsecs);
+		clock->error += current_tick_length();
+		clock->error -= clock->xtime_interval << (TICK_LENGTH_SHIFT - clock->shift);
+	}
 
-		/* correct the clock when NTP error is too big */
-		remainder_snsecs += make_ntp_adj(clock, offset, &error);
+	/* correct the clock when NTP error is too big */
+	clocksource_adjust(clock, offset);
 
-		if (remainder_snsecs >= snsecs_per_sec) {
-			remainder_snsecs -= snsecs_per_sec;
-			xtime.tv_sec++;
-			second_overflow();
-		}
-	}
-	/* store full nanoseconds into xtime */
-	xtime.tv_nsec = remainder_snsecs >> clock->shift;
-	remainder_snsecs -= (s64)xtime.tv_nsec << clock->shift;
+	clock->xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
 
 	/* check to see if there is a new clocksource to use */
 	if (change_clocksource()) {
-		error = 0;
-		remainder_snsecs = 0;
+		clock->error = 0;
+		clock->xtime_nsec = 0;
+		xtime.tv_nsec = 0;
 		clocksource_calculate_interval(clock, tick_nsec);
 	}
 }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fix and optimize clock source update
  2006-06-21 12:38 [PATCH] fix and optimize clock source update Roman Zippel
@ 2006-06-21 20:58 ` john stultz
  2006-06-21 21:38   ` Roman Zippel
  0 siblings, 1 reply; 6+ messages in thread
From: john stultz @ 2006-06-21 20:58 UTC (permalink / raw)
  To: Roman Zippel; +Cc: Andrew Morton, linux-kernel

On Wed, 2006-06-21 at 14:38 +0200, Roman Zippel wrote:
> This fixes the clock source updates in update_wall_time() to correctly
> track the time coming in via current_tick_length(). Optimize the fast
> paths to be as short as possible to keep the overhead low.

Unfortunately it also introduces time inconsistencies when clocksources
are changed. Comments and a fix below:

> Andrew, please apply this patch, it's essential so that we can finally
> get the clock source stuff merged. It keeps the overhead added by all
> the patches in a not really great but acceptable range.
> Tested with 2.6.17-rc6-mm2, applies and compiles with 2.6.17-mm1
> 
> 
> 
>  arch/powerpc/kernel/time.c  |    4 -
>  include/linux/clocksource.h |  113 ++---------------------------------
>  include/linux/timex.h       |    4 -
>  kernel/timer.c              |  142 ++++++++++++++++++++++++++++++--------------
>  4 files changed, 113 insertions(+), 150 deletions(-)
> 
> Index: linux-2.6-mm/arch/powerpc/kernel/time.c
> ===================================================================
> --- linux-2.6-mm.orig/arch/powerpc/kernel/time.c	2006-06-21 13:59:35.000000000 +0200
> +++ linux-2.6-mm/arch/powerpc/kernel/time.c	2006-06-21 14:01:22.000000000 +0200
> @@ -102,7 +102,7 @@ EXPORT_SYMBOL(tb_ticks_per_sec);	/* for 
>  u64 tb_to_xs;
>  unsigned tb_to_us;
>  
> -#define TICKLEN_SCALE	(SHIFT_SCALE - 10)
> +#define TICKLEN_SCALE	TICK_LENGTH_SHIFT
>  u64 last_tick_len;	/* units are ns / 2^TICKLEN_SCALE */
>  u64 ticklen_to_xs;	/* 0.64 fraction */
>  
> @@ -534,7 +534,7 @@ static __inline__ void timer_recalc_offs
>  
>  	if (__USE_RTC())
>  		return;
> -	tlen = current_tick_length(SHIFT_SCALE - 10);
> +	tlen = current_tick_length();
>  	offset = cur_tb - do_gtod.varp->tb_orig_stamp;
>  	if (tlen == last_tick_len && offset < 0x80000000u)
>  		return;
> Index: linux-2.6-mm/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6-mm.orig/include/linux/clocksource.h	2006-06-21 14:00:38.000000000 +0200
> +++ linux-2.6-mm/include/linux/clocksource.h	2006-06-21 14:01:22.000000000 +0200
> @@ -46,8 +46,8 @@ typedef u64 cycle_t;
>   * @shift:		cycle to nanosecond divisor (power of two)
>   * @update_callback:	called when safe to alter clocksource values
>   * @is_continuous:	defines if clocksource is free-running.
> - * @interval_cycles:	Used internally by timekeeping core, please ignore.
> - * @interval_snsecs:	Used internally by timekeeping core, please ignore.
> + * @cycle_interval:	Used internally by timekeeping core, please ignore.
> + * @xtime_interval:	Used internally by timekeeping core, please ignore.
>   */

Where the variable name changes really necessary (I find them less
clear)? You also forgot to add error here as it is added below.

>  struct clocksource {
>  	char *name;
> @@ -61,8 +61,9 @@ struct clocksource {
>  	int is_continuous;
>  
>  	/* timekeeping specific data, ignore */
> -	cycle_t interval_cycles;
> -	u64 interval_snsecs;
> +	cycle_t cycle_last, cycle_interval;
> +	u64 xtime_nsec, xtime_interval;
> +	s64 error;
>  };
>  
[snip]
> +
> +#define clocksource_adjustcheck(sign, error, interval, offset) ({	\
> +	int adj = sign;							\
> +	error >>= 2;							\
> +	if (unlikely(sign > 0 ? error > interval : error < interval)) {	\
> +		adj = clocksource_bigadjust(sign, error,		\
> +					    interval, offset);		\
> +		interval <<= adj;					\
> +		offset <<= adj;						\
> +		adj = sign << adj;					\
> +	}								\
> +	adj;								\
> +})

That's still a #define with side effects. Yuck.

> +/*
> + * adjust the multiplier to reduce the error value,
> + * this is optimized for the most common adjustments of -1,0,1,
> + * for other values we can do a bit more work.
> + */
> +static void clocksource_adjust(struct clocksource *clock, s64 offset)
> +{
> +	s64 error, interval = clock->cycle_interval;
> +	int adj;
> +
> +	error = clock->error >> (TICK_LENGTH_SHIFT - clock->shift - 1);
> +	if (error > interval) {
> +		adj = clocksource_adjustcheck(1, error, interval, offset);
> +	} else if (error < -interval) {
> +		interval = -interval;
> +		offset = -offset;
> +		adj = clocksource_adjustcheck(-1, error, interval, offset);
> +	} else
> +		goto done;
> +
> +	clock->mult += adj;
> +	clock->xtime_interval += interval;
> +	clock->xtime_nsec -= offset;
> +	clock->error -= (interval - offset) << (TICK_LENGTH_SHIFT - clock->shift);
> +done:
> +	/* store full nanoseconds into xtime */
> +	xtime.tv_nsec = clock->xtime_nsec >> clock->shift;
> +}

Why are you setting xtime.tv_nsec in clocksource_adjust()?
That should be kept in update_wall_time. 


> +/*
>   * update_wall_time - Uses the current clocksource to increment the wall time
>   *
>   * Called from the timer interrupt, must hold a write on xtime_lock.
>   */
>  static void update_wall_time(void)
>  {
> -	static s64 remainder_snsecs, error;
> -	s64 snsecs_per_sec;
> -	cycle_t now, offset;
> +	cycle_t offset;
>  
> -	snsecs_per_sec = (s64)NSEC_PER_SEC << clock->shift;
> -	remainder_snsecs += (s64)xtime.tv_nsec << clock->shift;
> +	clock->xtime_nsec += (s64)xtime.tv_nsec << clock->shift;
>  
> -	now = clocksource_read(clock);
> -	offset = (now - last_clock_cycle)&clock->mask;
> +#ifdef CONFIG_GENERIC_TIME
> +	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
> +#else
> +	offset = clock->cycle_interval;
> +#endif

This looks ok, but I'd prefer the GENERIC_TIME case to be less dense
(not doing the read in the same line).

>  	/* normally this loop will run just once, however in the
>  	 * case of lost or late ticks, it will accumulate correctly.
>  	 */
> -	while (offset > clock->interval_cycles) {
> -		/* get the ntp interval in clock shifted nanoseconds */
> -		s64 ntp_snsecs	= current_tick_length(clock->shift);
> -
> +	while (offset > clock->cycle_interval) {

Shouldn't that be >= ? That was one of the fixes I made in my other
patch.

[snip]
> -		/* correct the clock when NTP error is too big */
> -		remainder_snsecs += make_ntp_adj(clock, offset, &error);
> +	/* correct the clock when NTP error is too big */
> +	clocksource_adjust(clock, offset);
>  
> -		if (remainder_snsecs >= snsecs_per_sec) {
> -			remainder_snsecs -= snsecs_per_sec;
> -			xtime.tv_sec++;
> -			second_overflow();
> -		}
> -	}
> -	/* store full nanoseconds into xtime */
> -	xtime.tv_nsec = remainder_snsecs >> clock->shift;
> -	remainder_snsecs -= (s64)xtime.tv_nsec << clock->shift;
> +	clock->xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
>  
>  	/* check to see if there is a new clocksource to use */
>  	if (change_clocksource()) {
> -		error = 0;
> -		remainder_snsecs = 0;
> +		clock->error = 0;
> +		clock->xtime_nsec = 0;
> +		xtime.tv_nsec = 0;
>  		clocksource_calculate_interval(clock, tick_nsec);
>  	}

Setting xtime.tv_nsec to zero in the above is incorrect and causes the
inconsistencies I mentioned at the top.

Overall, I've got some gripes, but I'm not going to block this because
of them. I appreciate you sending this and I think moving forward is
more important. We can continue to hash out the smaller details in the
future.

Include the following small patch which fixes the issue I've found in my
testing and this will get my ACK.

thanks
-john

Index: romanfix/kernel/timer.c
===================================================================
--- romanfix.orig/kernel/timer.c
+++ romanfix/kernel/timer.c
@@ -1062,15 +1062,12 @@ static void clocksource_adjust(struct cl
 		offset = -offset;
 		adj = clocksource_adjustcheck(-1, error, interval, offset);
 	} else
-		goto done;
+		return;
 
 	clock->mult += adj;
 	clock->xtime_interval += interval;
 	clock->xtime_nsec -= offset;
 	clock->error -= (interval - offset) << (TICK_LENGTH_SHIFT - clock->shift);
-done:
-	/* store full nanoseconds into xtime */
-	xtime.tv_nsec = clock->xtime_nsec >> clock->shift;
 }
 
 /*
@@ -1119,13 +1116,14 @@ static void update_wall_time(void)
 	/* correct the clock when NTP error is too big */
 	clocksource_adjust(clock, offset);
 
+	/* store full nanoseconds into xtime */
+	xtime.tv_nsec = clock->xtime_nsec >> clock->shift;
 	clock->xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
 
 	/* check to see if there is a new clocksource to use */
 	if (change_clocksource()) {
 		clock->error = 0;
 		clock->xtime_nsec = 0;
-		xtime.tv_nsec = 0;
 		clocksource_calculate_interval(clock, tick_nsec);
 	}
 }



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fix and optimize clock source update
  2006-06-21 20:58 ` john stultz
@ 2006-06-21 21:38   ` Roman Zippel
  2006-06-21 21:51     ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Roman Zippel @ 2006-06-21 21:38 UTC (permalink / raw)
  To: john stultz; +Cc: Andrew Morton, linux-kernel

Hi,

On Wed, 21 Jun 2006, john stultz wrote:

> >   * @is_continuous:	defines if clocksource is free-running.
> > - * @interval_cycles:	Used internally by timekeeping core, please ignore.
> > - * @interval_snsecs:	Used internally by timekeeping core, please ignore.
> > + * @cycle_interval:	Used internally by timekeeping core, please ignore.
> > + * @xtime_interval:	Used internally by timekeeping core, please ignore.
> >   */
> 
> Where the variable name changes really necessary (I find them less
> clear)? You also forgot to add error here as it is added below.

I actually find them more clear this way, cycle values start with cycle_, 
xtime related variables start with xtime_ and update intervals end with 
_interval, this makes everything quite consistent.
In the middle term I also want to document them properly, so I didn't 
update these comments.

> > +#define clocksource_adjustcheck(sign, error, interval, offset) ({	\
> > +	int adj = sign;							\
> > +	error >>= 2;							\
> > +	if (unlikely(sign > 0 ? error > interval : error < interval)) {	\
> > +		adj = clocksource_bigadjust(sign, error,		\
> > +					    interval, offset);		\
> > +		interval <<= adj;					\
> > +		offset <<= adj;						\
> > +		adj = sign << adj;					\
> > +	}								\
> > +	adj;								\
> > +})
> 
> That's still a #define with side effects. Yuck.

The alternative is duplicating the code and an inline function which takes 
the address of these variables would likely generate worse code.

> > +	clock->mult += adj;
> > +	clock->xtime_interval += interval;
> > +	clock->xtime_nsec -= offset;
> > +	clock->error -= (interval - offset) << (TICK_LENGTH_SHIFT - clock->shift);
> > +done:
> > +	/* store full nanoseconds into xtime */
> > +	xtime.tv_nsec = clock->xtime_nsec >> clock->shift;
> > +}
> 
> Why are you setting xtime.tv_nsec in clocksource_adjust()?
> That should be kept in update_wall_time. 

clock->xtime_nsec and xtime.tv_nsec are closely related, in the long term 
xtime.tv_nsec should be unused.

> > -	now = clocksource_read(clock);
> > -	offset = (now - last_clock_cycle)&clock->mask;
> > +#ifdef CONFIG_GENERIC_TIME
> > +	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
> > +#else
> > +	offset = clock->cycle_interval;
> > +#endif
> 
> This looks ok, but I'd prefer the GENERIC_TIME case to be less dense
> (not doing the read in the same line).

The "now" definition would then require another #ifdef to avoid a warning.

> >  	/* normally this loop will run just once, however in the
> >  	 * case of lost or late ticks, it will accumulate correctly.
> >  	 */
> > -	while (offset > clock->interval_cycles) {
> > -		/* get the ntp interval in clock shifted nanoseconds */
> > -		s64 ntp_snsecs	= current_tick_length(clock->shift);
> > -
> > +	while (offset > clock->cycle_interval) {
> 
> Shouldn't that be >= ? That was one of the fixes I made in my other
> patch.

Sorry, I indeed missed this, I was only looking at my patches.

> >  	/* check to see if there is a new clocksource to use */
> >  	if (change_clocksource()) {
> > -		error = 0;
> > -		remainder_snsecs = 0;
> > +		clock->error = 0;
> > +		clock->xtime_nsec = 0;
> > +		xtime.tv_nsec = 0;
> >  		clocksource_calculate_interval(clock, tick_nsec);
> >  	}
> 
> Setting xtime.tv_nsec to zero in the above is incorrect and causes the
> inconsistencies I mentioned at the top.

Indeed, but luckily it's not a common event.

bye, Roman

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fix and optimize clock source update
  2006-06-21 21:38   ` Roman Zippel
@ 2006-06-21 21:51     ` Andrew Morton
  2006-06-22 11:19       ` Roman Zippel
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-06-21 21:51 UTC (permalink / raw)
  To: Roman Zippel; +Cc: johnstul, linux-kernel

On Wed, 21 Jun 2006 23:38:32 +0200 (CEST)
Roman Zippel <zippel@linux-m68k.org> wrote:

> > > +#define clocksource_adjustcheck(sign, error, interval, offset) ({	\
> > > +	int adj = sign;							\
> > > +	error >>= 2;							\
> > > +	if (unlikely(sign > 0 ? error > interval : error < interval)) {	\
> > > +		adj = clocksource_bigadjust(sign, error,		\
> > > +					    interval, offset);		\
> > > +		interval <<= adj;					\
> > > +		offset <<= adj;						\
> > > +		adj = sign << adj;					\
> > > +	}								\
> > > +	adj;								\
> > > +})
> > 
> > That's still a #define with side effects. Yuck.
> 
> The alternative is duplicating the code and an inline function which takes 
> the address of these variables would likely generate worse code.

Can you verify that please?  It is pretty sick.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fix and optimize clock source update
  2006-06-21 21:51     ` Andrew Morton
@ 2006-06-22 11:19       ` Roman Zippel
  2006-06-22 14:51         ` john stultz
  0 siblings, 1 reply; 6+ messages in thread
From: Roman Zippel @ 2006-06-22 11:19 UTC (permalink / raw)
  To: Andrew Morton; +Cc: johnstul, linux-kernel

Hi,

On Wed, 21 Jun 2006, Andrew Morton wrote:

> > The alternative is duplicating the code and an inline function which takes 
> > the address of these variables would likely generate worse code.
> 
> Can you verify that please?  It is pretty sick.

I'm pleasantly surprised. I already expected to be no difference with 
gcc-4.0, but gcc-3.x can deal with it as well.
Below is a new patch which moves the macro into the bigadjust function and 
includes John's changes, while at it I also added a few more comments.

bye, Roman




This fixes the clock source updates in update_wall_time() to correctly
track the time coming in via current_tick_length(). Optimize the fast
paths to be as short as possible to keep the overhead low.

Signed-off-by: Roman Zippel <zippel@linux-m68k.org>

---

Andrew, please apply this patch, it's essential so that we can finally
get the clock source stuff merged. It keeps the overhead added by all
this in a not really good but acceptable range.
Tested with 2.6.17-rc6-mm2, applies and compiles with 2.6.17-mm1

 arch/powerpc/kernel/time.c  |    4 -
 include/linux/clocksource.h |  113 ++------------------------------
 include/linux/timex.h       |    4 -
 kernel/timer.c              |  151 +++++++++++++++++++++++++++++++-------------
 4 files changed, 123 insertions(+), 149 deletions(-)

Index: linux-2.6-mm/arch/powerpc/kernel/time.c
===================================================================
--- linux-2.6-mm.orig/arch/powerpc/kernel/time.c	2006-06-22 11:56:06.000000000 +0200
+++ linux-2.6-mm/arch/powerpc/kernel/time.c	2006-06-22 11:56:24.000000000 +0200
@@ -102,7 +102,7 @@ EXPORT_SYMBOL(tb_ticks_per_sec);	/* for 
 u64 tb_to_xs;
 unsigned tb_to_us;
 
-#define TICKLEN_SCALE	(SHIFT_SCALE - 10)
+#define TICKLEN_SCALE	TICK_LENGTH_SHIFT
 u64 last_tick_len;	/* units are ns / 2^TICKLEN_SCALE */
 u64 ticklen_to_xs;	/* 0.64 fraction */
 
@@ -534,7 +534,7 @@ static __inline__ void timer_recalc_offs
 
 	if (__USE_RTC())
 		return;
-	tlen = current_tick_length(SHIFT_SCALE - 10);
+	tlen = current_tick_length();
 	offset = cur_tb - do_gtod.varp->tb_orig_stamp;
 	if (tlen == last_tick_len && offset < 0x80000000u)
 		return;
Index: linux-2.6-mm/include/linux/clocksource.h
===================================================================
--- linux-2.6-mm.orig/include/linux/clocksource.h	2006-06-22 11:56:06.000000000 +0200
+++ linux-2.6-mm/include/linux/clocksource.h	2006-06-22 11:56:24.000000000 +0200
@@ -46,8 +46,8 @@ typedef u64 cycle_t;
  * @shift:		cycle to nanosecond divisor (power of two)
  * @update_callback:	called when safe to alter clocksource values
  * @is_continuous:	defines if clocksource is free-running.
- * @interval_cycles:	Used internally by timekeeping core, please ignore.
- * @interval_snsecs:	Used internally by timekeeping core, please ignore.
+ * @cycle_interval:	Used internally by timekeeping core, please ignore.
+ * @xtime_interval:	Used internally by timekeeping core, please ignore.
  */
 struct clocksource {
 	char *name;
@@ -61,8 +61,9 @@ struct clocksource {
 	int is_continuous;
 
 	/* timekeeping specific data, ignore */
-	cycle_t interval_cycles;
-	u64 interval_snsecs;
+	cycle_t cycle_last, cycle_interval;
+	u64 xtime_nsec, xtime_interval;
+	s64 error;
 };
 
 /* simplify initialization of mask field */
@@ -168,107 +169,11 @@ static inline void clocksource_calculate
 	tmp += c->mult/2;
 	do_div(tmp, c->mult);
 
-	c->interval_cycles = (cycle_t)tmp;
-	if(c->interval_cycles == 0)
-		c->interval_cycles = 1;
+	c->cycle_interval = (cycle_t)tmp;
+	if (c->cycle_interval == 0)
+		c->cycle_interval = 1;
 
-	c->interval_snsecs = (u64)c->interval_cycles * c->mult;
-}
-
-
-/**
- * error_aproximation - calculates an error adjustment for a given error
- *
- * @error:	Error value (unsigned)
- * @unit:	Adjustment unit
- *
- * For a given error value, this function takes the adjustment unit
- * and uses binary approximation to return a power of two adjustment value.
- *
- * This function is only for use by the the make_ntp_adj() function
- * and you must hold a write on the xtime_lock when calling.
- */
-static inline int error_aproximation(u64 error, u64 unit)
-{
-	static int saved_adj = 0;
-	u64 adjusted_unit = unit << saved_adj;
-
-	if (error > (adjusted_unit * 2)) {
-		/* large error, so increment the adjustment factor */
-		saved_adj++;
-	} else if (error > adjusted_unit) {
-		/* just right, don't touch it */
-	} else if (saved_adj) {
-		/* small error, so drop the adjustment factor */
-		saved_adj--;
-		return 0;
-	}
-
-	return saved_adj;
-}
-
-
-/**
- * make_ntp_adj - Adjusts the specified clocksource for a given error
- *
- * @clock:		Pointer to clock to be adjusted
- * @cycles_delta:	Current unacounted cycle delta
- * @error:		Pointer to current error value
- *
- * Returns clock shifted nanosecond adjustment to be applied against
- * the accumulated time value (ie: xtime).
- *
- * If the error value is large enough, this function calulates the
- * (power of two) adjustment value, and adjusts the clock's mult and
- * interval_snsecs values accordingly.
- *
- * However, since there may be some unaccumulated cycles, to avoid
- * time inconsistencies we must adjust the accumulation value
- * accordingly.
- *
- * This is not very intuitive, so the following proof should help:
- * The basic timeofday algorithm:  base + cycle * mult
- * Thus:
- *    new_base + cycle * new_mult = old_base + cycle * old_mult
- *    new_base = old_base + cycle * old_mult - cycle * new_mult
- *    new_base = old_base + cycle * (old_mult - new_mult)
- *    new_base - old_base = cycle * (old_mult - new_mult)
- *    base_delta = cycle * (old_mult - new_mult)
- *    base_delta = cycle * (mult_delta)
- *
- * Where mult_delta is the adjustment value made to mult
- *
- */
-static inline s64 make_ntp_adj(struct clocksource *clock,
-				cycles_t cycles_delta, s64* error)
-{
-	s64 ret = 0;
-	if (*error  > ((s64)clock->interval_cycles+1)/2) {
-		/* calculate adjustment value */
-		int adjustment = error_aproximation(*error,
-						clock->interval_cycles);
-		/* adjust clock */
-		clock->mult += 1 << adjustment;
-		clock->interval_snsecs += clock->interval_cycles << adjustment;
-
-		/* adjust the base and error for the adjustment */
-		ret =  -(cycles_delta << adjustment);
-		*error -= clock->interval_cycles << adjustment;
-		/* XXX adj error for cycle_delta offset? */
-	} else if ((-(*error))  > ((s64)clock->interval_cycles+1)/2) {
-		/* calculate adjustment value */
-		int adjustment = error_aproximation(-(*error),
-						clock->interval_cycles);
-		/* adjust clock */
-		clock->mult -= 1 << adjustment;
-		clock->interval_snsecs -= clock->interval_cycles << adjustment;
-
-		/* adjust the base and error for the adjustment */
-		ret =  cycles_delta << adjustment;
-		*error += clock->interval_cycles << adjustment;
-		/* XXX adj error for cycle_delta offset? */
-	}
-	return ret;
+	c->xtime_interval = (u64)c->cycle_interval * c->mult;
 }
 
 
Index: linux-2.6-mm/include/linux/timex.h
===================================================================
--- linux-2.6-mm.orig/include/linux/timex.h	2006-06-22 11:56:06.000000000 +0200
+++ linux-2.6-mm/include/linux/timex.h	2006-06-22 11:56:24.000000000 +0200
@@ -303,8 +303,10 @@ time_interpolator_reset(void)
 
 #endif /* !CONFIG_TIME_INTERPOLATION */
 
+#define TICK_LENGTH_SHIFT	32
+
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
-extern u64 current_tick_length(long);
+extern u64 current_tick_length(void);
 
 extern int do_adjtimex(struct timex *);
 
Index: linux-2.6-mm/kernel/timer.c
===================================================================
--- linux-2.6-mm.orig/kernel/timer.c	2006-06-22 11:56:07.000000000 +0200
+++ linux-2.6-mm/kernel/timer.c	2006-06-22 12:29:37.000000000 +0200
@@ -771,7 +771,7 @@ static void update_ntp_one_tick(void)
  * specified number of bits to the right of the binary point.
  * This function has no side-effects.
  */
-u64 current_tick_length(long shift)
+u64 current_tick_length(void)
 {
 	long delta_nsec;
 	u64 ret;
@@ -780,14 +780,8 @@ u64 current_tick_length(long shift)
 	 *    ie: nanosecond value shifted by (SHIFT_SCALE - 10)
 	 */
 	delta_nsec = tick_nsec + adjtime_adjustment() * 1000;
-	ret = ((u64) delta_nsec << (SHIFT_SCALE - 10)) + time_adj;
-
-	/* convert from (SHIFT_SCALE - 10) to specified shift scale: */
-	shift = shift - (SHIFT_SCALE - 10);
-	if (shift < 0)
-		ret >>= -shift;
-	else
-		ret <<= shift;
+	ret = (u64)delta_nsec << TICK_LENGTH_SHIFT;
+	ret += (s64)time_adj << (TICK_LENGTH_SHIFT - (SHIFT_SCALE - 10));
 
 	return ret;
 }
@@ -795,7 +789,6 @@ u64 current_tick_length(long shift)
 /* XXX - all of this timekeeping code should be later moved to time.c */
 #include <linux/clocksource.h>
 static struct clocksource *clock; /* pointer to current clocksource */
-static cycle_t last_clock_cycle;  /* cycle value at last update_wall_time */
 
 #ifdef CONFIG_GENERIC_TIME
 /**
@@ -814,7 +807,7 @@ static inline s64 __get_nsec_offset(void
 	cycle_now = clocksource_read(clock);
 
 	/* calculate the delta since the last update_wall_time: */
-	cycle_delta = (cycle_now - last_clock_cycle) & clock->mask;
+	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 
 	/* convert to nanoseconds: */
 	ns_offset = cyc2ns(clock, cycle_delta);
@@ -928,7 +921,7 @@ static int change_clocksource(void)
 		timespec_add_ns(&xtime, nsec);
 
 		clock = new;
-		last_clock_cycle = now;
+		clock->cycle_last = now;
 		printk(KERN_INFO "Time: %s clocksource has been installed.\n",
 					clock->name);
 		return 1;
@@ -969,7 +962,7 @@ void __init timekeeping_init(void)
 	write_seqlock_irqsave(&xtime_lock, flags);
 	clock = clocksource_get_next();
 	clocksource_calculate_interval(clock, tick_nsec);
-	last_clock_cycle = clocksource_read(clock);
+	clock->cycle_last = clocksource_read(clock);
 	ntp_clear();
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 }
@@ -989,7 +982,7 @@ static int timekeeping_resume(struct sys
 
 	write_seqlock_irqsave(&xtime_lock, flags);
 	/* restart the last cycle value */
-	last_clock_cycle = clocksource_read(clock);
+	clock->cycle_last = clocksource_read(clock);
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 	return 0;
 }
@@ -1016,60 +1009,134 @@ static int __init timekeeping_init_devic
 device_initcall(timekeeping_init_device);
 
 /*
+ * If the error is already larger, we look ahead another tick,
+ * to compensate for late or lost adjustments.
+ */
+static __always_inline int clocksource_bigadjust(int sign, s64 error, s64 *interval, s64 *offset)
+{
+	int adj;
+
+	/*
+	 * As soon as the machine is synchronized to the external time
+	 * source this should be the common case.
+	 */
+	error >>= 2;
+	if (likely(sign > 0 ? error <= *interval : error >= *interval))
+		return sign;
+
+	/*
+	 * An extra look ahead dampens the effect of the current error,
+	 * which can grow quite large with continously late updates, as
+	 * it would dominate the adjustment value and can lead to
+	 * oscillation.
+	 */
+	error += current_tick_length() >> (TICK_LENGTH_SHIFT - clock->shift + 1);
+	error -= clock->xtime_interval >> 1;
+
+	adj = 0;
+	while (1) {
+		error >>= 1;
+		if (sign > 0 ? error <= *interval : error >= *interval)
+			break;
+		adj++;
+	}
+
+	/*
+	 * Add the current adjustments to the error and take the offset
+	 * into account, the latter can cause the error to be hardly
+	 * reduced at the next tick. Check the error again if there's
+	 * room for another adjustment, thus further reducing the error
+	 * which otherwise had to be corrected at the next update.
+	 */
+	error = (error << 1) - *interval + *offset;
+	if (sign > 0 ? error > *interval : error < *interval)
+		adj++;
+
+	*interval <<= adj;
+	*offset <<= adj;
+	return sign << adj;
+}
+
+/*
+ * Adjust the multiplier to reduce the error value,
+ * this is optimized for the most common adjustments of -1,0,1,
+ * for other values we can do a bit more work.
+ */
+static void clocksource_adjust(struct clocksource *clock, s64 offset)
+{
+	s64 error, interval = clock->cycle_interval;
+	int adj;
+
+	error = clock->error >> (TICK_LENGTH_SHIFT - clock->shift - 1);
+	if (error > interval) {
+		adj = clocksource_bigadjust(1, error, &interval, &offset);
+	} else if (error < -interval) {
+		interval = -interval;
+		offset = -offset;
+		adj = clocksource_bigadjust(-1, error, &interval, &offset);
+	} else
+		return;
+
+	clock->mult += adj;
+	clock->xtime_interval += interval;
+	clock->xtime_nsec -= offset;
+	clock->error -= (interval - offset) << (TICK_LENGTH_SHIFT - clock->shift);
+}
+
+/*
  * update_wall_time - Uses the current clocksource to increment the wall time
  *
  * Called from the timer interrupt, must hold a write on xtime_lock.
  */
 static void update_wall_time(void)
 {
-	static s64 remainder_snsecs, error;
-	s64 snsecs_per_sec;
-	cycle_t now, offset;
+	cycle_t offset;
 
-	snsecs_per_sec = (s64)NSEC_PER_SEC << clock->shift;
-	remainder_snsecs += (s64)xtime.tv_nsec << clock->shift;
+	clock->xtime_nsec += (s64)xtime.tv_nsec << clock->shift;
 
-	now = clocksource_read(clock);
-	offset = (now - last_clock_cycle)&clock->mask;
+#ifdef CONFIG_GENERIC_TIME
+	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
+#else
+	offset = clock->cycle_interval;
+#endif
 
 	/* normally this loop will run just once, however in the
 	 * case of lost or late ticks, it will accumulate correctly.
 	 */
-	while (offset > clock->interval_cycles) {
-		/* get the ntp interval in clock shifted nanoseconds */
-		s64 ntp_snsecs	= current_tick_length(clock->shift);
-
+	while (offset >= clock->cycle_interval) {
 		/* accumulate one interval */
-		remainder_snsecs += clock->interval_snsecs;
-		last_clock_cycle += clock->interval_cycles;
-		offset -= clock->interval_cycles;
+		clock->xtime_nsec += clock->xtime_interval;
+		clock->cycle_last += clock->cycle_interval;
+		offset -= clock->cycle_interval;
+
+		if (clock->xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
+			clock->xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
+			xtime.tv_sec++;
+			second_overflow();
+		}
 
 		/* interpolator bits */
-		time_interpolator_update(clock->interval_snsecs
+		time_interpolator_update(clock->xtime_interval
 						>> clock->shift);
 		/* increment the NTP state machine */
 		update_ntp_one_tick();
 
 		/* accumulate error between NTP and clock interval */
-		error += (ntp_snsecs - (s64)clock->interval_snsecs);
+		clock->error += current_tick_length();
+		clock->error -= clock->xtime_interval << (TICK_LENGTH_SHIFT - clock->shift);
+	}
 
-		/* correct the clock when NTP error is too big */
-		remainder_snsecs += make_ntp_adj(clock, offset, &error);
+	/* correct the clock when NTP error is too big */
+	clocksource_adjust(clock, offset);
 
-		if (remainder_snsecs >= snsecs_per_sec) {
-			remainder_snsecs -= snsecs_per_sec;
-			xtime.tv_sec++;
-			second_overflow();
-		}
-	}
 	/* store full nanoseconds into xtime */
-	xtime.tv_nsec = remainder_snsecs >> clock->shift;
-	remainder_snsecs -= (s64)xtime.tv_nsec << clock->shift;
+	xtime.tv_nsec = clock->xtime_nsec >> clock->shift;
+	clock->xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
 
 	/* check to see if there is a new clocksource to use */
 	if (change_clocksource()) {
-		error = 0;
-		remainder_snsecs = 0;
+		clock->error = 0;
+		clock->xtime_nsec = 0;
 		clocksource_calculate_interval(clock, tick_nsec);
 	}
 }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fix and optimize clock source update
  2006-06-22 11:19       ` Roman Zippel
@ 2006-06-22 14:51         ` john stultz
  0 siblings, 0 replies; 6+ messages in thread
From: john stultz @ 2006-06-22 14:51 UTC (permalink / raw)
  To: Roman Zippel; +Cc: Andrew Morton, linux-kernel

On Thu, 2006-06-22 at 13:19 +0200, Roman Zippel wrote:
> This fixes the clock source updates in update_wall_time() to correctly
> track the time coming in via current_tick_length(). Optimize the fast
> paths to be as short as possible to keep the overhead low.
> 
> Signed-off-by: Roman Zippel <zippel@linux-m68k.org>
> 

Thanks Roman!

The inconsistencies at clocksource change are resolved. I'm having a
real spotty time getting my airo card to work w/ 2.6.17-mm1, so I'll run
more tests w/ ntp later today when I'm wired.

ACKed-by: John Stultz <johnstul@us.ibm.com>

thanks again,
-john


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2006-06-22 14:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-06-21 12:38 [PATCH] fix and optimize clock source update Roman Zippel
2006-06-21 20:58 ` john stultz
2006-06-21 21:38   ` Roman Zippel
2006-06-21 21:51     ` Andrew Morton
2006-06-22 11:19       ` Roman Zippel
2006-06-22 14:51         ` john stultz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).