| Message ID | 1448982493-8236-8-git-send-email-ivan.khoronzhuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 02.12.15 10:20, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT >> Ivan Khoronzhuk >> Sent: Tuesday, December 01, 2015 5:08 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [API-NEXT PATCH v5 7/9] linux-generic: align with new >> wall time API >> >> The local time API supposes the time source is wall time. >> So correct linux-generic implementation. >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> --- >> platform/linux-generic/include/odp_internal.h | 2 + >> platform/linux-generic/odp_schedule.c | 9 ++-- >> platform/linux-generic/odp_time.c | 61 +++++++++++++++++++--- >> ----- >> 3 files changed, 48 insertions(+), 24 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_internal.h >> b/platform/linux-generic/include/odp_internal.h >> index 74e48a9..50bf6c8 100644 >> --- a/platform/linux-generic/include/odp_internal.h >> +++ b/platform/linux-generic/include/odp_internal.h >> @@ -79,6 +79,8 @@ int odp_schedule_term_local(void); >> int odp_timer_init_global(void); >> int odp_timer_disarm_all(void); >> >> +int odp_time_local_init(void); >> + >> int odp_tm_init_global(void); >> >> void _odp_flush_caches(void); >> diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux- >> generic/odp_schedule.c >> index 96b3ac5..3017876 100644 >> --- a/platform/linux-generic/odp_schedule.c >> +++ b/platform/linux-generic/odp_schedule.c >> @@ -586,7 +586,7 @@ static int schedule_loop(odp_queue_t *out_queue, >> uint64_t wait, >> odp_event_t out_ev[], >> unsigned int max_num, unsigned int max_deq) >> { >> - odp_time_t start_time, time, diff, wtime; >> + odp_time_t next, wtime; >> int first = 1; >> int ret; >> >> @@ -604,15 +604,12 @@ static int schedule_loop(odp_queue_t *out_queue, >> uint64_t wait, >> >> if (first) { >> wtime = odp_time_local_from_ns(wait); >> - start_time = odp_time_local(); >> + next = odp_time_sum(odp_time_local(), wtime); >> first = 0; >> continue; >> } >> >> - time = odp_time_local(); >> - diff = odp_time_diff(time, start_time); >> - >> - if (odp_time_cmp(wtime, diff) < 0) >> + if (odp_time_cmp(next, odp_time_local()) < 0) >> break; >> } >> >> diff --git a/platform/linux-generic/odp_time.c b/platform/linux- >> generic/odp_time.c >> index b725086..1aae88c 100644 >> --- a/platform/linux-generic/odp_time.c >> +++ b/platform/linux-generic/odp_time.c >> @@ -11,19 +11,21 @@ >> #include <odp/hints.h> >> #include <odp_debug_internal.h> >> >> -odp_time_t odp_time_local(void) >> +static __thread struct timespec start_local; > > Local time implementation does not have to be thread local. The time source is global, so there's no need to keep thread local start time. A global variable here would enable using the same implementation to global time. > > Performance is also better when there only one copy of the start time (instead of N). Time is local for each thread. It's wall time, that means it starts count from beginning a thread is started. So keep the same approach for all threads not depending on implementation and current CPU. Here source is global, but it has start from 0 for each thread. On other implementations it can have different time sources anyway. With this approach application has ability to get time how much thread is started. > > >> + >> +static inline >> +uint64_t _odp_time_to_ns(odp_time_t val) > > Static function and data (not visible outside of this file) does not need _odp prefix. It's cleaner to not prefix functions that are not intended to be used outside of this file. > >> { >> - int ret; >> - struct timespec time; >> + uint64_t ns; >> >> - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); >> - if (odp_unlikely(ret != 0)) >> - ODP_ABORT("clock_gettime failed\n"); >> + ns = val.tv_sec * ODP_TIME_SEC_IN_NS; >> + ns += val.tv_nsec; >> >> - return time; >> + return ns; >> } >> >> -odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1) >> +static inline >> +odp_time_t _odp_time_diff(odp_time_t t2, odp_time_t t1) > > Drop _odp prefix. Ok. > > >> { >> struct timespec time; >> >> @@ -38,24 +40,36 @@ odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1) >> return time; >> } >> >> -uint64_t odp_time_to_ns(odp_time_t time) >> +odp_time_t odp_time_local(void) >> { >> - uint64_t ns; >> + int ret; >> + struct timespec time; >> + >> + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); >> + if (odp_unlikely(ret != 0)) >> + ODP_ABORT("clock_gettime failed\n"); >> >> - ns = time.tv_sec * ODP_TIME_SEC_IN_NS; >> - ns += time.tv_nsec; >> + return _odp_time_diff(time, start_local); >> +} >> >> - return ns; >> +odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1) >> +{ >> + return _odp_time_diff(t2, t1); >> +} >> + >> +uint64_t odp_time_to_ns(odp_time_t time) >> +{ >> + return _odp_time_to_ns(time); >> } >> >> odp_time_t odp_time_local_from_ns(uint64_t ns) >> { >> - struct timespec time; >> + struct timespec val; >> >> - time.tv_sec = ns / ODP_TIME_SEC_IN_NS; >> - time.tv_nsec = ns % ODP_TIME_SEC_IN_NS; >> + val.tv_sec = ns / ODP_TIME_SEC_IN_NS; >> + val.tv_nsec = ns % ODP_TIME_SEC_IN_NS; > > Not sure if compiler realizes that it can avoid modulo here and do: > > val.tv_nsec = ns - val.tv_sec * ODP_TIME_SEC_IN_NS; > > I'd use multiply since it's faster operation than modulo. We cannot predict this behavior for each archs. So, Ok, I will correct on: ns - val.tv_sec * ODP_TIME_SEC_IN_NS; > > > -Petri > > >> >> - return time; >> + return val; >> } >> >> int odp_time_cmp(odp_time_t t2, odp_time_t t1) >> @@ -96,10 +110,21 @@ uint64_t odp_time_to_u64(odp_time_t time) >> >> resolution = (uint64_t)tres.tv_nsec; >> >> - return odp_time_to_ns(time) / resolution; >> + return _odp_time_to_ns(time) / resolution; >> } >> >> odp_time_t odp_time_null(void) >> { >> return (struct timespec) {0, 0}; >> } >> + >> +int odp_time_local_init(void) >> +{ >> + int ret; >> + struct timespec time; >> + >> + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); >> + start_local = ret ? odp_time_null() : time; >> + >> + return ret; >> +} >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
On 02.12.15 13:39, Ivan Khoronzhuk wrote: > > > On 02.12.15 13:21, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>>> >>>>> diff --git a/platform/linux-generic/odp_time.c b/platform/linux- >>>>> generic/odp_time.c >>>>> index b725086..1aae88c 100644 >>>>> --- a/platform/linux-generic/odp_time.c >>>>> +++ b/platform/linux-generic/odp_time.c >>>>> @@ -11,19 +11,21 @@ >>>>> #include <odp/hints.h> >>>>> #include <odp_debug_internal.h> >>>>> >>>>> -odp_time_t odp_time_local(void) >>>>> +static __thread struct timespec start_local; >>>> >>>> Local time implementation does not have to be thread local. The time >>> source is global, so there's no need to keep thread local start time. A >>> global variable here would enable using the same implementation to global >>> time. >>>> >>>> Performance is also better when there only one copy of the start time >>> (instead of N). >>> >>> Time is local for each thread. >>> It's wall time, that means it starts count from beginning a thread is >>> started. >>> So keep the same approach for all threads not depending on implementation >>> and current CPU. >>> Here source is global, but it has start from 0 for each thread. On other >>> implementations it >>> can have different time sources anyway. With this approach application has >>> ability to get time >>> how much thread is started. >> >> >> "...initialized to zero during ODP startup...", means that it's initialized to zero at some point in ODP instance startup (after global_init()). > Oh, really? > > Initialized to zero during ODP startup is only for global time. > For local time it's initialized to zero at local init, it is convenient. > >> Local time does not explicitly measure thread life time. > Not. But it is useful feature for thread itself. > >> It's specified like this just for the reason that global time source can be used directly > Adding local time startup doesn't prevent global time source usage. > >> for local time also. Thread local copy just adds complexity here, when the time source is not truly local. > Any way it requires to extract start time. > This var is local for each thread. No sharing. IMHO, it should be corrected on "...initialized to zero during local_init()". Complexity is ridiculous. > >> >> >> -Petri >> >> >
On 02.12.15 14:07, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Wednesday, December 02, 2015 1:56 PM >> To: Savolainen, Petri (Nokia - FI/Espoo); LNG ODP Mailman List >> Subject: Re: [lng-odp] [API-NEXT PATCH v5 7/9] linux-generic: align with >> new wall time API >> >> >> >> On 02.12.15 13:39, Ivan Khoronzhuk wrote: >>> >>> >>> On 02.12.15 13:21, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>>>>> >>>>>>> diff --git a/platform/linux-generic/odp_time.c b/platform/linux- >>>>>>> generic/odp_time.c >>>>>>> index b725086..1aae88c 100644 >>>>>>> --- a/platform/linux-generic/odp_time.c >>>>>>> +++ b/platform/linux-generic/odp_time.c >>>>>>> @@ -11,19 +11,21 @@ >>>>>>> #include <odp/hints.h> >>>>>>> #include <odp_debug_internal.h> >>>>>>> >>>>>>> -odp_time_t odp_time_local(void) >>>>>>> +static __thread struct timespec start_local; >>>>>> >>>>>> Local time implementation does not have to be thread local. The time >>>>> source is global, so there's no need to keep thread local start time. >> A >>>>> global variable here would enable using the same implementation to >> global >>>>> time. >>>>>> >>>>>> Performance is also better when there only one copy of the start time >>>>> (instead of N). >>>>> >>>>> Time is local for each thread. >>>>> It's wall time, that means it starts count from beginning a thread is >>>>> started. >>>>> So keep the same approach for all threads not depending on >> implementation >>>>> and current CPU. >>>>> Here source is global, but it has start from 0 for each thread. On >> other >>>>> implementations it >>>>> can have different time sources anyway. With this approach application >> has >>>>> ability to get time >>>>> how much thread is started. >>>> >>>> >>>> "...initialized to zero during ODP startup...", means that it's >> initialized to zero at some point in ODP instance startup (after >> global_init()). >>> Oh, really? >>> >>> Initialized to zero during ODP startup is only for global time. >>> For local time it's initialized to zero at local init, it is convenient. >>> >>>> Local time does not explicitly measure thread life time. >>> Not. But it is useful feature for thread itself. >>> >>>> It's specified like this just for the reason that global time source >> can be used directly >>> Adding local time startup doesn't prevent global time source usage. >>> >>>> for local time also. Thread local copy just adds complexity here, when >> the time source is not truly local. >>> Any way it requires to extract start time. >>> This var is local for each thread. No sharing. >> >> IMHO, it should be corrected on "...initialized to zero during >> local_init()". Complexity is ridiculous. > > > Both local and global timestamp calls can directly return a global HW counter value (zeroed at ODP init) when it's defined like this. > > If user really needs thread life time, we can add odp_thread_life_time() API call for that. > But we can keep it in sync with word "local", any global actions tout local to global time. And no need in additional API calls. > > -Petri >
On 02.12.15 14:07, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Wednesday, December 02, 2015 1:56 PM >> To: Savolainen, Petri (Nokia - FI/Espoo); LNG ODP Mailman List >> Subject: Re: [lng-odp] [API-NEXT PATCH v5 7/9] linux-generic: align with >> new wall time API >> >> >> >> On 02.12.15 13:39, Ivan Khoronzhuk wrote: >>> >>> >>> On 02.12.15 13:21, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>>>>> >>>>>>> diff --git a/platform/linux-generic/odp_time.c b/platform/linux- >>>>>>> generic/odp_time.c >>>>>>> index b725086..1aae88c 100644 >>>>>>> --- a/platform/linux-generic/odp_time.c >>>>>>> +++ b/platform/linux-generic/odp_time.c >>>>>>> @@ -11,19 +11,21 @@ >>>>>>> #include <odp/hints.h> >>>>>>> #include <odp_debug_internal.h> >>>>>>> >>>>>>> -odp_time_t odp_time_local(void) >>>>>>> +static __thread struct timespec start_local; >>>>>> >>>>>> Local time implementation does not have to be thread local. The time >>>>> source is global, so there's no need to keep thread local start time. >> A >>>>> global variable here would enable using the same implementation to >> global >>>>> time. >>>>>> >>>>>> Performance is also better when there only one copy of the start time >>>>> (instead of N). >>>>> >>>>> Time is local for each thread. >>>>> It's wall time, that means it starts count from beginning a thread is >>>>> started. >>>>> So keep the same approach for all threads not depending on >> implementation >>>>> and current CPU. >>>>> Here source is global, but it has start from 0 for each thread. On >> other >>>>> implementations it >>>>> can have different time sources anyway. With this approach application >> has >>>>> ability to get time >>>>> how much thread is started. >>>> >>>> >>>> "...initialized to zero during ODP startup...", means that it's >> initialized to zero at some point in ODP instance startup (after >> global_init()). >>> Oh, really? >>> >>> Initialized to zero during ODP startup is only for global time. >>> For local time it's initialized to zero at local init, it is convenient. >>> >>>> Local time does not explicitly measure thread life time. >>> Not. But it is useful feature for thread itself. >>> >>>> It's specified like this just for the reason that global time source >> can be used directly >>> Adding local time startup doesn't prevent global time source usage. >>> >>>> for local time also. Thread local copy just adds complexity here, when >> the time source is not truly local. >>> Any way it requires to extract start time. >>> This var is local for each thread. No sharing. >> >> IMHO, it should be corrected on "...initialized to zero during >> local_init()". Complexity is ridiculous. > > > Both local and global timestamp calls can directly return a global HW counter value (zeroed at ODP init) when it's defined like this. > > If user really needs thread life time, we can add odp_thread_life_time() API call for that. > > > -Petri > Is it supposed that linux-generic can run tasks instead of threads?
On 03.12.15 14:10, Ivan Khoronzhuk wrote: > > > On 02.12.15 14:07, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >> >>> -----Original Message----- >>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>> Sent: Wednesday, December 02, 2015 1:56 PM >>> To: Savolainen, Petri (Nokia - FI/Espoo); LNG ODP Mailman List >>> Subject: Re: [lng-odp] [API-NEXT PATCH v5 7/9] linux-generic: align with >>> new wall time API >>> >>> >>> >>> On 02.12.15 13:39, Ivan Khoronzhuk wrote: >>>> >>>> >>>> On 02.12.15 13:21, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>>>>>> >>>>>>>> diff --git a/platform/linux-generic/odp_time.c b/platform/linux- >>>>>>>> generic/odp_time.c >>>>>>>> index b725086..1aae88c 100644 >>>>>>>> --- a/platform/linux-generic/odp_time.c >>>>>>>> +++ b/platform/linux-generic/odp_time.c >>>>>>>> @@ -11,19 +11,21 @@ >>>>>>>> #include <odp/hints.h> >>>>>>>> #include <odp_debug_internal.h> >>>>>>>> >>>>>>>> -odp_time_t odp_time_local(void) >>>>>>>> +static __thread struct timespec start_local; >>>>>>> >>>>>>> Local time implementation does not have to be thread local. The time >>>>>> source is global, so there's no need to keep thread local start time. >>> A >>>>>> global variable here would enable using the same implementation to >>> global >>>>>> time. >>>>>>> >>>>>>> Performance is also better when there only one copy of the start time >>>>>> (instead of N). >>>>>> >>>>>> Time is local for each thread. >>>>>> It's wall time, that means it starts count from beginning a thread is >>>>>> started. >>>>>> So keep the same approach for all threads not depending on >>> implementation >>>>>> and current CPU. >>>>>> Here source is global, but it has start from 0 for each thread. On >>> other >>>>>> implementations it >>>>>> can have different time sources anyway. With this approach application >>> has >>>>>> ability to get time >>>>>> how much thread is started. >>>>> >>>>> >>>>> "...initialized to zero during ODP startup...", means that it's >>> initialized to zero at some point in ODP instance startup (after >>> global_init()). >>>> Oh, really? >>>> >>>> Initialized to zero during ODP startup is only for global time. >>>> For local time it's initialized to zero at local init, it is convenient. >>>> >>>>> Local time does not explicitly measure thread life time. >>>> Not. But it is useful feature for thread itself. >>>> >>>>> It's specified like this just for the reason that global time source >>> can be used directly >>>> Adding local time startup doesn't prevent global time source usage. >>>> >>>>> for local time also. Thread local copy just adds complexity here, when >>> the time source is not truly local. >>>> Any way it requires to extract start time. >>>> This var is local for each thread. No sharing. >>> >>> IMHO, it should be corrected on "...initialized to zero during >>> local_init()". Complexity is ridiculous. >> >> >> Both local and global timestamp calls can directly return a global HW counter value (zeroed at ODP init) when it's defined like this. >> >> If user really needs thread life time, we can add odp_thread_life_time() API call for that. >> >> >> -Petri >> > > Is it supposed that linux-generic can run tasks instead of threads? > > If no, I can simply remove __thread -> static struct timespec start_local; and int odp_time_local_init(void) -> int odp_time_global_init(void)
diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h index 74e48a9..50bf6c8 100644 --- a/platform/linux-generic/include/odp_internal.h +++ b/platform/linux-generic/include/odp_internal.h @@ -79,6 +79,8 @@ int odp_schedule_term_local(void); int odp_timer_init_global(void); int odp_timer_disarm_all(void); +int odp_time_local_init(void); + int odp_tm_init_global(void); void _odp_flush_caches(void); diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c index 96b3ac5..3017876 100644 --- a/platform/linux-generic/odp_schedule.c +++ b/platform/linux-generic/odp_schedule.c @@ -586,7 +586,7 @@ static int schedule_loop(odp_queue_t *out_queue, uint64_t wait, odp_event_t out_ev[], unsigned int max_num, unsigned int max_deq) { - odp_time_t start_time, time, diff, wtime; + odp_time_t next, wtime; int first = 1; int ret; @@ -604,15 +604,12 @@ static int schedule_loop(odp_queue_t *out_queue, uint64_t wait, if (first) { wtime = odp_time_local_from_ns(wait); - start_time = odp_time_local(); + next = odp_time_sum(odp_time_local(), wtime); first = 0; continue; } - time = odp_time_local(); - diff = odp_time_diff(time, start_time); - - if (odp_time_cmp(wtime, diff) < 0) + if (odp_time_cmp(next, odp_time_local()) < 0) break; } diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c index b725086..1aae88c 100644 --- a/platform/linux-generic/odp_time.c +++ b/platform/linux-generic/odp_time.c @@ -11,19 +11,21 @@ #include <odp/hints.h> #include <odp_debug_internal.h> -odp_time_t odp_time_local(void) +static __thread struct timespec start_local; + +static inline +uint64_t _odp_time_to_ns(odp_time_t val) { - int ret; - struct timespec time; + uint64_t ns; - ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); - if (odp_unlikely(ret != 0)) - ODP_ABORT("clock_gettime failed\n"); + ns = val.tv_sec * ODP_TIME_SEC_IN_NS; + ns += val.tv_nsec; - return time; + return ns; } -odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1) +static inline +odp_time_t _odp_time_diff(odp_time_t t2, odp_time_t t1) { struct timespec time; @@ -38,24 +40,36 @@ odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1) return time; } -uint64_t odp_time_to_ns(odp_time_t time) +odp_time_t odp_time_local(void) { - uint64_t ns; + int ret; + struct timespec time; + + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); + if (odp_unlikely(ret != 0)) + ODP_ABORT("clock_gettime failed\n"); - ns = time.tv_sec * ODP_TIME_SEC_IN_NS; - ns += time.tv_nsec; + return _odp_time_diff(time, start_local); +} - return ns; +odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1) +{ + return _odp_time_diff(t2, t1); +} + +uint64_t odp_time_to_ns(odp_time_t time) +{ + return _odp_time_to_ns(time); } odp_time_t odp_time_local_from_ns(uint64_t ns) { - struct timespec time; + struct timespec val; - time.tv_sec = ns / ODP_TIME_SEC_IN_NS; - time.tv_nsec = ns % ODP_TIME_SEC_IN_NS; + val.tv_sec = ns / ODP_TIME_SEC_IN_NS; + val.tv_nsec = ns % ODP_TIME_SEC_IN_NS; - return time; + return val; } int odp_time_cmp(odp_time_t t2, odp_time_t t1) @@ -96,10 +110,21 @@ uint64_t odp_time_to_u64(odp_time_t time) resolution = (uint64_t)tres.tv_nsec; - return odp_time_to_ns(time) / resolution; + return _odp_time_to_ns(time) / resolution; } odp_time_t odp_time_null(void) { return (struct timespec) {0, 0}; } + +int odp_time_local_init(void) +{ + int ret; + struct timespec time; + + ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); + start_local = ret ? odp_time_null() : time; + + return ret; +}
The local time API supposes the time source is wall time. So correct linux-generic implementation. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- platform/linux-generic/include/odp_internal.h | 2 + platform/linux-generic/odp_schedule.c | 9 ++-- platform/linux-generic/odp_time.c | 61 +++++++++++++++++++-------- 3 files changed, 48 insertions(+), 24 deletions(-)