On Sun, Dec 04, 2016 at 10:56:57PM -0800, Denny Page wrote:
> I’m still chasing the offset issue. However on the spikes/noise issue, the 
> patch below addresses the noise and low level spikes that I was seeing. I was 
> seeing errors of 2-60ns off in the average offset for the nic clock, which 
> this corrects.

If I understand your change correctly, it increases the minimum
acceptable delay, which increases the number of combined readings and
makes the offset more stable. I'm worried on a busy system bad
readings could be included and disrupt the mean value. Do you know
what is a typical precision of a PHC? To me it would make more sense
to use something like "min_delay + sys_precision + phc_precision"
instead of "min_delay * 1.1". In any case, I guess we could use a
median instead of mean to discard bad readings if a small number of
them get below that minimum delay. I'll try that.

Note that if you save the offset between PHC and system clock to a
double, you will lose precision when the offset is large as double has
a 53-bit precision.

> *** ntp_io_linux.c.org  Wed Nov 23 08:41:54 2016
> --- ntp_io_linux.c      Sun Dec  4 22:32:45 2016
> ***************
> *** 288,295 ****
>   {
>     struct ptp_sys_offset sys_off;
>     struct timespec ts1, ts2, ts3, phc_tss[PHC_READINGS], 
> sys_tss[PHC_READINGS];
> !   double min_delay = 0.0, delays[PHC_READINGS], phc_sum, local_sum, 
> local_prec;
> !   int i, n;
>   
>     /* Silence valgrind */
>     memset(&sys_off, 0, sizeof (sys_off));
> --- 288,295 ----
>   {
>     struct ptp_sys_offset sys_off;
>     struct timespec ts1, ts2, ts3, phc_tss[PHC_READINGS], 
> sys_tss[PHC_READINGS];
> !   double delays[PHC_READINGS], delay_limit, delay_sum, offset_sum;
> !   int min_delay = 0, i, n;
>   
>     /* Silence valgrind */
>     memset(&sys_off, 0, sizeof (sys_off));
> ***************
> *** 317,344 ****
>         /* Step in the middle of a PHC reading? */
>         return 0;
>   
> !     if (!i || delays[i] < min_delay)
> !       min_delay = delays[i];
>     }
>   
> -   local_prec = LCL_GetSysPrecisionAsQuantum();
> - 
>     /* Combine best readings */
> !   for (i = n = 0, phc_sum = local_sum = 0.0; i < PHC_READINGS; i++) {
> !     if (delays[i] > min_delay + local_prec)
>         continue;
>   
> !     phc_sum += UTI_DiffTimespecsToDouble(&phc_tss[i], &phc_tss[0]);
> !     local_sum += UTI_DiffTimespecsToDouble(&sys_tss[i], &sys_tss[0]) + 
> delays[i] / 2.0;
>       n++;
>     }
>   
>     assert(n);
>   
> !   UTI_AddDoubleToTimespec(&phc_tss[0], phc_sum / n, phc_ts);
> !   UTI_AddDoubleToTimespec(&sys_tss[0], local_sum / n, &ts1);
>     LCL_CookTime(&ts1, local_ts, NULL);
> !   *p_delay = min_delay;
>   
>     return 1;
>   }
> --- 317,345 ----
>         /* Step in the middle of a PHC reading? */
>         return 0;
>   
> !     if (delays[i] < delays[min_delay])
> !       min_delay = i;
>     }
>   
>     /* Combine best readings */
> !   delay_limit = delays[min_delay] * 1.1;
> !   for (i = n = 0, delay_sum = offset_sum = 0.0; i < PHC_READINGS; i++) {
> !     if (delays[i] > delay_limit)
>         continue;
>   
> !     delay_sum += delays[i];
> !     UTI_AddDoubleToTimespec(&sys_tss[i], delays[i] / 2.0, &ts1);
> !     offset_sum += UTI_DiffTimespecsToDouble(&phc_tss[i], &ts1);
>       n++;
>     }
>   
>     assert(n);
>   
> !   ts1 = sys_tss[min_delay];
> !   UTI_AddDoubleToTimespec(&ts1, offset_sum / n, phc_ts);
> ! 
>     LCL_CookTime(&ts1, local_ts, NULL);
> !   *p_delay = delays[min_delay];
>   
>     return 1;
>   }
> 
> 
> --
> To unsubscribe email chrony-dev-requ...@chrony.tuxfamily.org with 
> "unsubscribe" in the subject.
> For help email chrony-dev-requ...@chrony.tuxfamily.org with "help" in the 
> subject.
> Trouble?  Email listmas...@chrony.tuxfamily.org.
> 

-- 
Miroslav Lichvar

-- 
To unsubscribe email chrony-dev-requ...@chrony.tuxfamily.org with "unsubscribe" 
in the subject.
For help email chrony-dev-requ...@chrony.tuxfamily.org with "help" in the 
subject.
Trouble?  Email listmas...@chrony.tuxfamily.org.

Reply via email to