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

["Jörg-Volker Peetz" <jvpeetz@web.de>]

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.