I don't see a resolution to the request to break up the patch.
On 21 January 2015 at 07:43, Ola Liljedahl <[email protected]> wrote: > 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 > -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
