Anders you added comment to break this patch on two. How is it critical for now?

Maxim.

On 01/21/2015 09:24 PM, Ola Liljedahl wrote:
On 21 January 2015 at 17:56, Mike Holmes <[email protected]> wrote:
I don't see a resolution to the request to break up the patch.
The original patch has been broken up into four independent patches
(you actually reviewed at least one of them and complained on the
usage of perror and abort). I linked to the patches in the delta doc.
What else is missing?


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


_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp

Reply via email to