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

[Bryan Christianson <bryan@xxxxxxxxxxxxx>]

> On 18/08/2015, at 12:43 am, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> 
> 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.

Thanks


> 
>> @@ -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.

OK - I had seen the interval go negative by a 100 nano seconds or so and wasn't sure if that was legitimate or not. It is caused by delays in the scheduler which seems to always start the removal process slightly later than expected. In fact I was wondering if we should account for that delay and make the next interval slightly shorter to compensate, but clamping to 0 achieves that same end a lot more simply.

> 
>> +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?


Initially I had DRIFT_REMOVAL_INTERVAL_MIN = 0.1 and my thinking was to minimise the wakeups if connectivity was lost. I think having raised the minimum to 0.5 secs this is no longer a problem so I'll remove it.

I've made both changes and will let it run overnight (1:00 am here at the moment) and resubmit the patch tomorrow morning.

-- 
Bryan Christianson


--
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.