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:[email protected]] > Sent: Tuesday, October 27, 2015 2:22 PM > To: Savolainen, Petri (Nokia - FI/Espoo); [email protected] > 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:[email protected]] > >>> Sent: Thursday, October 22, 2015 3:02 PM > >>> To: [email protected]; 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 <[email protected]> > >>> --- > >>> 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 _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
