Re: [chrony-dev] [PATCH] MacOS X dynamic drift removal interval

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

On Mon, Aug 17, 2015 at 04:42:17PM +1200, Bryan Christianson wrote:
> Adjust the drift removal interval based on the observed offset_sd.
> A newly calculated interval goes into effect after the current drift removal has completed.
> When offset_sd is high, the interval is increased resulting in fewer wakeups to adjust the drift offset.
> At lower values of offset_sd the drift removal adjustment interval is pinned to 0.5 seconds.
> The predicted error applied at the start of an adjustment is based on the remaining time of the drift removal that is currently in effect.

This looks great. Just two comments.

> @@ -120,7 +138,14 @@ start_adjust(void)
>  
>    UTI_DiffTimevalsToDouble(&elapsed, &T1, &T0);
>    accrued_error = elapsed * current_freq;
> -  predicted_error = DRIFT_REMOVAL_INTERVAL / 2.0 * current_freq;
> +
> +  UTI_DiffTimevalsToDouble(&drift_removal_elapsed, &T1, &Tdrift);

If the clock was stepped recently, the Tdrift timestamp could be too
far from T1 and drift_removal_elapsed might be too large. I think it
would good to clamp it to [0, drift] here.

> +static void
> +set_sync_status(int synchronised, double est_error, double max_error)
> +{
> +  double interval;
> +  drift_removal_interval = DRIFT_REMOVAL_INTERVAL;

Should it be reset to DRIFT_REMOVAL_INTERVAL when the clock is no
longer synchronized? Why not keep it as it was before synchronization
was lost?

> +  if (!synchronised) {
> +    return;
> +  }
> +
> +  interval = ERROR_WEIGHT * est_error / (fabs(current_freq) + FREQUENCY_RES);
> +  drift_removal_interval = MAX(interval, DRIFT_REMOVAL_INTERVAL_MIN);

Thanks,

-- 
Miroslav Lichvar

-- 
To unsubscribe email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "unsubscribe" in the subject.
For help email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "help" in the subject.
Trouble?  Email listmaster@xxxxxxxxxxxxxxxxxxxx.