On 21 May 2015 at 09:53, Jerin Jacob <[email protected]> wrote: > On Wed, May 20, 2015 at 05:28:24PM +0200, Ola Liljedahl wrote: >> On 20 May 2015 at 16:16, Jerin Jacob <[email protected]> wrote: >> > On Wed, May 20, 2015 at 12:42:29PM +0200, Ola Liljedahl wrote: >> >> On 20 May 2015 at 06:56, Jerin Jacob <[email protected]> >> >> wrote: >> >> > On Wed, May 20, 2015 at 12:25:12AM +0200, Ola Liljedahl wrote: >> >> >> On 19 May 2015 at 15:34, Jacob, Jerin >> >> >> <[email protected]> wrote: >> >> >> > Ola, >> >> >> > >> >> >> > Is there any specific reason for following check in timer validation >> >> >> > test ? >> >> >> pa >> >> >> > >> >> >> > diff --git a/test/validation/odp_timer.c >> >> >> > b/test/validation/odp_timer.c >> >> >> > index 554b353..724026e 100644 >> >> >> > --- a/test/validation/odp_timer.c >> >> >> > +++ b/test/validation/odp_timer.c >> >> >> > @@ -260,7 +260,7 @@ static void handle_tmo(odp_event_t ev, bool >> >> >> > stale, uint64_t prev_tick) >> >> >> > >> >> >> > if (ttp != NULL) { >> >> >> > /* Internal error */ >> >> >> > - CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID); >> >> >> > +----------> CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID); >> >> >> > ttp->ev = ev; >> >> >> > } >> >> >> > } >> >> >> > >> >> >> > AFAIU, I should be CU_ASSERT_FATAL(ttp->ev != ODP_EVENT_INVALID) as >> >> >> > tt[i].ev = odp_timeout_to_event(odp_timeout_alloc(tbp)) specified >> >> >> > while preparing all timers. >> >> >> Yes the timers are still inactive and the timeout event is stored in >> >> >> the 'ev' member. >> >> >> >> >> >> handle_timeout() is called for received timeouts (timer has expired). >> >> >> In that case, the corresponding 'ev' member should not contain any >> >> >> timeout event. >> >> >> >> >> >> > >> >> >> > Am I missing something in the timer specification ? >> >> >> Or the timer specification is missing something? >> >> >> >> >> >> odp_timer_set_abs(tt[i].tim, tck, &tt[i].ev); (line 309) is supposed >> >> >> to grab the timeout event (on success) and clear the variable (write >> >> >> ODP_TIMEOUT_INVALID), that's why the timeout is passed by reference >> >> >> ("&tt[i].ev"). >> >> >> >> >> >> Possibly this is not specified clearly enough in timer.h: >> >> >> * @param[in,out] tmo_ev Reference to an event variable that points to >> >> >> * timeout event or NULL to reuse the existing timeout event. Any >> >> >> existing >> >> >> * timeout event that is replaced by a successful set operation will be >> >> >> * returned here. >> >> >> >> >> >> The new timeout event is read from *tmo_ev. The old timeout event (if >> >> >> timer was active) or ODP_TIMEOUT_INVALID (if timer was inactive) is >> >> >> stored in *tmo_ev. I hope this is at least clear in the reference >> >> >> implementation. >> >> > >> >> > We are on same page, except the last notes >> >> > IMO, linux generic timer implementation details leaked into creating >> >> > the test case. >> >> Well I don't agree and I hope I can convince you. >> >> >> >> > >> >> > AFAIU, *tmo_ev should have the event that used for _arming_ the timer so >> >> > that application can do some look up after receiving event through >> >> > queue or something similar.. >> >> > What is the point of providing "ODP_TIMEOUT_INVALID" to application >> >> > back, What the >> >> > use of it for the application. >> >> It is possible to set an already active timer (which then is already >> >> associated with a timeout). If the user specifies a new timeout, the >> >> old timeout must be returned to the user (because all alloc and free >> >> of timeouts is the responsibility of the user). So any old timeout >> >> (for an already active timer) is return in "*tmo_ev". But it is >> >> possible that the timer has already expired (and the timeout been >> >> delivered) or wasn't active to start with. We want the application to >> >> be able to differ between these two scenarios and we achieve this by >> >> updating "*tmo_ev" accordingly. When the timer_set call return, if >> >> *tmo_ev != ODP_EVENT_INVALID, an timeout has been returned and the >> >> application needs to do something with it. If *tno_ev == >> >> ODP_EVENT_INVALID, no timeout was returned. >> > >> > >> > Just to understand the usecase, What application is gonna do with returned >> > *tmp_ev >> > if timer is active and it returned the associated timeout ? >> Either the application specified a new timeout in the timer_set call >> and it is that timeout which will be delivered upon timer expiration. >> If a timeout is returned (the old timeout for an already active >> timer), the application should free it or re-use it. >> >> > it can't free as it will be cause double free when it comes back in >> > app mainloop(it will have odp_timeout_free() there). >> If a timeout is returned in *tmo_ev then it is not the same timeout. >> Old vs. new. >> >> > >> > and application can't use the "returned associated timeout" for long time >> > what if it event is delivered and free'ed it in the main loop. >> > Typical main loop application >> > processing will be check for event type, process it and free the resources >> > >> > Is this scheme is replacement for the API like odp_timer_active() to find >> > the timer active or not ? >> > >> > I thought the reason for returning "*tmo_ev" in timer set operation >> > is that, if application is lazy(ie don't want to manage) to create the >> > timeout event then >> > it can ask for the implementation with 'NULL' so that implementation >> > can get a odp timer event from the implementation and if application >> > want explicit control on any event(say packet event type) then it can set >> > through explicit event on timer set call :-) >> No, the application is always responsible for allocating and freeing >> timeouts. This is how it eventually became even if it wasn't so in >> earlier (never merged) proposals. > > OK. I was thinking inline with "old" proposal where implementation creates the > event in case of "*tmo_ev" == ODP_TIMEOUT_INVALID and that created the > complications > on responsibility of freeing the resources(app/implementation).Now is easy > for the implementation :-) > > Now its looks logical to me as when implementation gets "*tmo_ev" == > ODP_TIMEOUT_INVALID > request then I can cancel the existing event delivery and re-arm the same > event for future > time. > I am glad that we agree now and that the semantics will be simple for you to implement.
I will post a patch that updates the descriptions in timer.h. Me and Bala had a chat about this. > > >> >> Appl calls odp_timer_set_abs() with a new timeout event in *tmo_ev. If >> the timer was already active (and thus associated with a timeout >> event), the old timeout is returned in *tmo_ev, otherwise >> ODP_TIMEOUT_INVALID is stored in *tmo_ev (so the value of *tmo_ev is >> always valid when the call returns). So *tmo_ev is both input (new >> timeout) and output (old timeout). >> >> Appl can also call odp_timer_set_abs() with >> *tmo_ev=ODP_TIMEOUT_INVALID, resetting the timer with a new timeout >> time but reusing any existing timeout event. I expect this to be the >> normal usage, just kick the timer into the future for every sent or >> received packet. If the timer isn't active (and thus is not associated >> with any timeout event), the operation fails and the call returns >> ODP_TIMER_NOEVENT. >> >> The timeout event can be of any type, not necessarily ODP_EVENT_TIMEOUT. >> >> > >> >> >> >> I hope you can agree with this line of thinking. >> >> >> >> > >> >> > IMO, two way we can come to a conclusion for this issue. >> >> > 1) Remove CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID) in handle_tmo >> >> > function in unit testcase >> >> > 2) Or some reason, If application wants ODP_TIMEOUT_INVALID(for >> >> > inactive case) in *tmo_ev >> >> > lets update the specification so that it will clear for odp implementer >> >> Yes the spec needs to be clearer. >> >> >> >> > >> >> > any thought from application perspective ? >> >> > >> >> > >> >> >> >> >> >> > >> >> >> > Thanks, >> >> >> > Jerin. >> >> >> > _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
