[ntp:questions] Re: Tinker and tos configuration commands....

David L. Mills mills at udel.edu
Mon Feb 7 15:27:37 UTC 2005


Brad,

Where are we going with this? You point out some comment 
inconsistencies. The code that leaves me is loaded with comments, some 
of which are fifteen years old. I don't guarantee in any sense of the 
word the accuracy of those comments, just good faith and intent. As for 
the limits checks, we are dealing with floating doubles here; the 
difference between inclusive and exclusive limits is something like 
10e-9. Big deal. As for maxdist, see the beginning of the clock_select 
routine. It is not the intent to set anything at startup and change 
later, but to set parameters that apply throughout the life of the program.

Can we get rid of the questions thing? The purpose is obscure and 
doesn't contribute to the discussion here.

Dave

Brad Knowles wrote:

> Folks,
> 
>     Apropos of the "tos" and "tinker" configuration commands, I've got 
> some questions.  Checking the contents of the 2005-02-06 snapshot, I 
> find the following code in ntpd/ntp_config.c starting at line 233:
> 
> /*
>  * "tos" modifier keywords
>  */
> static struct keyword tos_keywords[] = {
>         { "minclock",           CONF_TOS_MINCLOCK },
>         { "minsane",            CONF_TOS_MINSANE },
>         { "floor",              CONF_TOS_FLOOR },
>         { "ceiling",            CONF_TOS_CEILING },
>         { "cohort",             CONF_TOS_COHORT },
>         { "maxdist",            CONF_TOS_MAXDIST },
>         { "",                   CONFIG_UNKNOWN }
> };
> 
>     Starting at line 1044, we have:
> 
>                     case CONFIG_TOS:
>                         for (i = 1; i < ntokens; i++) {
>                             int temp;
>                             double ftemp;
> 
>                             temp = matchkey(tokens[i++], tos_keywords, 1);
>                             if (i > ntokens - 1) {
>                                 msyslog(LOG_ERR,
>                                     "tinker: missing argument");
>                                 errflg++;
>                                 break;
>                             }
> 
>     Shouldn't that "tinker" in the line above instead be "tos"?  It 
> looks to me like this section of code was cut-n-pasted from the code for 
> "tinker", but this example of the word wasn't changed.
> 
>     Anyway, MAXDIST is a new option that we can tweak, and is not 
> documented on the page at 
> <http://www.eecis.udel.edu/~mills/ntp/html/manyopt.html>.  Looking in 
> ntpd/ntp_proto.c, and following the trail through the header files, we 
> find that MAXDIST defaults to a value of 1.  Interestingly, we have this 
> code starting at line 3225:
> 
>         /*
>          * Set the cohort switch.
>          */
>         case PROTO_MAXDIST:
>                 sys_maxdist = dvalue;
>                 break;
> 
>         /*
>          * Set the cohort switch.
>          */
>         case PROTO_COHORT:
>                 sys_cohort = (int)dvalue;
>                 break;
> 
>     Another case of cut-n-paste without correction?  ;)
> 
>     Unfortunately, it's still not clear to me how MAXDIST affects the 
> startup process.  When Googling for "ntp" and "maxdist", the only 
> potentially useful hit I get is 
> <http://www.funet.fi/pub/unix/packages/ntp/dart.txt>, which turns out to 
> be an report from Dr. Mills in the early 1990s regarding DARTnet clock 
> synchronization, and the code there is about as illuminating to me as 
> the existing code.
> 
>     For me, this is a dead end.  Maybe someone who understands the code 
> better can explain it to me?
> 
> 
>     Okay, now let's take a look at tinker.  Looking at 
> ntpd/ntp_config.c, starting at line 219, we have the following:
> 
> /*
>  * "tinker" modifier keywords
>  */
> static struct keyword tinker_keywords[] = {
>         { "step",               CONF_CLOCK_MAX },
>         { "panic",              CONF_CLOCK_PANIC },
>         { "dispersion",         CONF_CLOCK_PHI },
>         { "stepout",            CONF_CLOCK_MINSTEP },
>         { "allan",              CONF_CLOCK_ALLAN },
>         { "huffpuff",           CONF_CLOCK_HUFFPUFF },
>         { "freq",               CONF_CLOCK_FREQ },
>         { "",                   CONFIG_UNKNOWN }
> };
> 
> 
>     Checking out ntpd/ntp_loopfilter.c, starting at line 876, we have 
> the code:
> 
>                 /*
>                  * If the frequency value is reasonable, set the initial
>                  * frequency to the given value and the state to S_FSET.
>                  * Otherwise, the drift file may be missing or broken,
>                  * so set the frequency to zero. This erases past
>                  * history should somebody break something.
>                  */
>                 if (freq <= NTP_MAXFREQ && freq >= -NTP_MAXFREQ) {
>                         drift_comp = freq;
>                         rstclock(S_FSET, 0, 0);
>                 } else {
>                         drift_comp = 0;
>                 }
> 
>     The way I'm reading that comment, these checks should be for strict 
> less-than and greater-than, so that we make sure to throw out any drift 
> contents that equal the maximum allowed drift values.  I mean, it makes 
> sense to check that you're within the minimum and maximum values, but 
> shouldn't you throw out the values at the boundaries?  Am I missing 
> something?
> 
>     Anyway, starting at line 907, we have:
> 
>         /*
>          * Special tinker variables for Ulrich Windl. Very dangerous.
>          */
>         case LOOP_MAX:                  /* step threshold */
>                 clock_max = freq;
>                 break;
> 
>         case LOOP_PANIC:                /* panic threshold */
>                 clock_panic = freq;
>                 break;
> 
>         case LOOP_PHI:                  /* dispersion rate */
>                 clock_phi = freq;
>                 break;
> 
>         case LOOP_MINSTEP:              /* watchdog bark */
>                 clock_minstep = freq;
>                 break;
> 
>         case LOOP_ALLAN:                /* Allan intercept */
>                 allan_xpt = freq;
>                 break;
> 
>         case LOOP_HUFFPUFF:             /* huff-n'-puff filter length */
>                 if (freq < HUFFPUFF)
>                         freq = HUFFPUFF;
>                 sys_hufflen = (int)(freq / HUFFPUFF);
>                 sys_huffpuff = (double *)emalloc(sizeof(double) *
>                     sys_hufflen);
>                 for (i = 0; i < sys_hufflen; i++)
>                         sys_huffpuff[i] = 1e9;
>                 sys_mindly = 1e9;
>                 break;
> 
>         case LOOP_FREQ:                 /* initial frequency */
>                 drift_comp = freq / 1e6;
>                 rstclock(S_FSET, 0, 0);
>                 break;
>         }
> 
>     Starting at line 30 of this file, we find:
> 
> /*
>  * This is an implementation of the clock discipline algorithm described
>  * in UDel TR 97-4-3, as amended. It operates as an adaptive parameter,
>  * hybrid phase/frequency-lock loop. A number of sanity checks are
>  * included to protect against timewarps, timespikes and general mayhem.
>  * All units are in s and s/s, unless noted otherwise.
>  */
> #define CLOCK_MAX       .128    /* default step threshold (s) */
> #define CLOCK_MINSTEP   900.    /* default stepout threshold (s) */
> #define CLOCK_PANIC     1000.   /* default panic threshold (s) */
> #define CLOCK_PHI       15e-6   /* max frequency error (s/s) */
> #define CLOCK_PLL       16.     /* PLL loop gain (log2) */
> #define CLOCK_FLL       (NTP_MAXPOLL + 1.) /* FLL loop gain */
> #define CLOCK_AVG       4.      /* parameter averaging constant */
> #define CLOCK_ALLAN     1500.   /* compromise Allan intercept (s) */
> #define CLOCK_DAY       86400.  /* one day in seconds (s) */
> #define CLOCK_LIMIT     30      /* poll-adjust threshold */
> #define CLOCK_PGATE     4.      /* poll-adjust gate */
> #define PPS_MAXAGE      120     /* kernel pps signal timeout (s) */
> 
> 
>     So, the only problem here appears to be that you can't modify the 
> "step" parameter only on startup, and leave it set to the normal value 
> for regular operations.  Correct?
> 



More information about the questions mailing list