> > diff --git a/platform/linux-generic/arch/x86/odp_cpu_arch.c
> b/platform/linux-generic/arch/x86/odp_cpu_arch.c
> > index c8cf27b6..9ba601a3 100644
> > --- a/platform/linux-generic/arch/x86/odp_cpu_arch.c
> > +++ b/platform/linux-generic/arch/x86/odp_cpu_arch.c
> > @@ -3,7 +3,14 @@
> > *
> > * SPDX-License-Identifier: BSD-3-Clause
> > */
> > +
> > +#include <odp_posix_extensions.h>
> > +
> > #include <odp/api/cpu.h>
> > +#include <odp_time_internal.h>
> > +#include <odp_debug_internal.h>
> > +
> > +#include <time.h>
> >
> > uint64_t odp_cpu_cycles(void)
> > {
> > @@ -31,3 +38,55 @@ uint64_t odp_cpu_cycles_resolution(void)
> > {
> > return 1;
> > }
> > +
> > +uint64_t cpu_global_time(void)
> > +{
> > + return odp_cpu_cycles();
>
> A cycle counter cannot always be used to measure time. Even on x86,
> odp_cpu_cycles() will return the value of RDTSC which is not actually
> representative of the cycle count. Even if the x86 processor is set
> to a fixed frequency, the Invariant TSC may run at a different fixed
> frequency. Please take a look at the odp_tick_t proposal here:
>
> https://docs.google.com/document/d/1sY7rOxqCNu-bMqjBiT5_keAIohrX1ZW-
> eL0oGLAQ4OM/edit?usp=sharing
>
From coverletter:
"This patch set modifies time implementation to use TSC when running on a x86
CPU that has invarint TSC CPU flag set. Otherwise, the same Linux system time
is used as before. TSC is much more efficient both in performance and
latency/jitter wise than Linux system call. This can be seen also with
scheduler latency test which time stamps events with this API. All latency
measurements (min, ave, max) improved significantly."
This function (cpu_global_time()) is called only when we have first checked
that TSC is invariant. Also we measure the TSC frequency in that case. This
function is defined in the same file as cpu_cycles(), and the file is x86
specific. So, we know what we are doing, and just re-using the code to read TSC.
> > +}
> > +
> > +#define SEC_IN_NS 1000000000ULL
> > +
> > +/* Measure TSC frequency. Frequency information registers are defined
> for x86,
> > + * but those are often not enumerated. */
> > +uint64_t cpu_global_time_freq(void)
> > +{
>
> The 0x40000010 leaf is standardized in Hyper-V as returning the TSC's
> frequency in kHz. It would be better to use this when running under a
> hypervisor. It appears to work under VMware as well.
Sure, that can be done later, if we want to add any code into odp-linux that
would behave differently under VM vs. host. Today, we don't have any.
Also there's leaf 0x15, which should give you TSC frequency but apparently is
not enumerated often. So, I ended up to do the same as DPDK - just measure it.
> > 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. We need to support Posix timespec anyway.
Timespec is used when there's no x86 invariant TSC available - so all non-x86
and even on some x86 systems.
Also "count" here starts from zero (we save cpu counter value at start). So, it
takes 580 years to wrap at 1GHz. Why 64 bit counter value (that starts from
zero) is not good for calendar time ?
> > +/*
> > + * Posix timespec based functions
> > + */
> >
> > -static inline odp_time_t time_diff(odp_time_t t2, odp_time_t t1)
> > +static inline odp_time_t time_spec_diff(odp_time_t t2, odp_time_t t1)
>
> Static functions in .c files will always be considered for inlining, so
> the
> inline qualifier is unnecessary here. You can use always_inline compiler
> attribute if the disassembly and run time performance is not as expected.
>
> Comment applies to nearly every function in this file.
This is because all our code (in this and other files) does "static inline" for
functions that we want to be inlined. Static is used for functions that are
static, but we don't care if those are inlined (slow path). Most time API
functions are fast path.
> >
> > -static inline uint64_t time_local_res(void)
> > +static inline uint64_t time_hw_res(void)
>
> I think 'freq' is a more descriptive name than 'res'.
>
> It would also be good to document at either the function declaration
> or the function definition (if multiple implementations differ) whether
> the frequency is in Hz or kHz. For example, on ARMv8 it is just a
> register read to get Hz. No estimation, and no lowering to kHz.
This implements resolution API functions ...
/**
* Global time resolution in hertz
*
* @return Global time resolution in hertz
*/
uint64_t odp_time_global_res(void);
... so, from that context you know that it's Hz. In this implementation, I
lower the measured Hz to be conservative.
> > +
> > +static inline odp_time_t time_hw_sum(odp_time_t t1, odp_time_t t2)
> > +{
> > + odp_time_t time;
> > +
> > + time.hw.res = 0;
> > + time.hw.count = t1.hw.count + t2.hw.count;
> > +
> > + return time;
> > +}
>
> If a single u64 were used this code wouldn't need to exist.
Which code? hw.res = 0 ? It not actually used for anything and could be
removed. Although, the performance gain would not be huge. Anyway, I'll remove
it for v2.
-Petri
