[PATCHv3,4/5] validation: odp_timer.c: cunit cleanup

Message ID 1422529273-31282-5-git-send-email-ola.liljedahl@linaro.org
State New
Headers show

Commit Message

Ola Liljedahl Jan. 29, 2015, 11:01 a.m.
Use CU_ASSERT_FATAL instead of plain assert so to work better with the cunit
test framework.
Use LOG_DBG instead of printf. Remove some unnecessary printouts.
Ensure we run at least one worker thread.

Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
---
(This document/code contribution attached is provided under the terms of
agreement LES-LTM-21309)

 test/validation/odp_timer.c | 50 ++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

Comments

Anders Roxell Jan. 29, 2015, 2:15 p.m. | #1
On 2015-01-29 12:01, Ola Liljedahl wrote:
> Use CU_ASSERT_FATAL instead of plain assert so to work better with the cunit
> test framework.
> Use LOG_DBG instead of printf. Remove some unnecessary printouts.
> Ensure we run at least one worker thread.

I don't think this is a cleanup its an improvement.
Belongs in its own patch right?

Cheers,
Anders

> 
> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
> ---
> (This document/code contribution attached is provided under the terms of
> agreement LES-LTM-21309)
> 
>  test/validation/odp_timer.c | 50 ++++++++++++++++++++-------------------------
>  1 file changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
> index ce5ffb1..d78f6b6 100644
> --- a/test/validation/odp_timer.c
> +++ b/test/validation/odp_timer.c
> @@ -10,7 +10,6 @@
>  
>  /* For rand_r and nanosleep */
>  #define _POSIX_C_SOURCE 200112L
> -#include <assert.h>
>  #include <time.h>
>  #include <unistd.h>
>  #include <odp.h>
> @@ -54,8 +53,7 @@ struct test_timer {
>  /* @private Handle a received (timeout) event */
>  static void handle_tmo(odp_event_t ev, bool stale, uint64_t prev_tick)
>  {
> -	/* Use assert() for internal correctness checks of test program */
> -	assert(ev != ODP_EVENT_INVALID);
> +	CU_ASSERT_FATAL(ev != ODP_EVENT_INVALID); /* Internal error */
>  	if (odp_event_type(ev) != ODP_EVENT_TIMEOUT) {
>  		/* Not a timeout event */
>  		CU_FAIL("Unexpected event type received");
> @@ -233,14 +231,14 @@ 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 */
> @@ -264,7 +262,7 @@ static void *worker_entrypoint(void *arg)
>  	if (ev != ODP_EVENT_INVALID)
>  		CU_FAIL("Unexpected event received");
>  
> -	printf("Thread %u: exiting\n", thr);
> +	LOG_DBG("Thread %u: exiting\n", thr);
>  	return NULL;
>  }
>  
> @@ -273,9 +271,13 @@ static void test_odp_timer_all(void)
>  {
>  	odp_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 pools */
>  	params.tmo.num = (NTIMERS + 1) * num_workers;
> @@ -309,19 +311,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) {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Ola Liljedahl Jan. 29, 2015, 3:32 p.m. | #2
On 29 January 2015 at 15:15, Anders Roxell <anders.roxell@linaro.org> wrote:
> On 2015-01-29 12:01, Ola Liljedahl wrote:
>> Use CU_ASSERT_FATAL instead of plain assert so to work better with the cunit
>> test framework.
>> Use LOG_DBG instead of printf. Remove some unnecessary printouts.
>> Ensure we run at least one worker thread.
>
> I don't think this is a cleanup its an improvement.
> Belongs in its own patch right?
In your dreams.

>
> Cheers,
> Anders
>
>>
>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>> ---
>> (This document/code contribution attached is provided under the terms of
>> agreement LES-LTM-21309)
>>
>>  test/validation/odp_timer.c | 50 ++++++++++++++++++++-------------------------
>>  1 file changed, 22 insertions(+), 28 deletions(-)
>>
>> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
>> index ce5ffb1..d78f6b6 100644
>> --- a/test/validation/odp_timer.c
>> +++ b/test/validation/odp_timer.c
>> @@ -10,7 +10,6 @@
>>
>>  /* For rand_r and nanosleep */
>>  #define _POSIX_C_SOURCE 200112L
>> -#include <assert.h>
>>  #include <time.h>
>>  #include <unistd.h>
>>  #include <odp.h>
>> @@ -54,8 +53,7 @@ struct test_timer {
>>  /* @private Handle a received (timeout) event */
>>  static void handle_tmo(odp_event_t ev, bool stale, uint64_t prev_tick)
>>  {
>> -     /* Use assert() for internal correctness checks of test program */
>> -     assert(ev != ODP_EVENT_INVALID);
>> +     CU_ASSERT_FATAL(ev != ODP_EVENT_INVALID); /* Internal error */
>>       if (odp_event_type(ev) != ODP_EVENT_TIMEOUT) {
>>               /* Not a timeout event */
>>               CU_FAIL("Unexpected event type received");
>> @@ -233,14 +231,14 @@ 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 */
>> @@ -264,7 +262,7 @@ static void *worker_entrypoint(void *arg)
>>       if (ev != ODP_EVENT_INVALID)
>>               CU_FAIL("Unexpected event received");
>>
>> -     printf("Thread %u: exiting\n", thr);
>> +     LOG_DBG("Thread %u: exiting\n", thr);
>>       return NULL;
>>  }
>>
>> @@ -273,9 +271,13 @@ static void test_odp_timer_all(void)
>>  {
>>       odp_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 pools */
>>       params.tmo.num = (NTIMERS + 1) * num_workers;
>> @@ -309,19 +311,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) {
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp

Patch

diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
index ce5ffb1..d78f6b6 100644
--- a/test/validation/odp_timer.c
+++ b/test/validation/odp_timer.c
@@ -10,7 +10,6 @@ 
 
 /* For rand_r and nanosleep */
 #define _POSIX_C_SOURCE 200112L
-#include <assert.h>
 #include <time.h>
 #include <unistd.h>
 #include <odp.h>
@@ -54,8 +53,7 @@  struct test_timer {
 /* @private Handle a received (timeout) event */
 static void handle_tmo(odp_event_t ev, bool stale, uint64_t prev_tick)
 {
-	/* Use assert() for internal correctness checks of test program */
-	assert(ev != ODP_EVENT_INVALID);
+	CU_ASSERT_FATAL(ev != ODP_EVENT_INVALID); /* Internal error */
 	if (odp_event_type(ev) != ODP_EVENT_TIMEOUT) {
 		/* Not a timeout event */
 		CU_FAIL("Unexpected event type received");
@@ -233,14 +231,14 @@  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 */
@@ -264,7 +262,7 @@  static void *worker_entrypoint(void *arg)
 	if (ev != ODP_EVENT_INVALID)
 		CU_FAIL("Unexpected event received");
 
-	printf("Thread %u: exiting\n", thr);
+	LOG_DBG("Thread %u: exiting\n", thr);
 	return NULL;
 }
 
@@ -273,9 +271,13 @@  static void test_odp_timer_all(void)
 {
 	odp_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 pools */
 	params.tmo.num = (NTIMERS + 1) * num_workers;
@@ -309,19 +311,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) {