Change of odp_time_local implementation in odp-dpdk

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

Commit Message

Savolainen, Petri (Nokia - FI/Espoo) April 21, 2016, 10:12 a.m.
Third try, still >100KB. Now In plain text.


From: Savolainen, Petri (Nokia - FI/Espoo) 

Sent: Thursday, April 21, 2016 1:10 PM
To: 'EXT Wenxian Li' <wenxian.li@linaro.org>
Cc: 'Zoltan Kiss' <zoltan.kiss@linaro.org>; 'lng-odp-dpdk@lists.linaro.org' <lng-odp-dpdk@lists.linaro.org>
Subject: RE: [lng-odp-dpdk] Change of odp_time_local implementation in odp-dpdk

A new try. List blocked >100KB HTML mail??

From: Savolainen, Petri (Nokia - FI/Espoo) 

Sent: Thursday, April 21, 2016 11:00 AM
To: 'EXT Wenxian Li' <wenxian.li@linaro.org>
Cc: Zoltan Kiss <zoltan.kiss@linaro.org>; lng-odp-dpdk@lists.linaro.org
Subject: RE: [lng-odp-dpdk] Change of odp_time_local implementation in odp-dpdk

Hi,

Overall the diff looks good - so the next step is to send it as a patch to the list (using git format-patch and git send-email). See more comments under.



From: EXT Wenxian Li [mailto:wenxian.li@linaro.org] 

Sent: Wednesday, April 20, 2016 7:29 PM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com>
Cc: Zoltan Kiss <zoltan.kiss@linaro.org>; lng-odp-dpdk@lists.linaro.org
Subject: Re: [lng-odp-dpdk] Change of odp_time_local implementation in odp-dpdk

Hi Zoltan & Petri, 

I update the diff. 
What I changed includes:
i)   Copy odp_time.c from linux_generic to odp_dpdk. Makefile is updated. 
ii)  Change odp_time.c file to leveraging dpdk api. 
iii) The priority of timer source is: TSC (if invariant TSC is supported) > HPET (If enabled) > syscall clockgettime

Could you help review the diff? 
Any comments are welcomed.

Thanks,
Wenxian




 {
          uint64_t ns;

@@ -29,7 +37,14 @@ uint64_t time_to_ns(odp_time_t time)
          return ns;
 }

-static inline odp_time_t time_diff(odp_time_t t2, odp_time_t t1)
+static inline
+uint64_t time_to_ns_dpdk(odp_time_t time)

Write entire function prototype on single line (including “static inline”)


+{
+        return (time.ticks*ODP_TIME_SEC_IN_NS/time_local_res_dpdk());
+}
+
+
+static inline odp_time_t time_diff_linux(odp_time_t t2, odp_time_t t1)
 {
          odp_time_t time;

@@ -44,19 +59,45 @@ static inline odp_time_t time_diff(odp_time_t t2, odp_time_t t1)
          return time;
 }

-static inline odp_time_t time_local(void)
+static inline odp_time_t time_diff_dpdk(odp_time_t t2, odp_time_t t1)
+{
+        odp_time_t time;
+        time.ticks = t2.ticks - t1.ticks;
+        return time;
+}
+
+
+static inline odp_time_t time_curr_linux(void)
 {
          int ret;
          _odp_time_t time;

          ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in);
-         if (odp_unlikely(ret != 0))
+        if (odp_unlikely(ret != 0)) {
                      ODP_ABORT("clock_gettime failed\n");

Abort should call exit here, so no need to init for return. If there’s a real problem with uninitialized, it’s better to correct that into odp-linux repo (and re-use the same code here).

+                    time.ex.tv_sec = 0;
+                    time.ex.tv_nsec = 0;
+        }
+        return time.ex;
+}
+
+static inline odp_time_t time_curr_dpdk(void)
+{
+        odp_time_t time;
+        time.ticks = rte_get_timer_cycles();
+        return time;
+}

-         return time_diff(time.ex, start_time);
+static inline odp_time_t time_local(void)
+{
+        odp_time_t time;
+        time = time_handler.time_curr();
+        return time_handler.time_diff(time, start_time);
 }

-static inline int time_cmp(odp_time_t t2, odp_time_t t1)
+
+
+static inline int time_cmp_linux(odp_time_t t2, odp_time_t t1)
 {
          if (t2.tv_sec < t1.tv_sec)
                      return -1;
@@ -67,7 +108,13 @@ static inline int time_cmp(odp_time_t t2, odp_time_t t1)
          return t2.tv_nsec - t1.tv_nsec;
 }

-static inline odp_time_t time_sum(odp_time_t t1, odp_time_t t2)
+static inline int time_cmp_dpdk(odp_time_t t2, odp_time_t t1)
+{
+        return t2.ticks - t1.ticks;

Ticks is u64 and return value is int. This overflows with large differences (>31 bits == about 1 sec @2GHz), so some more math/comparison is needed.

+}
+
+
+static inline odp_time_t time_sum_linux(odp_time_t t1, odp_time_t t2)
 {
          odp_time_t time;

@@ -82,7 +129,15 @@ static inline odp_time_t time_sum(odp_time_t t1, odp_time_t t2)
          return time;
 }

-static inline odp_time_t time_local_from_ns(uint64_t ns)
+static inline odp_time_t time_sum_dpdk(odp_time_t t1, odp_time_t t2)
+{
+        odp_time_t time;
+        time.ticks = t2.ticks + t1.ticks;
+        return time;
+}
+
+
+static inline odp_time_t time_local_from_ns_linux(uint64_t ns)
 {
          odp_time_t time;

@@ -92,16 +147,23 @@ static inline odp_time_t time_local_from_ns(uint64_t ns)
          return time;
 }

+static inline odp_time_t time_local_from_ns_dpdk(uint64_t ns)
+{
+        odp_time_t time;
+        time.ticks = ns * time_local_res_dpdk() / ODP_TIME_SEC_IN_NS;
+        return time;
+}
+
 static inline void time_wait_until(odp_time_t time)
 {
          odp_time_t cur;

          do {
                      cur = time_local();
-         } while (time_cmp(time, cur) > 0);
+        } while (time_handler.time_cmp(time, cur) > 0);
 }

-static inline uint64_t time_local_res(void)
+static inline uint64_t time_local_res_linux(void)
 {
          int ret;
          struct timespec tres;
@@ -125,49 +187,49 @@ odp_time_t odp_time_global(void)

 odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1)
 {
-         return time_diff(t2, t1);
+        return time_handler.time_diff(t2, t1);
 }

 uint64_t odp_time_to_ns(odp_time_t time)
 {
-         return time_to_ns(time);
+        return time_handler.time_to_ns(time);
 }

 odp_time_t odp_time_local_from_ns(uint64_t ns)
 {
-         return time_local_from_ns(ns);
+        return time_handler.time_local_from_ns(ns);
 }

 odp_time_t odp_time_global_from_ns(uint64_t ns)
 {
-         return time_local_from_ns(ns);
+        return time_handler.time_local_from_ns(ns);
 }

 int odp_time_cmp(odp_time_t t2, odp_time_t t1)
 {
-         return time_cmp(t2, t1);
+        return time_handler.time_cmp(t2, t1);
 }

 odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2)
 {
-         return time_sum(t1, t2);
+        return time_handler.time_sum(t1, t2);
 }

 uint64_t odp_time_local_res(void)
 {
-         return time_local_res();
+        return time_handler.time_local_res();
 }

 uint64_t odp_time_global_res(void)
 {
-         return time_local_res();
+        return time_handler.time_local_res();
 }

 void odp_time_wait_ns(uint64_t ns)
 {
          odp_time_t cur = time_local();
-         odp_time_t wait = time_local_from_ns(ns);
-         odp_time_t end_time = time_sum(cur, wait);
+        odp_time_t wait = time_handler.time_local_from_ns(ns);
+        odp_time_t end_time = time_handler.time_sum(cur, wait);

          time_wait_until(end_time);
 }
@@ -177,7 +239,7 @@ void odp_time_wait_until(odp_time_t time)
          return time_wait_until(time);
 }

-uint64_t odp_time_to_u64(odp_time_t time)
+static uint64_t time_to_u64_linux(odp_time_t time)
 {
          int ret;
          struct timespec tres;
@@ -189,18 +251,99 @@ uint64_t odp_time_to_u64(odp_time_t time)

          resolution = (uint64_t)tres.tv_nsec;

-         return time_to_ns(time) / resolution;
+        return time_handler.time_to_ns(time) / resolution;
 }

-int odp_time_init_global(void)
+static uint64_t time_to_u64_dpdk(odp_time_t time)
 {
-         int ret;
-         _odp_time_t time;
+        return time.ticks;
+}

-         ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in);
-         start_time = ret ? ODP_TIME_NULL : time.ex;
+uint64_t odp_time_to_u64(odp_time_t time)
+{
+        return time_handler.time_to_u64(time);
+}
+
+
+static bool
+is_invariant_tsc_supported(void)

Entire prototype on single line. I’d rather use odp_bool_t or int, so that code is less dependent on C99.

+{
+        FILE  *file;
+        char  *line         = NULL;
+        size_t len          = 0;
+        bool   nonstop_tsc  = false;
+        bool   constant_tsc = false;
+        bool   ret = false;
+
+        file = fopen("/proc/cpuinfo", "rt");
+        while (getline(&line, &len, file) != -1) {
+                    if (strstr(line, "flags") !=NULL) {
+                                if (strstr(line, "constant_tsc") !=NULL) {
+                                            constant_tsc = true;
+                                }

Checkpatch will complain about parenthesis with single line if-statement. Run “perl scripts/checkpatch.pl *.patch” for your patches next time.



+                                if (strstr(line, "nonstop_tsc") != NULL) {
+                                            nonstop_tsc = true;
+                                }
+                                if (constant_tsc && nonstop_tsc) {
+                                            ret = true;
+                                } else {
+                                            ret = false;
+                                }
+                                free(line);
+                                fclose(file);
+                                return ret;
+                    }
+        }
+        free(line);
+        fclose(file);
+        return false;
+}
+
+static inline bool
+is_dpdk_timer_cycles_support(void)

Prototype on single line.

+{
+        if (is_invariant_tsc_supported()==true) {
+                    return true;
+        }
+        #ifdef RTE_LIBEAL_USE_HPET
+        if (rte_eal_hpet_init(1) == 0) {
+                    return true;
+        }
+        #endif
+        return false;
+}

-         return ret;
+int odp_time_init_global(void)
+{
+        if (is_dpdk_timer_cycles_support()) {
+                    time_handler.time_to_ns         = time_to_ns_dpdk;
+                    time_handler.time_diff          = time_diff_dpdk;
+                    time_handler.time_curr          = time_curr_dpdk;
+                    time_handler.time_cmp           = time_cmp_dpdk;
+                    time_handler.time_sum           = time_sum_dpdk;
+                    time_handler.time_local_from_ns = time_local_from_ns_dpdk;
+                    time_handler.time_local_res     = time_local_res_dpdk;
+                    time_handler.time_to_u64        = time_to_u64_dpdk;
+        } else {
+                    time_handler.time_to_ns         = time_to_ns_linux;
+                    time_handler.time_diff          = time_diff_linux;
+                    time_handler.time_curr          = time_curr_linux;
+                    time_handler.time_cmp           = time_cmp_linux;
+                    time_handler.time_sum           = time_sum_linux;
+                    time_handler.time_local_from_ns = time_local_from_ns_linux;
+                    time_handler.time_local_res     = time_local_res_linux;
+                    time_handler.time_to_u64        = time_to_u64_linux;
+        }
+
+        start_time = time_handler.time_curr();
+        odp_time_t zero_time;

All variables must be introduced in the beginning of a code block (first lines of the function in this case).

-Petri


+        zero_time = ODP_TIME_NULL;
+        if (time_handler.time_cmp(start_time, ODP_) == 0) {
+                    ODP_ABORT("initiate odp time failed\n");
+                    return -1;
+        } else {
+                    return 0;
+        }
 }

 int odp_time_term_global(void)

Comments

Mike Holmes April 21, 2016, 11:06 a.m. | #1
On 21 April 2016 at 06:12, Savolainen, Petri (Nokia - FI/Espoo) <
petri.savolainen@nokia.com> wrote:

> Third try, still >100KB. Now In plain text.

>


The list is set to block at 100K, I will bump it up to 200k


>

>

> From: Savolainen, Petri (Nokia - FI/Espoo)

> Sent: Thursday, April 21, 2016 1:10 PM

> To: 'EXT Wenxian Li' <wenxian.li@linaro.org>

> Cc: 'Zoltan Kiss' <zoltan.kiss@linaro.org>; 'lng-odp-dpdk@lists.linaro.org'

> <lng-odp-dpdk@lists.linaro.org>

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

> odp-dpdk

>

> A new try. List blocked >100KB HTML mail??

>

> From: Savolainen, Petri (Nokia - FI/Espoo)

> Sent: Thursday, April 21, 2016 11:00 AM

> To: 'EXT Wenxian Li' <wenxian.li@linaro.org>

> Cc: Zoltan Kiss <zoltan.kiss@linaro.org>; lng-odp-dpdk@lists.linaro.org

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

> odp-dpdk

>

> Hi,

>

> Overall the diff looks good - so the next step is to send it as a patch to

> the list (using git format-patch and git send-email). See more comments

> under.

>

>

>

> From: EXT Wenxian Li [mailto:wenxian.li@linaro.org]

> Sent: Wednesday, April 20, 2016 7:29 PM

> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com>

> Cc: Zoltan Kiss <zoltan.kiss@linaro.org>; lng-odp-dpdk@lists.linaro.org

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

> odp-dpdk

>

> Hi Zoltan & Petri,

>

> I update the diff.

> What I changed includes:

> i)   Copy odp_time.c from linux_generic to odp_dpdk. Makefile is updated.

> ii)  Change odp_time.c file to leveraging dpdk api.

> iii) The priority of timer source is: TSC (if invariant TSC is supported)

> > HPET (If enabled) > syscall clockgettime

>

> Could you help review the diff?

> Any comments are welcomed.

>

> Thanks,

> Wenxian

>

> diff --git a/platform/linux-dpdk/include/odp/plat/time_types.h

> b/platform/linux-dpdk/include/odp/plat/time_types.h

> deleted file mode 120000

> index 31d16ad..0000000

> --- a/platform/linux-dpdk/include/odp/plat/time_types.h

> +++ /dev/null

> @@ -1 +0,0 @@

> -../../../../linux-generic/include/odp/plat/time_types.h

> \ No newline at end of file

> diff --git a/platform/linux-dpdk/include/odp/plat/time_types.h

> b/platform/linux-dpdk/include/odp/plat/time_types.h

> new file mode 100644

> index 0000000..00a28e5

> --- /dev/null

> +++ b/platform/linux-dpdk/include/odp/plat/time_types.h

> @@ -0,0 +1,69 @@

> +/* Copyright (c) 2015, Linaro Limited

>

> Year 2016

>

>

> + * All rights reserved.

> + *

> + * SPDX-License-Identifier:     BSD-3-Clause

> + */

> +

> +/**

> + * @file

> + *

> + * ODP time service

> + */

> +

> +#ifndef ODP_TIME_TYPES_H_

> +#define ODP_TIME_TYPES_H_

> +

> +#ifdef __cplusplus

> +extern "C" {

> +#endif

> +

> +/** @addtogroup odp_time

> + *  @{

> + **/

>

> These group tags can be removed. Original odp-linux file has those, but

> those should not be needed. Internal documentation of these types is not

> needed in API documentation (that’s why @internal is used under).

>

> +

> +/**

> + * @internal Time structure used to isolate linux-dpdk implementation from

> + * the linux timespec structure, which is dependent on POSIX extension

> level.

> + */

> +typedef union {

> +    struct {

> +        uint64_t ticks;  /**< @internal ticks */

> +        uint64_t pad;

> +    };

> +    struct {

> +        int64_t tv_sec;      /**< @internal Seconds */

> +        int64_t tv_nsec;     /**< @internal Nanoseconds */

> +    };

> +} odp_time_t;

> +

> +#define ODP_TIME_NULL ((odp_time_t){{0}})

> +

> +typedef uint64_t (*time_to_ns_fn) (odp_time_t time);

> +typedef odp_time_t (*time_diff_fn) (odp_time_t t2, odp_time_t t1);

> +typedef odp_time_t (*time_curr_fn)(void);

> +typedef int (*time_cmp_fn) (odp_time_t t2, odp_time_t t1);

> +typedef odp_time_t (*time_sum_fn) (odp_time_t t1, odp_time_t t2);

> +typedef odp_time_t (*time_local_from_ns_fn) (uint64_t ns);

> +typedef uint64_t (*time_local_res_fn)(void);

> +typedef uint64_t (*time_to_u64_fn) (odp_time_t time);

> +

> +typedef struct time_handler_ {

> +    time_to_ns_fn         time_to_ns;

> +    time_diff_fn          time_diff;

> +    time_curr_fn          time_curr;

> +    time_cmp_fn           time_cmp;

> +    time_sum_fn           time_sum;

> +    time_local_from_ns_fn time_local_from_ns;

> +    time_local_res_fn     time_local_res;

> +    time_to_u64_fn        time_to_u64;

> +} time_handler_t;

> +

>

> Move this struct and the typedefs above it into a new internal header

> file. If the definitions are here, those are visible to application through

> the API (unnecessary confusion and name space pollution).

>

> The new file would be: platform/linux-dpdk/include/odp_time_internal.h

>

>

> +/**

> + * @}

> + */

>

> Remember to remove this with @addtogroup

>

> +

> +#ifdef __cplusplus

> +}

> +#endif

> +

> +#endif

> diff --git a/platform/linux-dpdk/odp_init.c

> b/platform/linux-dpdk/odp_init.c

> index 833621d..1d01aea 100644

> --- a/platform/linux-dpdk/odp_init.c

> +++ b/platform/linux-dpdk/odp_init.c

> @@ -246,12 +246,6 @@ int odp_init_global(const odp_init_t *params,

>                                   odp_global_data.abort_fn =

> params->abort_fn;

>           }

>

> -         if (odp_time_init_global()) {

> -                     ODP_ERR("ODP time init failed.\n");

> -                     goto init_failed;

> -         }

> -         stage = TIME_INIT;

> -

>           if (odp_system_info_init()) {

>                       ODP_ERR("ODP system_info init failed.\n");

>                       goto init_failed;

> @@ -263,6 +257,12 @@ int odp_init_global(const odp_init_t *params,

>                       return -1;

>           }

>

> +        if (odp_time_init_global()) {

> +                    ODP_ERR("ODP time init failed.\n");

> +                    goto init_failed;

> +        }

> +        stage = TIME_INIT;

> +

>

> I’d do the same change in odp-linux, so that odp-dpdk does not need to be

> different here. It’s common need to first parse system information and then

> use that information on the following global inits.

>

>

>

>           if (odp_shm_init_global()) {

>                       ODP_ERR("ODP shm init failed.\n");

>                       goto init_failed;

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

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

> index ca8a955..a7f885b 100644

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

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

> @@ -10,6 +10,8 @@

>  #include <odp/time.h>

>  #include <odp/hints.h>

>  #include <odp_debug_internal.h>

> +#include <rte_cycles.h>

> +#include <string.h>

>

>  typedef union {

>           odp_time_t      ex;

> @@ -17,9 +19,15 @@ typedef union {

>  } _odp_time_t;

>

>  static odp_time_t start_time;

> +static time_handler_t time_handler;

> +

> +static inline uint64_t time_local_res_dpdk(void)

> +{

> +        return rte_get_timer_hz();

> +}

>

>  static inline

> -uint64_t time_to_ns(odp_time_t time)

> +uint64_t time_to_ns_linux(odp_time_t time)

>

>

> Would it be easier to maintain the code, if these linux functions remain

> as is (in sync with odp-linux) and add only new functions and structs.

>

>

>

>  {

>           uint64_t ns;

>

> @@ -29,7 +37,14 @@ uint64_t time_to_ns(odp_time_t time)

>           return ns;

>  }

>

> -static inline odp_time_t time_diff(odp_time_t t2, odp_time_t t1)

> +static inline

> +uint64_t time_to_ns_dpdk(odp_time_t time)

>

> Write entire function prototype on single line (including “static inline”)

>

>

> +{

> +        return (time.ticks*ODP_TIME_SEC_IN_NS/time_local_res_dpdk());

> +}

> +

> +

> +static inline odp_time_t time_diff_linux(odp_time_t t2, odp_time_t t1)

>  {

>           odp_time_t time;

>

> @@ -44,19 +59,45 @@ static inline odp_time_t time_diff(odp_time_t t2,

> odp_time_t t1)

>           return time;

>  }

>

> -static inline odp_time_t time_local(void)

> +static inline odp_time_t time_diff_dpdk(odp_time_t t2, odp_time_t t1)

> +{

> +        odp_time_t time;

> +        time.ticks = t2.ticks - t1.ticks;

> +        return time;

> +}

> +

> +

> +static inline odp_time_t time_curr_linux(void)

>  {

>           int ret;

>           _odp_time_t time;

>

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

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

> +        if (odp_unlikely(ret != 0)) {

>                       ODP_ABORT("clock_gettime failed\n");

>

> Abort should call exit here, so no need to init for return. If there’s a

> real problem with uninitialized, it’s better to correct that into odp-linux

> repo (and re-use the same code here).

>

> +                    time.ex.tv_sec = 0;

> +                    time.ex.tv_nsec = 0;

> +        }

> +        return time.ex;

> +}

> +

> +static inline odp_time_t time_curr_dpdk(void)

> +{

> +        odp_time_t time;

> +        time.ticks = rte_get_timer_cycles();

> +        return time;

> +}

>

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

> +static inline odp_time_t time_local(void)

> +{

> +        odp_time_t time;

> +        time = time_handler.time_curr();

> +        return time_handler.time_diff(time, start_time);

>  }

>

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

> +

> +

> +static inline int time_cmp_linux(odp_time_t t2, odp_time_t t1)

>  {

>           if (t2.tv_sec < t1.tv_sec)

>                       return -1;

> @@ -67,7 +108,13 @@ static inline int time_cmp(odp_time_t t2, odp_time_t

> t1)

>           return t2.tv_nsec - t1.tv_nsec;

>  }

>

> -static inline odp_time_t time_sum(odp_time_t t1, odp_time_t t2)

> +static inline int time_cmp_dpdk(odp_time_t t2, odp_time_t t1)

> +{

> +        return t2.ticks - t1.ticks;

>

> Ticks is u64 and return value is int. This overflows with large

> differences (>31 bits == about 1 sec @2GHz), so some more math/comparison

> is needed.

>

> +}

> +

> +

> +static inline odp_time_t time_sum_linux(odp_time_t t1, odp_time_t t2)

>  {

>           odp_time_t time;

>

> @@ -82,7 +129,15 @@ static inline odp_time_t time_sum(odp_time_t t1,

> odp_time_t t2)

>           return time;

>  }

>

> -static inline odp_time_t time_local_from_ns(uint64_t ns)

> +static inline odp_time_t time_sum_dpdk(odp_time_t t1, odp_time_t t2)

> +{

> +        odp_time_t time;

> +        time.ticks = t2.ticks + t1.ticks;

> +        return time;

> +}

> +

> +

> +static inline odp_time_t time_local_from_ns_linux(uint64_t ns)

>  {

>           odp_time_t time;

>

> @@ -92,16 +147,23 @@ static inline odp_time_t time_local_from_ns(uint64_t

> ns)

>           return time;

>  }

>

> +static inline odp_time_t time_local_from_ns_dpdk(uint64_t ns)

> +{

> +        odp_time_t time;

> +        time.ticks = ns * time_local_res_dpdk() / ODP_TIME_SEC_IN_NS;

> +        return time;

> +}

> +

>  static inline void time_wait_until(odp_time_t time)

>  {

>           odp_time_t cur;

>

>           do {

>                       cur = time_local();

> -         } while (time_cmp(time, cur) > 0);

> +        } while (time_handler.time_cmp(time, cur) > 0);

>  }

>

> -static inline uint64_t time_local_res(void)

> +static inline uint64_t time_local_res_linux(void)

>  {

>           int ret;

>           struct timespec tres;

> @@ -125,49 +187,49 @@ odp_time_t odp_time_global(void)

>

>  odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1)

>  {

> -         return time_diff(t2, t1);

> +        return time_handler.time_diff(t2, t1);

>  }

>

>  uint64_t odp_time_to_ns(odp_time_t time)

>  {

> -         return time_to_ns(time);

> +        return time_handler.time_to_ns(time);

>  }

>

>  odp_time_t odp_time_local_from_ns(uint64_t ns)

>  {

> -         return time_local_from_ns(ns);

> +        return time_handler.time_local_from_ns(ns);

>  }

>

>  odp_time_t odp_time_global_from_ns(uint64_t ns)

>  {

> -         return time_local_from_ns(ns);

> +        return time_handler.time_local_from_ns(ns);

>  }

>

>  int odp_time_cmp(odp_time_t t2, odp_time_t t1)

>  {

> -         return time_cmp(t2, t1);

> +        return time_handler.time_cmp(t2, t1);

>  }

>

>  odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2)

>  {

> -         return time_sum(t1, t2);

> +        return time_handler.time_sum(t1, t2);

>  }

>

>  uint64_t odp_time_local_res(void)

>  {

> -         return time_local_res();

> +        return time_handler.time_local_res();

>  }

>

>  uint64_t odp_time_global_res(void)

>  {

> -         return time_local_res();

> +        return time_handler.time_local_res();

>  }

>

>  void odp_time_wait_ns(uint64_t ns)

>  {

>           odp_time_t cur = time_local();

> -         odp_time_t wait = time_local_from_ns(ns);

> -         odp_time_t end_time = time_sum(cur, wait);

> +        odp_time_t wait = time_handler.time_local_from_ns(ns);

> +        odp_time_t end_time = time_handler.time_sum(cur, wait);

>

>           time_wait_until(end_time);

>  }

> @@ -177,7 +239,7 @@ void odp_time_wait_until(odp_time_t time)

>           return time_wait_until(time);

>  }

>

> -uint64_t odp_time_to_u64(odp_time_t time)

> +static uint64_t time_to_u64_linux(odp_time_t time)

>  {

>           int ret;

>           struct timespec tres;

> @@ -189,18 +251,99 @@ uint64_t odp_time_to_u64(odp_time_t time)

>

>           resolution = (uint64_t)tres.tv_nsec;

>

> -         return time_to_ns(time) / resolution;

> +        return time_handler.time_to_ns(time) / resolution;

>  }

>

> -int odp_time_init_global(void)

> +static uint64_t time_to_u64_dpdk(odp_time_t time)

>  {

> -         int ret;

> -         _odp_time_t time;

> +        return time.ticks;

> +}

>

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

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

> +uint64_t odp_time_to_u64(odp_time_t time)

> +{

> +        return time_handler.time_to_u64(time);

> +}

> +

> +

> +static bool

> +is_invariant_tsc_supported(void)

>

> Entire prototype on single line. I’d rather use odp_bool_t or int, so that

> code is less dependent on C99.

>

> +{

> +        FILE  *file;

> +        char  *line         = NULL;

> +        size_t len          = 0;

> +        bool   nonstop_tsc  = false;

> +        bool   constant_tsc = false;

> +        bool   ret = false;

> +

> +        file = fopen("/proc/cpuinfo", "rt");

> +        while (getline(&line, &len, file) != -1) {

> +                    if (strstr(line, "flags") !=NULL) {

> +                                if (strstr(line, "constant_tsc") !=NULL) {

> +                                            constant_tsc = true;

> +                                }

>

> Checkpatch will complain about parenthesis with single line if-statement.

> Run “perl scripts/checkpatch.pl *.patch” for your patches next time.

>

>

>

> +                                if (strstr(line, "nonstop_tsc") != NULL) {

> +                                            nonstop_tsc = true;

> +                                }

> +                                if (constant_tsc && nonstop_tsc) {

> +                                            ret = true;

> +                                } else {

> +                                            ret = false;

> +                                }

> +                                free(line);

> +                                fclose(file);

> +                                return ret;

> +                    }

> +        }

> +        free(line);

> +        fclose(file);

> +        return false;

> +}

> +

> +static inline bool

> +is_dpdk_timer_cycles_support(void)

>

> Prototype on single line.

>

> +{

> +        if (is_invariant_tsc_supported()==true) {

> +                    return true;

> +        }

> +        #ifdef RTE_LIBEAL_USE_HPET

> +        if (rte_eal_hpet_init(1) == 0) {

> +                    return true;

> +        }

> +        #endif

> +        return false;

> +}

>

> -         return ret;

> +int odp_time_init_global(void)

> +{

> +        if (is_dpdk_timer_cycles_support()) {

> +                    time_handler.time_to_ns         = time_to_ns_dpdk;

> +                    time_handler.time_diff          = time_diff_dpdk;

> +                    time_handler.time_curr          = time_curr_dpdk;

> +                    time_handler.time_cmp           = time_cmp_dpdk;

> +                    time_handler.time_sum           = time_sum_dpdk;

> +                    time_handler.time_local_from_ns =

> time_local_from_ns_dpdk;

> +                    time_handler.time_local_res     = time_local_res_dpdk;

> +                    time_handler.time_to_u64        = time_to_u64_dpdk;

> +        } else {

> +                    time_handler.time_to_ns         = time_to_ns_linux;

> +                    time_handler.time_diff          = time_diff_linux;

> +                    time_handler.time_curr          = time_curr_linux;

> +                    time_handler.time_cmp           = time_cmp_linux;

> +                    time_handler.time_sum           = time_sum_linux;

> +                    time_handler.time_local_from_ns =

> time_local_from_ns_linux;

> +                    time_handler.time_local_res     =

> time_local_res_linux;

> +                    time_handler.time_to_u64        = time_to_u64_linux;

> +        }

> +

> +        start_time = time_handler.time_curr();

> +        odp_time_t zero_time;

>

> All variables must be introduced in the beginning of a code block (first

> lines of the function in this case).

>

> -Petri

>

>

> +        zero_time = ODP_TIME_NULL;

> +        if (time_handler.time_cmp(start_time, ODP_) == 0) {

> +                    ODP_ABORT("initiate odp time failed\n");

> +                    return -1;

> +        } else {

> +                    return 0;

> +        }

>  }

>

>  int odp_time_term_global(void)

> _______________________________________________

> lng-odp-dpdk mailing list

> lng-odp-dpdk@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp-dpdk

>




-- 
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
"Work should be fun and collaborative, the rest follows"

Patch hide | download patch | download mbox

diff --git a/platform/linux-dpdk/include/odp/plat/time_types.h b/platform/linux-dpdk/include/odp/plat/time_types.h
deleted file mode 120000
index 31d16ad..0000000
--- a/platform/linux-dpdk/include/odp/plat/time_types.h
+++ /dev/null
@@ -1 +0,0 @@ 
-../../../../linux-generic/include/odp/plat/time_types.h
\ No newline at end of file
diff --git a/platform/linux-dpdk/include/odp/plat/time_types.h b/platform/linux-dpdk/include/odp/plat/time_types.h
new file mode 100644
index 0000000..00a28e5
--- /dev/null
+++ b/platform/linux-dpdk/include/odp/plat/time_types.h
@@ -0,0 +1,69 @@ 
+/* Copyright (c) 2015, Linaro Limited

Year 2016


+ * All rights reserved.
+ *
+ * SPDX-License-Identifier:     BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP time service
+ */
+
+#ifndef ODP_TIME_TYPES_H_
+#define ODP_TIME_TYPES_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/** @addtogroup odp_time
+ *  @{
+ **/

These group tags can be removed. Original odp-linux file has those, but those should not be needed. Internal documentation of these types is not needed in API documentation (that’s why @internal is used under). 

+
+/**
+ * @internal Time structure used to isolate linux-dpdk implementation from
+ * the linux timespec structure, which is dependent on POSIX extension level.
+ */
+typedef union {
+    struct {
+        uint64_t ticks;  /**< @internal ticks */
+        uint64_t pad;
+    };
+    struct {
+        int64_t tv_sec;      /**< @internal Seconds */
+        int64_t tv_nsec;     /**< @internal Nanoseconds */
+    };
+} odp_time_t;
+
+#define ODP_TIME_NULL ((odp_time_t){{0}})
+
+typedef uint64_t (*time_to_ns_fn) (odp_time_t time);
+typedef odp_time_t (*time_diff_fn) (odp_time_t t2, odp_time_t t1);
+typedef odp_time_t (*time_curr_fn)(void);
+typedef int (*time_cmp_fn) (odp_time_t t2, odp_time_t t1);
+typedef odp_time_t (*time_sum_fn) (odp_time_t t1, odp_time_t t2);
+typedef odp_time_t (*time_local_from_ns_fn) (uint64_t ns);
+typedef uint64_t (*time_local_res_fn)(void);
+typedef uint64_t (*time_to_u64_fn) (odp_time_t time);
+
+typedef struct time_handler_ {
+    time_to_ns_fn         time_to_ns;
+    time_diff_fn          time_diff;
+    time_curr_fn          time_curr;
+    time_cmp_fn           time_cmp;
+    time_sum_fn           time_sum;
+    time_local_from_ns_fn time_local_from_ns;
+    time_local_res_fn     time_local_res;
+    time_to_u64_fn        time_to_u64;
+} time_handler_t;
+

Move this struct and the typedefs above it into a new internal header file. If the definitions are here, those are visible to application through the API (unnecessary confusion and name space pollution).

The new file would be: platform/linux-dpdk/include/odp_time_internal.h


+/**
+ * @}
+ */

Remember to remove this with @addtogroup

+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-dpdk/odp_init.c
index 833621d..1d01aea 100644
--- a/platform/linux-dpdk/odp_init.c
+++ b/platform/linux-dpdk/odp_init.c
@@ -246,12 +246,6 @@  int odp_init_global(const odp_init_t *params,
                                  odp_global_data.abort_fn = params->abort_fn;
          }

-         if (odp_time_init_global()) {
-                     ODP_ERR("ODP time init failed.\n");
-                     goto init_failed;
-         }
-         stage = TIME_INIT;
-
          if (odp_system_info_init()) {
                      ODP_ERR("ODP system_info init failed.\n");
                      goto init_failed;
@@ -263,6 +257,12 @@  int odp_init_global(const odp_init_t *params,
                      return -1;
          }

+        if (odp_time_init_global()) {
+                    ODP_ERR("ODP time init failed.\n");
+                    goto init_failed;
+        }
+        stage = TIME_INIT;
+

I’d do the same change in odp-linux, so that odp-dpdk does not need to be different here. It’s common need to first parse system information and then use that information on the following global inits.



          if (odp_shm_init_global()) {
                      ODP_ERR("ODP shm init failed.\n");
                      goto init_failed;
diff --git a/platform/linux-dpdk/odp_time.c b/platform/linux-dpdk/odp_time.c
index ca8a955..a7f885b 100644
--- a/platform/linux-dpdk/odp_time.c
+++ b/platform/linux-dpdk/odp_time.c
@@ -10,6 +10,8 @@ 
 #include <odp/time.h>
 #include <odp/hints.h>
 #include <odp_debug_internal.h>
+#include <rte_cycles.h>
+#include <string.h>

 typedef union {
          odp_time_t      ex;
@@ -17,9 +19,15 @@  typedef union {
 } _odp_time_t;

 static odp_time_t start_time;
+static time_handler_t time_handler;
+
+static inline uint64_t time_local_res_dpdk(void)
+{
+        return rte_get_timer_hz();
+}

 static inline
-uint64_t time_to_ns(odp_time_t time)
+uint64_t time_to_ns_linux(odp_time_t time)


Would it be easier to maintain the code, if these linux functions remain as is (in sync with odp-linux) and add only new functions and structs.