On 21 May 2015 at 16:44, Ola Liljedahl <ola.liljed...@linaro.org> wrote:
> On 21 May 2015 at 09:53, Jerin Jacob <jerin.ja...@caviumnetworks.com> wrote:
>> On Wed, May 20, 2015 at 05:28:24PM +0200, Ola Liljedahl wrote:
>>> On 20 May 2015 at 16:16, Jerin Jacob <jerin.ja...@caviumnetworks.com> wrote:
>>> > On Wed, May 20, 2015 at 12:42:29PM +0200, Ola Liljedahl wrote:
>>> >> On 20 May 2015 at 06:56, Jerin Jacob <jerin.ja...@caviumnetworks.com>
>>> >> wrote:
>>> >> > On Wed, May 20, 2015 at 12:25:12AM +0200, Ola Liljedahl wrote:
>>> >> >> On 19 May 2015 at 15:34, Jacob, Jerin
>>> >> >> <jerin.ja...@caviumnetworks.com> 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.
* 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.
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
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
