RE: [PATCH 3/4] hv_util: use do_adjtimex() to update system time

[Alex Ng (LIS)]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx]
> Sent: Monday, January 2, 2017 11:41 AM
> To: devel@xxxxxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; John Stultz
> <john.stultz@xxxxxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Alex Ng
> (LIS) <alexng@xxxxxxxxxxxxx>
> Subject: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
> 
> With TimeSync version 4 protocol support we started updating system time
> continuously through the whole lifetime of Hyper-V guests. Every 5 seconds
> there is a time sample from the host which triggers do_settimeofday[64]().
> While the time from the host is very accurate such adjustments may cause
> issues:
> - Time is jumping forward and backward, some applications may misbehave.
> - In case an NTP client is run in parallel things may go south, e.g. when
>   an NTP client tries to adjust tick/frequency with ADJ_TICK/ADJ_FREQUENCY
>   the Hyper-V module will not see this changes and time will oscillate and
>   never converge.
> - Systemd starts annoying you by printing "Time has been changed" every 5
>   seconds to the system log.

These are all good points. I am working on a patch to address point 2.
It will allow new TimeSync behavior to be disabled even if the TimeSync IC is
enabled from the host. This can be set to prevent TimeSync IC from interfering
with NTP client.

> 
> Instead of calling do_settimeofday64() we can pretend being an NTP client
> and use do_adjtimex().
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
>  drivers/hv/hv_util.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
> 94719eb..4c0fbb0 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -182,9 +182,10 @@ struct adj_time_work {  static void
> hv_set_host_time(struct work_struct *work)  {
>  	struct adj_time_work	*wrk;
> -	s64 host_tns;
> +	s64 host_tns, our_tns, delta;
>  	u64 newtime;
> -	struct timespec64 host_ts;
> +	struct timespec64 host_ts, our_ts;
> +	struct timex txc = {0};
> 
>  	wrk = container_of(work, struct adj_time_work, work);
> 
> @@ -205,7 +206,25 @@ static void hv_set_host_time(struct work_struct
> *work)
>  	host_tns = (newtime - WLTIMEDELTA) * 100;
>  	host_ts = ns_to_timespec64(host_tns);
> 
> -	do_settimeofday64(&host_ts);
> +	getnstimeofday64(&our_ts);
> +	our_tns = timespec64_to_ns(&our_ts);
> +
> +	/* Difference between our time and host time */
> +	delta = host_tns - our_tns;
> +
> +	/* Try adjusting time by using phase adjustment if possible */
> +	if (abs(delta) > MAXPHASE) {
> +		do_settimeofday64(&host_ts);
> +		return;
> +	}

We should also call do_settimeofday64() if the host sends flag
ICTIMESYNCFLAG_SYNC. This is a signal from host that the guest
shall sync with host time immediately (often when the guest has
just booted).

> +
> +	txc.modes = ADJ_TICK | ADJ_FREQUENCY | ADJ_OFFSET |
> ADJ_NANO |
> +		ADJ_STATUS;
> +	txc.tick = TICK_USEC;
> +	txc.freq = 0;

I'm not familiar with the ADJ_FREQUENCY flag. What does setting this to 'zero' achieve?
Are there any side-effects from doing this?

> +	txc.status = STA_PLL;
> +	txc.offset = delta;
> +	do_adjtimex(&txc);

Might be a good idea to handle the return code from do_adjtimex() and log something
in case of error.

>  }
> 
>  /*
> --
> 2.9.3