| Message ID | 1462366907-28457-2-git-send-email-matias.elo@nokia.com |
|---|---|
| State | Accepted |
| Commit | 08fe6f0ea4c57cf9721dca8f40b15f5c94db4a93 |
| Headers | show |
Hi Matias, The odp_time_local and others functions are time sensitive functions, that's why it was decided to avoid copping as more as possible. The timespec is not simple "long type". Its type is arch dependent but is always 64bit. In case of 32 bit system it's defined as long long. The same for odp_time_t struct. So, at least for now it seems to be the same for both 32 and 64 bit systems. And I think Bill Fischofer knew about this while adding this ,at first glance, strange union, right Bill? Yes, it's not the best decision from style point of view, but it's fast and in case of an error is supposed to be caught by time validation tests. On 04.05.16 16:01, Matias Elo wrote: > The size of 'struct timespec' may vary on different host > architectures as it includes type long members. This breaks > time functions on a 32-bit x86 host. Fix this by > individually copying struct timespec members to odp_time_t. > > Signed-off-by: Matias Elo <matias.elo@nokia.com> > --- > platform/linux-generic/odp_time.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c > index 040f754..81e0522 100644 > --- a/platform/linux-generic/odp_time.c > +++ b/platform/linux-generic/odp_time.c > @@ -11,11 +11,6 @@ > #include <odp/api/hints.h> > #include <odp_debug_internal.h> > > -typedef union { > - odp_time_t ex; > - struct timespec in; > -} _odp_time_t; > - > static odp_time_t start_time; > > static inline > @@ -47,13 +42,17 @@ 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; > + odp_time_t time; > + struct timespec sys_time; > > - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in); > + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &sys_time); > if (odp_unlikely(ret != 0)) > ODP_ABORT("clock_gettime failed\n"); > > - return time_diff(time.ex, start_time); > + time.tv_sec = sys_time.tv_sec; > + time.tv_nsec = sys_time.tv_nsec; > + > + return time_diff(time, start_time); > } > > static inline int time_cmp(odp_time_t t2, odp_time_t t1) > @@ -195,10 +194,15 @@ uint64_t odp_time_to_u64(odp_time_t time) > int odp_time_init_global(void) > { > int ret; > - _odp_time_t time; > + struct timespec time; > > - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in); > - start_time = ret ? ODP_TIME_NULL : time.ex; > + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); > + if (ret) { > + start_time = ODP_TIME_NULL; > + } else { > + start_time.tv_sec = time.tv_sec; > + start_time.tv_nsec = time.tv_nsec; > + } > > return ret; > } >
On Mon, May 16, 2016 at 9:27 AM, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org > wrote: > Hi Matias, > > The odp_time_local and others functions are time sensitive functions, > that's why it was decided to avoid copping as more as possible. > > The timespec is not simple "long type". Its type is arch dependent but is > always 64bit. > In case of 32 bit system it's defined as long long. > The same for odp_time_t struct. So, at least for now it seems to be the > same for > both 32 and 64 bit systems. And I think Bill Fischofer knew about this > while adding this > ,at first glance, strange union, right Bill? > Yes. The original purpose of that union was not efficiency (though that's a happy side-effect) but rather to remove the need for ODP applications to be dependent on the variable expansion of the Linux timespec struct, which is dependent on POSIX level. That was an earlier bug that caused much grief. > Yes, it's not the best decision from style point of view, but it's fast > and in case of an error > is supposed to be caught by time validation tests. > > > On 04.05.16 16:01, Matias Elo wrote: > >> The size of 'struct timespec' may vary on different host >> architectures as it includes type long members. This breaks >> time functions on a 32-bit x86 host. Fix this by >> individually copying struct timespec members to odp_time_t. >> >> Signed-off-by: Matias Elo <matias.elo@nokia.com> >> --- >> platform/linux-generic/odp_time.c | 26 +++++++++++++++----------- >> 1 file changed, 15 insertions(+), 11 deletions(-) >> >> diff --git a/platform/linux-generic/odp_time.c >> b/platform/linux-generic/odp_time.c >> index 040f754..81e0522 100644 >> --- a/platform/linux-generic/odp_time.c >> +++ b/platform/linux-generic/odp_time.c >> @@ -11,11 +11,6 @@ >> #include <odp/api/hints.h> >> #include <odp_debug_internal.h> >> >> -typedef union { >> - odp_time_t ex; >> - struct timespec in; >> -} _odp_time_t; >> - >> static odp_time_t start_time; >> >> static inline >> @@ -47,13 +42,17 @@ 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; >> + odp_time_t time; >> + struct timespec sys_time; >> >> - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in); >> + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &sys_time); >> if (odp_unlikely(ret != 0)) >> ODP_ABORT("clock_gettime failed\n"); >> >> - return time_diff(time.ex, start_time); >> + time.tv_sec = sys_time.tv_sec; >> + time.tv_nsec = sys_time.tv_nsec; >> + >> + return time_diff(time, start_time); >> } >> >> static inline int time_cmp(odp_time_t t2, odp_time_t t1) >> @@ -195,10 +194,15 @@ uint64_t odp_time_to_u64(odp_time_t time) >> int odp_time_init_global(void) >> { >> int ret; >> - _odp_time_t time; >> + struct timespec time; >> >> - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in); >> - start_time = ret ? ODP_TIME_NULL : time.ex; >> + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); >> + if (ret) { >> + start_time = ODP_TIME_NULL; >> + } else { >> + start_time.tv_sec = time.tv_sec; >> + start_time.tv_nsec = time.tv_nsec; >> + } >> >> return ret; >> } >> >> > -- > Regards, > Ivan Khoronzhuk > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
> Hi Matias, > > The odp_time_local and others functions are time sensitive functions, > that's why it was decided to avoid copping as more as possible. > > The timespec is not simple "long type". Its type is arch dependent but is always > 64bit. > In case of 32 bit system it's defined as long long. > The same for odp_time_t struct. So, at least for now it seems to be the same for > both 32 and 64 bit systems. Hi Ivan, At least for 32/64-bit Ubuntu this is not the case. On a 32-bit system (Ubuntu 16.04) the size of struct timespec is 8 bytes (type is long int) and on a 64-bit system it is 16 bytes. The validation tests were failing before this patch on a 32-bit system. When it comes to the performance, I wouldn't believe this patch having much of an impact. It's an inline function and the compiler should be able to optimize out the added copy operations. -Matias And I think Bill Fischofer knew about this while adding > this > ,at first glance, strange union, right Bill? > Yes, it's not the best decision from style point of view, but it's fast and in case of > an error > is supposed to be caught by time validation tests. >
On 17.05.16 13:23, Elo, Matias (Nokia - FI/Espoo) wrote: >> Hi Matias, >> >> The odp_time_local and others functions are time sensitive functions, >> that's why it was decided to avoid copping as more as possible. >> >> The timespec is not simple "long type". Its type is arch dependent but is always >> 64bit. >> In case of 32 bit system it's defined as long long. >> The same for odp_time_t struct. So, at least for now it seems to be the same for >> both 32 and 64 bit systems. > > Hi Ivan, > > At least for 32/64-bit Ubuntu this is not the case. On a 32-bit system (Ubuntu 16.04) the size of struct timespec is 8 bytes (type is long int) and on a 64-bit system it is 16 bytes. The validation tests were failing before this patch on a 32-bit system. It's nice to see it captured by val tests, that was expected. > > When it comes to the performance, I wouldn't believe this patch having much of an impact. It's an inline function and the compiler should be able to optimize out the added copy operations. That's not always true. At least in case of "32/64" issue in question and when optimization is disabled. The types are different, then better to make odp_time_t to be same size of timespec. Initially, it was smth like typedef timespec odp_time_t, but new versions of Ubuntu have some problem with leaking some library internals to ODP project with such definition (not sure what it was, I'm using 14.04). Maybe better to align the size of odp_time_t struct with timespec ...? > > -Matias > > And I think Bill Fischofer knew about this while adding >> this >> ,at first glance, strange union, right Bill? >> Yes, it's not the best decision from style point of view, but it's fast and in case of >> an error >> is supposed to be caught by time validation tests. >> >
> The types are different, then better to make odp_time_t to be same size of > timespec. > > Initially, it was smth like typedef timespec odp_time_t, but new versions of > Ubuntu > have some problem with leaking some library internals to ODP project with such > definition > (not sure what it was, I'm using 14.04). > Maybe better to align the size of odp_time_t struct with timespec ...? If someone implements this reliably, preferably with no ifdefs and there's a considerable performance gain it's fine by me. However, there are probably much more "cost effective" optimization targets available. -Matias
diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c index 040f754..81e0522 100644 --- a/platform/linux-generic/odp_time.c +++ b/platform/linux-generic/odp_time.c @@ -11,11 +11,6 @@ #include <odp/api/hints.h> #include <odp_debug_internal.h> -typedef union { - odp_time_t ex; - struct timespec in; -} _odp_time_t; - static odp_time_t start_time; static inline @@ -47,13 +42,17 @@ 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; + odp_time_t time; + struct timespec sys_time; - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in); + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &sys_time); if (odp_unlikely(ret != 0)) ODP_ABORT("clock_gettime failed\n"); - return time_diff(time.ex, start_time); + time.tv_sec = sys_time.tv_sec; + time.tv_nsec = sys_time.tv_nsec; + + return time_diff(time, start_time); } static inline int time_cmp(odp_time_t t2, odp_time_t t1) @@ -195,10 +194,15 @@ uint64_t odp_time_to_u64(odp_time_t time) int odp_time_init_global(void) { int ret; - _odp_time_t time; + struct timespec time; - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time.in); - start_time = ret ? ODP_TIME_NULL : time.ex; + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); + if (ret) { + start_time = ODP_TIME_NULL; + } else { + start_time.tv_sec = time.tv_sec; + start_time.tv_nsec = time.tv_nsec; + } return ret; }
The size of 'struct timespec' may vary on different host architectures as it includes type long members. This breaks time functions on a 32-bit x86 host. Fix this by individually copying struct timespec members to odp_time_t. Signed-off-by: Matias Elo <matias.elo@nokia.com> --- platform/linux-generic/odp_time.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)