Re: [patch 4/5] PTP: add PTP_SYS_OFFSET emulation via cross timestamps infrastructure

[Paolo Bonzini]

On 20/01/2017 15:02, Radim Krcmar wrote:
> 2017-01-20 14:36+0100, Paolo Bonzini:
>> On 20/01/2017 14:07, Marcelo Tosatti wrote:
>>> On Fri, Jan 20, 2017 at 01:55:27PM +0100, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 20/01/2017 13:20, Marcelo Tosatti wrote:
>>>>>  kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++
>>>>
>>>> Why not leave this in drivers/ptp/ptp_chardev.c?
>>>
>>> timekeeper_lock
>>
>> Why does emulate_ptp_sys_offset need it, if the current PTP_SYS_OFFSET
>> code doesn't?  Is the latency acceptable (considering this is a raw spin
>> lock) or is there a seqlock that we can use instead (such as tk_core.seq
>> like in get_device_system_crosststamp)?
> 
> The spinlock prevents writers to take the tk_core.seq, which means that
> time cannot be changed during that.
> 
> The simplest alternative would be to use tk_core.seq for all our reads,
> but that would increse the chance of re-reading, even infinitely.

How much?  If a hypercall takes 1 microsecond, and PTP_MAX_SAMPLES is
25, we should be done in less than 50 microseconds.  If update_wall-time
is called with 250 Hz frequency (sounds like a lot), that's still 4000
microseconds so the probability of even 3-4 consecutive retries should
be very low.

> But we don't need to read everything with the same time base -- if the
> time is changed (by NTP/user/...) between our reads, then the value will
> be off, but if writer took tk_core.seq just to accumulate current time,
> then the time after accumulation stays the same and it would work as if
> we had the tk_core.seq for the whole time.

You mean only check seqlock separately for each sample, but restart the
entire loop upon changes to cs_was_changed_seq or clock_was_set_seq?
That would work too.

> Another solution would be to do just one one read and set it to all
> saples -- the difference between t[i] and t[i+2] would be 0.  We are
> quite sure the just one read is enough, this hack could be even better.

I'd be afraid of messing up chrony's stats...

Paolo

>>>>> +		if (ptp->info->emulate_ptp_sys_offset_mean) {
>>>>> +			err = emulate_ptp_sys_offset(ptp->info, sysoff, arg);
>>>>> +			break;
>>>>> +		}
>>>>
>>>> I think this should be simply "if (!ptp->info->gettime64)" and,
>>>> likewise, there should be an emulation based getcrosststamp in
>>>> ptp_clock_gettime.
>>>>
>>>> Paolo
>>>
>>> gettime64 is called directly via ptp_clock_gettime.
>>
>> Yes, but ptp_clock_gettime can be taught to use getcrosststamp instead.
> 
> I agree,
> 
>   if (!gettime64)
>       use getcrosststamp
> 
> and KVM PTP device will not implement gettime64().
>