Re: [PATCH 1/5] ntp: add hardpps implementation

[Alexander Gordeev <lasaine@lvk.cs.msu.su>]

[-- Attachment #1: Type: text/plain, Size: 8878 bytes --]

Thanks for the review!

В Wed, 03 Feb 2010 14:08:50 -0800
john stultz <johnstul@us.ibm.com> пишет:

> On Wed, 2010-02-03 at 23:56 +0300, Alexander Gordeev wrote:
> > This commit adds hardpps() implementation based upon the original
> > one from the NTPv4 reference kernel code. However, it is highly
> > optimized towards very fast syncronization and maximum stickness to
> > PPS signal. The typical error is less then a microsecond.
> > To make it sync faster I had to throw away exponential phase filter
> > so that the full phase offset is corrected immediately. Then I also
> > had to throw away median phase filter because it gives a bigger
> > error itself if used without exponential filter.
> > Maybe we will find an appropriate filtering scheme in the future but
> > it's not necessary if the signal quality is ok.
> > 
> > Signed-off-by: Alexander Gordeev <lasaine@lvk.cs.msu.su>
> 
> Very cool! Bunch of style comments below.
> 
> One other thing: From the comments in the code, it looks like this may
> have come straight from the reference implementation. You might want
> to provide some extra documentation or comment providing proper
> credit, and clarifying that the code is in fact GPLv2 compatible. 
> 
> Also please review Section 12 of Documentation/SubmittingPatches 

Indeed, this code is a direct descendant of the reference
implementation. However it is mostly rewritten because the original
code was too hard to read. But I thought it is not an issue because the
whole ntp.c seems to have a lot of code from it... Well, I'm not quite
good in license compatibility. :)

Here is the copyright notice from the original code for reference:
/***********************************************************************
 *                                                                     *
 * Copyright (c) David L. Mills 1993-2001                              *
 *                                                                     *
 * Permission to use, copy, modify, and distribute this software and   *
 * its documentation for any purpose and without fee is hereby         *
 * granted, provided that the above copyright notice appears in all    *
 * copies and that both the copyright notice and this permission       *
 * notice appear in supporting documentation, and that the name        *
 * University of Delaware not be used in advertising or publicity      *
 * pertaining to distribution of the software without specific,        *
 * written prior permission. The University of Delaware makes no       *
 * representations about the suitability this software for any         *
 * purpose. It is provided "as is" without express or implied          *
 * warranty.                                                           *
 *                                                                     *
 **********************************************************************/

Maybe adding a comment like the one above second_overflow() is enough?


> [snip]
> > +long pps_tf[3];			/* phase median filter */
> > +s64 pps_freq;			/* scaled frequency offset
> > (ns/s) */ +s64 pps_fcount;			/* frequency
> > accumulator */ +long pps_jitter;		/* nominal jitter
> > (ns) */ +long pps_stabil;		/* nominal stability
> > (scaled ns/s) */ +int pps_valid;			/* signal
> > watchdog counter */ +int pps_shift;			/*
> > nominal interval duration (s) (shift) */ +int
> > pps_shiftmax;		/* max interval duration (s) (shift)
> > */ +int pps_intcnt;			/* interval counter */ +
> > +/*
> > + * PPS signal quality monitors
> > + */
> > +long pps_calcnt;		/* calibration intervals */
> > +long pps_jitcnt;		/* jitter limit exceeded */
> > +long pps_stbcnt;		/* stability limit exceeded */
> > +long pps_errcnt;		/* calibration errors */
> > +
> 
> Shouldn't all of the above values be made static?

Sure, thanks.

> [snip]
> 
> >  /*
> > @@ -233,8 +277,6 @@ static enum hrtimer_restart
> > ntp_leap_second(struct hrtimer *timer) */
> >  void second_overflow(void)
> >  {
> > -	s64 delta;
> > -
> >  	/* Bump the maxerror field */
> >  	time_maxerror += MAXFREQ / NSEC_PER_USEC;
> >  	if (time_maxerror > NTP_PHASE_LIMIT) {
> > @@ -248,9 +290,27 @@ void second_overflow(void)
> >  	 */
> >  	tick_length	 = tick_length_base;
> > 
> > -	delta		 = shift_right(time_offset, SHIFT_PLL
> > + time_constant);
> > -	time_offset	-= delta;
> > -	tick_length	+= delta;
> > +#ifdef CONFIG_NTP_PPS
> > +	if (time_status & STA_PPSTIME && time_status &
> > STA_PPSSIGNAL) {
> > +		tick_length	+= time_offset;
> > +		time_offset	 = 0;
> > +	} else
> > +#endif /* CONFIG_NTP_PPS */
> > +	{
> > +		s64 delta;
> > +		delta		 =
> > +			shift_right(time_offset, SHIFT_PLL +
> > time_constant);
> > +		time_offset	-= delta;
> > +		tick_length	+= delta;
> > +	}
> 
> 
> Ugh. Surely there's a better way to do this then using the ifdefs in
> code? 
> 
> Maybe something like:
> 
> #ifdef CONFIG_NTP_PPS
> /* Comment about how pps consumes the whole offset */
> static inline s64 ntp_offset_chunk(s64 offset)
> {
> 	if (time_status & STA_PPSTIME && time_status & STA_PPSSIGNAL)
> 		return offset
> 	else
> 		return shift_right(offset, SHIFT_PLL + time_constant);
> }
> #else
> static inline s64 ntp_offset_chunk(s64 offset)
> {
> 	return shift_right(offset, SHIFT_PLL + time_constant);
> }
> #endif
> 
> Then in second_overflow():
> 
> delta		 = ntp_offset_chunk(time_offset);
> time_offset	-= delta;
> tick_length	+= delta;
> 
> Feel free to use a better name then ntp_offset_chunk(), but this keeps
> the logic from being obfuscated by all the #ifdefs
>
> > +#ifdef	CONFIG_NTP_PPS
> > +	if (pps_valid > 0)
> > +		pps_valid--;
> > +	else
> > +		time_status &= ~(STA_PPSSIGNAL | STA_PPSJITTER |
> > +				 STA_PPSWANDER | STA_PPSERROR);
> > +#endif /* CONFIG_NTP_PPS */
> 
> Similarly, can some sort of pps_dec_valid() inline function be used
> here?

Ok, it is better indeed.

> [snip]
> 
> > +#ifdef	CONFIG_NTP_PPS
> > +
> > +/* normalize the timestamp so that nsec is in the
> > +   ( -NSEC_PER_SEC / 2, NSEC_PER_SEC / 2 ] interval */
> > +static inline struct timespec pps_normalize_ts(struct timespec ts)
> > +{
> > +	if (ts.tv_nsec > (NSEC_PER_SEC >> 1)) {
> > +		ts.tv_nsec -= NSEC_PER_SEC;
> > +		ts.tv_sec++;
> > +	}
> > +
> > +	return ts;
> > +}
> 
> Hrmm.. So by converting a timespec into a second + [-NSEC_PER_SEC/2,
> NSEC_PER_SEC/2] value, you aren't really using a timespec anymore,
> right? I mean, the timepsec_valid() would fail in many cases, for
> instance.
> 
> I realize its pretty close, and you *can* use a timespec this way, but
> to me it seems reasonable to introduce a new structure (pps_normtime?)
> so that its clear you shouldn't exchange timespec values and
> pps_normtime values directly?
> 
> This is sort of a nit, and maybe others disagree, so not the most
> critical issue. But it seems like you're abusing the structure a bit.

Well, I don't mind adding a structure for this. Seems like timespec
is not expected to be used this way.

> [snip]
> > +/* decrease frequency calibration interval length */
> > +static inline void pps_dec_freq_interval(void)
> > +{
> > +	if (--pps_intcnt <= -4) {
> > +		pps_intcnt = -4;
> 
> Is -4 a magic value?
> 
> [snip]
> > +	} else {	/* good sample */
> > +		if (++pps_intcnt >= 4) {
> > +			pps_intcnt = 4;
> 
> Again, the magic 4?

Yep :)
The values are from the reference implementation. Frequency calculation
is mostly the same as in the original code because it works good.
I'll add a define for these values.

> [snip]
> > +	pps_stabil += (div_s64(((s64)delta_mod) <<
> > +				(NTP_SCALE_SHIFT - SHIFT_USEC),
> > +				NSEC_PER_USEC) - pps_stabil) >>
> > PPS_FAVG;
> 
> Hmm. Two 64bit divides in this path? This code would run each second,
> right? It would probably be good to see if this could be optimized a
> little bit more, but I guess its similar to ntp_update_frequency()
> which is called on adjtimex() calls (every 16 seconds at most with
> networked ntp).

Well, this is not quite right. The interval is at least 4 seconds (1
<< PPS_FAVG) and increases if the signal is good. Maximum interval
length is 256 seconds (1 << PPS_FAVGDEF).
I don't see how it can be optimized right now.

> > +void hardpps(const struct timespec *p_ts, s64 nsec)
> > +{
> > +	struct timespec ts_norm, freq_norm;
> > +
> > +	ts_norm = pps_normalize_ts(*p_ts);
> > +	freq_norm = pps_normalize_ts(ns_to_timespec(nsec));
> > +
> [snip]
> > +
> > +	write_seqlock_irq(&xtime_lock);
> 
> This is called possibly in irq context, right? So you probably want to
> use write_seqlock_irqsave(), no?

Right, thanks, it's bug. However, I think of moving it to a worqueue.

-- 
  Alexander

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]