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: Tuesday, January 3, 2017 4:32 AM
> To: Alex Ng (LIS) <alexng@xxxxxxxxxxxxx>
> Cc: devel@xxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; KY
> Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>;
> John Stultz <john.stultz@xxxxxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Subject: Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
> 
> "Alex Ng (LIS)" <alexng@xxxxxxxxxxxxx> writes:
> 
> >> -----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.
> >
> 
> Good, this can happen in parallel to my series, right?

Yes, that is correct.

> 
> >>
> >> 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).
> 
> Ok, point taken, will do in v2. We don't get ICTIMESYNCFLAG_SYNC very
> often, right?

This is correct. SYNC flags are sent rarely and usually only after a guest has
been resumed from a pause.

> 
> >
> >> +
> >> +	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?
> 
> Zero means no frequency adjustment required (we reset it in case it was
> previously made by an NTP client).
> 
> >
> >> +	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.
> 
> I can add a debug message here but as this is a regular action we don't want
> to get a flood of messages in case this fails permanently. I'd avoid printing
> info messages here.
> 

Agree. A debug level message is reasonable.

> >
> >>  }
> >>
> >>  /*
> >> --
> >> 2.9.3
> 
> --
>   Vitaly