From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [patch 4/5] PTP: add PTP_SYS_OFFSET emulation via cross timestamps infrastructure Date: Fri, 20 Jan 2017 15:23:08 +0100 Message-ID: <66e145c8-0e7c-5ae4-486b-385a058f7e05@redhat.com> References: <20170120122025.665985919@redhat.com> <20170120122503.746158230@redhat.com> <48bb2650-ed00-ec07-31bf-8780d3ab5568@redhat.com> <20170120130711.GA27440@amt.cnet> <2d213ad9-fa40-1f1e-90a9-404764969d35@redhat.com> <20170120140216.GA1358@potion> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: Marcelo Tosatti , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Richard Cochran , Miroslav Lichvar To: Radim Krcmar Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39034 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752121AbdATOX3 (ORCPT ); Fri, 20 Jan 2017 09:23:29 -0500 In-Reply-To: <20170120140216.GA1358@potion> Sender: kvm-owner@vger.kernel.org List-ID: 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(). >