You have the patches, what are you waiting for? On 20 January 2015 at 17:45, Mike Holmes <[email protected]> wrote: > This is a blocker since it breaks CI, we need an update the test to not fail > on discovering that timeouts were late or disable the timer tests with XFAIL > > On 16 January 2015 at 08:49, Mike Holmes <[email protected]> wrote: >> >> >> >> On 13 January 2015 at 18:27, Anders Roxell <[email protected]> >> wrote: >>> >>> On 13 January 2015 at 20:15, Mike Holmes <[email protected]> wrote: >>> > >>> > >>> > On 12 January 2015 at 16:20, Ola Liljedahl <[email protected]> >>> > wrote: >>> >> >>> >> Don't report too late timeouts using CU_FAIL as this interferes with >>> >> the >>> >> cunit >>> >> test framework. Just count and report (using LOG_DBG) the number of >>> >> late >>> >> timeouts. >>> >> Use CU_ASSERT_FATAL instead of plain assert so to work wither with the >>> >> cunit >>> >> test framework. >>> >> Don't dereference pointer after successful check for NULL as this >>> >> makes >>> >> Coverity >>> >> complain. >>> >> Use LOG_DBG instead of printf. Remove some unnecessary printouts. >>> >> Use nanosleep instead of the deprecated usleep. >>> >> >>> >> Signed-off-by: Ola Liljedahl <[email protected]> >>> > >>> > >>> > Reviewed-and-tested-by: Mike Holmes <[email protected]> >>> >>> May I suggest splitting this as a patch set? >>> one patch for ODP_DBG, one for nanosleep, CU_ASSERT_FAIL to name a few. >>> >>> Cheers, >>> Anders >>> >> >> I think that although what is considered the correct level of division for >> a patch is debatable, following the rule - "fix one thing per patch" bug >> fixes should always be their own patch and that is an easily identified and >> enforced division. >> Even if issues did not make it to bugzilla and have a bug ID, the patch >> description often tells the story as in this case >> >> One bug - regression framework >> Don't report too late timeouts using CU_FAIL as this interferes with the >> cunit >> test framework. Just count and report (using LOG_DBG) the number of late >> timeouts. If it had one the BZ ID should be here >> >> Two bug - static analysis >> Don't dereference pointer after successful check for NULL as this makes >> Coverity >> complain. - should reference the COV: ID for this issue >> >> Three - clean up to test as per review comments >> Use CU_ASSERT_FATAL instead of plain assert so to work wither with the >> cunit >> test framework. >> Use LOG_DBG instead of printf. Remove some unnecessary printouts. >> Use nanosleep instead of the deprecated usleep. >> >> >> >>> > >>> >> >>> >>> >> --- >>> >> (This document/code contribution attached is provided under the terms >>> >> of >>> >> agreement LES-LTM-21309) >>> >> >>> >> test/validation/odp_timer.c | 102 >>> >> +++++++++++++++++++++++++------------------- >>> >> 1 file changed, 59 insertions(+), 43 deletions(-) >>> >> >>> >> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c >>> >> index 4c93f71..d893646 100644 >>> >> --- a/test/validation/odp_timer.c >>> >> +++ b/test/validation/odp_timer.c >>> >> @@ -8,10 +8,13 @@ >>> >> * @file >>> >> */ >>> >> >>> >> -#include <assert.h> >>> >> +/* For rand_r and nanosleep */ >>> >> +#define _POSIX_C_SOURCE 200112L >>> >> +#include <time.h> >>> >> #include <unistd.h> >>> >> #include <odp.h> >>> >> #include "odp_cunit_common.h" >>> >> +#include "test_debug.h" >>> >> >>> >> /** @private Timeout range in milliseconds (ms) */ >>> >> #define RANGE_MS 2000 >>> >> @@ -28,6 +31,9 @@ static odp_buffer_pool_t tbp; >>> >> /** @private Timer pool handle used by all threads */ >>> >> static odp_timer_pool_t tp; >>> >> >>> >> +/** @private Count of timeouts delivered too late */ >>> >> +static odp_atomic_u32_t ndelivtoolate; >>> >> + >>> >> /** @private min() function */ >>> >> static int min(int a, int b) >>> >> { >>> >> @@ -47,8 +53,7 @@ struct test_timer { >>> >> /* @private Handle a received (timeout) buffer */ >>> >> static void handle_tmo(odp_buffer_t buf, bool stale, uint64_t >>> >> prev_tick) >>> >> { >>> >> - /* Use assert() for internal correctness checks of test >>> >> program */ >>> >> - assert(buf != ODP_BUFFER_INVALID); >>> >> + CU_ASSERT_FATAL(buf != ODP_BUFFER_INVALID); /* Internal error >>> >> */ >>> >> if (odp_buffer_type(buf) != ODP_BUFFER_TYPE_TIMEOUT) { >>> >> /* Not a timeout buffer */ >>> >> CU_FAIL("Unexpected buffer type received"); >>> >> @@ -65,38 +70,41 @@ static void handle_tmo(odp_buffer_t buf, bool >>> >> stale, >>> >> uint64_t prev_tick) >>> >> if (ttp == NULL) >>> >> CU_FAIL("odp_timeout_user_ptr() null user ptr"); >>> >> >>> >> - if (ttp->buf2 != buf) >>> >> + if (ttp != NULL && ttp->buf2 != buf) >>> >> CU_FAIL("odp_timeout_user_ptr() wrong user ptr"); >>> >> - if (ttp->tim != tim) >>> >> + if (ttp != NULL && ttp->tim != tim) >>> >> CU_FAIL("odp_timeout_timer() wrong timer"); >>> >> if (stale) { >>> >> if (odp_timeout_fresh(tmo)) >>> >> CU_FAIL("Wrong status (fresh) for stale >>> >> timeout"); >>> >> /* Stale timeout => local timer must have invalid tick >>> >> */ >>> >> - if (ttp->tick != TICK_INVALID) >>> >> + if (ttp != NULL && ttp->tick != TICK_INVALID) >>> >> CU_FAIL("Stale timeout for active timer"); >>> >> } else { >>> >> if (!odp_timeout_fresh(tmo)) >>> >> CU_FAIL("Wrong status (stale) for fresh >>> >> timeout"); >>> >> /* Fresh timeout => local timer must have matching >>> >> tick */ >>> >> - if (ttp->tick != tick) { >>> >> - printf("Wrong tick: expected %"PRIu64" actual >>> >> %"PRIu64"\n", >>> >> - ttp->tick, tick); >>> >> + if (ttp != NULL && ttp->tick != tick) { >>> >> + LOG_DBG("Wrong tick: expected %"PRIu64" actual >>> >> %"PRIu64"\n", >>> >> + ttp->tick, tick); >>> >> CU_FAIL("odp_timeout_tick() wrong tick"); >>> >> } >>> >> /* Check that timeout was delivered 'timely' */ >>> >> if (tick > odp_timer_current_tick(tp)) >>> >> CU_FAIL("Timeout delivered early"); >>> >> if (tick < prev_tick) { >>> >> - printf("Too late tick: %"PRIu64" prev_tick >>> >> %"PRIu64"\n", >>> >> - tick, prev_tick); >>> >> - CU_FAIL("Timeout delivered late"); >>> >> + LOG_DBG("Too late tick: %"PRIu64" prev_tick >>> >> %"PRIu64"\n", >>> >> + tick, prev_tick); >>> >> + /* We don't report late timeouts using CU_FAIL >>> >> */ >>> >> + odp_atomic_inc_u32(&ndelivtoolate); >>> >> } >>> >> } >>> >> >>> >> - /* Use assert() for correctness check of test program itself >>> >> */ >>> >> - assert(ttp->buf == ODP_BUFFER_INVALID); >>> >> - ttp->buf = buf; >>> >> + if (ttp != NULL) { >>> >> + /* Internal error */ >>> >> + CU_ASSERT_FATAL(ttp->buf == ODP_BUFFER_INVALID); >>> >> + ttp->buf = buf; >>> >> + } >>> >> } >>> >> >>> >> /* @private Worker thread entrypoint which performs timer >>> >> alloc/set/cancel/free >>> >> @@ -203,8 +211,11 @@ static void *worker_entrypoint(void *arg) >>> >> /* Save expected expiration tick */ >>> >> tt[i].tick = cur_tick + tck; >>> >> } >>> >> - if (usleep(1000/*1ms*/) < 0) >>> >> - perror("usleep"), abort(); >>> >> + struct timespec ts; >>> >> + ts.tv_sec = 0; >>> >> + ts.tv_nsec = 1000000; /* 1ms */ >>> >> + if (nanosleep(&ts, NULL) < 0) >>> >> + perror("nanosleep"), abort(); >>> >> } >>> >> >>> >> /* Cancel and free all timers */ >>> >> @@ -220,18 +231,22 @@ static void *worker_entrypoint(void *arg) >>> >> CU_FAIL("odp_timer_free"); >>> >> } >>> >> >>> >> - printf("Thread %u: %u timers set\n", thr, nset); >>> >> - printf("Thread %u: %u timers reset\n", thr, nreset); >>> >> - printf("Thread %u: %u timers cancelled\n", thr, ncancel); >>> >> - printf("Thread %u: %u timers reset/cancelled too late\n", >>> >> - thr, ntoolate); >>> >> - printf("Thread %u: %u timeouts received\n", thr, nrcv); >>> >> - printf("Thread %u: %u stale timeout(s) after >>> >> odp_timer_free()\n", >>> >> - thr, nstale); >>> >> + LOG_DBG("Thread %u: %u timers set\n", thr, nset); >>> >> + LOG_DBG("Thread %u: %u timers reset\n", thr, nreset); >>> >> + LOG_DBG("Thread %u: %u timers cancelled\n", thr, ncancel); >>> >> + LOG_DBG("Thread %u: %u timers reset/cancelled too late\n", >>> >> + thr, ntoolate); >>> >> + LOG_DBG("Thread %u: %u timeouts received\n", thr, nrcv); >>> >> + LOG_DBG("Thread %u: %u stale timeout(s) after >>> >> odp_timer_free()\n", >>> >> + thr, nstale); >>> >> >>> >> /* Delay some more to ensure timeouts for expired timers can >>> >> be >>> >> * received */ >>> >> - usleep(1000/*1ms*/); >>> >> + struct timespec ts; >>> >> + ts.tv_sec = 0; >>> >> + ts.tv_nsec = 1000000; /* 1ms */ >>> >> + if (nanosleep(&ts, NULL) < 0) >>> >> + perror("nanosleep"), abort(); >>> >> while (nstale != 0) { >>> >> odp_buffer_t buf = odp_queue_deq(queue); >>> >> if (buf != ODP_BUFFER_INVALID) { >>> >> @@ -247,7 +262,7 @@ static void *worker_entrypoint(void *arg) >>> >> if (buf != ODP_BUFFER_INVALID) >>> >> CU_FAIL("Unexpected buffer received"); >>> >> >>> >> - printf("Thread %u: exiting\n", thr); >>> >> + LOG_DBG("Thread %u: exiting\n", thr); >>> >> return NULL; >>> >> } >>> >> >>> >> @@ -256,9 +271,13 @@ static void test_odp_timer_all(void) >>> >> { >>> >> odp_buffer_pool_param_t params; >>> >> odp_timer_pool_param_t tparam; >>> >> - /* This is a stressfull test - need to reserve some cpu cycles >>> >> - * @TODO move to test/performance */ >>> >> - int num_workers = min(odp_sys_cpu_count()-1, MAX_WORKERS); >>> >> + /* Reserve at least one core for running other processes so >>> >> the >>> >> timer >>> >> + * test hopefully can run undisturbed and thus get better >>> >> timing >>> >> + * results. */ >>> >> + int num_workers = min(odp_sys_cpu_count() - 1, MAX_WORKERS); >>> >> + /* On a single-CPU machine run at least one thread */ >>> >> + if (num_workers < 1) >>> >> + num_workers = 1; >>> >> >>> >> /* Create timeout buffer pools */ >>> >> params.buf_size = 0; >>> >> @@ -294,19 +313,11 @@ static void test_odp_timer_all(void) >>> >> CU_ASSERT(tpinfo.param.res_ns == RES); >>> >> CU_ASSERT(tpinfo.param.min_tmo == MIN); >>> >> CU_ASSERT(tpinfo.param.max_tmo == MAX); >>> >> - printf("Timer pool\n"); >>> >> - printf("----------\n"); >>> >> - printf(" name: %s\n", tpinfo.name); >>> >> - printf(" resolution: %"PRIu64" ns (%"PRIu64" us)\n", >>> >> - tpinfo.param.res_ns, tpinfo.param.res_ns / 1000); >>> >> - printf(" min tmo: %"PRIu64" ns\n", tpinfo.param.min_tmo); >>> >> - printf(" max tmo: %"PRIu64" ns\n", tpinfo.param.max_tmo); >>> >> - printf("\n"); >>> >> - >>> >> - printf("#timers..: %u\n", NTIMERS); >>> >> - printf("Tmo range: %u ms (%"PRIu64" ticks)\n", RANGE_MS, >>> >> - odp_timer_ns_to_tick(tp, 1000000ULL * RANGE_MS)); >>> >> - printf("\n"); >>> >> + CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); >>> >> + >>> >> + LOG_DBG("#timers..: %u\n", NTIMERS); >>> >> + LOG_DBG("Tmo range: %u ms (%"PRIu64" ticks)\n", RANGE_MS, >>> >> + odp_timer_ns_to_tick(tp, 1000000ULL * RANGE_MS)); >>> >> >>> >> uint64_t tick; >>> >> for (tick = 0; tick < 1000000000000ULL; tick += 1000000ULL) { >>> >> @@ -319,6 +330,9 @@ static void test_odp_timer_all(void) >>> >> /* Initialize barrier used by worker threads for >>> >> synchronization >>> >> */ >>> >> odp_barrier_init(&test_barrier, num_workers); >>> >> >>> >> + /* Initialize the shared timeout counter */ >>> >> + odp_atomic_init_u32(&ndelivtoolate, 0); >>> >> + >>> >> /* Create and start worker threads */ >>> >> pthrd_arg thrdarg; >>> >> thrdarg.testcase = 0; >>> >> @@ -327,6 +341,8 @@ static void test_odp_timer_all(void) >>> >> >>> >> /* Wait for worker threads to exit */ >>> >> odp_cunit_thread_exit(&thrdarg); >>> >> + LOG_DBG("Number of timeouts delivered/received too late: >>> >> %u\n", >>> >> + odp_atomic_load_u32(&ndelivtoolate)); >>> >> >>> >> /* Check some statistics after the test */ >>> >> if (odp_timer_pool_info(tp, &tpinfo) != 0) >>> >> -- >>> >> 1.9.1 >>> >> >>> >> >>> >> _______________________________________________ >>> >> lng-odp mailing list >>> >> [email protected] >>> >> http://lists.linaro.org/mailman/listinfo/lng-odp >>> > >>> > >>> > >>> > >>> > -- >>> > Mike Holmes >>> > Linaro Sr Technical Manager >>> > LNG - ODP >>> > >>> > _______________________________________________ >>> > lng-odp mailing list >>> > [email protected] >>> > http://lists.linaro.org/mailman/listinfo/lng-odp >>> > >> >> >> >> >> -- >> Mike Holmes >> Linaro Sr Technical Manager >> LNG - ODP > > > > > -- > Mike Holmes > Linaro Sr Technical Manager > LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
