| Message ID | 1473873362-15242-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
there is odp_time_diff(), forget about this patch. Maxim. On 09/14/16 20:16, Maxim Uvarov wrote: > Previously we had odp_time_sum but missing call to subtract > some value from odp_time_t. Because on different platforms it > can be implemented differently we need this call. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > include/odp/api/spec/time.h | 10 ++++++++++ > platform/linux-generic/odp_time.c | 17 +++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/include/odp/api/spec/time.h b/include/odp/api/spec/time.h > index fcc94c9..5e94b70 100644 > --- a/include/odp/api/spec/time.h > +++ b/include/odp/api/spec/time.h > @@ -87,6 +87,16 @@ odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1); > odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); > > /** > + * Time subtract > + * > + * @param t1 Time stamp to subtract from > + * @param t2 Value to subtract > + * > + * @return Time stamp t1 - t2 > + */ > +odp_time_t odp_time_sub(odp_time_t t1, odp_time_t t2); > + > +/** > * Convert time to nanoseconds > * > * @param time Time > diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c > index 81e0522..7fef46f 100644 > --- a/platform/linux-generic/odp_time.c > +++ b/platform/linux-generic/odp_time.c > @@ -81,6 +81,18 @@ static inline odp_time_t time_sum(odp_time_t t1, odp_time_t t2) > return time; > } > > +static inline odp_time_t time_sub(odp_time_t t1, odp_time_t t2) > +{ > + uint64_t ns; > + odp_time_t time; > + > + ns = time_to_ns(t1) - time_to_ns(t2); > + time.tv_sec = ns / ODP_TIME_SEC_IN_NS; > + time.tv_nsec = ns - time.tv_sec * ODP_TIME_SEC_IN_NS; > + > + return time; > +} > + > static inline odp_time_t time_local_from_ns(uint64_t ns) > { > odp_time_t time; > @@ -152,6 +164,11 @@ odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2) > return time_sum(t1, t2); > } > > +odp_time_t odp_time_sub(odp_time_t t1, odp_time_t t2) > +{ > + return time_sub(t1, t2); > +} > + > uint64_t odp_time_local_res(void) > { > return time_local_res();
On 09/15/16 15:18, Maxim Uvarov wrote: > there is odp_time_diff(), forget about this patch. > Sorry, odp_time_diff() is for difference. odp_time_sub() still needed for thing like calculate interval for execution. Please review api patch. Maxim. > Maxim. > > On 09/14/16 20:16, Maxim Uvarov wrote: >> Previously we had odp_time_sum but missing call to subtract >> some value from odp_time_t. Because on different platforms it >> can be implemented differently we need this call. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> --- >> include/odp/api/spec/time.h | 10 ++++++++++ >> platform/linux-generic/odp_time.c | 17 +++++++++++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/include/odp/api/spec/time.h b/include/odp/api/spec/time.h >> index fcc94c9..5e94b70 100644 >> --- a/include/odp/api/spec/time.h >> +++ b/include/odp/api/spec/time.h >> @@ -87,6 +87,16 @@ odp_time_t odp_time_diff(odp_time_t t2, odp_time_t >> t1); >> odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); >> /** >> + * Time subtract >> + * >> + * @param t1 Time stamp to subtract from >> + * @param t2 Value to subtract >> + * >> + * @return Time stamp t1 - t2 >> + */ >> +odp_time_t odp_time_sub(odp_time_t t1, odp_time_t t2); >> + >> +/** >> * Convert time to nanoseconds >> * >> * @param time Time >> diff --git a/platform/linux-generic/odp_time.c >> b/platform/linux-generic/odp_time.c >> index 81e0522..7fef46f 100644 >> --- a/platform/linux-generic/odp_time.c >> +++ b/platform/linux-generic/odp_time.c >> @@ -81,6 +81,18 @@ static inline odp_time_t time_sum(odp_time_t t1, >> odp_time_t t2) >> return time; >> } >> +static inline odp_time_t time_sub(odp_time_t t1, odp_time_t t2) >> +{ >> + uint64_t ns; >> + odp_time_t time; >> + >> + ns = time_to_ns(t1) - time_to_ns(t2); >> + time.tv_sec = ns / ODP_TIME_SEC_IN_NS; >> + time.tv_nsec = ns - time.tv_sec * ODP_TIME_SEC_IN_NS; >> + >> + return time; >> +} >> + >> static inline odp_time_t time_local_from_ns(uint64_t ns) >> { >> odp_time_t time; >> @@ -152,6 +164,11 @@ odp_time_t odp_time_sum(odp_time_t t1, >> odp_time_t t2) >> return time_sum(t1, t2); >> } >> +odp_time_t odp_time_sub(odp_time_t t1, odp_time_t t2) >> +{ >> + return time_sub(t1, t2); >> +} >> + >> uint64_t odp_time_local_res(void) >> { >> return time_local_res(); >
On 09/16/16 09:41, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Maxim >> Uvarov >> Sent: Thursday, September 15, 2016 7:07 PM >> To: lng-odp-forward <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [API-NEXT PATCHv2] api: odp_time_sub >> >> On 09/15/16 15:18, Maxim Uvarov wrote: >>> there is odp_time_diff(), forget about this patch. >>> >> Sorry, odp_time_diff() is for difference. odp_time_sub() still needed >> for thing like calculate interval for execution. >> >> Please review api patch. >> >> Maxim. >> > Could you give an example. E.g. this kind of interval calculation does not need substract: > > t1 = time(); > t2 = t1 + wait_time; > > while (time() < t2) > spin(); > > // now it's time to continue > foo(); > > > -Petri Looks like I lost again. "diff" word is confusing for me: /** * Time difference * * @param t2 Second time stamp * @param t1 First time stamp * * @return Difference of time stamps */ odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1); In fact this function (at least in linux-generic) subtracts t1 from t2. Assuming that t2 > t1. I'm going to send proposal api patch to implement function: int odp_schedule_stats(odp_schedule_stats_t *stats); which will return times spend in and out scheduler. Later extend it with more counters like context switches and etc. t1 = odp_time_local(); ev = sched_api->schedule(from, wait); t2 = odp_time_local(); Then time in schedule will be: odp_time_t sched_time = odp_time_diff(t2, t1) So I think everything ok but I would refine doxygen to be more clear what is odp_time_diff() for and probably rename it to odp_time_sub() to be more symmetrical to odp_time_sum(). Maxim.
On 09/16/16 12:51, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >> Sent: Friday, September 16, 2016 10:05 AM >> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- >> labs.com>; lng-odp-forward <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [API-NEXT PATCHv2] api: odp_time_sub >> >> On 09/16/16 09:41, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>> -----Original Message----- >>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> Maxim >>>> Uvarov >>>> Sent: Thursday, September 15, 2016 7:07 PM >>>> To: lng-odp-forward <lng-odp@lists.linaro.org> >>>> Subject: Re: [lng-odp] [API-NEXT PATCHv2] api: odp_time_sub >>>> >>>> On 09/15/16 15:18, Maxim Uvarov wrote: >>>>> there is odp_time_diff(), forget about this patch. >>>>> >>>> Sorry, odp_time_diff() is for difference. odp_time_sub() still needed >>>> for thing like calculate interval for execution. >>>> >>>> Please review api patch. >>>> >>>> Maxim. >>>> >>> Could you give an example. E.g. this kind of interval calculation does >> not need substract: >>> t1 = time(); >>> t2 = t1 + wait_time; >>> >>> while (time() < t2) >>> spin(); >>> >>> // now it's time to continue >>> foo(); >>> >>> >>> -Petri >> Looks like I lost again. "diff" word is confusing for me: >> /** >> * Time difference >> * >> * @param t2 Second time stamp >> * @param t1 First time stamp >> * >> * @return Difference of time stamps >> */ >> odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1); >> >> >> In fact this function (at least in linux-generic) subtracts t1 from t2. >> Assuming that t2 > t1. >> >> I'm going to send proposal api patch to implement function: >> int odp_schedule_stats(odp_schedule_stats_t *stats); >> which will return times spend in and out scheduler. Later extend it >> with more counters like context switches and etc. >> >> t1 = odp_time_local(); >> ev = sched_api->schedule(from, wait); >> t2 = odp_time_local(); >> >> Then time in schedule will be: >> odp_time_t sched_time = odp_time_diff(t2, t1) >> >> So I think everything ok but I would refine doxygen to be more clear >> what is odp_time_diff() for >> and probably rename it to odp_time_sub() to be more symmetrical to >> odp_time_sum(). >> >> Maxim. >> >> > Diff is commonly used for time difference where only t2 - t1 is allowed (not t1 - t2). Sub is more generic term and would indicate that also t1 - t2 is allowed, which would produce a *negative* result. We'd not want to support negative time. Also odp_time_diff is similar to e.g. POSIX difftime( time_t time_end, time_t time_beg ) ok. > About scheduler statistics. Features that are commonly HW accelerated fit into the API, while those that are not commonly supported should not be added. When e.g. scheduler execution time statistics is not commonly calculated by the HW, it's better to leave it to the (interested) application. Additional statistics add implementation complexity and decrease performance. Time read is bad for performance since it requires synchronization (system call, read a SoC global register, etc) and cannot be reordered. > > Application can easily do: > > t1 = odp_time_local(); > ev = odp_schedule(from, wait); > t2 = odp_time_local(); > > there's no room for implementation to do it any faster. > > > -Petri > Yes, anyway there will be slow down. I'm looking to card ODP-332 <https://projects.linaro.org/browse/ODP-332>. Time spend for app is odp_time_local() - sched_time. And sched time calculated as code above. Time spend can be calculated with cpu cycles or performance counters on cpu. But any accounting will take time for such critical function. And probably there will be no common values which we can get from hardware or from OS. Maybe it's better to close that card. Maxim.
diff --git a/include/odp/api/spec/time.h b/include/odp/api/spec/time.h index fcc94c9..5e94b70 100644 --- a/include/odp/api/spec/time.h +++ b/include/odp/api/spec/time.h @@ -87,6 +87,16 @@ odp_time_t odp_time_diff(odp_time_t t2, odp_time_t t1); odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2); /** + * Time subtract + * + * @param t1 Time stamp to subtract from + * @param t2 Value to subtract + * + * @return Time stamp t1 - t2 + */ +odp_time_t odp_time_sub(odp_time_t t1, odp_time_t t2); + +/** * Convert time to nanoseconds * * @param time Time diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c index 81e0522..7fef46f 100644 --- a/platform/linux-generic/odp_time.c +++ b/platform/linux-generic/odp_time.c @@ -81,6 +81,18 @@ static inline odp_time_t time_sum(odp_time_t t1, odp_time_t t2) return time; } +static inline odp_time_t time_sub(odp_time_t t1, odp_time_t t2) +{ + uint64_t ns; + odp_time_t time; + + ns = time_to_ns(t1) - time_to_ns(t2); + time.tv_sec = ns / ODP_TIME_SEC_IN_NS; + time.tv_nsec = ns - time.tv_sec * ODP_TIME_SEC_IN_NS; + + return time; +} + static inline odp_time_t time_local_from_ns(uint64_t ns) { odp_time_t time; @@ -152,6 +164,11 @@ odp_time_t odp_time_sum(odp_time_t t1, odp_time_t t2) return time_sum(t1, t2); } +odp_time_t odp_time_sub(odp_time_t t1, odp_time_t t2) +{ + return time_sub(t1, t2); +} + uint64_t odp_time_local_res(void) { return time_local_res();
Previously we had odp_time_sum but missing call to subtract some value from odp_time_t. Because on different platforms it can be implemented differently we need this call. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- include/odp/api/spec/time.h | 10 ++++++++++ platform/linux-generic/odp_time.c | 17 +++++++++++++++++ 2 files changed, 27 insertions(+) -- 2.7.1.250.gff4ea60