Change of odp_time_local implementation in odp-dpdk

Message ID A400FC85CF2669428A5A081F01B94F532169B1D7@DEMUMBX002.nsn-intra.net
State New
Headers show

Commit Message

Savolainen, Petri (Nokia - FI/Espoo) April 11, 2016, 12:19 p.m.
RDTSC counter may not be monotonic in all systems. Maybe some extra CPU flag check could be done to make sure that it is not affected by CPU frequency scaling. When RDTSC is not “invariant”,  DPDK seems to just warn and sample RDTSC for 1 sec get some estimate of the counter frequency. See more comments under.



From: lng-odp-dpdk [mailto:lng-odp-dpdk-bounces@lists.linaro.org] On Behalf Of EXT Wenxian Li

Sent: Thursday, April 07, 2016 2:41 PM
To: Zoltan Kiss <zoltan.kiss@linaro.org>
Cc: lng-odp-dpdk@lists.linaro.org
Subject: [lng-odp-dpdk] Change of odp_time_local implementation in odp-dpdk

Hi Zoltan,

Regarding the odp_time_local api, I made some change leveraging some DPDK api to avoiding system call.

I have use my change in my project, which benefits the performance.
Below is my diff.

Could you help review it? Any comments would be appreciated.

Thanks,
Wenxian

So for best performance, all functions should be re-written to use RDTSC values, instead of conversions between timespec and RDTSC counter value.

 }

 static inline int time_cmp(odp_time_t t2, odp_time_t t1)
@@ -103,14 +102,7 @@ static inline void time_wait_until(odp_time_t time)

 static inline uint64_t time_local_res(void)
 {
-         int ret;
-         struct timespec tres;
-
-         ret = clock_getres(CLOCK_MONOTONIC_RAW, &tres);
-         if (odp_unlikely(ret != 0))
-                     ODP_ABORT("clock_getres failed\n");
-
-         return ODP_TIME_SEC_IN_NS / (uint64_t)tres.tv_nsec;
+    return rte_get_tsc_hz();
 }

 odp_time_t odp_time_local(void)
@@ -179,28 +171,14 @@ void odp_time_wait_until(odp_time_t time)

 uint64_t odp_time_to_u64(odp_time_t time)
 {
-         int ret;
-         struct timespec tres;
-         uint64_t resolution;
-
-         ret = clock_getres(CLOCK_MONOTONIC_RAW, &tres);
-         if (odp_unlikely(ret != 0))
-                     ODP_ABORT("clock_getres failed\n");
-
-         resolution = (uint64_t)tres.tv_nsec;
-
-         return time_to_ns(time) / resolution;
+    return time_to_ns(time) * time_local_res() / ODP_TIME_SEC_IN_NS;

When u64 rdtsc value is stored into _odp_time_t, this would be simply
return time.u64;

 }

 int odp_time_init_global(void)
 {
-         int ret;
-         _odp_time_t time;
-
-         ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in<http://time.in>);
-         start_time = ret ? ODP_TIME_NULL : time.ex;
-
-         return ret;
+    start_time = ODP_TIME_NULL;
+    start_time = odp_time_local();
+    return 0;

Since RDTSC counters of various cores may run out of sync, some check would be needed (in init phase) if RDTSC can be used for global time stamp. For example, if  frequency scaling affects the value, counters will soon be out of sync after boot.



-Petri



 }

 int odp_time_term_global(void)

Comments

Wenxian Li April 11, 2016, 12:58 p.m. | #1
Thanks Petri & Zoltan.
Let me update the diff.

Thanks,
Wenxian

On 11 April 2016 at 20:19, Savolainen, Petri (Nokia - FI/Espoo) <
petri.savolainen@nokia.com> wrote:

> RDTSC counter may not be monotonic in all systems. Maybe some extra CPU

> flag check could be done to make sure that it is not affected by CPU

> frequency scaling. When RDTSC is not “invariant”,  DPDK seems to just warn

> and sample RDTSC for 1 sec get some estimate of the counter frequency. See

> more comments under.

>

>

>

>

>

>

>

> *From:* lng-odp-dpdk [mailto:lng-odp-dpdk-bounces@lists.linaro.org] *On

> Behalf Of *EXT Wenxian Li

> *Sent:* Thursday, April 07, 2016 2:41 PM

> *To:* Zoltan Kiss <zoltan.kiss@linaro.org>

> *Cc:* lng-odp-dpdk@lists.linaro.org

> *Subject:* [lng-odp-dpdk] Change of odp_time_local implementation in

> odp-dpdk

>

>

>

> Hi Zoltan,

>

>

>

> Regarding the odp_time_local api, I made some change leveraging some DPDK

> api to avoiding system call.

>

>

>

> I have use my change in my project, which benefits the performance.

>

> Below is my diff.

>

>

>

> Could you help review it? Any comments would be appreciated.

>

>

>

> Thanks,

>

> Wenxian

>

>

>

> diff --git a/platform/linux-generic/odp_time.c

> b/platform/linux-generic/odp_time.c

>

> index ca8a955..565ce93 100644

>

> --- a/platform/linux-generic/odp_time.c

>

> +++ b/platform/linux-generic/odp_time.c

>

> @@ -10,6 +10,7 @@

>

>  #include <odp/time.h>

>

>  #include <odp/hints.h>

>

>  #include <odp_debug_internal.h>

>

> +#include <rte_cycles.h>

>

>

>

>  typedef union {

>

>           odp_time_t      ex;

>

> @@ -17,6 +18,8 @@ typedef union {

>

>  } _odp_time_t;

>

>

>

>  static odp_time_t start_time;

>

> +static inline odp_time_t time_local_from_ns(uint64_t ns);

>

> +static inline uint64_t time_local_res(void);

>

>

>

>  static inline

>

>  uint64_t time_to_ns(odp_time_t time)

>

> @@ -46,14 +49,10 @@ static inline odp_time_t time_diff(odp_time_t t2,

> odp_time_t t1)

>

>

>

>  static inline odp_time_t time_local(void)

>

>  {

>

> -         int ret;

>

> -         _odp_time_t time;

>

> -

>

> -         ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in);

>

> -         if (odp_unlikely(ret != 0))

>

> -                     ODP_ABORT("clock_gettime failed\n");

>

> -

>

> -         return time_diff(time.ex, start_time);

>

> +    uint64_t ns = ((double)rte_rdtsc()/(double)time_local_res())

>

> +                  *ODP_TIME_SEC_IN_NS;

>

> +    odp_time_t time = time_local_from_ns(ns);

>

> +    return time_diff(time, start_time);

>

>

>

> ODP API is defined so that implementation can avoid these kind of (float)

> division operations per timestamp operation. This function should just read

> time stamp and store it into _*odp_time*_t (e.g. add a u64 bit field for

> that). Conversion to nsec should be done only when needed (in from/to_ns

> functions).

>

>

>

> So for best performance, all functions should be re-written to use RDTSC

> values, instead of conversions between timespec and RDTSC counter value.

>

>

>

>  }

>

>

>

>  static inline int time_cmp(odp_time_t t2, odp_time_t t1)

>

> @@ -103,14 +102,7 @@ static inline void time_wait_until(odp_time_t time)

>

>

>

>  static inline uint64_t time_local_res(void)

>

>  {

>

> -         int ret;

>

> -         struct timespec tres;

>

> -

>

> -         ret = clock_getres(CLOCK_MONOTONIC_RAW, &tres);

>

> -         if (odp_unlikely(ret != 0))

>

> -                     ODP_ABORT("clock_getres failed\n");

>

> -

>

> -         return ODP_TIME_SEC_IN_NS / (uint64_t)tres.tv_nsec;

>

> +    return rte_get_tsc_hz();

>

>  }

>

>

>

>  odp_time_t odp_time_local(void)

>

> @@ -179,28 +171,14 @@ void odp_time_wait_until(odp_time_t time)

>

>

>

>  uint64_t odp_time_to_u64(odp_time_t time)

>

>  {

>

> -         int ret;

>

> -         struct timespec tres;

>

> -         uint64_t resolution;

>

> -

>

> -         ret = clock_getres(CLOCK_MONOTONIC_RAW, &tres);

>

> -         if (odp_unlikely(ret != 0))

>

> -                     ODP_ABORT("clock_getres failed\n");

>

> -

>

> -         resolution = (uint64_t)tres.tv_nsec;

>

> -

>

> -         return time_to_ns(time) / resolution;

>

> +    return time_to_ns(time) * time_local_res() / ODP_TIME_SEC_IN_NS;

>

>

>

> When u64 rdtsc value is stored into _*odp_time*_t, this would be simply

>

> return time.u64;

>

>

>

>  }

>

>

>

>  int odp_time_init_global(void)

>

>  {

>

> -         int ret;

>

> -         _odp_time_t time;

>

> -

>

> -         ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in);

>

> -         start_time = ret ? ODP_TIME_NULL : time.ex;

>

> -

>

> -         return ret;

>

> +    start_time = ODP_TIME_NULL;

>

> +    start_time = odp_time_local();

>

> +    return 0;

>

>

>

> Since RDTSC counters of various cores may run out of sync, some check

> would be needed (in init phase) if RDTSC can be used for global time stamp.

> For example, if  frequency scaling affects the value, counters will soon be

> out of sync after boot.

>

>

>

>

>

>

>

> -Petri

>

>

>

>

>

>

>

>  }

>

>

>

>  int odp_time_term_global(void)

>

Patch hide | download patch | download mbox

diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c
index ca8a955..565ce93 100644
--- a/platform/linux-generic/odp_time.c
+++ b/platform/linux-generic/odp_time.c
@@ -10,6 +10,7 @@ 
 #include <odp/time.h>
 #include <odp/hints.h>
 #include <odp_debug_internal.h>
+#include <rte_cycles.h>

 typedef union {
          odp_time_t      ex;
@@ -17,6 +18,8 @@  typedef union {
 } _odp_time_t;

 static odp_time_t start_time;
+static inline odp_time_t time_local_from_ns(uint64_t ns);
+static inline uint64_t time_local_res(void);

 static inline
 uint64_t time_to_ns(odp_time_t time)
@@ -46,14 +49,10 @@  static inline odp_time_t time_diff(odp_time_t t2, odp_time_t t1)

 static inline odp_time_t time_local(void)
 {
-         int ret;
-         _odp_time_t time;
-
-         ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in<http://time.in>);
-         if (odp_unlikely(ret != 0))
-                     ODP_ABORT("clock_gettime failed\n");
-
-         return time_diff(time.ex, start_time);
+    uint64_t ns = ((double)rte_rdtsc()/(double)time_local_res())
+                  *ODP_TIME_SEC_IN_NS;
+    odp_time_t time = time_local_from_ns(ns);
+    return time_diff(time, start_time);

ODP API is defined so that implementation can avoid these kind of (float) division operations per timestamp operation. This function should just read time stamp and store it into _odp_time_t (e.g. add a u64 bit field for that). Conversion to nsec should be done only when needed (in from/to_ns functions).