Re: [PATCH RFC] hv_utils: implement Hyper-V PTP source

[Thomas Gleixner]

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