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 de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel