lkml.org 
[lkml]   [2006]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 5/10] NTP: convert time_offset to nsec
Hi,

On Wed, 11 Jan 2006, john stultz wrote:

> > if (mtemp < MAXSEC) {
> > - ltemp *= mtemp;
> > - time_freq += shift_right(ltemp,(time_constant +
> > + freq_adj = (s64)ltemp * mtemp;
> > + time_freq += shift_right(freq_adj,(time_constant +
> > time_constant +
> > SHIFT_KF - SHIFT_USEC));
> > } else /* calibration interval too long (p. 12) */
>
> While you're there, mind changing mtemp to adjtime_interval or something
> more clear? Probably something similar to ltemp as well.

Ok, but I think I'll rename existing variables in separate patch.

> > @@ -332,6 +340,7 @@ int do_adjtimex(struct timex *txc)
> > }
> > time_freq = min(time_freq, time_tolerance);
> > time_freq = max(time_freq, -time_tolerance);
> > + time_offset = (time_offset / HZ) << SHIFT_HZ;
>
> time_offset has already been bounded and set. Mind commenting on what
> this is doing?

This scales the time_offset value so that later at every tick you add
only (time_offset >> SHIFT_HZ) and after HZ ticks time is changed by
time_offset and so makes the crude compensation later completely
unnecessary.

> > + if (time_offset < 0) {
> > + ltemp = -time_offset;
> > + if (!(time_status & STA_FLL))
> > + ltemp >>= SHIFT_KG + time_constant;
> > + if (ltemp > (((MAXPHASE / HZ) << SHIFT_HZ) / MINSEC))
> > + ltemp = ((MAXPHASE / HZ) << SHIFT_HZ) / MINSEC;
> > + time_offset += ltemp;
> > + tick_nsec_curr -= ltemp >> SHIFT_HZ;
> > + time_adj_curr -= (ltemp << (SHIFT_SCALE - SHIFT_HZ)) & (FINENSEC - 1);
> > + if (time_adj_curr < 0) {
> > + tick_nsec_curr--;
> > + time_adj_curr += FINENSEC;
> > + }
> > + } else {
> > + ltemp = time_offset;
> > + if (!(time_status & STA_FLL))
> > + ltemp >>= SHIFT_KG + time_constant;
> > + if (ltemp > (((MAXPHASE / HZ) << SHIFT_HZ) / MINSEC))
> > + ltemp = ((MAXPHASE / HZ) << SHIFT_HZ) / MINSEC;
> > + time_offset -= ltemp;
> > + tick_nsec_curr += ltemp >> SHIFT_HZ;
> > + time_adj_curr += (ltemp << (SHIFT_SCALE - SHIFT_HZ)) & (FINENSEC - 1);
> > + if (time_adj_curr >= FINENSEC) {
> > + tick_nsec_curr++;
> > + time_adj_curr -= FINENSEC;
> > + }
> > + }
>
> Again, more comments would help.
>
> Also, these big "if negative, lot of coding adding else lots of code
> subtracting" switches strike me as code duplication. It may not be as
> efficient, but would you consider re-working this to save off the sign,
> doing the math once and then applying the sign back? Parts of the above
> already do this. See also the shift_right() macro.

This part will change at some point anyway, when tick_nsec_curr/
time_adj_curr are merged to a single 64 bit value, until then I prefer to
keep it a bit more verbose, but also a bit more efficient.

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

\
 
 \ /
  Last update: 2006-01-12 14:03    [W:0.050 / U:0.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site