From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758026AbZBDTpB (ORCPT ); Wed, 4 Feb 2009 14:45:01 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754604AbZBDTot (ORCPT ); Wed, 4 Feb 2009 14:44:49 -0500 Received: from e2.ny.us.ibm.com ([32.97.182.142]:46138 "EHLO e2.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754325AbZBDTor (ORCPT ); Wed, 4 Feb 2009 14:44:47 -0500 Subject: Re: [PATCH NET-NEXT 02/10] time sync: generic infrastructure to map between time stamps generated by a time counter and system time From: john stultz To: Patrick Ohly Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, David Miller , Thomas Gleixner In-Reply-To: <1233752517-30010-3-git-send-email-patrick.ohly@intel.com> References: <1233752517-30010-1-git-send-email-patrick.ohly@intel.com> <1233752517-30010-2-git-send-email-patrick.ohly@intel.com> <1233752517-30010-3-git-send-email-patrick.ohly@intel.com> Content-Type: text/plain Date: Wed, 04 Feb 2009 11:44:40 -0800 Message-Id: <1233776680.6994.30.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2009-02-04 at 14:01 +0100, Patrick Ohly wrote: > Mapping from time counter to system time is implemented. This is sufficient to use > this code in a network device driver which wants to support hardware time stamping > and transformation of hardware time stamps to system time. > > The interface could have been made more versatile by not depending on a time counter, > but this wasn't done to avoid writing glue code elsewhere. > > The method implemented here is the one used and analyzed under the name > "assisted PTP" in the LCI PTP paper: > http://www.linuxclustersinstitute.org/conferences/archive/2008/PDF/Ohly_92221.pdf > --- > include/linux/clocksync.h | 102 +++++++++++++++++++++++ > kernel/time/Makefile | 2 +- > kernel/time/clocksync.c | 197 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 300 insertions(+), 1 deletions(-) > create mode 100644 include/linux/clocksync.h > create mode 100644 kernel/time/clocksync.c I think my main critique of this somewhat trivial, but still important, as confusion is common in this area. I sort of object to the name clocksync, as you're not really doing anything to sync clocks in the code. One, "clock" is an way overloaded term in the kernel. Two, you're really seem to be just providing deltas and skew rates between notions of time. I want to avoid someone thinking "Oh, NTP must use this code". So maybe something like timecompare.c? If this code is really PTP purposed, maybe ptp should be in the name? > diff --git a/include/linux/clocksync.h b/include/linux/clocksync.h > new file mode 100644 > index 0000000..07c0cc1 > --- /dev/null > +++ b/include/linux/clocksync.h > @@ -0,0 +1,102 @@ > +/* > + * Utility code which helps transforming between hardware time stamps > + * generated by a clocksource and system time. The clocksource is > + * assumed to return monotonically increasing time (but this code does > + * its best to compensate if that is not the case) whereas system time > + * may jump. You're not using clocksources here anymore, right? Probably needs an update. > + * > + * Copyright(c) 2009 Intel Corporation. > + * Author: Patrick Ohly > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. * See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA. > + */ > +#ifndef _LINUX_CLOCKSYNC_H > +#define _LINUX_CLOCKSYNC_H > + > +#include > +#include > + > +/** > + * struct clocksync - stores state and configuration for the two clocks > + * > + * Initialize to zero, then set clock, systime, num_samples. > + * > + * Transformation between HW time and system time is done with: So hw time is overloaded as well. It tends to be thought of as the CMOS/RTC clock. Would PTP or NIC time be ok? (It avoids network-time which also has ntp connotations) Or are there other uses for this code other then the PTP code? > + * HW time transformed = HW time + offset + > + * (HW time - last_update) * skew / > + * CLOCKSYNC_SKEW_RESOLUTION > + * > + * @clock: the source for HW time stamps (%clocksource_read_time) nix clocksource. > + * @systime: function returning current system time (ktime_get > + * for monotonic time, or ktime_get_real for wall clock) So, are non-CLOCK_REALTIME clockids actually used? > + * @num_samples: number of times that HW time and system time are to > + * be compared when determining their offset > + * @offset: (system time - HW time) at the time of the last update > + * @skew: average (system time - HW time) / delta HW time * > + * CLOCKSYNC_SKEW_RESOLUTION > + * @last_update: last HW time stamp when clock offset was measured > + */ > +struct clocksync { struct time_comparator { ? > + struct timecounter *clock; > + ktime_t (*systime)(void); > + int num_samples; > + > + s64 offset; > + s64 skew; > + u64 last_update; > +}; > + > +/** > + * clocksync_hw2sys - transform HW time stamp into corresponding system time > + * @sync: context for clock sync > + * @hwtstamp: the result of timecounter_read() or > + * timecounter_cyc2time() > + */ > +extern ktime_t clocksync_hw2sys(struct clocksync *sync, > + u64 hwtstamp); Ugh. hw2sys again is overloaded for reading the cmos/RTC persistent clock and setting the system time. So overall I don't have any objections with the code itself. Its fairly isolated and doesn't interact with the timekeeping code itself. Sorry for taking so long to get feedback to you, I had started looking at this right before the holiday and lost context after the break. thanks -john