> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Tuesday, January 3, 2017 4:32 AM
> To: Alex Ng (LIS) <ale...@microsoft.com>
> Cc: de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; KY
> Srinivasan <k...@microsoft.com>; Haiyang Zhang <haiya...@microsoft.com>;
> John Stultz <john.stu...@linaro.org>; Thomas Gleixner <t...@linutronix.de>
> Subject: Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
>
> "Alex Ng (LIS)" <ale...@microsoft.com> writes:
>
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> >> Sent: Monday, January 2, 2017 11:41 AM
> >> To: de...@linuxdriverproject.org
> >> Cc: linux-ker...@vger.kernel.org; KY Srinivasan <k...@microsoft.com>;
> >> Haiyang Zhang <haiya...@microsoft.com>; John Stultz
> >> <john.stu...@linaro.org>; Thomas Gleixner <t...@linutronix.de>; Alex
> >> Ng
> >> (LIS) <ale...@microsoft.com>
> >> 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 <vkuzn...@redhat.com>
> >> ---
> >> 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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
