Re: [PATCH] correct inconsistent ntp interval/tick_length usage

Jörg-Volker Peetz
2008-02-29T19:00:23Z

john stultz wrote:
<snip>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index c88b591..fe25c94 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -43,19 +43,32 @@ long time_freq; /* frequency offset (scaled ppm)*/
> static long time_reftime; /* time at last adjustment (s) */
> long time_adjust;
>
> +static s64 granularity_error_adjust;
> +
> +void ntp_set_granularity_error(s64 len)
> +{
> + granularity_error_adjust = len * NTP_INTERVAL_FREQ;
> +}
> +
> static void ntp_update_frequency(void)
> {
> u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
> << TICK_LENGTH_SHIFT;
> - second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
> - second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
> + s64 adj;
> +
> + /* Compensate for clocksource granularity error */
> + second_length += granularity_error_adjust;
> +
> + /* Scale the base second length by the frequency adjustment */
> + adj = second_length * time_freq;
> + do_div(adj, 1000000);
> + second_length += adj>>SHIFT_NSEC;
>
> tick_length_base = second_length;
> + do_div(tick_length_base, NTP_INTERVAL_FREQ);
>
> do_div(second_length, HZ);
> tick_nsec = second_length >> TICK_LENGTH_SHIFT;
> -
> - do_div(tick_length_base, NTP_INTERVAL_FREQ);
> }
>
> /**
<snip>

Hi John,

out of curiosity and inspired by the patch you suggested, I did a test
with the following ntp_update_frequency function in kernel/time/ntp.c
of kernel 2.6.24.3 using NO_HZ and the hpet timer:

static void ntp_update_frequency(void)
{
s64 adj;
u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
<< TICK_LENGTH_SHIFT;

printk(KERN_NOTICE "*ntp* timefreq = %lld\n", (s64)time_freq);
printk(KERN_NOTICE "*ntp* s len = %lld\n", second_length);

printk(KERN_NOTICE "*ntp* corr 1 = %lld\n",
(s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC));

/* Scale the base second length by the frequency adjustment */
adj = second_length * time_freq;
do_div(adj, 1000000);
printk(KERN_NOTICE "*ntp* corr 2 = %lld\n", adj>>SHIFT_NSEC);

second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);

tick_length_base = second_length;
do_div(tick_length_base, NTP_INTERVAL_FREQ);

do_div(second_length, HZ);
tick_nsec = second_length >> TICK_LENGTH_SHIFT;
}

Running this kernel and ntpd I get numbers like the following in my
syslog file:

Feb 29 12:28:16 skadi kernel: *ntp* timefreq = 305345
Feb 29 12:28:16 skadi kernel: *ntp* s len = 4294967296000000000
Feb 29 12:28:16 skadi kernel: *ntp* corr 1 = 320177438720
Feb 29 12:28:16 skadi kernel: *ntp* corr 2 = 3030411349
Feb 29 12:36:53 skadi kernel: *ntp* timefreq = 730456
Feb 29 12:36:53 skadi kernel: *ntp* s len = 4294967296000000000
Feb 29 12:36:53 skadi kernel: *ntp* corr 1 = 765938630656
Feb 29 12:36:53 skadi kernel: *ntp* corr 2 = 2434829845
Feb 29 12:39:03 skadi kernel: *ntp* timefreq = 868771
Feb 29 12:39:03 skadi kernel: *ntp* s len = 4294967296000000000
Feb 29 12:39:03 skadi kernel: *ntp* corr 1 = 910972420096
Feb 29 12:39:03 skadi kernel: *ntp* corr 2 = 2301870005

So the original correction and the correction suggested by you
differ significantly by a factor of approximately 1000.
Incidentally, both corrections are nearly neglectable compared to
second_length.
But I think the correction suggested by you is calculated wrong due to
an overflow of the multiplication

second_length * time_freq

Calculating the correction as

(second_length / 1000000) * time_freq

the correction would be 1311446788997120000 which is much bigger as
the original correction 320177438720 (by a factor of 10^7).

Is it normal to have a second_length of approx. 4 * 10^18 ?
In what units?
--
Regards,
Jörg-Volker.