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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
