RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-05 Thread Thomas Gleixner 
On Fri, 4 Sep 2015, Hall, Christopher S wrote:
> > Right, but we still do not know how that is going to be used. And
> > that's the key question. As long as that is not answered all can do is
> > wild guessing.
> 
> It's not wild guessing.  We do have it working on other OSs and have
> a pretty good idea of how it will work.  The DSP firmware will be
> largely identical for Linux.  I think now, we have a chicken and egg
> problem.

You have a totally different problem. You are just refusing to explain
how all that stuff is supposed to work and what kind of functionality
you need exactly.

Is it that hard to describe the technical requirements and the
presumably assbackwards restrictions of the firmware?

Thanks,

tglx




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-05 Thread Ingo Molnar 

* Thomas Gleixner  wrote:

> On Fri, 4 Sep 2015, Hall, Christopher S wrote:
> > > Right, but we still do not know how that is going to be used. And
> > > that's the key question. As long as that is not answered all can do is
> > > wild guessing.
> > 
> > It's not wild guessing.  We do have it working on other OSs and have a 
> > pretty 
> > good idea of how it will work.  The DSP firmware will be largely identical 
> > for 
> > Linux.  I think now, we have a chicken and egg problem.
> 
> You have a totally different problem. You are just refusing to explain how 
> all 
> that stuff is supposed to work and what kind of functionality you need 
> exactly.

Yeah, so I'm just going to NAK this until things are improved:

   NAKed-by: Ingo Molnar 

it's not like we are overly bored in timekeeping and need the extra complexity 
as 
much as possible.

Proper, comprehensive, proactive technical description is needed, with proper 
changelogs, not just half-baked notes. If all that is fixed I'll lift my NAK.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Thomas Gleixner 
On Thu, 3 Sep 2015, Hall, Christopher S wrote:

Can you please teach your mail client to add proper line breaks around
80? Your mail renders horrible in a text based mail client.

> In addition to the network interface, ART will be used in the audio
> interface as well.  We need to support the case where an audio
> co-processor will control the audio device.  In this case, the
> get_ts() function supplied by the audio driver will be very slow
> (several milliseconds) and the result will be out of date by some
> fraction of that amount.

You are not telling at all, what this driver is supposed to do, what
this get_ts() function is for and how that co-processor thing works.

You just make claims, that you need this without explaining WHY. And
that WHY is the most interesting part.

> This loop makes strict requirements on the latency and recency. Is
> it possible to relax that requirement in some way?

No. This function is explicitely for the precise timestamp usecase,
which is required by PTP and other sane use cases.

> For example, supply the ART value as an argument and, in the case of
> the realtime clock, keep a short history of clock changes.  It would

It's not only clock realtime which is affected by those.

> fail in cases where there are a lot of calls to adjtimex(),

That has nothing to do with lots of adjtimex calls. The kernel does a
slow correction of the conversion values itself to avoid time jumping
around.

> but it will would work most of the time.

Will, would, most? - Could, perhaps, sometimes?

Looks like a design from the trainwreck engineering departement. We
want to have it very precise, but we don't care if it behaves like a
random number generator.

Can you folks please get your act together and provide coherent
explanations about the usecase and the constraints instead of
proposing random functions with obscure semantics?

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Peter Zijlstra 
On Fri, Sep 04, 2015 at 10:11:22AM +0200, Richard Cochran wrote:
> On Thu, Sep 03, 2015 at 11:20:37PM +, Hall, Christopher S wrote:
> > In addition to the network interface, ART will be used in the audio 
> > interface as well.
> > We need to support the case where an audio co-processor will control the 
> > audio device.
> > In this case, the get_ts() function supplied by the audio driver will be 
> > very slow
> > (several milliseconds) and the result will be out of date by some fraction 
> > of that 
> > amount.
> 
> Why does it take milliseconds to read one audio time stamp?

So what I suspect, but please correct me if I'm wrong Chris, is that a
DSP will buffer and process audio signals, and only later wake up the
main CPU.

So by the time the CPU is made aware of the data, it's 'old'.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Thomas Gleixner 
On Fri, 4 Sep 2015, Peter Zijlstra wrote:
> On Fri, Sep 04, 2015 at 05:17:43PM +0200, Richard Cochran wrote:
> > On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> > > I think what they're getting at is asking if there's a rate limit to
> > > time adjustments, without that, saving the last n transition points will
> > > still not cover any given length of history.
> > 
> > As if the ntp code isn't complex enough already - now we're adding
> > sample histories and adjustment rating limiting?
> > 
> > And all for some unknown DSP in a mythical sound card??
> 
> Hehe, I'm just a 'translator' here. But going by you answer I'm taking
> it there isn't in fact a rate-limit to adjustments. Which, even if you
> were not opposed to that direction, makes it an unfeasible proposition.
> 
> Also, I'm not thinking its too mythical, sound/soc/intel/ is full of
> audio DSP stuff, I think a newer version will just gain ART support.

Right, but we still do not know how that is going to be used. And
that's the key question. As long as that is not answered all can do is
wild guessing.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Peter Zijlstra 
On Fri, Sep 04, 2015 at 03:02:19PM +0200, Thomas Gleixner wrote:
> > For example, supply the ART value as an argument and, in the case of
> > the realtime clock, keep a short history of clock changes.  It would
> 
> It's not only clock realtime which is affected by those.
> 
> > fail in cases where there are a lot of calls to adjtimex(),
> 
> That has nothing to do with lots of adjtimex calls. The kernel does a
> slow correction of the conversion values itself to avoid time jumping
> around.

I think what they're getting at is asking if there's a rate limit to
time adjustments, without that, saving the last n transition points will
still not cover any given length of history.

So what I think they're looking for; is given an upper bound on the DSP
delaying its data, come up with a fixed minimal amount of transitions
points we must store to cover the history.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Richard Cochran 
On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> I think what they're getting at is asking if there's a rate limit to
> time adjustments, without that, saving the last n transition points will
> still not cover any given length of history.

As if the ntp code isn't complex enough already - now we're adding
sample histories and adjustment rating limiting?

And all for some unknown DSP in a mythical sound card??


Thanks,
Richard



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Richard Cochran 
On Fri, Sep 04, 2015 at 03:02:19PM +0200, Thomas Gleixner wrote:
> No. This function is explicitely for the precise timestamp usecase,
> which is required by PTP and other sane use cases.

Right.  The audio department only needs to know the (ART, ptp) offset.
The kernel and user space never need the (ART, mediaclock) offset.
That is private information for the DSP.

As long as user space reads (ART, ptp) and provides this regulary to
the audio DSP, then the DSP will have all the information it needs to
figure out (ptp, mediaclock).

Thanks,
Richard


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Hall, Christopher S 
> -Original Message-
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Friday, September 04, 2015 9:35 AM
> To: Peter Zijlstra
> Cc: Richard Cochran; Hall, Christopher S; Kirsher, Jeffrey T;
> h...@zytor.com; mi...@redhat.com; john.stu...@linaro.org; x...@kernel.org;
> linux-ker...@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> l...@lists.osuosl.org
> Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time
> from an auxiliary clocksource
> 
> On Fri, 4 Sep 2015, Peter Zijlstra wrote:
> > On Fri, Sep 04, 2015 at 05:17:43PM +0200, Richard Cochran wrote:
> > > On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> > > > I think what they're getting at is asking if there's a rate limit to
> > > > time adjustments, without that, saving the last n transition points
> will
> > > > still not cover any given length of history.
> > >
> > > As if the ntp code isn't complex enough already - now we're adding
> > > sample histories and adjustment rating limiting?
> > >
> > > And all for some unknown DSP in a mythical sound card??
> >
> > Hehe, I'm just a 'translator' here. But going by you answer I'm taking
> > it there isn't in fact a rate-limit to adjustments. Which, even if you
> > were not opposed to that direction, makes it an unfeasible proposition.
> >
> > Also, I'm not thinking its too mythical, sound/soc/intel/ is full of
> > audio DSP stuff, I think a newer version will just gain ART support.
> 
> Right, but we still do not know how that is going to be used. And
> that's the key question. As long as that is not answered all can do is
> wild guessing.

It's not wild guessing.  We do have it working on other OSs and have a pretty 
good 
idea of how it will work.  The DSP firmware will be largely identical for 
Linux.  I 
think now, we have a chicken and egg problem.

We can't post audio drivers that break, or are broken by, the current ART 
interface.  
How do I move this forward?  Should I minimally (I don't know exactly what that 
means 
just yet) rewrite the ART interface so that the audio driver is mostly not 
broken and 
post that along with the audio driver code?  Is this an acceptable approach?

> 
> Thanks,
> 
>   tglx
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Hall, Christopher S 
> -Original Message-
> From: Peter Zijlstra [mailto:pet...@infradead.org]
> Sent: Friday, September 04, 2015 7:28 AM
> To: Richard Cochran
> Cc: Hall, Christopher S; Thomas Gleixner; Kirsher, Jeffrey T;
> h...@zytor.com; mi...@redhat.com; john.stu...@linaro.org; x...@kernel.org;
> linux-ker...@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> l...@lists.osuosl.org
> Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time
> from an auxiliary clocksource
> 
> > > (several milliseconds) and the result will be out of date by some
> fraction of that
> > > amount.
> >
> > Why does it take milliseconds to read one audio time stamp?
> 
> So what I suspect, but please correct me if I'm wrong Chris, is that a
> DSP will buffer and process audio signals, and only later wake up the
> main CPU.
> 
> So by the time the CPU is made aware of the data, it's 'old'.

That's about right.  The DSP runs on a 1 ms cadence.  Any access to registers 
controlled by the DSP will take 1-2 DSP ticks to access.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Peter Zijlstra 
On Fri, Sep 04, 2015 at 05:17:43PM +0200, Richard Cochran wrote:
> On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> > I think what they're getting at is asking if there's a rate limit to
> > time adjustments, without that, saving the last n transition points will
> > still not cover any given length of history.
> 
> As if the ntp code isn't complex enough already - now we're adding
> sample histories and adjustment rating limiting?
> 
> And all for some unknown DSP in a mythical sound card??

Hehe, I'm just a 'translator' here. But going by you answer I'm taking
it there isn't in fact a rate-limit to adjustments. Which, even if you
were not opposed to that direction, makes it an unfeasible proposition.

Also, I'm not thinking its too mythical, sound/soc/intel/ is full of
audio DSP stuff, I think a newer version will just gain ART support.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread John Stultz 
On Thu, Sep 3, 2015 at 4:20 PM, Hall, Christopher S
 wrote:
> For example, supply the ART value as an argument and, in the case of the 
> realtime
> clock, keep a short history of clock changes.  It would fail in cases where 
> there
> are a lot of calls to adjtimex(), but it will would work most of the time.

So, I really don't think something like this would be reasonable. For
one, keeping track of the adjtimex adjustments would be difficult
enough to do sanely, but the real issue is that the clock has its own
long-term error correction adjustments that it does in order to keep
long term frequency accuracy with coarsely adjusted clocksources.
Trying to track those small oscillation intervals would be even more
complicated.

I still think that being able to calculate the CLOCK_MONOTONIC_RAW
value for a given ART counter value is reasonable, and then one can
use the getnstime_raw_and_real() to get a current raw/real sync point,
which you can then calculate the raw delta, and subtract that from the
sycned real timestamp.

You're error there would be bound by the maxium clocksource adjustment
rate * the raw-delta interval length.

To clarify on the need to understand if this error would be
reasonable, can you provide a sense of what the delay from an ART read
to trying to calculate a REALTIME value might be?

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-04 Thread Richard Cochran 
On Thu, Sep 03, 2015 at 11:20:37PM +, Hall, Christopher S wrote:
> In addition to the network interface, ART will be used in the audio interface 
> as well.
> We need to support the case where an audio co-processor will control the 
> audio device.
> In this case, the get_ts() function supplied by the audio driver will be very 
> slow
> (several milliseconds) and the result will be out of date by some fraction of 
> that 
> amount.

Why does it take milliseconds to read one audio time stamp?

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-09-03 Thread Hall, Christopher S 
> -Original Message-
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Saturday, August 22, 2015 1:17 PM
> To: Hall, Christopher S
> Cc: Kirsher, Jeffrey T; h...@zytor.com; mi...@redhat.com;
> john.stu...@linaro.org; richardcoch...@gmail.com; x...@kernel.org; linux-
> ker...@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> l...@lists.osuosl.org; pet...@infradead.org
> Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time
> from an auxiliary clocksource
>  
> > +/**
> > + * get_correlated_timestamp - Get a correlated timestamp
> > + *
> > + * Reads a timestamp from a device and correlates it to system time
> > + */
> > +int get_correlated_timestamp(struct correlated_ts *crt,
> > +struct correlated_cs *crs)
> > +{
> > +   struct timekeeper *tk = _core.timekeeper;
> > +   unsigned long seq;
> > +   cycles_t cycles;
> > +   ktime_t base;
> > +   s64 nsecs;
> > +   int ret;
> > +
> > +   do {
> > +   seq = read_seqcount_begin(_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;
> > +
> > +   /*
> > +* Try to get a timestamp from the device.
> > +*/
> > +   ret = crt->get_ts(crt);
> > +   if (ret)
> > +   return ret;
> > +
[Re-added code for context]

In addition to the network interface, ART will be used in the audio interface 
as well.
We need to support the case where an audio co-processor will control the audio 
device.
In this case, the get_ts() function supplied by the audio driver will be very 
slow
(several milliseconds) and the result will be out of date by some fraction of 
that 
amount.

This loop makes strict requirements on the latency and recency. Is it possible 
to relax
that requirement in some way?

For example, supply the ART value as an argument and, in the case of the 
realtime
clock, keep a short history of clock changes.  It would fail in cases where 
there
are a lot of calls to adjtimex(), but it will would work most of the time.

What can you suggest? Thanks

Chris

> > +   } while (read_seqcount_retry(_core.seq, seq));
> > +   return 0;
> > +}


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

2015-08-22 Thread Thomas Gleixner 
On Fri, 21 Aug 2015, Christopher S. Hall wrote:

 Add struct correlated_cs with pointer to original clocksource and
   function pointer to convert correlated clocksource to the original
 
 Add get_correlated_timestamp() function which given specific correlated_cs
   and correlated_ts convert correlated counter value to system time

This is not a proper changelog.

1) The subject line lacks a subsystem prefix

   timekeeping:

   Is the proper choice here

2) The subject line should be short and precise

   timekeeping: Add mechanism to gather correlated timestamps

   Might be an informative one.

3) The changelog itself should describe the reason why we want this
   change, the purpose of the change etc.

   Add foo
   Add bar

   Is pointless because we can see that from the patch itself.

   What the patch cannot not explain is the WHY. That's what the
   changelog is for.

4) You dropped the authorship

   The proper way to do this is to add a 'FROM: author' at the top of
   the changelog body.

As I wrote the patch, so I give you a changelog along with it:

---
Subject: timekeeping: Add mechanism to gather correlated timestamps

From: Thomas Gleixner t...@linutronix.de

Modern Intel hardware provides the so called Always Running Timer
(ART). The TSC which is usually used for timekeeping is derived from
ART and runs with a fixed frequency ratio to it. ART is routed to
devices and allows to take atomic timestamp samples from the device
clock and the ART. One use case is PTP timestamps on network cards. We
want to utilize this feature as it allows us to better correlate the
PTP timestamp to the system time.

In order to gather precise timestamps we need to make sure that the
conversion from ART to TSC and the following conversion from TSC to
clock realtime happens synchronized with the ongoing timekeeping
updates. Otherwise we might convert an ART timestamp from point A in
time with the conversion factors of point B in time. These conversion
factors can differ due to NTP/PTP frequency adjustments and therefor
the resulting clock realtime timestamp would be slightly off, which is
contrary to the whole purpose of synchronized hardware timestamps.

Provide data structures which describe the correlation between two
clocksources and a function to gather correlated and convert
timestamps from a device. The function is as any other timekeeping
function protected against current timekeeper updates via the
timekeeper sequence lock. It calls the device function to gather the
hardware timestamps and converts them to clock real time and clock
monotonic raw.

Signed-off-by: Thomas Gleixner t...@linutronix.de



Can you see the difference?

 Signed-off-by: Christopher S. Hall christopher.s.h...@intel.com
 ---
  include/linux/clocksource.h | 33 +++
  include/linux/timekeeping.h |  4 +++
  kernel/time/timekeeping.c   | 65 
 +
  3 files changed, 102 insertions(+)
 
 diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
 index 278dd27..4bedadb 100644
 --- a/include/linux/clocksource.h
 +++ b/include/linux/clocksource.h
 @@ -258,4 +258,37 @@ void acpi_generic_timer_init(void);
  static inline void acpi_generic_timer_init(void) { }
  #endif
  
 +/*
 + * struct correlated_cs - Descriptor for a clocksource correlated to another
 + *   clocksource

Don't believe checkpatch here. KernelDoc requires that this is one
line, 80 char limit or not.

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

Lacks the parameter documentation:

* @crt: Pointer to a correlated timestamp structure which provides
*   the device specific timestamp function and is used to store
*   the raw and the correlated timestamps.
* @crs: Pointer to a correlated clocksource structure which describes 
*   the correlated clocksource and provides a conversion function
*   to the timekeeping clocksource

 + return 0;
 +}
 +EXPORT_SYMBOL(get_correlated_timestamp);

EXPORT_SYMBOL_GPL please.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html