2007-01-10 13:28:50

by Bernhard Schiffner

[permalink] [raw]
Subject: ntp.c : possible inconsistency?

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;


2007-01-10 14:30:49

by Roman Zippel

[permalink] [raw]
Subject: Re: ntp.c : possible inconsistency?

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

2007-01-10 15:29:51

by Bernhard Schiffner

[permalink] [raw]
Subject: Re: ntp.c : possible inconsistency?

> 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

2007-01-15 12:43:29

by Roman Zippel

[permalink] [raw]
Subject: Re: ntp.c : possible inconsistency?

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