On Fri, 13 Jan 2017, Vitaly Kuznetsov wrote:
> 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 server runs in parallel and uses something else for time
> sync (network, PTP,...) system time will never converge.
> - Systemd starts annoying you by printing "Time has been changed" every 5
> seconds to the system log.
>
> Instead of doing in-kernel time adjustments offload the work to an
> NTP client by exposing TimeSync messages as a PTP device. Users my now
> decide what they want to use as a source.
>
> I tested the solution with chrony, the config was:
>
> refclock PHC /dev/ptp0 poll 3 precision 1e-9
>
> The result I'm seeing is accurate enough, the time delta between the guest
> and the host is almost always within [-10us, +10us], the in-kernel solution
> was giving us comparable results.
>
> I also tried implementing PPS device instead of PTP by using not currently
> used Hyper-V synthetic timers (we use only one of four for clockevent) but
> with PPS source only chrony wasn't able to give me the required accuracy,
> the delta often more that 100us.
Makes sense. The PTP based solution is really nice!
> static void hv_set_host_time(struct work_struct *work)
> {
> struct adj_time_work *wrk;
> - s64 host_tns;
> u64 newtime;
> struct timespec64 host_ts;
Just a nitpick. Ordering variables in reverse fir tree (length) order:
struct adj_time_work *wrk;
struct timespec64 host_ts;
u64 newtime;
makes is simpler to parse
> +
> +static struct {
> + u64 host_time;
> + u64 ref_time;
> + spinlock_t lock;
> +} host_ts;
Another formatting nit. If you arrange the members in tabular fashion it
becomes simpler to parse:
static struct {
u64 host_time;
u64 ref_time;
spinlock_t lock;
} host_ts;
Also the struct might do with some comment explaning that it is the storage
for the PTP machinery,
> +static inline void adj_guesttime(u64 hosttime, u64 reftime, u8 adj_flags)
> {
> + unsigned long flags;
>
> /*
> * This check is safe since we are executing in the
> * interrupt context and time synch messages arre always
> * delivered on the same CPU.
> */
> - if (work_pending(&wrk.work))
> - return;
> + if (adj_flags & ICTIMESYNCFLAG_SYNC) {
> + if (work_pending(&wrk.work))
> + return;
>
> - wrk.host_time = hosttime;
> - wrk.ref_time = reftime;
> - wrk.flags = flags;
> - if ((flags & (ICTIMESYNCFLAG_SYNC | ICTIMESYNCFLAG_SAMPLE)) != 0) {
> + wrk.host_time = hosttime;
> + wrk.ref_time = reftime;
> + wrk.flags = adj_flags;
> schedule_work(&wrk.work);
> + } else {
> + spin_lock_irqsave(&host_ts.lock, flags);
> + host_ts.host_time = hosttime;
> +
> + if (ts_srv_version <= TS_VERSION_3)
> + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, host_ts.ref_time);
I'm confused here. The reftime / hosttime pair is accurate at sampling time
on the host. So why reading the MSR here? I'm certainly missing something,
but then this wants to have a comment like the other one in
get_timeadj_latency().
> + else
> + host_ts.ref_time = reftime;
> + spin_unlock_irqrestore(&host_ts.lock, flags);
> }
> }
Other than that: Nice work!
Thanks,
tglx
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
