Message-Id: <1233776680.6994.30.camel@localhost.localdomain>
Date:	Wed, 04 Feb 2009 11:44:40 -0800
From:	john stultz <johnstul@...ibm.com>
To:	Patrick Ohly <patrick.ohly@...el.com>
Cc:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	David Miller <davem@...emloft.net>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH NET-NEXT 02/10] time sync: generic infrastructure to
	map between time stamps generated by a time counter and system time

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 <patrick.ohly@...el.com>
> + *
> + * 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 <linux/clocksource.h>
> +#include <linux/ktime.h>
> +
> +/**
> + * 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists