| Message ID | 1445515299-18496-4-git-send-email-ivan.khoronzhuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] > Sent: Thursday, October 22, 2015 3:02 PM > To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo) > Cc: Ivan Khoronzhuk > Subject: [lng-odp] [API-NEXT PATCH v5 3/5] api: time: unbind CPU cycles > from time API > > Current time API supposes that frequency of counter is equal > to CPU frequency. But that's not always true, for instance, > in case if no access to CPU cycle counter, another hi-resolution > timer can be used, and it`s rate can be different from CPU > rate. There is no big difference in which cycles to measure > time, the better hi-resolution timer the better measurements. > So, unbind CPU cycle counter from time API by eliminating word > "cycle" as it's believed to be used with CPU. > > Also add new opaque type for time odp_time_t, as it asks user to use > API and abstracts time from units. New odp_time_t requires several > additional API functions to be added: > > odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); > int odp_time_cmp(odp_time_t t1, odp_time_t t2); > uint64_t odp_time_to_u64(odp_time_t hdl); > > Also added new definition that represents 0 ticks for time - > ODP_TIME_NULL. It can be used instead of odp_time_from_ns(0) for > comparison and initialization. > > This patch changes only used time API, it doesn't change used var > names for simplicity. > > This time API can be implemented with local timer counter, so > shouldn't be used between threads. > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > --- > example/generator/odp_generator.c | 12 +++---- > include/odp/api/time.h | 68 ++++++++++++++++++++++++++++--- > ---- > test/performance/odp_pktio_perf.c | 49 +++++++++++++------------ > test/validation/pktio/pktio.c | 20 +++++------ > test/validation/scheduler/scheduler.c | 5 +-- > test/validation/time/time.c | 27 +++++++------- > 6 files changed, 114 insertions(+), 67 deletions(-) > > diff --git a/example/generator/odp_generator.c > b/example/generator/odp_generator.c > index be9597b..f84adc4 100644 > --- a/example/generator/odp_generator.c > +++ b/example/generator/odp_generator.c > @@ -585,7 +585,7 @@ static void *gen_recv_thread(void *arg) > */ > static void print_global_stats(int num_workers) > { > - uint64_t start, wait, diff; > + odp_time_t start, wait, diff; > uint64_t pkts, pkts_prev = 0, pps, maximum_pps = 0; > int verbose_interval = 20; > odp_thrmask_t thrd_mask; > @@ -593,8 +593,8 @@ static void print_global_stats(int num_workers) > while (odp_thrmask_worker(&thrd_mask) < num_workers) > continue; > > - wait = odp_time_ns_to_cycles(verbose_interval * ODP_TIME_SEC); > - start = odp_time_cycles(); > + wait = odp_time_from_ns(verbose_interval * ODP_TIME_SEC); > + start = odp_time(); > > while (odp_thrmask_worker(&thrd_mask) == num_workers) { > if (args->appl.number != -1 && > @@ -603,11 +603,11 @@ static void print_global_stats(int num_workers) > break; > } > > - diff = odp_time_diff_cycles(start, odp_time_cycles()); > - if (diff < wait) > + diff = odp_time_diff(start, odp_time()); > + if (odp_time_cmp(diff, wait) > 0) > continue; > > - start = odp_time_cycles(); > + start = odp_time(); > > if (args->appl.mode == APPL_MODE_RCV) { > pkts = odp_atomic_load_u64(&counters.udp); > diff --git a/include/odp/api/time.h b/include/odp/api/time.h > index b0072fc..7ed4734 100644 > --- a/include/odp/api/time.h > +++ b/include/odp/api/time.h > @@ -28,14 +28,25 @@ extern "C" { > #define ODP_TIME_MSEC 1000000ULL /**< Millisecond in nsec */ > #define ODP_TIME_SEC 1000000000ULL /**< Second in nsec */ New name for these could be XXX_IN_NS ODP_TIME_SEC_IN_NS 1000000000ULL /**< Second in nsec */ > > +/** > + * @typedef odp_time_t > + * ODP time stamp. Time stamp can be local, so shouldn't be shared between > + * threads. > + */ It's OK to implement local time first, but I'd define it so that global time is easy to add later. /** * @typedef odp_time_t * ODP time stamp. Time stamp can be represent a time stamp from local or global time source. * A local time stamp must not be shared between threads. API calls work correctly only when * all time stamps for input are from the same time source. */ In case of 64 bit time stamps, implementation can either define odp_time_t as a small struct or use couple of MSB bits for flags (e.g. time stamp would wrap in 290 years instead of 580 years) > > /** > - * Current time in CPU cycles > - * > - * @return Current time in CPU cycles > + * @def ODP_TIME_NULL > + * Zero time stamp > */ > -uint64_t odp_time_cycles(void); > > +/** > + * Current time stamp. > + * > + * It should be hi-resolution time. > + * > + * @return Time stamp. > + */ > +odp_time_t odp_time(void); This is then renamed to be the local time source. /** * Current local time * * Returns current thread local time stamp value. The local time * source provides high resolution but may wrap between two time * stamp calls. Local time should be used only for short time * range calculations or comparisons. * * @return Local time stamp */ odp_time_t odp_time_local(void); We can add later a call to query the local time stamp wrap around time (max range). All other calls can remain as is. -Petri > > /** > * Time difference > @@ -43,29 +54,60 @@ uint64_t odp_time_cycles(void); > * @param t1 First time stamp > * @param t2 Second time stamp > * > - * @return Difference of time stamps in CPU cycles > + * @return Difference of time stamps > */ > -uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2); > +odp_time_t odp_time_diff(odp_time_t t1, odp_time_t t2); > > +/** > + * Time sum > + * > + * @param t1 Time stamp > + * @param t2 Time stamp > + * > + * @return Sum of time stamps > + */ > +odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); > > /** > - * Convert CPU cycles to nanoseconds > + * Convert time to nanoseconds > * > - * @param cycles Time in CPU cycles > + * @param time Time > * > * @return Time in nanoseconds > */ > -uint64_t odp_time_cycles_to_ns(uint64_t cycles); > - > +uint64_t odp_time_to_ns(odp_time_t time); > > /** > - * Convert nanoseconds to CPU cycles > + * Convert nanoseconds to time > * > * @param ns Time in nanoseconds > * > - * @return Time in CPU cycles > + * @return Time stamp > + */ > +odp_time_t odp_time_from_ns(uint64_t ns); > + > +/** > + * Compare two times as absolute ranges > + * > + * @param t1 First time > + * @param t2 Second time > + * > + * @retval -1 if t2 < t1, 0 if t1 = t2, 1 if t2 > t1 > + */ > +int odp_time_cmp(odp_time_t t1, odp_time_t t2); > + > +/** > + * 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_ns_to_cycles(uint64_t ns); > +uint64_t odp_time_to_u64(odp_time_t time); >
Hi, Petri Thanks for the reply. On 23.10.15 10:57, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Thursday, October 22, 2015 3:02 PM >> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo) >> Cc: Ivan Khoronzhuk >> Subject: [lng-odp] [API-NEXT PATCH v5 3/5] api: time: unbind CPU cycles >> from time API >> >> Current time API supposes that frequency of counter is equal >> to CPU frequency. But that's not always true, for instance, >> in case if no access to CPU cycle counter, another hi-resolution >> timer can be used, and it`s rate can be different from CPU >> rate. There is no big difference in which cycles to measure >> time, the better hi-resolution timer the better measurements. >> So, unbind CPU cycle counter from time API by eliminating word >> "cycle" as it's believed to be used with CPU. >> >> Also add new opaque type for time odp_time_t, as it asks user to use >> API and abstracts time from units. New odp_time_t requires several >> additional API functions to be added: >> >> odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); >> int odp_time_cmp(odp_time_t t1, odp_time_t t2); >> uint64_t odp_time_to_u64(odp_time_t hdl); >> >> Also added new definition that represents 0 ticks for time - >> ODP_TIME_NULL. It can be used instead of odp_time_from_ns(0) for >> comparison and initialization. >> >> This patch changes only used time API, it doesn't change used var >> names for simplicity. >> >> This time API can be implemented with local timer counter, so >> shouldn't be used between threads. >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> --- >> example/generator/odp_generator.c | 12 +++---- >> include/odp/api/time.h | 68 ++++++++++++++++++++++++++++--- >> ---- >> test/performance/odp_pktio_perf.c | 49 +++++++++++++------------ >> test/validation/pktio/pktio.c | 20 +++++------ >> test/validation/scheduler/scheduler.c | 5 +-- >> test/validation/time/time.c | 27 +++++++------- >> 6 files changed, 114 insertions(+), 67 deletions(-) >> >> diff --git a/example/generator/odp_generator.c >> b/example/generator/odp_generator.c >> index be9597b..f84adc4 100644 >> --- a/example/generator/odp_generator.c >> +++ b/example/generator/odp_generator.c >> @@ -585,7 +585,7 @@ static void *gen_recv_thread(void *arg) >> */ >> static void print_global_stats(int num_workers) >> { >> - uint64_t start, wait, diff; >> + odp_time_t start, wait, diff; >> uint64_t pkts, pkts_prev = 0, pps, maximum_pps = 0; >> int verbose_interval = 20; >> odp_thrmask_t thrd_mask; >> @@ -593,8 +593,8 @@ static void print_global_stats(int num_workers) >> while (odp_thrmask_worker(&thrd_mask) < num_workers) >> continue; >> >> - wait = odp_time_ns_to_cycles(verbose_interval * ODP_TIME_SEC); >> - start = odp_time_cycles(); >> + wait = odp_time_from_ns(verbose_interval * ODP_TIME_SEC); >> + start = odp_time(); >> >> while (odp_thrmask_worker(&thrd_mask) == num_workers) { >> if (args->appl.number != -1 && >> @@ -603,11 +603,11 @@ static void print_global_stats(int num_workers) >> break; >> } >> >> - diff = odp_time_diff_cycles(start, odp_time_cycles()); >> - if (diff < wait) >> + diff = odp_time_diff(start, odp_time()); >> + if (odp_time_cmp(diff, wait) > 0) >> continue; >> >> - start = odp_time_cycles(); >> + start = odp_time(); >> >> if (args->appl.mode == APPL_MODE_RCV) { >> pkts = odp_atomic_load_u64(&counters.udp); >> diff --git a/include/odp/api/time.h b/include/odp/api/time.h >> index b0072fc..7ed4734 100644 >> --- a/include/odp/api/time.h >> +++ b/include/odp/api/time.h >> @@ -28,14 +28,25 @@ extern "C" { >> #define ODP_TIME_MSEC 1000000ULL /**< Millisecond in nsec */ >> #define ODP_TIME_SEC 1000000000ULL /**< Second in nsec */ > > > New name for these could be XXX_IN_NS > ODP_TIME_SEC_IN_NS 1000000000ULL /**< Second in nsec */ Ok. As was suggested, I will add it in separate patchset. > > >> >> +/** >> + * @typedef odp_time_t >> + * ODP time stamp. Time stamp can be local, so shouldn't be shared between >> + * threads. >> + */ > > > It's OK to implement local time first, but I'd define it so that global time is easy to add later. I propose to split it by API, and don't use same function for that. It allows to free casual time API from redundancy required by global time and different types of time. Requirements for global time API: - it must be 64-bit value, it's hard to imagine 32-bit source to compare with different threads. - it must be wall time (in another case we cannot compare it) - it never overflow (as it wall time) and if so, no need in _diff, _sum, _cmr functions, as we can do direct operations with 64-bit value - it can require additional synchronization in implementation, so can be slower. - we can get wall time as in "ticks", as in "ns" and directly compare it in appropriate units. For local time it's not required to be wall time, as so wall time is always global. I think it's better to leave current API as local by default, and use additional wall time several calls as global (it's never wrap in practice) Like: uint64_t odp_time_wall(); uint64_t odp_time_wall_ns(); and conversion functions for them: odp_time_wall_from_ns(); // don't take into account any start time and overlap, simply convert ranges odp_time_wall_to_ns(); // don't take into account any start time and overlap, simply convert ranges With these calls no need to change time odp_time() to odp_time_local(). It global time needed (and it must be wall) application uses wall time calls. > > /** > * @typedef odp_time_t > * ODP time stamp. Time stamp can be represent a time stamp from local or global time source. > * A local time stamp must not be shared between threads. I thinks that's enough. API calls work correctly only when all time stamps for input are from the same time source. It's redundancy. > */ > > In case of 64 bit time stamps, implementation can either define odp_time_t as a small struct or use couple of MSB bits for flags (e.g. time stamp would wrap in 290 years instead of 580 years) With separate calls for wall time it's not needed. > > >> >> /** >> - * Current time in CPU cycles >> - * >> - * @return Current time in CPU cycles >> + * @def ODP_TIME_NULL >> + * Zero time stamp >> */ >> -uint64_t odp_time_cycles(void); >> >> +/** >> + * Current time stamp. >> + * >> + * It should be hi-resolution time. >> + * >> + * @return Time stamp. >> + */ >> +odp_time_t odp_time(void); > > > This is then renamed to be the local time source I think no need to overload API with time type here. It's obvious that it cannot be used as global in case if we have wall time API and correctly document it. > > /** > * Current local time > * > * Returns current thread local time stamp value. The local time > * source provides high resolution but may wrap between two time > * stamp calls. Local time should be used only for short time > * range calculations or comparisons. > * > * @return Local time stamp > */ > odp_time_t odp_time_local(void); > > > We can add later a call to query the local time stamp wrap around time (max range). We can, but what practical usage for that. Application can use it at init only for check if time source is appropriate for that. But more correct way is to write correct application and for huge time ranges use timers. And I'm mostly sure that 32-bit counter will not be used for that, but anyway, why not to add it in the future. > > All other calls can remain as is. > > -Petri > >> >> /** >> * Time difference >> @@ -43,29 +54,60 @@ uint64_t odp_time_cycles(void); >> * @param t1 First time stamp >> * @param t2 Second time stamp >> * >> - * @return Difference of time stamps in CPU cycles >> + * @return Difference of time stamps >> */ >> -uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2); >> +odp_time_t odp_time_diff(odp_time_t t1, odp_time_t t2); >> >> +/** >> + * Time sum >> + * >> + * @param t1 Time stamp >> + * @param t2 Time stamp >> + * >> + * @return Sum of time stamps >> + */ >> +odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); >> >> /** >> - * Convert CPU cycles to nanoseconds >> + * Convert time to nanoseconds >> * >> - * @param cycles Time in CPU cycles >> + * @param time Time >> * >> * @return Time in nanoseconds >> */ >> -uint64_t odp_time_cycles_to_ns(uint64_t cycles); >> - >> +uint64_t odp_time_to_ns(odp_time_t time); >> >> /** >> - * Convert nanoseconds to CPU cycles >> + * Convert nanoseconds to time >> * >> * @param ns Time in nanoseconds >> * >> - * @return Time in CPU cycles >> + * @return Time stamp >> + */ >> +odp_time_t odp_time_from_ns(uint64_t ns); >> + >> +/** >> + * Compare two times as absolute ranges >> + * >> + * @param t1 First time >> + * @param t2 Second time >> + * >> + * @retval -1 if t2 < t1, 0 if t1 = t2, 1 if t2 > t1 >> + */ >> +int odp_time_cmp(odp_time_t t1, odp_time_t t2); >> + >> +/** >> + * 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_ns_to_cycles(uint64_t ns); >> +uint64_t odp_time_to_u64(odp_time_t time); >>
ping. On 23.10.15 11:35, Ivan Khoronzhuk wrote: > Hi, Petri > Thanks for the reply. > > On 23.10.15 10:57, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >> >>> -----Original Message----- >>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>> Sent: Thursday, October 22, 2015 3:02 PM >>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo) >>> Cc: Ivan Khoronzhuk >>> Subject: [lng-odp] [API-NEXT PATCH v5 3/5] api: time: unbind CPU cycles >>> from time API >>> >>> Current time API supposes that frequency of counter is equal >>> to CPU frequency. But that's not always true, for instance, >>> in case if no access to CPU cycle counter, another hi-resolution >>> timer can be used, and it`s rate can be different from CPU >>> rate. There is no big difference in which cycles to measure >>> time, the better hi-resolution timer the better measurements. >>> So, unbind CPU cycle counter from time API by eliminating word >>> "cycle" as it's believed to be used with CPU. >>> >>> Also add new opaque type for time odp_time_t, as it asks user to use >>> API and abstracts time from units. New odp_time_t requires several >>> additional API functions to be added: >>> >>> odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); >>> int odp_time_cmp(odp_time_t t1, odp_time_t t2); >>> uint64_t odp_time_to_u64(odp_time_t hdl); >>> >>> Also added new definition that represents 0 ticks for time - >>> ODP_TIME_NULL. It can be used instead of odp_time_from_ns(0) for >>> comparison and initialization. >>> >>> This patch changes only used time API, it doesn't change used var >>> names for simplicity. >>> >>> This time API can be implemented with local timer counter, so >>> shouldn't be used between threads. >>> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>> --- >>> example/generator/odp_generator.c | 12 +++---- >>> include/odp/api/time.h | 68 ++++++++++++++++++++++++++++--- >>> ---- >>> test/performance/odp_pktio_perf.c | 49 +++++++++++++------------ >>> test/validation/pktio/pktio.c | 20 +++++------ >>> test/validation/scheduler/scheduler.c | 5 +-- >>> test/validation/time/time.c | 27 +++++++------- >>> 6 files changed, 114 insertions(+), 67 deletions(-) >>> >>> diff --git a/example/generator/odp_generator.c >>> b/example/generator/odp_generator.c >>> index be9597b..f84adc4 100644 >>> --- a/example/generator/odp_generator.c >>> +++ b/example/generator/odp_generator.c >>> @@ -585,7 +585,7 @@ static void *gen_recv_thread(void *arg) >>> */ >>> static void print_global_stats(int num_workers) >>> { >>> - uint64_t start, wait, diff; >>> + odp_time_t start, wait, diff; >>> uint64_t pkts, pkts_prev = 0, pps, maximum_pps = 0; >>> int verbose_interval = 20; >>> odp_thrmask_t thrd_mask; >>> @@ -593,8 +593,8 @@ static void print_global_stats(int num_workers) >>> while (odp_thrmask_worker(&thrd_mask) < num_workers) >>> continue; >>> >>> - wait = odp_time_ns_to_cycles(verbose_interval * ODP_TIME_SEC); >>> - start = odp_time_cycles(); >>> + wait = odp_time_from_ns(verbose_interval * ODP_TIME_SEC); >>> + start = odp_time(); >>> >>> while (odp_thrmask_worker(&thrd_mask) == num_workers) { >>> if (args->appl.number != -1 && >>> @@ -603,11 +603,11 @@ static void print_global_stats(int num_workers) >>> break; >>> } >>> >>> - diff = odp_time_diff_cycles(start, odp_time_cycles()); >>> - if (diff < wait) >>> + diff = odp_time_diff(start, odp_time()); >>> + if (odp_time_cmp(diff, wait) > 0) >>> continue; >>> >>> - start = odp_time_cycles(); >>> + start = odp_time(); >>> >>> if (args->appl.mode == APPL_MODE_RCV) { >>> pkts = odp_atomic_load_u64(&counters.udp); >>> diff --git a/include/odp/api/time.h b/include/odp/api/time.h >>> index b0072fc..7ed4734 100644 >>> --- a/include/odp/api/time.h >>> +++ b/include/odp/api/time.h >>> @@ -28,14 +28,25 @@ extern "C" { >>> #define ODP_TIME_MSEC 1000000ULL /**< Millisecond in nsec */ >>> #define ODP_TIME_SEC 1000000000ULL /**< Second in nsec */ >> >> >> New name for these could be XXX_IN_NS >> ODP_TIME_SEC_IN_NS 1000000000ULL /**< Second in nsec */ > Ok. > As was suggested, I will add it in separate patchset. > >> >> >>> >>> +/** >>> + * @typedef odp_time_t >>> + * ODP time stamp. Time stamp can be local, so shouldn't be shared between >>> + * threads. >>> + */ >> >> >> It's OK to implement local time first, but I'd define it so that global time is easy to add later. > > I propose to split it by API, and don't use same function for that. > It allows to free casual time API from redundancy required by global time and different types of time. > Requirements for global time API: > - it must be 64-bit value, it's hard to imagine 32-bit source to compare with different threads. > - it must be wall time (in another case we cannot compare it) > - it never overflow (as it wall time) and if so, no need in _diff, _sum, _cmr functions, as we can do direct operations with 64-bit value > - it can require additional synchronization in implementation, so can be slower. > - we can get wall time as in "ticks", as in "ns" and directly compare it in appropriate units. > > For local time it's not required to be wall time, as so wall time is always global. > I think it's better to leave current API as local by default, and use additional > wall time several calls as global (it's never wrap in practice) > Like: > uint64_t odp_time_wall(); > uint64_t odp_time_wall_ns(); > > and conversion functions for them: > odp_time_wall_from_ns(); // don't take into account any start time and overlap, simply convert ranges > odp_time_wall_to_ns(); // don't take into account any start time and overlap, simply convert ranges > > With these calls no need to change time odp_time() to odp_time_local(). > It global time needed (and it must be wall) application uses wall time calls. > >> >> /** >> * @typedef odp_time_t >> * ODP time stamp. Time stamp can be represent a time stamp from local or global time source. >> * A local time stamp must not be shared between threads. > I thinks that's enough. > > API calls work correctly only when all time stamps for input are from the same time source. > It's redundancy. > >> */ >> >> In case of 64 bit time stamps, implementation can either define odp_time_t as a small struct or use couple of MSB bits for flags (e.g. time stamp would wrap in 290 years instead of 580 years) > With separate calls for wall time it's not needed. > >> >> >>> >>> /** >>> - * Current time in CPU cycles >>> - * >>> - * @return Current time in CPU cycles >>> + * @def ODP_TIME_NULL >>> + * Zero time stamp >>> */ >>> -uint64_t odp_time_cycles(void); >>> >>> +/** >>> + * Current time stamp. >>> + * >>> + * It should be hi-resolution time. >>> + * >>> + * @return Time stamp. >>> + */ >>> +odp_time_t odp_time(void); >> >> >> This is then renamed to be the local time source > I think no need to overload API with time type here. > It's obvious that it cannot be used as global in case if we have wall time API > and correctly document it. > >> >> /** >> * Current local time >> * >> * Returns current thread local time stamp value. The local time >> * source provides high resolution but may wrap between two time >> * stamp calls. Local time should be used only for short time >> * range calculations or comparisons. >> * >> * @return Local time stamp >> */ >> odp_time_t odp_time_local(void); >> >> >> We can add later a call to query the local time stamp wrap around time (max range). > We can, but what practical usage for that. > Application can use it at init only for check if time source is appropriate for that. > But more correct way is to write correct application and for huge time ranges use timers. > And I'm mostly sure that 32-bit counter will not be used for that, but anyway, > why not to add it in the future. > >> >> All other calls can remain as is. >> >> -Petri >> >>> >>> /** >>> * Time difference >>> @@ -43,29 +54,60 @@ uint64_t odp_time_cycles(void); >>> * @param t1 First time stamp >>> * @param t2 Second time stamp >>> * >>> - * @return Difference of time stamps in CPU cycles >>> + * @return Difference of time stamps >>> */ >>> -uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2); >>> +odp_time_t odp_time_diff(odp_time_t t1, odp_time_t t2); >>> >>> +/** >>> + * Time sum >>> + * >>> + * @param t1 Time stamp >>> + * @param t2 Time stamp >>> + * >>> + * @return Sum of time stamps >>> + */ >>> +odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); >>> >>> /** >>> - * Convert CPU cycles to nanoseconds >>> + * Convert time to nanoseconds >>> * >>> - * @param cycles Time in CPU cycles >>> + * @param time Time >>> * >>> * @return Time in nanoseconds >>> */ >>> -uint64_t odp_time_cycles_to_ns(uint64_t cycles); >>> - >>> +uint64_t odp_time_to_ns(odp_time_t time); >>> >>> /** >>> - * Convert nanoseconds to CPU cycles >>> + * Convert nanoseconds to time >>> * >>> * @param ns Time in nanoseconds >>> * >>> - * @return Time in CPU cycles >>> + * @return Time stamp >>> + */ >>> +odp_time_t odp_time_from_ns(uint64_t ns); >>> + >>> +/** >>> + * Compare two times as absolute ranges >>> + * >>> + * @param t1 First time >>> + * @param t2 Second time >>> + * >>> + * @retval -1 if t2 < t1, 0 if t1 = t2, 1 if t2 > t1 >>> + */ >>> +int odp_time_cmp(odp_time_t t1, odp_time_t t2); >>> + >>> +/** >>> + * 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_ns_to_cycles(uint64_t ns); >>> +uint64_t odp_time_to_u64(odp_time_t time); >>> >
On 27.10.15 15:25, Savolainen, Petri (Nokia - FI/Espoo) wrote: > ODP time cannot be only local. Local time has limited use cases (measure < 4sec function call durations in time vs in CPU cycles). > 4s, with 64-bit > More interesting use cases demand global/sharable time. E.g. > every packet may be time stamped in packet input (e.g. 1588 time stamp). > Application is load balanced over multiple cores and those read/write/compare the packet time stamp along the processing pipeline. uint64_t odp_time_wall(); uint64_t odp_time_wall_ns(); ? > ODP time should be represented by one handle / type. hiding type is time consuming operation for time catching functions. > Timer time (timer ticks) is not used for time calculation/comparisons, > it's for controlling timers and timeout events. sure. > > C11 and DPDK uses struct timespec, which has sec + nsec fields. > We'd need to minimize need for converting between internal cycles and sec+nsec (performance penalty). sure. That's why conversion is made before loop. > > We can discuss this on the calls. > > -Petri > > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Tuesday, October 27, 2015 2:22 PM >> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [API-NEXT PATCH v5 3/5] api: time: unbind CPU cycles >> from time API >> >> ping. >> >> On 23.10.15 11:35, Ivan Khoronzhuk wrote: >>> Hi, Petri >>> Thanks for the reply. >>> >>> On 23.10.15 10:57, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>>>> Sent: Thursday, October 22, 2015 3:02 PM >>>>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo) >>>>> Cc: Ivan Khoronzhuk >>>>> Subject: [lng-odp] [API-NEXT PATCH v5 3/5] api: time: unbind CPU cycles >>>>> from time API >>>>> >>>>> Current time API supposes that frequency of counter is equal >>>>> to CPU frequency. But that's not always true, for instance, >>>>> in case if no access to CPU cycle counter, another hi-resolution >>>>> timer can be used, and it`s rate can be different from CPU >>>>> rate. There is no big difference in which cycles to measure >>>>> time, the better hi-resolution timer the better measurements. >>>>> So, unbind CPU cycle counter from time API by eliminating word >>>>> "cycle" as it's believed to be used with CPU. >>>>> >>>>> Also add new opaque type for time odp_time_t, as it asks user to use >>>>> API and abstracts time from units. New odp_time_t requires several >>>>> additional API functions to be added: >>>>> >>>>> odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); >>>>> int odp_time_cmp(odp_time_t t1, odp_time_t t2); >>>>> uint64_t odp_time_to_u64(odp_time_t hdl); >>>>> >>>>> Also added new definition that represents 0 ticks for time - >>>>> ODP_TIME_NULL. It can be used instead of odp_time_from_ns(0) for >>>>> comparison and initialization. >>>>> >>>>> This patch changes only used time API, it doesn't change used var >>>>> names for simplicity. >>>>> >>>>> This time API can be implemented with local timer counter, so >>>>> shouldn't be used between threads. >>>>> >>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>>>> --- >>>>> example/generator/odp_generator.c | 12 +++---- >>>>> include/odp/api/time.h | 68 >> ++++++++++++++++++++++++++++--- >>>>> ---- >>>>> test/performance/odp_pktio_perf.c | 49 +++++++++++++------------ >>>>> test/validation/pktio/pktio.c | 20 +++++------ >>>>> test/validation/scheduler/scheduler.c | 5 +-- >>>>> test/validation/time/time.c | 27 +++++++------- >>>>> 6 files changed, 114 insertions(+), 67 deletions(-) >>>>> >>>>> diff --git a/example/generator/odp_generator.c >>>>> b/example/generator/odp_generator.c >>>>> index be9597b..f84adc4 100644 >>>>> --- a/example/generator/odp_generator.c >>>>> +++ b/example/generator/odp_generator.c >>>>> @@ -585,7 +585,7 @@ static void *gen_recv_thread(void *arg) >>>>> */ >>>>> static void print_global_stats(int num_workers) >>>>> { >>>>> - uint64_t start, wait, diff; >>>>> + odp_time_t start, wait, diff; >>>>> uint64_t pkts, pkts_prev = 0, pps, maximum_pps = 0; >>>>> int verbose_interval = 20; >>>>> odp_thrmask_t thrd_mask; >>>>> @@ -593,8 +593,8 @@ static void print_global_stats(int num_workers) >>>>> while (odp_thrmask_worker(&thrd_mask) < num_workers) >>>>> continue; >>>>> >>>>> - wait = odp_time_ns_to_cycles(verbose_interval * ODP_TIME_SEC); >>>>> - start = odp_time_cycles(); >>>>> + wait = odp_time_from_ns(verbose_interval * ODP_TIME_SEC); >>>>> + start = odp_time(); >>>>> >>>>> while (odp_thrmask_worker(&thrd_mask) == num_workers) { >>>>> if (args->appl.number != -1 && >>>>> @@ -603,11 +603,11 @@ static void print_global_stats(int num_workers) >>>>> break; >>>>> } >>>>> >>>>> - diff = odp_time_diff_cycles(start, odp_time_cycles()); >>>>> - if (diff < wait) >>>>> + diff = odp_time_diff(start, odp_time()); >>>>> + if (odp_time_cmp(diff, wait) > 0) >>>>> continue; >>>>> >>>>> - start = odp_time_cycles(); >>>>> + start = odp_time(); >>>>> >>>>> if (args->appl.mode == APPL_MODE_RCV) { >>>>> pkts = odp_atomic_load_u64(&counters.udp); >>>>> diff --git a/include/odp/api/time.h b/include/odp/api/time.h >>>>> index b0072fc..7ed4734 100644 >>>>> --- a/include/odp/api/time.h >>>>> +++ b/include/odp/api/time.h >>>>> @@ -28,14 +28,25 @@ extern "C" { >>>>> #define ODP_TIME_MSEC 1000000ULL /**< Millisecond in nsec */ >>>>> #define ODP_TIME_SEC 1000000000ULL /**< Second in nsec */ >>>> >>>> >>>> New name for these could be XXX_IN_NS >>>> ODP_TIME_SEC_IN_NS 1000000000ULL /**< Second in nsec */ >>> Ok. >>> As was suggested, I will add it in separate patchset. >>> >>>> >>>> >>>>> >>>>> +/** >>>>> + * @typedef odp_time_t >>>>> + * ODP time stamp. Time stamp can be local, so shouldn't be shared >> between >>>>> + * threads. >>>>> + */ >>>> >>>> >>>> It's OK to implement local time first, but I'd define it so that global >> time is easy to add later. >>> >>> I propose to split it by API, and don't use same function for that. >>> It allows to free casual time API from redundancy required by global time >> and different types of time. >>> Requirements for global time API: >>> - it must be 64-bit value, it's hard to imagine 32-bit source to compare >> with different threads. >>> - it must be wall time (in another case we cannot compare it) >>> - it never overflow (as it wall time) and if so, no need in _diff, _sum, >> _cmr functions, as we can do direct operations with 64-bit value >>> - it can require additional synchronization in implementation, so can be >> slower. >>> - we can get wall time as in "ticks", as in "ns" and directly compare it >> in appropriate units. >>> >>> For local time it's not required to be wall time, as so wall time is >> always global. >>> I think it's better to leave current API as local by default, and use >> additional >>> wall time several calls as global (it's never wrap in practice) >>> Like: >>> uint64_t odp_time_wall(); >>> uint64_t odp_time_wall_ns(); >>> >>> and conversion functions for them: >>> odp_time_wall_from_ns(); // don't take into account any start time and >> overlap, simply convert ranges >>> odp_time_wall_to_ns(); // don't take into account any start time and >> overlap, simply convert ranges >>> >>> With these calls no need to change time odp_time() to odp_time_local(). >>> It global time needed (and it must be wall) application uses wall time >> calls. >>> >>>> >>>> /** >>>> * @typedef odp_time_t >>>> * ODP time stamp. Time stamp can be represent a time stamp from local >> or global time source. >>>> * A local time stamp must not be shared between threads. >>> I thinks that's enough. >>> >>> API calls work correctly only when all time stamps for input are from >> the same time source. >>> It's redundancy. >>> >>>> */ >>>> >>>> In case of 64 bit time stamps, implementation can either define >> odp_time_t as a small struct or use couple of MSB bits for flags (e.g. time >> stamp would wrap in 290 years instead of 580 years) >>> With separate calls for wall time it's not needed. >>> >>>> >>>> >>>>> >>>>> /** >>>>> - * Current time in CPU cycles >>>>> - * >>>>> - * @return Current time in CPU cycles >>>>> + * @def ODP_TIME_NULL >>>>> + * Zero time stamp >>>>> */ >>>>> -uint64_t odp_time_cycles(void); >>>>> >>>>> +/** >>>>> + * Current time stamp. >>>>> + * >>>>> + * It should be hi-resolution time. >>>>> + * >>>>> + * @return Time stamp. >>>>> + */ >>>>> +odp_time_t odp_time(void); >>>> >>>> >>>> This is then renamed to be the local time source >>> I think no need to overload API with time type here. >>> It's obvious that it cannot be used as global in case if we have wall >> time API >>> and correctly document it. >>> >>>> >>>> /** >>>> * Current local time >>>> * >>>> * Returns current thread local time stamp value. The local time >>>> * source provides high resolution but may wrap between two time >>>> * stamp calls. Local time should be used only for short time >>>> * range calculations or comparisons. >>>> * >>>> * @return Local time stamp >>>> */ >>>> odp_time_t odp_time_local(void); >>>> >>>> >>>> We can add later a call to query the local time stamp wrap around time >> (max range). >>> We can, but what practical usage for that. >>> Application can use it at init only for check if time source is >> appropriate for that. >>> But more correct way is to write correct application and for huge time >> ranges use timers. >>> And I'm mostly sure that 32-bit counter will not be used for that, but >> anyway, >>> why not to add it in the future. >>> >>>> >>>> All other calls can remain as is. >>>> >>>> -Petri >>>> >>>>> >>>>> /** >>>>> * Time difference >>>>> @@ -43,29 +54,60 @@ uint64_t odp_time_cycles(void); >>>>> * @param t1 First time stamp >>>>> * @param t2 Second time stamp >>>>> * >>>>> - * @return Difference of time stamps in CPU cycles >>>>> + * @return Difference of time stamps >>>>> */ >>>>> -uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2); >>>>> +odp_time_t odp_time_diff(odp_time_t t1, odp_time_t t2); >>>>> >>>>> +/** >>>>> + * Time sum >>>>> + * >>>>> + * @param t1 Time stamp >>>>> + * @param t2 Time stamp >>>>> + * >>>>> + * @return Sum of time stamps >>>>> + */ >>>>> +odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); >>>>> >>>>> /** >>>>> - * Convert CPU cycles to nanoseconds >>>>> + * Convert time to nanoseconds >>>>> * >>>>> - * @param cycles Time in CPU cycles >>>>> + * @param time Time >>>>> * >>>>> * @return Time in nanoseconds >>>>> */ >>>>> -uint64_t odp_time_cycles_to_ns(uint64_t cycles); >>>>> - >>>>> +uint64_t odp_time_to_ns(odp_time_t time); >>>>> >>>>> /** >>>>> - * Convert nanoseconds to CPU cycles >>>>> + * Convert nanoseconds to time >>>>> * >>>>> * @param ns Time in nanoseconds >>>>> * >>>>> - * @return Time in CPU cycles >>>>> + * @return Time stamp >>>>> + */ >>>>> +odp_time_t odp_time_from_ns(uint64_t ns); >>>>> + >>>>> +/** >>>>> + * Compare two times as absolute ranges >>>>> + * >>>>> + * @param t1 First time >>>>> + * @param t2 Second time >>>>> + * >>>>> + * @retval -1 if t2 < t1, 0 if t1 = t2, 1 if t2 > t1 >>>>> + */ >>>>> +int odp_time_cmp(odp_time_t t1, odp_time_t t2); >>>>> + >>>>> +/** >>>>> + * 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_ns_to_cycles(uint64_t ns); >>>>> +uint64_t odp_time_to_u64(odp_time_t time); >>>>> >>> >> >> -- >> Regards, >> Ivan Khoronzhuk
Hi Petri,
Let's clarify some aspects that can be taken into account if common time type is used
for global and local times. Mainly it reflected in operation/conversion functions.
Mostly that is why I want to have separate functions for global and local times.
Please say what do you think about all of that.
1) odp_time_diff(t1, t2) behaves differently for local/global/interval timestamps
- if at least t1 is global time it must return timestamp of global time.
- if t1 and t2 are time intervals it returns time interval (in units local |
global).
- if at least t1 is local time it must return timestamp of local time.
That can be implemented with hiding one additional var/flag under timestamp that
is redundancy and require to analyze it in every API call. Also it makes ns/time
conversion functions behave differently and returns one additional pseudo type
like interval, that is by a big account orthogonal to time type.
2) odp_time_cmp(odp_time_t t1, odp_time_t t2) behaves differently for
local/global/interval time
- if comparing global time stamps, results gives what time was first
- you cannot compare local times as local time can wrap
- if comparing intervals, results gives what interval is bigger (for local |
global).
3) uint64_t odp_time_to_ns(odp_time_t time) behaves differently for
local/global/interval timestamps
- if converting global time, result is in "wall" ns.
- if converting local time, result is in local ticks as is.
- if converting local interval or local time, result is interval ns.
- if converting global interval, reult is in interval ns.
By a big account user knows what he is doing, so at least that is not a problem,
except redundancy.
4) odp_time_t odp_time_from_ns(uint64_t ns) creates set of functions
- odp_time_local_from_ns()
- odp_global_from_ns()
- odp_global_interval_from_ns()
- odp_local_interval_from_ns() // can be replaced by odp_time_local_from_ns()
// but better have for consistency.
5) odp_time_to_u64() also must return time/interval depending of the units of
time type.
I'm sure that adding new time type can add even more redundancy in API, but
want to believe that local and global time are only times required, no see reason
to add smth else.
Also analyzing time type/interval/not interval in each function, several of which
them can be called in loops, is not very efficient. The same for global
timestamp for packets, it can be very frequently to call it stack, in which case
several saved CPU cycles for one call can save a thousands of cycles in
loops/stacks etc.
Adding global time as separate API can make life easier and reduce number of
API functions required to converting between different time types.
On 27.10.15 15:25, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> ODP time cannot be only local. Local time has limited use cases (measure < 4sec function call durations in time vs in CPU cycles). More interesting use cases demand global/sharable time. E.g. every packet may be time stamped in packet input (e.g. 1588 time stamp). Application is load balanced over multiple cores and those read/write/compare the packet time stamp along the processing pipeline. ODP time should be represented by one handle / type. Timer time (timer ticks) is not used for time calculation/comparisons, it's for controlling timers and timeout events.
>
> C11 and DPDK uses struct timespec, which has sec + nsec fields. We'd need to minimize need for converting between internal cycles and sec+nsec (performance penalty).
>
> We can discuss this on the calls.
>
> -Petri
>
>
>
>> -----Original Message-----
>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org]
>> Sent: Tuesday, October 27, 2015 2:22 PM
>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org
>> Subject: Re: [lng-odp] [API-NEXT PATCH v5 3/5] api: time: unbind CPU cycles
>> from time API
>>
>> ping.
>>
>> On 23.10.15 11:35, Ivan Khoronzhuk wrote:
>>> Hi, Petri
>>> Thanks for the reply.
>>>
>>> On 23.10.15 10:57, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org]
>>>>> Sent: Thursday, October 22, 2015 3:02 PM
>>>>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo)
>>>>> Cc: Ivan Khoronzhuk
>>>>> Subject: [lng-odp] [API-NEXT PATCH v5 3/5] api: time: unbind CPU cycles
>>>>> from time API
>>>>>
>>>>> Current time API supposes that frequency of counter is equal
>>>>> to CPU frequency. But that's not always true, for instance,
>>>>> in case if no access to CPU cycle counter, another hi-resolution
>>>>> timer can be used, and it`s rate can be different from CPU
>>>>> rate. There is no big difference in which cycles to measure
>>>>> time, the better hi-resolution timer the better measurements.
>>>>> So, unbind CPU cycle counter from time API by eliminating word
>>>>> "cycle" as it's believed to be used with CPU.
>>>>>
>>>>> Also add new opaque type for time odp_time_t, as it asks user to use
>>>>> API and abstracts time from units. New odp_time_t requires several
>>>>> additional API functions to be added:
>>>>>
>>>>> odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2);
>>>>> int odp_time_cmp(odp_time_t t1, odp_time_t t2);
>>>>> uint64_t odp_time_to_u64(odp_time_t hdl);
>>>>>
>>>>> Also added new definition that represents 0 ticks for time -
>>>>> ODP_TIME_NULL. It can be used instead of odp_time_from_ns(0) for
>>>>> comparison and initialization.
>>>>>
>>>>> This patch changes only used time API, it doesn't change used var
>>>>> names for simplicity.
>>>>>
>>>>> This time API can be implemented with local timer counter, so
>>>>> shouldn't be used between threads.
>>>>>
>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>>>> ---
>>>>> example/generator/odp_generator.c | 12 +++----
>>>>> include/odp/api/time.h | 68
>> ++++++++++++++++++++++++++++---
>>>>> ----
>>>>> test/performance/odp_pktio_perf.c | 49 +++++++++++++------------
>>>>> test/validation/pktio/pktio.c | 20 +++++------
>>>>> test/validation/scheduler/scheduler.c | 5 +--
>>>>> test/validation/time/time.c | 27 +++++++-------
>>>>> 6 files changed, 114 insertions(+), 67 deletions(-)
>>>>>
>>>>> diff --git a/example/generator/odp_generator.c
>>>>> b/example/generator/odp_generator.c
>>>>> index be9597b..f84adc4 100644
>>>>> --- a/example/generator/odp_generator.c
>>>>> +++ b/example/generator/odp_generator.c
>>>>> @@ -585,7 +585,7 @@ static void *gen_recv_thread(void *arg)
>>>>> */
>>>>> static void print_global_stats(int num_workers)
>>>>> {
>>>>> - uint64_t start, wait, diff;
>>>>> + odp_time_t start, wait, diff;
>>>>> uint64_t pkts, pkts_prev = 0, pps, maximum_pps = 0;
>>>>> int verbose_interval = 20;
>>>>> odp_thrmask_t thrd_mask;
>>>>> @@ -593,8 +593,8 @@ static void print_global_stats(int num_workers)
>>>>> while (odp_thrmask_worker(&thrd_mask) < num_workers)
>>>>> continue;
>>>>>
>>>>> - wait = odp_time_ns_to_cycles(verbose_interval * ODP_TIME_SEC);
>>>>> - start = odp_time_cycles();
>>>>> + wait = odp_time_from_ns(verbose_interval * ODP_TIME_SEC);
>>>>> + start = odp_time();
>>>>>
>>>>> while (odp_thrmask_worker(&thrd_mask) == num_workers) {
>>>>> if (args->appl.number != -1 &&
>>>>> @@ -603,11 +603,11 @@ static void print_global_stats(int num_workers)
>>>>> break;
>>>>> }
>>>>>
>>>>> - diff = odp_time_diff_cycles(start, odp_time_cycles());
>>>>> - if (diff < wait)
>>>>> + diff = odp_time_diff(start, odp_time());
>>>>> + if (odp_time_cmp(diff, wait) > 0)
>>>>> continue;
>>>>>
>>>>> - start = odp_time_cycles();
>>>>> + start = odp_time();
>>>>>
>>>>> if (args->appl.mode == APPL_MODE_RCV) {
>>>>> pkts = odp_atomic_load_u64(&counters.udp);
>>>>> diff --git a/include/odp/api/time.h b/include/odp/api/time.h
>>>>> index b0072fc..7ed4734 100644
>>>>> --- a/include/odp/api/time.h
>>>>> +++ b/include/odp/api/time.h
>>>>> @@ -28,14 +28,25 @@ extern "C" {
>>>>> #define ODP_TIME_MSEC 1000000ULL /**< Millisecond in nsec */
>>>>> #define ODP_TIME_SEC 1000000000ULL /**< Second in nsec */
>>>>
>>>>
>>>> New name for these could be XXX_IN_NS
>>>> ODP_TIME_SEC_IN_NS 1000000000ULL /**< Second in nsec */
>>> Ok.
>>> As was suggested, I will add it in separate patchset.
>>>
>>>>
>>>>
>>>>>
>>>>> +/**
>>>>> + * @typedef odp_time_t
>>>>> + * ODP time stamp. Time stamp can be local, so shouldn't be shared
>> between
>>>>> + * threads.
>>>>> + */
>>>>
>>>>
>>>> It's OK to implement local time first, but I'd define it so that global
>> time is easy to add later.
>>>
>>> I propose to split it by API, and don't use same function for that.
>>> It allows to free casual time API from redundancy required by global time
>> and different types of time.
>>> Requirements for global time API:
>>> - it must be 64-bit value, it's hard to imagine 32-bit source to compare
>> with different threads.
>>> - it must be wall time (in another case we cannot compare it)
>>> - it never overflow (as it wall time) and if so, no need in _diff, _sum,
>> _cmr functions, as we can do direct operations with 64-bit value
>>> - it can require additional synchronization in implementation, so can be
>> slower.
>>> - we can get wall time as in "ticks", as in "ns" and directly compare it
>> in appropriate units.
>>>
>>> For local time it's not required to be wall time, as so wall time is
>> always global.
>>> I think it's better to leave current API as local by default, and use
>> additional
>>> wall time several calls as global (it's never wrap in practice)
>>> Like:
>>> uint64_t odp_time_wall();
>>> uint64_t odp_time_wall_ns();
>>>
>>> and conversion functions for them:
>>> odp_time_wall_from_ns(); // don't take into account any start time and
>> overlap, simply convert ranges
>>> odp_time_wall_to_ns(); // don't take into account any start time and
>> overlap, simply convert ranges
>>>
>>> With these calls no need to change time odp_time() to odp_time_local().
>>> It global time needed (and it must be wall) application uses wall time
>> calls.
>>>
>>>>
>>>> /**
>>>> * @typedef odp_time_t
>>>> * ODP time stamp. Time stamp can be represent a time stamp from local
>> or global time source.
>>>> * A local time stamp must not be shared between threads.
>>> I thinks that's enough.
>>>
>>> API calls work correctly only when all time stamps for input are from
>> the same time source.
>>> It's redundancy.
>>>
>>>> */
>>>>
>>>> In case of 64 bit time stamps, implementation can either define
>> odp_time_t as a small struct or use couple of MSB bits for flags (e.g. time
>> stamp would wrap in 290 years instead of 580 years)
>>> With separate calls for wall time it's not needed.
>>>
>>>>
>>>>
>>>>>
>>>>> /**
>>>>> - * Current time in CPU cycles
>>>>> - *
>>>>> - * @return Current time in CPU cycles
>>>>> + * @def ODP_TIME_NULL
>>>>> + * Zero time stamp
>>>>> */
>>>>> -uint64_t odp_time_cycles(void);
>>>>>
>>>>> +/**
>>>>> + * Current time stamp.
>>>>> + *
>>>>> + * It should be hi-resolution time.
>>>>> + *
>>>>> + * @return Time stamp.
>>>>> + */
>>>>> +odp_time_t odp_time(void);
>>>>
>>>>
>>>> This is then renamed to be the local time source
>>> I think no need to overload API with time type here.
>>> It's obvious that it cannot be used as global in case if we have wall
>> time API
>>> and correctly document it.
>>>
>>>>
>>>> /**
>>>> * Current local time
>>>> *
>>>> * Returns current thread local time stamp value. The local time
>>>> * source provides high resolution but may wrap between two time
>>>> * stamp calls. Local time should be used only for short time
>>>> * range calculations or comparisons.
>>>> *
>>>> * @return Local time stamp
>>>> */
>>>> odp_time_t odp_time_local(void);
>>>>
>>>>
>>>> We can add later a call to query the local time stamp wrap around time
>> (max range).
>>> We can, but what practical usage for that.
>>> Application can use it at init only for check if time source is
>> appropriate for that.
>>> But more correct way is to write correct application and for huge time
>> ranges use timers.
>>> And I'm mostly sure that 32-bit counter will not be used for that, but
>> anyway,
>>> why not to add it in the future.
>>>
>>>>
>>>> All other calls can remain as is.
>>>>
>>>> -Petri
>>>>
>>>>>
>>>>> /**
>>>>> * Time difference
>>>>> @@ -43,29 +54,60 @@ uint64_t odp_time_cycles(void);
>>>>> * @param t1 First time stamp
>>>>> * @param t2 Second time stamp
>>>>> *
>>>>> - * @return Difference of time stamps in CPU cycles
>>>>> + * @return Difference of time stamps
>>>>> */
>>>>> -uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2);
>>>>> +odp_time_t odp_time_diff(odp_time_t t1, odp_time_t t2);
>>>>>
>>>>> +/**
>>>>> + * Time sum
>>>>> + *
>>>>> + * @param t1 Time stamp
>>>>> + * @param t2 Time stamp
>>>>> + *
>>>>> + * @return Sum of time stamps
>>>>> + */
>>>>> +odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2);
>>>>>
>>>>> /**
>>>>> - * Convert CPU cycles to nanoseconds
>>>>> + * Convert time to nanoseconds
>>>>> *
>>>>> - * @param cycles Time in CPU cycles
>>>>> + * @param time Time
>>>>> *
>>>>> * @return Time in nanoseconds
>>>>> */
>>>>> -uint64_t odp_time_cycles_to_ns(uint64_t cycles);
>>>>> -
>>>>> +uint64_t odp_time_to_ns(odp_time_t time);
>>>>>
>>>>> /**
>>>>> - * Convert nanoseconds to CPU cycles
>>>>> + * Convert nanoseconds to time
>>>>> *
>>>>> * @param ns Time in nanoseconds
>>>>> *
>>>>> - * @return Time in CPU cycles
>>>>> + * @return Time stamp
>>>>> + */
>>>>> +odp_time_t odp_time_from_ns(uint64_t ns);
>>>>> +
>>>>> +/**
>>>>> + * Compare two times as absolute ranges
>>>>> + *
>>>>> + * @param t1 First time
>>>>> + * @param t2 Second time
>>>>> + *
>>>>> + * @retval -1 if t2 < t1, 0 if t1 = t2, 1 if t2 > t1
>>>>> + */
>>>>> +int odp_time_cmp(odp_time_t t1, odp_time_t t2);
>>>>> +
>>>>> +/**
>>>>> + * 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_ns_to_cycles(uint64_t ns);
>>>>> +uint64_t odp_time_to_u64(odp_time_t time);
>>>>>
>>>
>>
>> --
>> Regards,
>> Ivan Khoronzhuk
diff --git a/example/generator/odp_generator.c b/example/generator/odp_generator.c index be9597b..f84adc4 100644 --- a/example/generator/odp_generator.c +++ b/example/generator/odp_generator.c @@ -585,7 +585,7 @@ static void *gen_recv_thread(void *arg) */ static void print_global_stats(int num_workers) { - uint64_t start, wait, diff; + odp_time_t start, wait, diff; uint64_t pkts, pkts_prev = 0, pps, maximum_pps = 0; int verbose_interval = 20; odp_thrmask_t thrd_mask; @@ -593,8 +593,8 @@ static void print_global_stats(int num_workers) while (odp_thrmask_worker(&thrd_mask) < num_workers) continue; - wait = odp_time_ns_to_cycles(verbose_interval * ODP_TIME_SEC); - start = odp_time_cycles(); + wait = odp_time_from_ns(verbose_interval * ODP_TIME_SEC); + start = odp_time(); while (odp_thrmask_worker(&thrd_mask) == num_workers) { if (args->appl.number != -1 && @@ -603,11 +603,11 @@ static void print_global_stats(int num_workers) break; } - diff = odp_time_diff_cycles(start, odp_time_cycles()); - if (diff < wait) + diff = odp_time_diff(start, odp_time()); + if (odp_time_cmp(diff, wait) > 0) continue; - start = odp_time_cycles(); + start = odp_time(); if (args->appl.mode == APPL_MODE_RCV) { pkts = odp_atomic_load_u64(&counters.udp); diff --git a/include/odp/api/time.h b/include/odp/api/time.h index b0072fc..7ed4734 100644 --- a/include/odp/api/time.h +++ b/include/odp/api/time.h @@ -28,14 +28,25 @@ extern "C" { #define ODP_TIME_MSEC 1000000ULL /**< Millisecond in nsec */ #define ODP_TIME_SEC 1000000000ULL /**< Second in nsec */ +/** + * @typedef odp_time_t + * ODP time stamp. Time stamp can be local, so shouldn't be shared between + * threads. + */ /** - * Current time in CPU cycles - * - * @return Current time in CPU cycles + * @def ODP_TIME_NULL + * Zero time stamp */ -uint64_t odp_time_cycles(void); +/** + * Current time stamp. + * + * It should be hi-resolution time. + * + * @return Time stamp. + */ +odp_time_t odp_time(void); /** * Time difference @@ -43,29 +54,60 @@ uint64_t odp_time_cycles(void); * @param t1 First time stamp * @param t2 Second time stamp * - * @return Difference of time stamps in CPU cycles + * @return Difference of time stamps */ -uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2); +odp_time_t odp_time_diff(odp_time_t t1, odp_time_t t2); +/** + * Time sum + * + * @param t1 Time stamp + * @param t2 Time stamp + * + * @return Sum of time stamps + */ +odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); /** - * Convert CPU cycles to nanoseconds + * Convert time to nanoseconds * - * @param cycles Time in CPU cycles + * @param time Time * * @return Time in nanoseconds */ -uint64_t odp_time_cycles_to_ns(uint64_t cycles); - +uint64_t odp_time_to_ns(odp_time_t time); /** - * Convert nanoseconds to CPU cycles + * Convert nanoseconds to time * * @param ns Time in nanoseconds * - * @return Time in CPU cycles + * @return Time stamp + */ +odp_time_t odp_time_from_ns(uint64_t ns); + +/** + * Compare two times as absolute ranges + * + * @param t1 First time + * @param t2 Second time + * + * @retval -1 if t2 < t1, 0 if t1 = t2, 1 if t2 > t1 + */ +int odp_time_cmp(odp_time_t t1, odp_time_t t2); + +/** + * 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_ns_to_cycles(uint64_t ns); +uint64_t odp_time_to_u64(odp_time_t time); /** * @} diff --git a/test/performance/odp_pktio_perf.c b/test/performance/odp_pktio_perf.c index ae5b4c0..cf29802 100644 --- a/test/performance/odp_pktio_perf.c +++ b/test/performance/odp_pktio_perf.c @@ -106,7 +106,7 @@ struct tx_stats_s { uint64_t tx_cnt; /* Packets transmitted */ uint64_t alloc_failures;/* Packet allocation failures */ uint64_t enq_failures; /* Enqueue failures */ - uint64_t idle_cycles; /* Idle cycle count in TX loop */ + odp_time_t idle_cycles; /* Idle cycle count in TX loop */ }; typedef union tx_stats_u { @@ -305,12 +305,12 @@ static void *run_thread_tx(void *arg) int thr_id; odp_queue_t outq; pkt_tx_stats_t *stats; - uint64_t burst_start_cycles, start_cycles, cur_cycles, send_duration; - uint64_t burst_gap_cycles; + odp_time_t start_cycles, cur_cycles, send_duration; + odp_time_t burst_start_cycles, burst_gap_cycles; uint32_t batch_len; int unsent_pkts = 0; odp_event_t tx_event[BATCH_LEN_MAX]; - uint64_t idle_start = 0; + odp_time_t idle_start = ODP_TIME_NULL; thread_args_t *targs = arg; @@ -328,30 +328,33 @@ static void *run_thread_tx(void *arg) if (outq == ODP_QUEUE_INVALID) LOG_ABORT("Failed to get output queue for thread %d\n", thr_id); - burst_gap_cycles = odp_time_ns_to_cycles( + burst_gap_cycles = odp_time_from_ns( ODP_TIME_SEC / (targs->pps / targs->batch_len)); - send_duration = odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); + send_duration = odp_time_from_ns(targs->duration * ODP_TIME_SEC); odp_barrier_wait(&globals->tx_barrier); - cur_cycles = odp_time_cycles(); + cur_cycles = odp_time(); start_cycles = cur_cycles; - burst_start_cycles = odp_time_diff_cycles(burst_gap_cycles, cur_cycles); - while (odp_time_diff_cycles(start_cycles, cur_cycles) < send_duration) { + burst_start_cycles = odp_time_diff(burst_gap_cycles, cur_cycles); + while (odp_time_diff(start_cycles, cur_cycles) < send_duration) { unsigned alloc_cnt = 0, tx_cnt; - if (odp_time_diff_cycles(burst_start_cycles, cur_cycles) + if (odp_time_diff(burst_start_cycles, cur_cycles) < burst_gap_cycles) { - cur_cycles = odp_time_cycles(); - if (idle_start == 0) + cur_cycles = odp_time(); + if (!odp_time_cmp(ODP_TIME_NULL, idle_start)) idle_start = cur_cycles; continue; } - if (idle_start) { - stats->s.idle_cycles += odp_time_diff_cycles( - idle_start, cur_cycles); - idle_start = 0; + if (odp_time_cmp(ODP_TIME_NULL, idle_start)) { + odp_time_t diff = odp_time_diff(idle_start, cur_cycles); + + stats->s.idle_cycles = + odp_time_sum(diff, stats->s.idle_cycles); + + idle_start = ODP_TIME_NULL; } burst_start_cycles += burst_gap_cycles; @@ -365,14 +368,14 @@ static void *run_thread_tx(void *arg) stats->s.enq_failures += unsent_pkts; stats->s.tx_cnt += tx_cnt; - cur_cycles = odp_time_cycles(); + cur_cycles = odp_time(); } VPRINT(" %02d: TxPkts %-8"PRIu64" EnqFail %-6"PRIu64 " AllocFail %-6"PRIu64" Idle %"PRIu64"ms\n", thr_id, stats->s.tx_cnt, stats->s.enq_failures, stats->s.alloc_failures, - odp_time_cycles_to_ns(stats->s.idle_cycles)/1000/1000); + odp_time_to_ns(stats->s.idle_cycles) / (uint64_t)ODP_TIME_MSEC); return NULL; } @@ -591,13 +594,13 @@ static int setup_txrx_masks(odp_cpumask_t *thd_mask_tx, */ static void busy_loop_ns(uint64_t wait_ns) { - uint64_t diff; - uint64_t start_time = odp_time_cycles(); - uint64_t wait = odp_time_ns_to_cycles(wait_ns); + odp_time_t diff; + odp_time_t start_time = odp_time(); + odp_time_t wait = odp_time_from_ns(wait_ns); do { - diff = odp_time_diff_cycles(start_time, odp_time_cycles()); - } while (diff < wait); + diff = odp_time_diff(start_time, odp_time()); + } while (odp_time_cmp(diff, wait) > 0); } /* diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c index 26ff4cd..d12e6b2 100644 --- a/test/validation/pktio/pktio.c +++ b/test/validation/pktio/pktio.c @@ -332,18 +332,18 @@ static int destroy_inq(odp_pktio_t pktio) static odp_event_t queue_deq_wait_time(odp_queue_t queue, uint64_t ns) { - uint64_t start, now, diff; + odp_time_t start, now, diff; odp_event_t ev; - start = odp_time_cycles(); + start = odp_time(); do { ev = odp_queue_deq(queue); if (ev != ODP_EVENT_INVALID) return ev; - now = odp_time_cycles(); - diff = odp_time_diff_cycles(start, now); - } while (odp_time_cycles_to_ns(diff) < ns); + now = odp_time(); + diff = odp_time_diff(start, now); + } while (odp_time_to_ns(diff) < ns); return ODP_EVENT_INVALID; } @@ -351,12 +351,12 @@ static odp_event_t queue_deq_wait_time(odp_queue_t queue, uint64_t ns) static odp_packet_t wait_for_packet(odp_queue_t queue, uint32_t seq, uint64_t ns) { - uint64_t start, now, diff; + odp_time_t start, now, diff; odp_event_t ev; odp_packet_t pkt = ODP_PACKET_INVALID; uint64_t wait; - start = odp_time_cycles(); + start = odp_time(); wait = odp_schedule_wait_time(ns); do { @@ -377,9 +377,9 @@ static odp_packet_t wait_for_packet(odp_queue_t queue, odp_event_free(ev); } - now = odp_time_cycles(); - diff = odp_time_diff_cycles(start, now); - } while (odp_time_cycles_to_ns(diff) < ns); + now = odp_time(); + diff = odp_time_diff(start, now); + } while (odp_time_to_ns(diff) < ns); CU_FAIL("failed to receive transmitted packet"); diff --git a/test/validation/scheduler/scheduler.c b/test/validation/scheduler/scheduler.c index 0c96dc3..96e0781 100644 --- a/test/validation/scheduler/scheduler.c +++ b/test/validation/scheduler/scheduler.c @@ -465,10 +465,11 @@ static void *schedule_common_(void *arg) CU_ASSERT(from != ODP_QUEUE_INVALID); if (locked) { int cnt; - uint64_t cycles = 0; + odp_time_t cycles = ODP_TIME_NULL; /* Do some work here to keep the thread busy */ for (cnt = 0; cnt < 1000; cnt++) - cycles += odp_time_cycles(); + cycles = odp_time_sum(cycles, + odp_time()); odp_spinlock_unlock(&globals->atomic_lock); } diff --git a/test/validation/time/time.c b/test/validation/time/time.c index 4b81c2c..a81d7ff 100644 --- a/test/validation/time/time.c +++ b/test/validation/time/time.c @@ -16,43 +16,44 @@ void time_test_odp_cycles_diff(void) { /* volatile to stop optimization of busy loop */ volatile int count = 0; - uint64_t diff, cycles1, cycles2; + odp_time_t diff, cycles1, cycles2; - cycles1 = odp_time_cycles(); + cycles1 = odp_time(); while (count < BUSY_LOOP_CNT) { count++; }; - cycles2 = odp_time_cycles(); - CU_ASSERT(cycles2 > cycles1); + cycles2 = odp_time(); + CU_ASSERT((odp_time_cmp(cycles1, cycles2) > 0)); - diff = odp_time_diff_cycles(cycles1, cycles2); - CU_ASSERT(diff > 0); + diff = odp_time_diff(cycles1, cycles2); + CU_ASSERT(odp_time_cmp(ODP_TIME_NULL, diff) > 0); } /* check that a negative cycles difference gives a reasonable result */ void time_test_odp_cycles_negative_diff(void) { - uint64_t diff, cycles1, cycles2; + odp_time_t diff, cycles1, cycles2; cycles1 = 10; cycles2 = 5; - diff = odp_time_diff_cycles(cycles1, cycles2); - CU_ASSERT(diff > 0); + diff = odp_time_diff(cycles1, cycles2); + CU_ASSERT(odp_time_cmp(ODP_TIME_NULL, diff) > 0); } /* check that related conversions come back to the same value */ void time_test_odp_time_conversion(void) { - uint64_t ns1, ns2, cycles; + uint64_t ns1, ns2; + odp_time_t cycles; uint64_t upper_limit, lower_limit; ns1 = 100; - cycles = odp_time_ns_to_cycles(ns1); - CU_ASSERT(cycles > 0); + cycles = odp_time_from_ns(ns1); + CU_ASSERT(odp_time_cmp(ODP_TIME_NULL, cycles) > 0); - ns2 = odp_time_cycles_to_ns(cycles); + ns2 = odp_time_to_ns(cycles); /* need to check within arithmetic tolerance that the same * value in ns is returned after conversions */
Current time API supposes that frequency of counter is equal to CPU frequency. But that's not always true, for instance, in case if no access to CPU cycle counter, another hi-resolution timer can be used, and it`s rate can be different from CPU rate. There is no big difference in which cycles to measure time, the better hi-resolution timer the better measurements. So, unbind CPU cycle counter from time API by eliminating word "cycle" as it's believed to be used with CPU. Also add new opaque type for time odp_time_t, as it asks user to use API and abstracts time from units. New odp_time_t requires several additional API functions to be added: odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); int odp_time_cmp(odp_time_t t1, odp_time_t t2); uint64_t odp_time_to_u64(odp_time_t hdl); Also added new definition that represents 0 ticks for time - ODP_TIME_NULL. It can be used instead of odp_time_from_ns(0) for comparison and initialization. This patch changes only used time API, it doesn't change used var names for simplicity. This time API can be implemented with local timer counter, so shouldn't be used between threads. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- example/generator/odp_generator.c | 12 +++---- include/odp/api/time.h | 68 ++++++++++++++++++++++++++++------- test/performance/odp_pktio_perf.c | 49 +++++++++++++------------ test/validation/pktio/pktio.c | 20 +++++------ test/validation/scheduler/scheduler.c | 5 +-- test/validation/time/time.c | 27 +++++++------- 6 files changed, 114 insertions(+), 67 deletions(-)