On 04/27 10:02:24, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > > -----Original Message----- > > From: Brian Brooks [mailto:[email protected]] > > Sent: Wednesday, April 19, 2017 10:05 PM > > To: Savolainen, Petri (Nokia - FI/Espoo) <[email protected]> > > Cc: [email protected] > > Subject: Re: [lng-odp] [API-NEXT PATCH 8/8] linux-gen: time: use hw time > > counter when available > > > > On 04/26 08:02:09, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > > > > > > > > > > > diff --git a/platform/linux- > > > > generic/include/odp/api/plat/time_types.h > > > > > > b/platform/linux-generic/include/odp/api/plat/time_types.h > > > > > > > index 4847f3b1..8ae7ce82 100644 > > > > > > > --- a/platform/linux-generic/include/odp/api/plat/time_types.h > > > > > > > +++ b/platform/linux-generic/include/odp/api/plat/time_types.h > > > > > > > @@ -26,11 +26,28 @@ extern "C" { > > > > > > > * the linux timespec structure, which is dependent on POSIX > > > > extension > > > > > > level. > > > > > > > */ > > > > > > > typedef struct odp_time_t { > > > > > > > - int64_t tv_sec; /**< @internal Seconds */ > > > > > > > - int64_t tv_nsec; /**< @internal Nanoseconds */ > > > > > > > + union { > > > > > > > + /** @internal Posix timespec */ > > > > > > > + struct { > > > > > > > + /** @internal Seconds */ > > > > > > > + int64_t tv_sec; > > > > > > > + > > > > > > > + /** @internal Nanoseconds */ > > > > > > > + int64_t tv_nsec; > > > > > > > + } spec; > > > > > > > + > > > > > > > + /** @internal HW time counter */ > > > > > > > + struct { > > > > > > > + /** @internal Counter value */ > > > > > > > + uint64_t count; > > > > > > > + > > > > > > > + /** @internal Reserved */ > > > > > > > + uint64_t res; > > > > > > > + } hw; > > > > > > > + }; > > > > > > > > > > > > A processor's tick counter cannot be used to measure calendar time > > by > > > > > > itself. The delta between two ticks, however, can be converted to > > > > > > calendar time. > > > > > > > > > > > > Please see the proposal that introduces odp_tick_t which > > eliminates > > > > > > the wasted u64 reserved field in the union above. > > > > > > > > > > > > > > > Nothing is wasted here today. > > > > > > > > The 64-bit CPU time is stored in a 128-bit data structure where 64- > > bits > > > > are wasted. You can use just a 64-bit type and then convert that to > > > > a timespec (using a starting timestamp taken on global init) if > > needed. > > > > > > > > > > Nothing is wasted compared to the situation today. Entire timespec is > > stored as before. If you want to compress timespec, it's another topic. > > Compress means additional complexity and overhead. This patch set just > > adds ability to use 64 but HW time when available. Timespec side > > implementation remains as is. > > > > You are re-using the calendar time type to store a value read from the CPU > > counter. A different approach is to use a different type (64 bits only for > > no > > wasted space) for the value read from the CPU. > > > > This value must be converted to a calendar time time if needed. It cannot > > just > > be used to represent calendar time. > > > > However, for some applications and use cases, you don't need to convert to > > calendar time > > in order to measure across two reads of the counter. You can also do > > direct arithmetic > > on the value instead of arithmetic on a timespec. > > > > If you want to read code instead of email or design documentation, look at > > this: > > > > > > https://git.linaro.org/people/ola.liljedahl/sw_scheduler.git/tree/time_ben > > ch.c > > https://git.linaro.org/people/ola.liljedahl/sw_scheduler.git/tree/time.h > > > > All you need is a 64 bit type, a way to read from the counter as well as > > get > > the frequency, and 2 very small conversion functions. > > > First, odp_time_t is our storage type for time values. Today odp-linux > defines that as 2x64bits. Nobody has complained about too high time storage > consumption. I'm not changing size of the type here. It's not purpose of this > patch set to e.g. compress timespec and thus minimize memory foot print of > time storage.
odp_time_t represents a calendar time. You can "get" an odp_time_t and tell what calendar time it is because its value represents a defined offset from some point in calendar time. You cannot do the same with the value read from the CPU counter because there is no defined offset from a point in calendar time. The HW does not guarantee this. That is why I have proposed a separate odp_tick_t type. This type need also only be 64 bits. Wasting an additional 64 bits (to store a tick in a odp_time_t) is costly when you have many in- flight events in the system. > Second, are you proposing an API change? Change of time type definition? Again, it is documented here: https://docs.google.com/document/d/1sY7rOxqCNu-bMqjBiT5_keAIohrX1ZW-eL0oGLAQ4OM/edit#heading=h.2a3mt9nt0qsp > Ola's cntr_now() function changes as soon as your time source is not a 64bit > counter, but e.g. timespec (2x64bit). It is not Ola's code, but if it changes the perspective in a positive way it can be. "as soon as" can be quantified as not ARMv8 and not x64_64. I think that is the exception rather than the norm. In this case, a struct timespec can be converted into nanosecond representation and stored in a 64 bit value. That is the way to design in favor of ARMv8 and x86_64; no space waste and no expensive conversion into a timespec. Of course, this means that application code has to use a new type: odp_tick_t. > It would need to compress that. How application would do arithmetic on the > compressed timespec ? You multiply seconds by 1,000,000,000 and add the nanoseconds to get the epoch offset in terms of nanoseconds - this can be stored in a uint64_t. And, now you can "spec" that time is in terms of nanoseconds not "implementation defined". The application can now do arithmetic too. > Third, compression/conversion usually mean division operation. Division is > tens of times heavier operation than sum, dec, mult. Currently, ODP API is > defined so that conversion is done only when application asks for it > (odp_time_t <-> nsec time). Compression is a trade-off between storage space > size and performance. I prefer to measure. Perhaps time operations using CPU tick masked by a timespec should be added here: https://git.linaro.org/people/ola.liljedahl/sw_scheduler.git/tree/time_bench.c Taking the difference between two 64-bit values and calling nsec_from_cntr() is quite fast - as fast (faster on ARM) as calling clock_gettime() once through the vDSO. > -Petri > >
