Re: [PATCH 2/10] NTP: normalize time_adj

[Roman Zippel]

Hi,

On Tue, 10 Jan 2006, john stultz wrote:

> > Index: linux-2.6-mm/include/linux/timex.h
> > ===================================================================
> > --- linux-2.6-mm.orig/include/linux/timex.h	2005-12-21 12:11:48.000000000 +0100
> > +++ linux-2.6-mm/include/linux/timex.h	2005-12-21 12:11:56.000000000 +0100
> > @@ -93,7 +93,7 @@
> >  #define SHIFT_SCALE 22		/* phase scale (shift) */
> >  #define SHIFT_UPDATE (SHIFT_KG + MAXTC) /* time offset scale (shift) */
> >  #define SHIFT_USEC 16		/* frequency offset scale (shift) */
> > -#define FINENSEC (1L << (SHIFT_SCALE - 10)) /* ~1 ns in phase units */
> > +#define FINENSEC (1L << SHIFT_SCALE) /* ~1 ns in phase units */
> 
> So this effectively increases the granularity of the phase units, right?

Basically yes, but it's part of a small cleanup I forgot to comment. (See 
below)

> > @@ -675,36 +677,38 @@ static void second_overflow(void)
> >  	ltemp = min(ltemp, (MAXPHASE / MINSEC) << SHIFT_UPDATE);
> >  	ltemp = max(ltemp, -(MAXPHASE / MINSEC) << SHIFT_UPDATE);
> >  	time_offset -= ltemp;
> > -	time_adj = ltemp << (SHIFT_SCALE - SHIFT_HZ - SHIFT_UPDATE);
> > +	adj = ltemp << (SHIFT_SCALE - SHIFT_HZ - SHIFT_UPDATE);
> 
> 	That could maybe use a better comment. The larger comment makes it
> clear we're converting the usec offset to phase adjustment units, but
> maybe something further to explain how usecs -> phase is connected to
> multiplying by 2^(SHIFT_SCALE - SHIFT_HZ - SHIFT_UPDATE) would help?

I agree that this could use better comments, although some changes are 
only temporary, so some things I'll probably only comment better in the 
changelog.

> >  	 * 0.78125% to get 1023.4375; => only 0.05% error (p. 14)
> >  	 */
> > -	time_adj += shift_right(time_adj, 6) + shift_right(time_adj, 7);
> > +	adj += shift_right(adj, 6) + shift_right(adj, 7);
> >  #endif
> > +	tick_nsec_curr += adj >> (SHIFT_SCALE - 10);
> > +	time_adj = (adj << 10) & (FINENSEC - 1);
> 
> Again, a comment here would help. 
> 
> We use time_adj to increment the phase which will adjust the per-tick
> nsec interval in update_wall_time_one_tick, so why are we adjusting
> tick_nsec_curr which is used there as well?

This is the part which normalizes time_adj, so it's always positive and 
saves two checks in update_wall_time_one_tick().
(BTW in the long term I want to merge these two into a single 64 bit 
variable, but it requires a few more changes.)

> Also why SHIFT_SCALE - 10?  And (adj<<10)&(FINSEC -1) looks like magic
> to me. :)

It's connected to the removed "- 10" above and below, it moves the crude 
usec to nsec conversion (it divides by 1024 instead of 1000) to a single 
spot, so it's easier to remove in a later patch.
I initially wanted to mention this in the changelog, but forgot about it, 
I'll update it.

> >  	time_phase += time_adj;
> > -	if ((time_phase >= FINENSEC) || (time_phase <= -FINENSEC)) {
> > -		long ltemp = shift_right(time_phase, (SHIFT_SCALE - 10));
> > -		time_phase -= ltemp << (SHIFT_SCALE - 10);
> > +	if (time_phase >= FINENSEC) {
> > +		long ltemp = time_phase >> SHIFT_SCALE;
> > +		time_phase -= ltemp << SHIFT_SCALE;
> >  		delta_nsec += ltemp;
> >  	}
> >  	xtime.tv_nsec += delta_nsec;
> 
> Again, same point as the last comment.

See above.

> I'll try to provide similar comments for the other patches tomorrow.

Thanks. :)

bye, Roman
-
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/