ntp.c : possible inconsistency?

ntp.c : possible inconsistency?

[Bernhard Schiffner]

Hi,

trying to find reasons for some crazy ntpd-behavior I read
http://lkml.org/lkml/2006/10/27/67

This thread doesn't result in a pulished patch, so I (hopefully) did what was 
said there. The patch doesn't break my system, but it doesn't change ntpd's 
crazyness too.
Nevertheless it should be discussed again in the sense of preventing an 
inconsistency.

Bernhard


PS:
Can someone point me to the reason for doing txc->constant + 4, please?

References:
lkml-thread (start)
http://lkml.org/lkml/2006/10/26/47

ntpd-crazyness (comparing 2.6.15 with 2.6.19)
http://ml.enneenne.com/pipermail/linuxpps/2006-December/000482.html

Patch:
index 3afeaa3..36d7ecc 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -263,7 +263,7 @@ int do_adjtimex(struct timex *txc)
                    result = -EINVAL;
                    goto leave;
                }
-               time_constant = min(txc->constant + 4, (long)MAXTC);
+               time_constant = min(txc->constant + 4, (long)MAXTC + 4);
            }

            if (txc->modes & ADJ_OFFSET) {      /* values checked earlier */
@@ -329,7 +329,7 @@ leave:      if ((time_status & (STA_UNSYNC|
STA_CLOCKERR)) != 0)
        txc->maxerror      = time_maxerror;
        txc->esterror      = time_esterror;
        txc->status        = time_status;
-       txc->constant      = time_constant;
+       txc->constant      = time_constant - 4;
        txc->precision     = 1;
        txc->tolerance     = MAXFREQ;
        txc->tick          = tick_usec;

Re: ntp.c : possible inconsistency?

[Roman Zippel]

Hi,

On Wed, 10 Jan 2007, Bernhard Schiffner wrote:

> trying to find reasons for some crazy ntpd-behavior I read
> http://lkml.org/lkml/2006/10/27/67
> 
> This thread doesn't result in a pulished patch, so I (hopefully) did what was 
> said there. The patch doesn't break my system, but it doesn't change ntpd's 
> crazyness too.
> Nevertheless it should be discussed again in the sense of preventing an 
> inconsistency.

Without a further explanation of this craziness, it's a little hard to 
discuss...

> PS:
> Can someone point me to the reason for doing txc->constant + 4, please?

The main reason is this part in ntpd/ntp_loopfilter.c of the ntp package:

        if (pll_nano) {
                ntv.offset = (int32)(clock_offset *
                    1e9 + dtemp);
                ntv.constant = sys_poll;
        } else {
                ntv.offset = (int32)(clock_offset *
                    1e6 + dtemp);
                ntv.constant = sys_poll - 4;
        }

It uses the pll_nano switch to distinguish between the old and new ntp 
model. The kernel now implements the new ntp model, but the actual 
userspace interface hasn't changed yet, so the kernel undoes the 
compensation.

Here is bit more information about where this adjustments comes from:
http://www.ntp.org/ntpfaq/NTP-s-compat.htm#Q-COMPAT

bye, Roman

Re: ntp.c : possible inconsistency?

[Bernhard Schiffner]

> Without a further explanation of this craziness, it's a little hard to
> discuss...
Let's try it:
time_constant is created for internal use of ntp.c and added by 4
-               time_constant = min(txc->constant + 4, (long)MAXTC);
+               time_constant = min(txc->constant + 4, (long)MAXTC + 4);

But sometimes it is written back to data referenced from outside. So let's do 
the + 4 backwards ...
-       txc->constant      = time_constant;
+       txc->constant      = time_constant - 4;

otherwise I'd like to speak of an inconsistency. It doesn't depend of actual 
use and other topics.

Bernhard

Re: ntp.c : possible inconsistency?

[Roman Zippel]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 818 bytes --]

Hi,

On Wed, 10 Jan 2007, Bernhard Schiffner wrote:

> > Without a further explanation of this craziness, it's a little hard to
> > discuss...
> Let's try it:
> time_constant is created for internal use of ntp.c and added by 4
> -               time_constant = min(txc->constant + 4, (long)MAXTC);
> +               time_constant = min(txc->constant + 4, (long)MAXTC + 4);

MAXTC is already adjusted.

> But sometimes it is written back to data referenced from outside. So let's do 
> the + 4 backwards ...
> -       txc->constant      = time_constant;
> +       txc->constant      = time_constant - 4;

ntpd doesn't read it back for it's own purposes, it only prints it, when 
the kernel info is queried, it doesn't adjust the constant there, so I 
didn't do it in the kernel either.

bye, Roman