On 12 March 2015 at 12:21, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> Ola, do you want to add your review-by to these patches? I think the patch should be reposted (by someone) with the user_ptr test enabled and the spelling error corrected. I could do this but then someone else needs to review it... -- Ola > > > Maxim. > > > On 03/12/15 13:26, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> Ping. I think it's time to merge these two patches. This sets the stage >> for testing odp_timeout_xxx() after timer cancel. >> >> -Petri >> >> -----Original Message----- >>> From: ext Ola Liljedahl [mailto:ola.liljed...@linaro.org] >>> Sent: Tuesday, March 10, 2015 4:11 PM >>> To: Savolainen, Petri (Nokia - FI/Espoo) >>> Cc: LNG ODP Mailman List >>> Subject: Re: [lng-odp] [PATCH 2/2] validation: timer: added cancel test >>> >>> On 10 March 2015 at 10:10, Savolainen, Petri (Nokia - FI/Espoo) >>> <petri.savolai...@nokia.com> wrote: >>> >>>> >>>> -----Original Message----- >>>>> From: ext Ola Liljedahl [mailto:ola.liljed...@linaro.org] >>>>> Sent: Monday, March 09, 2015 4:44 PM >>>>> To: Savolainen, Petri (Nokia - FI/Espoo) >>>>> Cc: LNG ODP Mailman List >>>>> Subject: Re: [lng-odp] [PATCH 2/2] validation: timer: added cancel test >>>>> >>>>> On 6 March 2015 at 15:30, Petri Savolainen <petri.savolai...@nokia.com >>>>> > >>>>> wrote: >>>>> >>>>>> Added timeout cancel test that simply creates everything >>>>>> needed for requensting >>>>>> >>>>> Spelling error. >>>>> >>>> Maybe Maxim can correct this during merge. >>>> >>>> one timeout and then cancels that >>>>>> before it expires. >>>>>> >>>>>> The timeout user_ptr bug can be reproduced with this by >>>>>> uncommenting the user_ptr check. >>>>>> >>>>>> Signed-off-by: Petri Savolainen <petri.savolai...@nokia.com> >>>>>> --- >>>>>> test/validation/odp_timer.c | 83 >>>>>> >>>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>>> >>>>>> 1 file changed, 83 insertions(+) >>>>>> >>>>>> diff --git a/test/validation/odp_timer.c >>>>>> >>>>> b/test/validation/odp_timer.c >>> >>>> index 2ef3b39..1def85b 100644 >>>>>> --- a/test/validation/odp_timer.c >>>>>> +++ b/test/validation/odp_timer.c >>>>>> @@ -124,6 +124,88 @@ static void test_timeout_pool_free(void) >>>>>> CU_ASSERT(odp_pool_destroy(pool) == 0); >>>>>> } >>>>>> >>>>>> +static void test_odp_timer_cancel(void) >>>>>> +{ >>>>>> + odp_pool_t pool; >>>>>> + odp_pool_param_t params; >>>>>> + odp_timer_pool_param_t tparam; >>>>>> + odp_timer_pool_t tp; >>>>>> + odp_queue_t queue; >>>>>> + odp_timer_t tim; >>>>>> + odp_event_t ev; >>>>>> + odp_timeout_t tmo; >>>>>> + odp_timer_set_t rc; >>>>>> + uint64_t tick; >>>>>> + >>>>>> + params.tmo.num = 1; >>>>>> + params.type = ODP_POOL_TIMEOUT; >>>>>> + pool = odp_pool_create("tmo_pool_for_cancel", ODP_SHM_NULL, >>>>>> >>>>> ¶ms); >>>>> >>>>>> + >>>>>> + if (pool == ODP_POOL_INVALID) >>>>>> + CU_FAIL_FATAL("Timeout pool create failed"); >>>>>> + >>>>>> + tparam.res_ns = 100 * ODP_TIME_MSEC; >>>>>> + tparam.min_tmo = 1 * ODP_TIME_SEC; >>>>>> + tparam.max_tmo = 10 * ODP_TIME_SEC; >>>>>> + tparam.num_timers = 1; >>>>>> + tparam.priv = 0; >>>>>> + tparam.clk_src = ODP_CLOCK_CPU; >>>>>> + tp = odp_timer_pool_create("timer_pool0", &tparam); >>>>>> + if (tp == ODP_TIMER_POOL_INVALID) >>>>>> + CU_FAIL_FATAL("Timer pool create failed"); >>>>>> + >>>>>> + /* Start all created timer pools */ >>>>>> + odp_timer_pool_start(); >>>>>> + >>>>>> + queue = odp_queue_create("timer_queue", ODP_QUEUE_TYPE_POLL, >>>>>> >>>>> NULL); >>>>> >>>>>> + if (queue == ODP_QUEUE_INVALID) >>>>>> + CU_FAIL_FATAL("Queue create failed"); >>>>>> + >>>>>> + #define USER_PTR ((void *)0xdead) >>>>>> + tim = odp_timer_alloc(tp, queue, USER_PTR); >>>>>> + if (tim == ODP_TIMER_INVALID) >>>>>> + CU_FAIL_FATAL("Failed to allocate timer"); >>>>>> + >>>>>> + ev = odp_timeout_to_event(odp_timeout_alloc(pool)); >>>>>> + if (ev == ODP_EVENT_INVALID) >>>>>> + CU_FAIL_FATAL("Failed to allocate timeout"); >>>>>> + >>>>>> + tick = odp_timer_ns_to_tick(tp, 2 * ODP_TIME_SEC); >>>>>> + >>>>>> + rc = odp_timer_set_rel(tim, tick, &ev); >>>>>> + if (rc != ODP_TIMER_SUCCESS) >>>>>> + CU_FAIL_FATAL("Failed to set timer (relative time)"); >>>>>> + >>>>>> + ev = ODP_EVENT_INVALID; >>>>>> + if (odp_timer_cancel(tim, &ev) != 0) >>>>>> + CU_FAIL_FATAL("Failed to cancel timer (relative >>>>>> >>>>> time)"); >>> >>>> + >>>>>> + if (ev == ODP_EVENT_INVALID) >>>>>> + CU_FAIL_FATAL("Cancel did not return event"); >>>>>> + >>>>>> + tmo = odp_timeout_from_event(ev); >>>>>> + if (tmo == ODP_TIMEOUT_INVALID) >>>>>> + CU_FAIL_FATAL("Cancel returned invalid tmo"); >>>>>> >>>>> No need for fatal error? CU_FAIL should be enough. >>>>> >>>> It's fatal for this test if something else than tmo is coming out from >>>> >>> the cancel. >>> True. >>> >>> >>>> + >>>>>> +/* Uncomment after user_ptr bug have been corrected >>>>>> + if (odp_timeout_user_ptr(tmo) != USER_PTR) >>>>>> + CU_FAIL_FATAL("Cancel corrupted user_ptr"); >>>>>> >>>>> CU_FAIL again. >>>>> >>>> True. Test can continue to next timeout metadata tests, but it in >>>> >>> comment now and need to updated (uncommented) any way after >>> implementation >>> fix. >>> >>>> Tests for odp_timeout_timer() and odp_timeout_tick()? >>>>> >>>> Yes. Those tests should be added after implementation is fixed. >>>> >>> Actually I think it is a bad idea to check that the timeout expiration >>> tick is always the latest set tick. That would require the >>> implementation to update this field *every* time a timer is reset, >>> forcing an access to a data structure that otherwise would not be >>> accessed. The expiration tick is set when the timer expires and the >>> timeout is enqueued. >>> >>> The timeout timer and user_ptr fields should be updated, this only has >>> to be done once, when a timeout event is specified in one of the >>> odp_timer_set_xxx calls. >>> >>> The expiration should not be of the same importance as e.g. the user >>> pointer. >>> >>> I suggest to merge this patch as is now, fix the implementation and then >>>> >>> fix/add these tests. >>> >>>> -Petri >>>> >>>> +*/ >>>>>> + odp_timeout_free(tmo); >>>>>> + >>>>>> + ev = odp_timer_free(tim); >>>>>> + if (ev != ODP_EVENT_INVALID) >>>>>> + CU_FAIL_FATAL("Free returned event"); >>>>>> + >>>>>> + odp_timer_pool_destroy(tp); >>>>>> + >>>>>> + if (odp_queue_destroy(queue) != 0) >>>>>> + CU_FAIL_FATAL("Failed to destroy queue"); >>>>>> + >>>>>> + if (odp_pool_destroy(pool) != 0) >>>>>> + CU_FAIL_FATAL("Failed to destroy pool"); >>>>>> +} >>>>>> + >>>>>> /* @private Handle a received (timeout) event */ >>>>>> static void handle_tmo(odp_event_t ev, bool stale, uint64_t >>>>>> >>>>> prev_tick) >>> >>>> { >>>>>> @@ -432,6 +514,7 @@ static void test_odp_timer_all(void) >>>>>> CU_TestInfo test_odp_timer[] = { >>>>>> {"test_timeout_pool_alloc", test_timeout_pool_alloc}, >>>>>> {"test_timeout_pool_free", test_timeout_pool_free}, >>>>>> + {"test_odp_timer_cancel", test_odp_timer_cancel}, >>>>>> {"test_odp_timer_all", test_odp_timer_all}, >>>>>> CU_TEST_INFO_NULL, >>>>>> }; >>>>>> -- >>>>>> 2.3.1 >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> lng-odp mailing list >>>>>> lng-odp@lists.linaro.org >>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>>> >>>>> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org http://lists.linaro.org/mailman/listinfo/lng-odp
