On 22 May 2015 at 06:57, Bala Manoharan <[email protected]> wrote:
> On 21 May 2015 at 16:44, Ola Liljedahl <[email protected]> wrote: > > 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. > > Hi Ola, > > Thanks for the info. I am summarising the discussions yesterday below > just to clarify any discrepancy. > > * There will be at the maximum only one odp_event_t associated with a > odp_timer_t at any point of time. > > * odp_event_t variable(tmo_ev) in odp_timer_set_abs() call cannot be > NULL when a timer is in inactive state. > ODP_EVENT_INVALID (which may be defined as NULL). > > * when odp_timer_set_abs() call is called with odp_event_t variable as > NULL for an existing active timer (ie a timer in which the event has > not yet been dispatched ) then the existing event associated with the > timer will be moved to the new tick location. > > * Let the current tick in the system be at position "0" and a > odp_timer_t T1is active and armed at tick location 100 with > odp_event_t value "X" , > If odp_timer_set_abs() call is executed on timer T1 with odp_event_t > "Y" to be armed at tick 150, > then the odp_event_t "X" will be cancelled and returned to the > application and the new odp_event_t "Y" will be armed at tick location > 150. > We agree. Regards, Bala Regards, Bala > >> >> >>> >>> 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
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
