Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1245271 > unrolled thread

[PATCH v4 0/4] Patchset enabling hardware based cross-timestamps for next gen Intel platforms

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/4] Patchset enabling hardware based cross-timestamps for next gen Intel platforms "Christopher S. Hall" <christopher.s.hall@intel.com> - 2015-10-13 03:50 +0200
    [PATCH v4 2/4] Always running timer correlated clocksource "Christopher S. Hall" <christopher.s.hall@intel.com> - 2015-10-13 03:50 +0200
      Re: [PATCH v4 2/4] Always running timer correlated clocksource kbuild test robot <lkp@intel.com> - 2015-10-13 04:10 +0200
    [PATCH v4 4/4] Adds hardware supported cross timestamp "Christopher S. Hall" <christopher.s.hall@intel.com> - 2015-10-13 03:50 +0200
      Re: [PATCH v4 4/4] Adds hardware supported cross timestamp David Miller <davem@davemloft.net> - 2015-10-13 04:20 +0200
      Re: [PATCH v4 4/4] Adds hardware supported cross timestamp kbuild test robot <lkp@intel.com> - 2015-10-13 04:20 +0200
    [PATCH v4 1/4] Produce system time from correlated clocksource "Christopher S. Hall" <christopher.s.hall@intel.com> - 2015-10-13 03:50 +0200
      Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-13 07:00 +0200
        Re: [PATCH v4 1/4] Produce system time from correlated clocksource Thomas Gleixner <tglx@linutronix.de> - 2015-10-13 10:00 +0200
          Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-13 10:40 +0200
            Re: [PATCH v4 1/4] Produce system time from correlated clocksource Thomas Gleixner <tglx@linutronix.de> - 2015-10-13 21:20 +0200
              Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-13 23:20 +0200
                Re: [PATCH v4 1/4] Produce system time from correlated clocksource Thomas Gleixner <tglx@linutronix.de> - 2015-10-14 09:30 +0200
                  Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-14 11:30 +0200
                    Re: [PATCH v4 1/4] Produce system time from correlated clocksource Thomas Gleixner <tglx@linutronix.de> - 2015-10-14 16:30 +0200
                      Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-14 18:20 +0200
                Re: [PATCH v4 1/4] Produce system time from correlated clocksource "Christopher Hall" <christopher.s.hall@intel.com> - 2015-10-15 04:40 +0200
                  Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-15 07:50 +0200
                    Re: [PATCH v4 1/4] Produce system time from correlated clocksource Thomas Gleixner <tglx@linutronix.de> - 2015-10-15 10:20 +0200
      Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-13 07:30 +0200
      Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-13 16:00 +0200
      Re: [PATCH v4 1/4] Produce system time from correlated clocksource Thomas Gleixner <tglx@linutronix.de> - 2015-10-13 21:50 +0200
        Re: [PATCH v4 1/4] Produce system time from correlated clocksource "Christopher Hall" <christopher.s.hall@intel.com> - 2015-10-15 04:00 +0200
          Re: [PATCH v4 1/4] Produce system time from correlated clocksource Richard Cochran <richardcochran@gmail.com> - 2015-10-15 08:00 +0200
          Re: [PATCH v4 1/4] Produce system time from correlated clocksource Thomas Gleixner <tglx@linutronix.de> - 2015-10-15 10:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1245744 — Re: [PATCH v4 1/4] Produce system time from correlated clocksource

Now that I am starting to understand what this code is trying to
achieve...

On Mon, Oct 12, 2015 at 11:45:19AM -0700, Christopher S. Hall wrote:
> The modification to the original patch accomodates these
> slow devices by adding the option of providing an ART value outside
> of the retry loop and adding a history which can consulted in the
> case of an out of date counter value. The history is kept by
> making the shadow_timekeeper an array. Each write to the
> timekeeper rotates through the array, preserving a
> history of updates.

You did not provide an example of how this function is called, in the
special case where correlated_ts.get_ts = NULL and the caller provides
system_ts and device_ts.  Until that user appears, we do not need the
interface at all.

> +/* This needs to be 3 or greater for backtracking to be useful */
> +#define SHADOW_HISTORY_DEPTH 7

Why then have you put 7?

This comment should really explain what is needed and why.

> +/**
> + * get_correlated_timestamp - Get a correlated timestamp

This function is tremendously confusing.  The overloading with special
semenatics when the callback is NULL is not nice.  There is hardly
much shared logic, and so nothing speaks against having two methods.
I would call them "get_correlated_timestamp" and "correlate_timestamp"
or similar.  The "get_" prefix in the name is totally misleading in
your special case.

> + * @crs: conversion between correlated clock and system clock
> + * @crt: callback to get simultaneous device and correlated clock value *or*
> + *	contains a valid correlated clock value and NULL callback

This description stinks.  What is crs?  It is *not* a conversion.  It is:

	struct correlated_ts - Descriptor for taking a correlated time stamp

What is crt?  It is *not* a callback.  It is:

	struct correlated_cs - Descriptor for a clocksource correlated to another

For the crt, the kerneldoc should explain which of the fields

	int			(*get_ts)(struct correlated_ts *ts);
	u64			system_ts;
	u64			device_ts;
	ktime_t			system_real;
	ktime_t			system_raw;
	void			*private;

are provided by the caller and which are set by the function.  The
fact that fields are set either by the caller or by the method is a
funny code smell.

> + *
> + * Reads a timestamp from a device and correlates it to system time.  This
> + * function can be used in two ways.  If a non-NULL get_ts function pointer is
> + * supplied in @crt, this function is called within the retry loop to
> + * read the current correlated clock value and associated device time.
> + * Otherwise (get_ts is NULL) a correlated clock value is supplied and
> + * the history in shadow_timekeeper is consulted if necessary.
> + */
> +int get_correlated_timestamp(struct correlated_ts *crt,
> +			     struct correlated_cs *crs)
> +{
...
> +	do {

This code is only used in the special case:

> +		/*
> +		 * Since the cycles value is supplied outside of the loop,
> +		 * there is no guarantee that it represents a time *after*
> +		 * cycle_last do some checks to figure out whether it's
> +		 * represents the past or the future taking rollover
> +		 * into account. If the value is in the past, try to backtrack
> +		 */

BTW, isn't there an easier way to deal with time stamps in the past?
(Compare with timecounter_cyc2time.)

> +		cycles_now = tk->tkr_mono.read(tk->tkr_mono.clock);
> +		cycles_last = tk->tkr_mono.cycle_last;
> +		if ((cycles >= cycles_last && cycles_now < cycles) ||
> +		    (cycles < cycles_last && cycles_now >= cycles_last)) {
> +			/* cycles is in the past try to backtrack */
> +			int backtrack_index = shadow_index;
> +
> +			while (get_prev_shadow_index(&backtrack_index)) {
> +				tk = shadow_timekeeper+backtrack_index;
> +				if (cycle_between(cycles_last, cycles,
> +						  tk->tkr_mono.cycle_last))
> +					goto do_convert;
> +				cycles_last = tk->tkr_mono.cycle_last;
> +			}
> +			return -EAGAIN;
> +		}

And this is the shared stuff:

> +do_convert:
> +		/* Convert to clock realtime */
> +		base = ktime_add(tk->tkr_mono.base,
> +				 tk_core.timekeeper.offs_real);
> +		nsecs = timekeeping_convert_to_ns(&tk->tkr_mono, cycles);
> +		crt->system_real = ktime_add_ns(base, nsecs);
> +
> +		/* Convert to clock raw monotonic */
> +		base = tk->tkr_raw.base;
> +		nsecs = timekeeping_convert_to_ns(&tk->tkr_raw, cycles);
> +		crt->system_raw = ktime_add_ns(base, nsecs);
> +
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +	return 0;
> +}

I suggest moving the shared stuff into a subroutine and providing two
different functions.

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

[toc] | [prev] | [next] | [standalone]

#1246090 — Re: [PATCH v4 1/4] Produce system time from correlated clocksource

On Mon, 12 Oct 2015, Christopher S. Hall wrote:
> Another representative use case of time sync and the correlated
> clocksource (in addition to PTP noted above) is PTP synchronized
> audio.

This wants to be a seperate patch, really.
 
> +/* This needs to be 3 or greater for backtracking to be useful */

Why?

> +#define SHADOW_HISTORY_DEPTH 7

And that number is 7 because?

>  static DEFINE_RAW_SPINLOCK(timekeeper_lock);
> -static struct timekeeper shadow_timekeeper;
> +static struct timekeeper shadow_timekeeper[SHADOW_HISTORY_DEPTH];
> +static int shadow_index = -1; /* incremented to zero in timekeeping_init() */

What's the point of this? Aside of that, please do not use tail comments.

> +static bool shadow_timekeeper_full;

That's silly. Make DEPTH a power of 2 and do:

       idx = (idx + 1) & (DEPTH - 1);

> +/*
> + * Modifies shadow index argument to point to the next array element
> + * Returns bool indicating shadow array fullness after the update
> + */
> +static bool get_next_shadow_index(int *shadow_index_out)
> +{
> +	*shadow_index_out = (shadow_index + 1) % SHADOW_HISTORY_DEPTH;
> +	/*
> +	 * If shadow timekeeper is full it stays full, otherwise compute
> +	 * the next value based on whether the index rolls over
> +	 */
> +	return shadow_timekeeper_full ?
> +		true : *shadow_index_out < shadow_index;

All this can go away.

> +	if (action & TK_MIRROR) {
> +		int next_shadow_index;
> +		bool next_shadow_full =
> +			get_next_shadow_index(&next_shadow_index);
> +		memcpy(shadow_timekeeper+next_shadow_index,
> +		       &tk_core.timekeeper, sizeof(tk_core.timekeeper));
> +		shadow_index = next_shadow_index;
> +		shadow_timekeeper_full = next_shadow_full;

Ditto.

> +	}
>  }
>  
>  /**
> @@ -884,6 +923,142 @@ EXPORT_SYMBOL(getnstime_raw_and_real);
>  
>  #endif /* CONFIG_NTP_PPS */
>  
> +/*
> + * Iterator-like function which can be called multiple times to return the
> + * previous shadow_index
> + * Returns false when finding previous is not possible because:
> + * - The array is not full
> + * - The previous shadow_index refers to an entry that may be in-flight
> + */
> +static bool get_prev_shadow_index(int *shadow_index_io)
> +{
> +	int guard_index;
> +	int ret = (*shadow_index_io - 1) % SHADOW_HISTORY_DEPTH;
> +
> +	ret += ret < 0 ? SHADOW_HISTORY_DEPTH : 0;
> +	/*
> +	 * guard_index references the next shadow entry, assume that this
> +	 * isn't valid since its not protected by sequence lock
> +	 */
> +	get_next_shadow_index(&guard_index);
> +	/* if the array isn't full and index references top (invalid) entry */
> +	if (!shadow_timekeeper_full && ret > *shadow_index_io)
> +		return false;
> +	/* the next entry may be in-flight and may be invalid  */
> +	if (ret == guard_index)
> +		return false;
> +	/* Also make sure that entry is valid based on current shadow_index */
> +	*shadow_index_io = ret;
> +	return true;

You surely try hard to do stuff in the most unreadable way. 

> +/**
> + * get_correlated_timestamp - Get a correlated timestamp
> + * @crs: conversion between correlated clock and system clock
> + * @crt: callback to get simultaneous device and correlated clock value *or*
> + *	contains a valid correlated clock value and NULL callback
> + *
> + * Reads a timestamp from a device and correlates it to system time.  This
> + * function can be used in two ways.  If a non-NULL get_ts function pointer is
> + * supplied in @crt, this function is called within the retry loop to
> + * read the current correlated clock value and associated device time.
> + * Otherwise (get_ts is NULL) a correlated clock value is supplied and
> + * the history in shadow_timekeeper is consulted if necessary.
> + */
> +int get_correlated_timestamp(struct correlated_ts *crt,
> +			     struct correlated_cs *crs)
> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	unsigned long seq;
> +	cycles_t cycles, cycles_now, cycles_last;
> +	ktime_t base;
> +	s64 nsecs;
> +	int ret;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		/*
> +		 * Verify that the correlated clocksoure is related to
> +		 * the currently installed timekeeper clocksoure
> +		 */
> +		if (tk->tkr_mono.clock != crs->related_cs)
> +			return -ENODEV;
> +
> +		/*
> +		 * Get a timestamp from the device if get_ts is non-NULL
> +		 */
> +		if( crt->get_ts ) {
> +			ret = crt->get_ts(crt);
> +			if (ret)
> +				return ret;
> +		}

What's the point of this? Why are you not making the few lines which
you can actually reuse a helper function and leave the PTP code alone?

> -- 
> 2.1.4

So I reached enf of patch and did not find anything in
timekeeping_init() which tells that the index is incremented to 0. It
really would need a comment, but why do you want to do that at all. It
does not matter whether the first entry is at 0 or 1. You need a
validity check for the entries anyway.

Thanks,

	tglx



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

[toc] | [prev] | [next] | [standalone]

#1247315 — Re: [PATCH v4 1/4] Produce system time from correlated clocksource

Thomas,

On Tue, 13 Oct 2015 12:42:52 -0700, Thomas Gleixner <tglx@linutronix.de>  
wrote:
> On Mon, 12 Oct 2015, Christopher S. Hall wrote:
>> audio.
>
> This wants to be a seperate patch, really.

OK. This makes sense, I'll do this the next time.

>> +/* This needs to be 3 or greater for backtracking to be useful */
>
> Why?

The current index points to a copy and the next may be being changed by  
update_wall_time(). Leaving n-2 entries available with useful history in  
them. I'll add more descriptive comments here.

>
>> +#define SHADOW_HISTORY_DEPTH 7
>
> And that number is 7 because?

Due to power of 2 it will be 8 instead. As above the useful history is  
8-2*1 ms (1 ms is the minimum jiffy length).  Array size 4 would not be  
enough history for the DSP which requires 4 ms of history, in the worst  
case.

>> +static int shadow_index = -1; /* incremented to zero in
>
> What's the point of this? Aside of that, please do not use tail comments.

It's removed.  A check for validity is added below and this isn't  
necessary.

> That's silly. Make DEPTH a power of 2 and do:
>
>        idx = (idx + 1) & (DEPTH - 1);

This is changed.

>> +		true : *shadow_index_out < shadow_index;
>
> All this can go away.

Yes.

>> +	/* Also make sure that entry is valid based on current shadow_index */
>> +	*shadow_index_io = ret;
>> +	return true;
>
> You surely try hard to do stuff in the most unreadable way.

Is like this easier to follow?

+static struct timekeeper *search_shadow_history(cycles_t cycles,
+                                               struct clocksource *cs)
+{
+       struct timekeeper *tk = &tk_core.timekeeper;
+       int srchidx = shadow_index;
+       cycles_t cycles_start, cycles_end;
+
+       cycles_start = tk->tkr_mono.cycle_last;
+       do {
+               srchidx = !srchidx-- ? srchidx+SHADOW_HISTORY_DEPTH :  
srchidx;
+               tk = shadow_timekeeper + srchidx;
+
+               /* The next shadow entry may be in flight, don't use it */
+               if (srchidx == ((shadow_index+1) &  
(SHADOW_HISTORY_DEPTH-1)))
+                       return NULL;
+
+               /* Make sure timekeeper is related to clock on this  
interval */
+               if (tk->tkr_mono.clock != cs)
+                       return NULL;
+
+               cycles_end = cycles_start;
+               cycles_start = tk->tkr_mono.cycle_last;
+       } while (!cycle_between(cycles_start, cycles, cycles_end));
+
+       return tk;
+}
A check for validity is added here using the clocksource pointer.

and inside of get_correlated_timestamp():

+                * into account. If the value is in the past, try to  
backtrack
+                */
+               cycles_end = tk->tkr_mono.read(tk->tkr_mono.clock);
+               cycles_start = tk->tkr_mono.cycle_last;
+               if (!cycle_between(cycles_start, cycles, cycles_end)) {
+                       tk = search_shadow_history(cycles,  
crs->related_cs);
+                       if (!tk)
+                               return -EAGAIN;
+               }



>> +		/*
>> +		 * Get a timestamp from the device if get_ts is non-NULL
>> +		 */
>> +		if( crt->get_ts ) {
>> +			ret = crt->get_ts(crt);
>> +			if (ret)
>> +				return ret;
>> +		}
>
> What's the point of this? Why are you not making the few lines which
> you can actually reuse a helper function and leave the PTP code alone?

The audio driver is structured in such a way that it's simpler to provide  
a value rather than a callback.  I changed this to allow the audio  
developers to provide an ART value as input.  If a callback is provided,  
the resulting counter value is guaranteed to be later than cycle_last and  
there is no need to do extra checking (the goto skips that check).  Is  
this an answer to your question?

> So I reached enf of patch and did not find anything in
> timekeeping_init() which tells that the index is incremented to 0. It
> really would need a comment, but why do you want to do that at all. It
> does not matter whether the first entry is at 0 or 1. You need a
> validity check for the entries anyway.

I think this should be resolved.  There's no sensitivity with regard to  
the start index with an added validity check.

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

[toc] | [prev] | [next] | [standalone]

#1247409 — Re: [PATCH v4 1/4] Produce system time from correlated clocksource

On Wed, Oct 14, 2015 at 06:57:33PM -0700, Christopher Hall wrote:
> >>+#define SHADOW_HISTORY_DEPTH 7
> >
> >And that number is 7 because?
> 
> Due to power of 2 it will be 8 instead. As above the useful history is 8-2*1
> ms (1 ms is the minimum jiffy length).  Array size 4 would not be enough
> history for the DSP which requires 4 ms of history, in the worst case.

Just as I suspected, the magic number 7 is based on the needs of one
particular user.  What about the next user who comes along needing 10
milliseconds?  That will not do.  Any new interface should be generic
enough to support a wide range of users.

So I think this approach is all wrong.  Here is an idea for you to
consider.  Instead of mucking with the TK, let the user code (possibly
in-kernel) sample ART/sys pairs and interpolate the ART/dev time
stamps.  That way, the user can choose the range and resolution that
he needs.

> The audio driver is structured in such a way that it's simpler to provide a
> value rather than a callback.

Can you please provide a link to the audio driver that uses this new
interface?

Thanks,
Richard

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

[toc] | [prev] | [next] | [standalone]

#1247552 — Re: [PATCH v4 1/4] Produce system time from correlated clocksource

On Wed, 14 Oct 2015, Christopher Hall wrote:
> On Tue, 13 Oct 2015 12:42:52 -0700, Thomas Gleixner <tglx@linutronix.de>
> wrote:
> > On Mon, 12 Oct 2015, Christopher S. Hall wrote:
> > > audio.
> > 
> > This wants to be a seperate patch, really.
> 
> OK. This makes sense, I'll do this the next time.
> 
> > > +/* This needs to be 3 or greater for backtracking to be useful */
> > 
> > Why?
> 
> The current index points to a copy and the next may be being changed by
> update_wall_time(). Leaving n-2 entries available with useful history in them.
> I'll add more descriptive comments here.
> 
> > 
> > > +#define SHADOW_HISTORY_DEPTH 7
> > 
> > And that number is 7 because?
> 
> Due to power of 2 it will be 8 instead. As above the useful history is 8-2*1
> ms (1 ms is the minimum jiffy length).  Array size 4 would not be enough
> history for the DSP which requires 4 ms of history, in the worst case.

And how exactly becomes 7 magically 8?
 
> > 
> > What's the point of this? Why are you not making the few lines which
> > you can actually reuse a helper function and leave the PTP code alone?
> 
> The audio driver is structured in such a way that it's simpler to provide a
> value rather than a callback.  I changed this to allow the audio developers to
> provide an ART value as input.  If a callback is provided, the resulting
> counter value is guaranteed to be later than cycle_last and there is no need
> to do extra checking (the goto skips that check).  Is this an answer to your
> question?

Make it a seperate function which can hand in the information and
leave the PTP specific sample/conversion function alone.

Thanks,

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

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web