lkml.org 
[lkml]   [2017]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
Date
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, January 3, 2017 4:32 AM
> To: Alex Ng (LIS) <alexng@microsoft.com>
> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; KY
> Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>;
> John Stultz <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>
> Subject: Re: [PATCH 3/4] hv_util: use do_adjtimex() to update system time
>
> "Alex Ng (LIS)" <alexng@microsoft.com> writes:
>
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Monday, January 2, 2017 11:41 AM
> >> To: devel@linuxdriverproject.org
> >> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> >> Haiyang Zhang <haiyangz@microsoft.com>; John Stultz
> >> <john.stultz@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Alex
> >> Ng
> >> (LIS) <alexng@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 <vkuznets@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

\
 
 \ /
  Last update: 2017-01-03 20:49    [W:0.429 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site