| Message ID | 20170428120958.17526-2-petri.savolainen@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use HW time counter | expand |
I agree that the validation test for odp_time_to_u64() is suspect and should be fixed, but why remove this API? We've established that various abstract types have odp_xxx_to_u64() routines designed for debugging use and this makes odp_time_t an exception to that rule. Also, shouldn't we deprecate it if we want to remove it? What's the point of having the deprecation framework in place if we're not going to bother with it? On Fri, Apr 28, 2017 at 7:09 AM, Petri Savolainen < petri.savolainen@linaro.org> wrote: > Debug function that converts odp_time_t to u64 is unnecessary > since odp_time_to_ns() returns time as a u64 (nsec) value. > Application can always use that as the 64 bit representation > of an odp_time_t value. Also validation tests for odp_time_to_u64() > were erroneous since those compared returned u64 values and > expected greater/lesser than relation. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > include/odp/api/spec/time.h | 13 ---------- > platform/linux-generic/odp_time.c | 15 ------------ > test/common_plat/validation/api/time/time.c | 37 > ----------------------------- > test/common_plat/validation/api/time/time.h | 2 -- > 4 files changed, 67 deletions(-) > > diff --git a/include/odp/api/spec/time.h b/include/odp/api/spec/time.h > index fcc94c98..29175eb5 100644 > --- a/include/odp/api/spec/time.h > +++ b/include/odp/api/spec/time.h > @@ -158,19 +158,6 @@ void odp_time_wait_until(odp_time_t time); > void odp_time_wait_ns(uint64_t ns); > > /** > - * Get printable value for an odp_time_t > - * > - * @param time time to be printed > - * > - * @return uint64_t value that can be used to print/display this time > - * > - * @note This routine is intended to be used for diagnostic purposes > - * to enable applications to generate a printable value that represents > - * an odp_time_t time. > - */ > -uint64_t odp_time_to_u64(odp_time_t time); > - > -/** > * @} > */ > > diff --git a/platform/linux-generic/odp_time.c > b/platform/linux-generic/odp_time.c > index 81e05224..0e5966c0 100644 > --- a/platform/linux-generic/odp_time.c > +++ b/platform/linux-generic/odp_time.c > @@ -176,21 +176,6 @@ void odp_time_wait_until(odp_time_t time) > return time_wait_until(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; > -} > - > int odp_time_init_global(void) > { > int ret; > diff --git a/test/common_plat/validation/api/time/time.c > b/test/common_plat/validation/api/time/time.c > index 530d5c07..df65c719 100644 > --- a/test/common_plat/validation/api/time/time.c > +++ b/test/common_plat/validation/api/time/time.c > @@ -398,41 +398,6 @@ void time_test_wait_ns(void) > } > } > > -static void time_test_to_u64(time_cb time) > -{ > - volatile int count = 0; > - uint64_t val1, val2; > - odp_time_t t1, t2; > - > - t1 = time(); > - > - val1 = odp_time_to_u64(t1); > - CU_ASSERT(val1 > 0); > - > - while (count < BUSY_LOOP_CNT) { > - count++; > - }; > - > - t2 = time(); > - val2 = odp_time_to_u64(t2); > - CU_ASSERT(val2 > 0); > - > - CU_ASSERT(val2 > val1); > - > - val1 = odp_time_to_u64(ODP_TIME_NULL); > - CU_ASSERT(val1 == 0); > -} > - > -void time_test_local_to_u64(void) > -{ > - time_test_to_u64(odp_time_local); > -} > - > -void time_test_global_to_u64(void) > -{ > - time_test_to_u64(odp_time_global); > -} > - > odp_testinfo_t time_suite_time[] = { > ODP_TEST_INFO(time_test_constants), > ODP_TEST_INFO(time_test_local_res), > @@ -443,14 +408,12 @@ odp_testinfo_t time_suite_time[] = { > ODP_TEST_INFO(time_test_local_sum), > ODP_TEST_INFO(time_test_local_wait_until), > ODP_TEST_INFO(time_test_wait_ns), > - ODP_TEST_INFO(time_test_local_to_u64), > ODP_TEST_INFO(time_test_global_res), > ODP_TEST_INFO(time_test_global_conversion), > ODP_TEST_INFO(time_test_global_cmp), > ODP_TEST_INFO(time_test_global_diff), > ODP_TEST_INFO(time_test_global_sum), > ODP_TEST_INFO(time_test_global_wait_until), > - ODP_TEST_INFO(time_test_global_to_u64), > ODP_TEST_INFO_NULL > }; > > diff --git a/test/common_plat/validation/api/time/time.h > b/test/common_plat/validation/api/time/time.h > index e5132a49..10956294 100644 > --- a/test/common_plat/validation/api/time/time.h > +++ b/test/common_plat/validation/api/time/time.h > @@ -24,8 +24,6 @@ void time_test_global_sum(void); > void time_test_local_wait_until(void); > void time_test_global_wait_until(void); > void time_test_wait_ns(void); > -void time_test_local_to_u64(void); > -void time_test_global_to_u64(void); > void time_test_monotony(void); > > /* test arrays: */ > -- > 2.11.0 > >
On 04/28/17 22:09, Bill Fischofer wrote: > I agree that the validation test for odp_time_to_u64() is suspect and > should be fixed, but why remove this API? We've established that various > abstract types have odp_xxx_to_u64() routines designed for debugging use > and this makes odp_time_t an exception to that rule. > > Also, shouldn't we deprecate it if we want to remove it? What's the point > of having the deprecation framework in place if we're not going to bother > with it? > I think odp_x_to_u64() makes sense only for handles which somehow related to odp event. I.e. it's reasonable to have it for timer, but not for time. odp_time_t is more data structure then handle of something. Maxim. > On Fri, Apr 28, 2017 at 7:09 AM, Petri Savolainen < > petri.savolainen@linaro.org> wrote: > >> Debug function that converts odp_time_t to u64 is unnecessary >> since odp_time_to_ns() returns time as a u64 (nsec) value. >> Application can always use that as the 64 bit representation >> of an odp_time_t value. Also validation tests for odp_time_to_u64() >> were erroneous since those compared returned u64 values and >> expected greater/lesser than relation. >> >> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> >> --- >> include/odp/api/spec/time.h | 13 ---------- >> platform/linux-generic/odp_time.c | 15 ------------ >> test/common_plat/validation/api/time/time.c | 37 >> ----------------------------- >> test/common_plat/validation/api/time/time.h | 2 -- >> 4 files changed, 67 deletions(-) >> >> diff --git a/include/odp/api/spec/time.h b/include/odp/api/spec/time.h >> index fcc94c98..29175eb5 100644 >> --- a/include/odp/api/spec/time.h >> +++ b/include/odp/api/spec/time.h >> @@ -158,19 +158,6 @@ void odp_time_wait_until(odp_time_t time); >> void odp_time_wait_ns(uint64_t ns); >> >> /** >> - * Get printable value for an odp_time_t >> - * >> - * @param time time to be printed >> - * >> - * @return uint64_t value that can be used to print/display this time >> - * >> - * @note This routine is intended to be used for diagnostic purposes >> - * to enable applications to generate a printable value that represents >> - * an odp_time_t time. >> - */ >> -uint64_t odp_time_to_u64(odp_time_t time); >> - >> -/** >> * @} >> */ >> >> diff --git a/platform/linux-generic/odp_time.c >> b/platform/linux-generic/odp_time.c >> index 81e05224..0e5966c0 100644 >> --- a/platform/linux-generic/odp_time.c >> +++ b/platform/linux-generic/odp_time.c >> @@ -176,21 +176,6 @@ void odp_time_wait_until(odp_time_t time) >> return time_wait_until(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; >> -} >> - >> int odp_time_init_global(void) >> { >> int ret; >> diff --git a/test/common_plat/validation/api/time/time.c >> b/test/common_plat/validation/api/time/time.c >> index 530d5c07..df65c719 100644 >> --- a/test/common_plat/validation/api/time/time.c >> +++ b/test/common_plat/validation/api/time/time.c >> @@ -398,41 +398,6 @@ void time_test_wait_ns(void) >> } >> } >> >> -static void time_test_to_u64(time_cb time) >> -{ >> - volatile int count = 0; >> - uint64_t val1, val2; >> - odp_time_t t1, t2; >> - >> - t1 = time(); >> - >> - val1 = odp_time_to_u64(t1); >> - CU_ASSERT(val1 > 0); >> - >> - while (count < BUSY_LOOP_CNT) { >> - count++; >> - }; >> - >> - t2 = time(); >> - val2 = odp_time_to_u64(t2); >> - CU_ASSERT(val2 > 0); >> - >> - CU_ASSERT(val2 > val1); >> - >> - val1 = odp_time_to_u64(ODP_TIME_NULL); >> - CU_ASSERT(val1 == 0); >> -} >> - >> -void time_test_local_to_u64(void) >> -{ >> - time_test_to_u64(odp_time_local); >> -} >> - >> -void time_test_global_to_u64(void) >> -{ >> - time_test_to_u64(odp_time_global); >> -} >> - >> odp_testinfo_t time_suite_time[] = { >> ODP_TEST_INFO(time_test_constants), >> ODP_TEST_INFO(time_test_local_res), >> @@ -443,14 +408,12 @@ odp_testinfo_t time_suite_time[] = { >> ODP_TEST_INFO(time_test_local_sum), >> ODP_TEST_INFO(time_test_local_wait_until), >> ODP_TEST_INFO(time_test_wait_ns), >> - ODP_TEST_INFO(time_test_local_to_u64), >> ODP_TEST_INFO(time_test_global_res), >> ODP_TEST_INFO(time_test_global_conversion), >> ODP_TEST_INFO(time_test_global_cmp), >> ODP_TEST_INFO(time_test_global_diff), >> ODP_TEST_INFO(time_test_global_sum), >> ODP_TEST_INFO(time_test_global_wait_until), >> - ODP_TEST_INFO(time_test_global_to_u64), >> ODP_TEST_INFO_NULL >> }; >> >> diff --git a/test/common_plat/validation/api/time/time.h >> b/test/common_plat/validation/api/time/time.h >> index e5132a49..10956294 100644 >> --- a/test/common_plat/validation/api/time/time.h >> +++ b/test/common_plat/validation/api/time/time.h >> @@ -24,8 +24,6 @@ void time_test_global_sum(void); >> void time_test_local_wait_until(void); >> void time_test_global_wait_until(void); >> void time_test_wait_ns(void); >> -void time_test_local_to_u64(void); >> -void time_test_global_to_u64(void); >> void time_test_monotony(void); >> >> /* test arrays: */ >> -- >> 2.11.0 >> >>
Aside from the deprecation question, for this series:
Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org>
Also, Parts 9 and 11 are fine but both cause checkpatch to hiccup:
bill@Ubuntu64:~/linaro/hubodp$ ./scripts/checkpatch.pl
0009-linux-gen-time-use-hw-time-counter-when-available.patch
Unescaped left brace in regex is deprecated, passed through in regex;
marked by <-- HERE in m/\((?^x:
(?^x:
(?:(?^:(?:(?^x:
const|
__percpu|
__nocast|
__safe|
__bitwise__|
__packed__|
__packed2__|
__naked|
__maybe_unused|
__always_unused|
__noreturn|
__used|
__cold|
__pure|
__noclone|
__deprecated|
__read_mostly|
__kprobes|
(?^:(?^:(?^:__(?:mem|cpu|dev|net_|))(?:initdata\b))|(?^:(?^:__(?:mem|cpu|dev|net_|))(?:initconst\b))|(?^:(?^:__(?:mem|cpu|dev|net_|))(?:init\b)))|
____cacheline_aligned|
____cacheline_aligned_in_smp|
____cacheline_internodealigned_in_smp|
__weak
)|(?^x:
__user|
__kernel|
__force|
__iomem|
__must_check|
__init_refok|
__kprobes|
__ref|
__rcu
)|(?x:
(?^:fastcall)
)))\s+|const\s+)*
(?:
(?:typeof|__typeof__)\s*\([^\)]*\)|
(?:(?^:(?x:
u_(?:char|short|int|long) | # bsd
u(?:nchar|short|int|long) # sysv
))\b)|
(?:(?^:(?x:
(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
atomic_t
))\b)|
(?:(?x:
(?^:void)|
(?^:(?:(?:un)?signed\s+)?char)|
(?^:(?:(?:un)?signed\s+)?short\s+int)|
(?^:(?:(?:un)?signed\s+)?short)|
(?^:(?:(?:un)?signed\s+)?int)|
(?^:(?:(?:un)?signed\s+)?long\s+int)|
(?^:(?:(?:un)?signed\s+)?long\s+long\s+int)|
(?^:(?:(?:un)?signed\s+)?long\s+long)|
(?^:(?:(?:un)?signed\s+)?long)|
(?^:(?:un)?signed)|
(?^:float)|
(?^:double)|
(?^:bool)|
(?^:struct\s+(?^x:
[A-Za-z_][A-Za-z\d_]*
(?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)*
))|
(?^:union\s+(?^x:
[A-Za-z_][A-Za-z\d_]*
(?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)*
))|
(?^:enum\s+(?^x:
[A-Za-z_][A-Za-z\d_]*
(?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)*
))|
(?^:(?^x:
[A-Za-z_][A-Za-z\d_]*
(?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)*
)_t)|
(?^:(?^x:
[A-Za-z_][A-Za-z\d_]*
(?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)*
)_handler)|
(?^:(?^x:
[A-Za-z_][A-Za-z\d_]*
(?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)*
)_handler_fn)|
(?^:char\s+(?:un)?signed)|
(?^:int\s+(?:(?:un)?signed\s+)?short\s)|
(?^:int\s+short(?:\s+(?:un)?signed))|
(?^:short\s+int(?:\s+(?:un)?signed))|
(?^:(?:un)?signed\s+int\s+short)|
(?^:short\s+(?:un)?signed)|
(?^:long\s+int\s+(?:un)?signed)|
(?^:int\s+long\s+(?:un)?signed)|
(?^:long\s+(?:un)?signed\s+int)|
(?^:int\s+(?:un)?signed\s+long)|
(?^:int\s+(?:un)?signed)|
(?^:int\s+long\s+long\s+(?:un)?signed)|
(?^:long\s+long\s+int\s+(?:un)?signed)|
(?^:long\s+long\s+(?:un)?signed\s+int)|
(?^:long\s+long\s+(?:un)?signed)|
(?^:long\s+(?:un)?signed)
)\b)
)
(?:\s+(?^:(?:(?^x:
const|
__percpu|
__nocast|
__safe|
__bitwise__|
__packed__|
__packed2__|
__naked|
__maybe_unused|
__always_unused|
__noreturn|
__used|
__cold|
__pure|
__noclone|
__deprecated|
__read_mostly|
__kprobes|
(?^:(?^:(?^:__(?:mem|cpu|dev|net_|))(?:initdata\b))|(?^:(?^:__(?:mem|cpu|dev|net_|))(?:initconst\b))|(?^:(?^:__(?:mem|cpu|dev|net_|))(?:init\b)))|
____cacheline_aligned|
____cacheline_aligned_in_smp|
____cacheline_internodealigned_in_smp|
__weak
)|(?^x:
__user|
__kernel|
__force|
__iomem|
__must_check|
__init_refok|
__kprobes|
__ref|
__rcu
)|(?x:
(?^:fastcall)
)))|\s+const)*
)
(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)?
(?:\s+(?^:inline|__always_inline|noinline|__inline|__inline__)|\s+(?^:(?:(?^x:
const|
__percpu|
__nocast|
__safe|
__bitwise__|
__packed__|
__packed2__|
__naked|
__maybe_unused|
__always_unused|
__noreturn|
__used|
__cold|
__pure|
__noclone|
__deprecated|
__read_mostly|
__kprobes|
(?^:(?^:(?^:__(?:mem|cpu|dev|net_|))(?:initdata\b))|(?^:(?^:__(?:mem|cpu|dev|net_|))(?:initconst\b))|(?^:(?^:__(?:mem|cpu|dev|net_|))(?:init\b)))|
____cacheline_aligned|
____cacheline_aligned_in_smp|
____cacheline_internodealigned_in_smp|
__weak
)|(?^x:
__user|
__kernel|
__force|
__iomem|
__must_check|
__init_refok|
__kprobes|
__ref|
__rcu
)|(?x:
(?^:fastcall)
))))*
)\){ <-- HERE / at ./scripts/checkpatch.pl line 3926.
total: 0 errors, 0 warnings, 0 checks, 645 lines checked
NOTE: Ignored message types: BIT_MACRO COMPARISON_TO_NULL
DEPRECATED_VARIABLE NEW_TYPEDEFS SPLIT_STRING SSCANF_TO_KSTRTO
0009-linux-gen-time-use-hw-time-counter-when-available.patch has no obvious
style problems and is ready for submission.
bill@Ubuntu64:~/linaro/hubodp$
On Fri, Apr 28, 2017 at 2:28 PM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:
> On 04/28/17 22:09, Bill Fischofer wrote:
> > I agree that the validation test for odp_time_to_u64() is suspect and
> > should be fixed, but why remove this API? We've established that various
> > abstract types have odp_xxx_to_u64() routines designed for debugging use
> > and this makes odp_time_t an exception to that rule.
> >
> > Also, shouldn't we deprecate it if we want to remove it? What's the point
> > of having the deprecation framework in place if we're not going to bother
> > with it?
> >
>
> I think odp_x_to_u64() makes sense only for handles which somehow
> related to odp event. I.e. it's reasonable to have it for timer, but not
> for time. odp_time_t is more data structure then handle of something.
>
> Maxim.
>
>
>
> > On Fri, Apr 28, 2017 at 7:09 AM, Petri Savolainen <
> > petri.savolainen@linaro.org> wrote:
> >
> >> Debug function that converts odp_time_t to u64 is unnecessary
> >> since odp_time_to_ns() returns time as a u64 (nsec) value.
> >> Application can always use that as the 64 bit representation
> >> of an odp_time_t value. Also validation tests for odp_time_to_u64()
> >> were erroneous since those compared returned u64 values and
> >> expected greater/lesser than relation.
> >>
> >> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
> >> ---
> >> include/odp/api/spec/time.h | 13 ----------
> >> platform/linux-generic/odp_time.c | 15 ------------
> >> test/common_plat/validation/api/time/time.c | 37
> >> -----------------------------
> >> test/common_plat/validation/api/time/time.h | 2 --
> >> 4 files changed, 67 deletions(-)
> >>
> >> diff --git a/include/odp/api/spec/time.h b/include/odp/api/spec/time.h
> >> index fcc94c98..29175eb5 100644
> >> --- a/include/odp/api/spec/time.h
> >> +++ b/include/odp/api/spec/time.h
> >> @@ -158,19 +158,6 @@ void odp_time_wait_until(odp_time_t time);
> >> void odp_time_wait_ns(uint64_t ns);
> >>
> >> /**
> >> - * Get printable value for an odp_time_t
> >> - *
> >> - * @param time time to be printed
> >> - *
> >> - * @return uint64_t value that can be used to print/display this time
> >> - *
> >> - * @note This routine is intended to be used for diagnostic purposes
> >> - * to enable applications to generate a printable value that represents
> >> - * an odp_time_t time.
> >> - */
> >> -uint64_t odp_time_to_u64(odp_time_t time);
> >> -
> >> -/**
> >> * @}
> >> */
> >>
> >> diff --git a/platform/linux-generic/odp_time.c
> >> b/platform/linux-generic/odp_time.c
> >> index 81e05224..0e5966c0 100644
> >> --- a/platform/linux-generic/odp_time.c
> >> +++ b/platform/linux-generic/odp_time.c
> >> @@ -176,21 +176,6 @@ void odp_time_wait_until(odp_time_t time)
> >> return time_wait_until(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;
> >> -}
> >> -
> >> int odp_time_init_global(void)
> >> {
> >> int ret;
> >> diff --git a/test/common_plat/validation/api/time/time.c
> >> b/test/common_plat/validation/api/time/time.c
> >> index 530d5c07..df65c719 100644
> >> --- a/test/common_plat/validation/api/time/time.c
> >> +++ b/test/common_plat/validation/api/time/time.c
> >> @@ -398,41 +398,6 @@ void time_test_wait_ns(void)
> >> }
> >> }
> >>
> >> -static void time_test_to_u64(time_cb time)
> >> -{
> >> - volatile int count = 0;
> >> - uint64_t val1, val2;
> >> - odp_time_t t1, t2;
> >> -
> >> - t1 = time();
> >> -
> >> - val1 = odp_time_to_u64(t1);
> >> - CU_ASSERT(val1 > 0);
> >> -
> >> - while (count < BUSY_LOOP_CNT) {
> >> - count++;
> >> - };
> >> -
> >> - t2 = time();
> >> - val2 = odp_time_to_u64(t2);
> >> - CU_ASSERT(val2 > 0);
> >> -
> >> - CU_ASSERT(val2 > val1);
> >> -
> >> - val1 = odp_time_to_u64(ODP_TIME_NULL);
> >> - CU_ASSERT(val1 == 0);
> >> -}
> >> -
> >> -void time_test_local_to_u64(void)
> >> -{
> >> - time_test_to_u64(odp_time_local);
> >> -}
> >> -
> >> -void time_test_global_to_u64(void)
> >> -{
> >> - time_test_to_u64(odp_time_global);
> >> -}
> >> -
> >> odp_testinfo_t time_suite_time[] = {
> >> ODP_TEST_INFO(time_test_constants),
> >> ODP_TEST_INFO(time_test_local_res),
> >> @@ -443,14 +408,12 @@ odp_testinfo_t time_suite_time[] = {
> >> ODP_TEST_INFO(time_test_local_sum),
> >> ODP_TEST_INFO(time_test_local_wait_until),
> >> ODP_TEST_INFO(time_test_wait_ns),
> >> - ODP_TEST_INFO(time_test_local_to_u64),
> >> ODP_TEST_INFO(time_test_global_res),
> >> ODP_TEST_INFO(time_test_global_conversion),
> >> ODP_TEST_INFO(time_test_global_cmp),
> >> ODP_TEST_INFO(time_test_global_diff),
> >> ODP_TEST_INFO(time_test_global_sum),
> >> ODP_TEST_INFO(time_test_global_wait_until),
> >> - ODP_TEST_INFO(time_test_global_to_u64),
> >> ODP_TEST_INFO_NULL
> >> };
> >>
> >> diff --git a/test/common_plat/validation/api/time/time.h
> >> b/test/common_plat/validation/api/time/time.h
> >> index e5132a49..10956294 100644
> >> --- a/test/common_plat/validation/api/time/time.h
> >> +++ b/test/common_plat/validation/api/time/time.h
> >> @@ -24,8 +24,6 @@ void time_test_global_sum(void);
> >> void time_test_local_wait_until(void);
> >> void time_test_global_wait_until(void);
> >> void time_test_wait_ns(void);
> >> -void time_test_local_to_u64(void);
> >> -void time_test_global_to_u64(void);
> >> void time_test_monotony(void);
> >>
> >> /* test arrays: */
> >> --
> >> 2.11.0
> >>
> >>
>
>
From: Bill Fischofer [mailto:bill.fischofer@linaro.org]
Sent: Friday, April 28, 2017 10:09 PM
To: Petri Savolainen <petri.savolainen@linaro.org>
Cc: lng-odp-forward <lng-odp@lists.linaro.org>
Subject: Re: [lng-odp] [API-NEXT PATCH v3 01/11] api: time: remove odp_time_to_u64 from API
I agree that the validation test for odp_time_to_u64() is suspect and should be fixed, but why remove this API? We've established that various abstract types have odp_xxx_to_u64() routines designed for debugging use and this makes odp_time_t an exception to that rule.
[Petri]
xxx_to_u64() is needed when application does not have any other way e.g. to print out a handle, since handle type sizes are unknown in advance. Odp_time_t is not a handle to resource, but an abstract value - and more importantly the API has a well-defined odp_time_t -> u64 conversion function already (namely odp_time_to_ns()). There's no need or much value in having two conversion functions. Actually, it's a source of confusion, if we have two functions. Also when debugging, conversion to nsec time is more valuable to the user, than conversion to an abstract 64 bit value (which would be implemented as 64bit nsec in most cases anyway).
Also, shouldn't we deprecate it if we want to remove it? What's the point of having the deprecation framework in place if we're not going to bother with it?
[Petri]
In this case, application have had always two choices for the same conversion, and I think odp_time_to_ns() has been the natural choice for that. So, I expect that odp_time_to_u64() has not been used at all - e.g. our code base didn't. Deprecation must be used when application does not have a choice (e.g. old name vs. new name). Here an application has two choices and is still backwards compatible after changing all odp_time_to_u64() calls to odp_time_to_ns().
Also the proper deprecation framework is not in the repo yet. I have waited/pinged for that (v2) to happen a month now.
-Petri
On Fri, Apr 28, 2017 at 7:09 AM, Petri Savolainen <mailto:petri.savolainen@linaro.org> wrote:
Debug function that converts odp_time_t to u64 is unnecessary
since odp_time_to_ns() returns time as a u64 (nsec) value.
Application can always use that as the 64 bit representation
of an odp_time_t value. Also validation tests for odp_time_to_ns()
were erroneous since those compared returned u64 values and
expected greater/lesser than relation.
diff --git a/include/odp/api/spec/time.h b/include/odp/api/spec/time.h index fcc94c98..29175eb5 100644 --- a/include/odp/api/spec/time.h +++ b/include/odp/api/spec/time.h @@ -158,19 +158,6 @@ void odp_time_wait_until(odp_time_t time); void odp_time_wait_ns(uint64_t ns); /** - * Get printable value for an odp_time_t - * - * @param time time to be printed - * - * @return uint64_t value that can be used to print/display this time - * - * @note This routine is intended to be used for diagnostic purposes - * to enable applications to generate a printable value that represents - * an odp_time_t time. - */ -uint64_t odp_time_to_u64(odp_time_t time); - -/** * @} */ diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c index 81e05224..0e5966c0 100644 --- a/platform/linux-generic/odp_time.c +++ b/platform/linux-generic/odp_time.c @@ -176,21 +176,6 @@ void odp_time_wait_until(odp_time_t time) return time_wait_until(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; -} - int odp_time_init_global(void) { int ret; diff --git a/test/common_plat/validation/api/time/time.c b/test/common_plat/validation/api/time/time.c index 530d5c07..df65c719 100644 --- a/test/common_plat/validation/api/time/time.c +++ b/test/common_plat/validation/api/time/time.c @@ -398,41 +398,6 @@ void time_test_wait_ns(void) } } -static void time_test_to_u64(time_cb time) -{ - volatile int count = 0; - uint64_t val1, val2; - odp_time_t t1, t2; - - t1 = time(); - - val1 = odp_time_to_u64(t1); - CU_ASSERT(val1 > 0); - - while (count < BUSY_LOOP_CNT) { - count++; - }; - - t2 = time(); - val2 = odp_time_to_u64(t2); - CU_ASSERT(val2 > 0); - - CU_ASSERT(val2 > val1); - - val1 = odp_time_to_u64(ODP_TIME_NULL); - CU_ASSERT(val1 == 0); -} - -void time_test_local_to_u64(void) -{ - time_test_to_u64(odp_time_local); -} - -void time_test_global_to_u64(void) -{ - time_test_to_u64(odp_time_global); -} - odp_testinfo_t time_suite_time[] = { ODP_TEST_INFO(time_test_constants), ODP_TEST_INFO(time_test_local_res), @@ -443,14 +408,12 @@ odp_testinfo_t time_suite_time[] = { ODP_TEST_INFO(time_test_local_sum), ODP_TEST_INFO(time_test_local_wait_until), ODP_TEST_INFO(time_test_wait_ns), - ODP_TEST_INFO(time_test_local_to_u64), ODP_TEST_INFO(time_test_global_res), ODP_TEST_INFO(time_test_global_conversion), ODP_TEST_INFO(time_test_global_cmp), ODP_TEST_INFO(time_test_global_diff), ODP_TEST_INFO(time_test_global_sum), ODP_TEST_INFO(time_test_global_wait_until), - ODP_TEST_INFO(time_test_global_to_u64), ODP_TEST_INFO_NULL }; diff --git a/test/common_plat/validation/api/time/time.h b/test/common_plat/validation/api/time/time.h index e5132a49..10956294 100644 --- a/test/common_plat/validation/api/time/time.h +++ b/test/common_plat/validation/api/time/time.h @@ -24,8 +24,6 @@ void time_test_global_sum(void); void time_test_local_wait_until(void); void time_test_global_wait_until(void); void time_test_wait_ns(void); -void time_test_local_to_u64(void); -void time_test_global_to_u64(void); void time_test_monotony(void); /* test arrays: */
Debug function that converts odp_time_t to u64 is unnecessary since odp_time_to_ns() returns time as a u64 (nsec) value. Application can always use that as the 64 bit representation of an odp_time_t value. Also validation tests for odp_time_to_u64() were erroneous since those compared returned u64 values and expected greater/lesser than relation. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- include/odp/api/spec/time.h | 13 ---------- platform/linux-generic/odp_time.c | 15 ------------ test/common_plat/validation/api/time/time.c | 37 ----------------------------- test/common_plat/validation/api/time/time.h | 2 -- 4 files changed, 67 deletions(-) -- 2.11.0