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.
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
