Re: [PATCH NET-NEXT 02/10] time sync: generic infrastructureto map between time stamps generated by a time counter and system time

[Patrick Ohly]

On Wed, 2009-02-04 at 21:44 +0200, john stultz wrote:
> 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.

Agreed, good names are important - choosing them is often harder than
getting the code to work ;-)

> 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? 

Fine with me.

> If this code is really PTP purposed, maybe ptp should be in the name?

It's not PTP specific. I'm not sure whether there are other uses for it,
but if something comes up, then having PTP in the name would be wrong.
So I prefer timecompare.

> > 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.

Right.

> > +/**
> > + * 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?

As said above, there might be. I should better avoid all references to
HW and system and just speak of "source" and "target" time, with just
one motivating example given that refers to NIC and system time.

> > + * @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?

Not at the moment, but I can imagine that this might be useful at some
point.

> > + * @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 { ?

Why not simply "timecompare"? It's the central data structure in this
module, similar to "clocksource" in "clocksource.[ch]". Apart from that
I don't mind using 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.

timecompare_transform()?

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.


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