DPDK CI discussions
 help / color / mirror / Atom feed
* [dpdk-ci] [PATCH] test: drop 'alarm_autotest' and 'cycles_autotest' from test suite
@ 2021-05-25  0:16 Aaron Conole
  2021-05-26 11:58 ` [dpdk-ci] [PATCH v2] test: remove strict timing requirements from alarm and cycles tests Aaron Conole
  2021-06-03 13:46 ` [dpdk-ci] [PATCH v3] " Aaron Conole
  0 siblings, 2 replies; 6+ messages in thread
From: Aaron Conole @ 2021-05-25  0:16 UTC (permalink / raw)
  To: dev; +Cc: David Marchand, ci, Michael Santana, Ilya Maximets

The tests 'alarm_autotest' and 'cycles_autotest' rely on the underlying
system having very accurate and precise timing.  On systems where the timing
isn't as rigid, or the load is particularly high, these tests are unreliable
since the wake latency from the scheduler can be high enough to miss the
timing window.

Remove these tests from the test suites.  Maybe it's useful for these
tests to be present as a diagnostics tool, but for normal unit testing,
they don't provide much value.  They have falsely flagged patches as
FAIL on various infrastructures.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 app/test/meson.build | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/test/meson.build b/app/test/meson.build
index 08c82d3d23..8dec48e81c 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -185,14 +185,12 @@ test_deps = [
 # to indicate whether it can run in no-huge mode.
 fast_tests = [
         ['acl_autotest', true],
-        ['alarm_autotest', false],
         ['atomic_autotest', false],
         ['bitops_autotest', true],
         ['byteorder_autotest', true],
         ['cmdline_autotest', true],
         ['common_autotest', true],
         ['cpuflags_autotest', true],
-        ['cycles_autotest', true],
         ['debug_autotest', true],
         ['eal_flags_c_opt_autotest', false],
         ['eal_flags_main_opt_autotest', false],
-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-ci] [PATCH v2] test: remove strict timing requirements from alarm and cycles tests
  2021-05-25  0:16 [dpdk-ci] [PATCH] test: drop 'alarm_autotest' and 'cycles_autotest' from test suite Aaron Conole
@ 2021-05-26 11:58 ` Aaron Conole
  2021-05-26 13:06   ` Aaron Conole
  2021-06-03 13:46 ` [dpdk-ci] [PATCH v3] " Aaron Conole
  1 sibling, 1 reply; 6+ messages in thread
From: Aaron Conole @ 2021-05-26 11:58 UTC (permalink / raw)
  To: dev; +Cc: David Marchand, ci, Michael Santana, Ilya Maximets, Thomas Monjalon

The tests 'alarm_autotest' and 'cycles_autotest' rely on the underlying
system having very accurate and precise timing.  On systems where the
timing isn't as rigid, or the load is particularly high, these tests are
unreliable since the wake latency from the scheduler can be high enough
to miss the timing window.

Remove the timing related tests from the test suites.  These tests now
ensure the add/remove callback infrastructure unit tests, but drop the
waits and reliance on system timing and load.

This avoids FAIL on various testing infrastructures.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
v2: Drop the timing requirements, but keep the API calls

 app/test/test_alarm.c  | 165 -----------------------------------------
 app/test/test_cycles.c |  78 -------------------
 2 files changed, 243 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 951b7f07b3..6eea751cae 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -28,175 +28,13 @@ test_alarm_callback(void *cb_arg)
 	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
 }
 
-static rte_atomic32_t cb_count;
-
-static void
-test_multi_cb(void *arg)
-{
-	rte_atomic32_inc(&cb_count);
-	printf("In %s - arg = %p\n", __func__, arg);
-}
-
-static volatile int recursive_error = 0;
-
-static void
-test_remove_in_callback(void *arg)
-{
-	printf("In %s - arg = %p\n", __func__, arg);
-	if (rte_eal_alarm_cancel(test_remove_in_callback, arg) ||
-			rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1)) {
-		printf("Error - cancelling callback from within function succeeded!\n");
-		recursive_error = 1;
-	}
-	flag = (int)((uintptr_t)arg);
-}
-
-static volatile int flag_2;
-
-static void
-test_remove_in_callback_2(void *arg)
-{
-	if (rte_eal_alarm_cancel(test_remove_in_callback_2, arg) || rte_eal_alarm_cancel(test_remove_in_callback_2, (void *)-1)) {
-		printf("Error - cancelling callback of test_remove_in_callback_2\n");
-		return;
-	}
-	flag_2 = 1;
-}
-
-static int
-test_multi_alarms(void)
-{
-	int rm_count = 0;
-	int count = 0;
-	cb_count.cnt = 0;
-
-	printf("Expect 6 callbacks in order...\n");
-	/* add two alarms in order */
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_multi_cb, (void *)2);
-
-	/* now add in reverse order */
-	rte_eal_alarm_set(60 * US_PER_MS, test_multi_cb, (void *)6);
-	rte_eal_alarm_set(50 * US_PER_MS, test_multi_cb, (void *)5);
-	rte_eal_alarm_set(40 * US_PER_MS, test_multi_cb, (void *)4);
-	rte_eal_alarm_set(30 * US_PER_MS, test_multi_cb, (void *)3);
-
-	/* wait for expiry */
-	rte_delay_ms(65);
-	if (cb_count.cnt != 6) {
-		printf("Missing callbacks\n");
-		/* remove any callbacks that might remain */
-		rte_eal_alarm_cancel(test_multi_cb, (void *)-1);
-		return -1;
-	}
-
-	cb_count.cnt = 0;
-	printf("Expect only callbacks with args 1 and 3...\n");
-	/* Add 3 flags, then delete one */
-	rte_eal_alarm_set(30 * US_PER_MS, test_multi_cb, (void *)3);
-	rte_eal_alarm_set(20 * US_PER_MS, test_multi_cb, (void *)2);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rm_count = rte_eal_alarm_cancel(test_multi_cb, (void *)2);
-
-	rte_delay_ms(35);
-	if (cb_count.cnt != 2 || rm_count != 1) {
-		printf("Error: invalid flags count or alarm removal failure"
-				" -  flags value = %d, expected = %d\n",
-				(int)cb_count.cnt, 2);
-		/* remove any callbacks that might remain */
-		rte_eal_alarm_cancel(test_multi_cb, (void *)-1);
-		return -1;
-	}
-
-	printf("Testing adding and then removing multiple alarms\n");
-	/* finally test that no callbacks are called if we delete them all*/
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)2);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)3);
-	rm_count = rte_eal_alarm_cancel(test_alarm_callback, (void *)-1);
-	if (rm_count != 0) {
-		printf("Error removing non-existant alarm succeeded\n");
-		rte_eal_alarm_cancel(test_multi_cb, (void *) -1);
-		return -1;
-	}
-	rm_count = rte_eal_alarm_cancel(test_multi_cb, (void *) -1);
-	if (rm_count != 3) {
-		printf("Error removing all pending alarm callbacks\n");
-		return -1;
-	}
-
-	/* Test that we cannot cancel an alarm from within the callback itself
-	 * Also test that we can cancel head-of-line callbacks ok.*/
-	flag = 0;
-	recursive_error = 0;
-	rte_eal_alarm_set(10 * US_PER_MS, test_remove_in_callback, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_remove_in_callback, (void *)2);
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback, (void *)1);
-	if (rm_count != 1) {
-		printf("Error cancelling head-of-list callback\n");
-		return -1;
-	}
-	rte_delay_ms(15);
-	if (flag != 0) {
-		printf("Error, cancelling head-of-list leads to premature callback\n");
-		return -1;
-	}
-
-	while (flag != 2 && count++ < RTE_TEST_MAX_REPEAT)
-		rte_delay_ms(10);
-
-	if (flag != 2) {
-		printf("Error - expected callback not called\n");
-		rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1);
-		return -1;
-	}
-	if (recursive_error == 1)
-		return -1;
-
-	/* Check if it can cancel all for the same callback */
-	printf("Testing canceling all for the same callback\n");
-	flag_2 = 0;
-	rte_eal_alarm_set(10 * US_PER_MS, test_remove_in_callback, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_remove_in_callback_2, (void *)2);
-	rte_eal_alarm_set(30 * US_PER_MS, test_remove_in_callback_2, (void *)3);
-	rte_eal_alarm_set(40 * US_PER_MS, test_remove_in_callback, (void *)4);
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback_2, (void *)-1);
-	if (rm_count != 2) {
-		printf("Error, cannot cancel all for the same callback\n");
-		return -1;
-	}
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1);
-	if (rm_count != 2) {
-		printf("Error, cannot cancel all for the same callback\n");
-		return -1;
-	}
-
-	return 0;
-}
-
 static int
 test_alarm(void)
 {
-	int count = 0;
 #ifdef RTE_EXEC_ENV_FREEBSD
 	printf("The alarm API is not supported on FreeBSD\n");
 	return 0;
 #endif
-	/* check if the callback will be called */
-	printf("check if the callback will be called\n");
-	flag = 0;
-	if (rte_eal_alarm_set(RTE_TEST_ALARM_TIMEOUT * US_PER_MS,
-			test_alarm_callback, NULL) < 0) {
-		printf("fail to set alarm callback\n");
-		return -1;
-	}
-	while (flag == 0 && count++ < RTE_TEST_MAX_REPEAT)
-		rte_delay_ms(RTE_TEST_CHECK_PERIOD);
-
-	if (flag == 0){
-		printf("Callback not called\n");
-		return -1;
-	}
 
 	/* check if it will fail to set alarm with wrong us value */
 	printf("check if it will fail to set alarm with wrong ms values\n");
@@ -225,9 +63,6 @@ test_alarm(void)
 		return -1;
 	}
 
-	if (test_multi_alarms() != 0)
-		return -1;
-
 	return 0;
 }
 
diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
index 97d42f3032..4765f11722 100644
--- a/app/test/test_cycles.c
+++ b/app/test/test_cycles.c
@@ -12,84 +12,6 @@
 
 #define N 10000
 
-/*
- * Cycles test
- * ===========
- *
- * - Loop N times and check that the timer always increments and
- *   never decrements during this loop.
- *
- * - Wait one second using rte_usleep() and check that the increment
- *   of cycles is correct with regard to the frequency of the timer.
- */
-
-static int
-check_wait_one_second(void)
-{
-	uint64_t cycles, prev_cycles;
-	uint64_t hz = rte_get_timer_hz();
-	uint64_t max_inc = (hz / 100); /* 10 ms max between 2 reads */
-
-	/* check that waiting 1 second is precise */
-	prev_cycles = rte_get_timer_cycles();
-	rte_delay_us(1000000);
-	cycles = rte_get_timer_cycles();
-
-	if ((uint64_t)(cycles - prev_cycles) > (hz + max_inc)) {
-		printf("delay_us is not accurate: too long\n");
-		return -1;
-	}
-	if ((uint64_t)(cycles - prev_cycles) < (hz - max_inc)) {
-		printf("delay_us is not accurate: too short\n");
-		return -1;
-	}
-
-	return 0;
-}
-
-static int
-test_cycles(void)
-{
-	unsigned i;
-	uint64_t start_cycles, cycles, prev_cycles;
-	uint64_t hz = rte_get_timer_hz();
-	uint64_t max_inc = (hz / 100); /* 10 ms max between 2 reads */
-
-	/* check that the timer is always incrementing */
-	start_cycles = rte_get_timer_cycles();
-	prev_cycles = start_cycles;
-	for (i=0; i<N; i++) {
-		cycles = rte_get_timer_cycles();
-		if ((uint64_t)(cycles - prev_cycles) > max_inc) {
-			printf("increment too high or going backwards\n");
-			return -1;
-		}
-		prev_cycles = cycles;
-	}
-
-	return check_wait_one_second();
-}
-
-REGISTER_TEST_COMMAND(cycles_autotest, test_cycles);
-
-/*
- * One second precision test with rte_delay_us_sleep.
- */
-
-static int
-test_delay_us_sleep(void)
-{
-	int rv;
-
-	rte_delay_us_callback_register(rte_delay_us_sleep);
-	rv = check_wait_one_second();
-	/* restore original delay function */
-	rte_delay_us_callback_register(rte_delay_us_block);
-
-	return rv;
-}
-
-REGISTER_TEST_COMMAND(delay_us_sleep_autotest, test_delay_us_sleep);
 
 /*
  * rte_delay_us_callback test
-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-ci] [PATCH v2] test: remove strict timing requirements from alarm and cycles tests
  2021-05-26 11:58 ` [dpdk-ci] [PATCH v2] test: remove strict timing requirements from alarm and cycles tests Aaron Conole
@ 2021-05-26 13:06   ` Aaron Conole
  0 siblings, 0 replies; 6+ messages in thread
From: Aaron Conole @ 2021-05-26 13:06 UTC (permalink / raw)
  To: dev; +Cc: David Marchand, ci, Michael Santana, Ilya Maximets, Thomas Monjalon

Aaron Conole <aconole@redhat.com> writes:

> The tests 'alarm_autotest' and 'cycles_autotest' rely on the underlying
> system having very accurate and precise timing.  On systems where the
> timing isn't as rigid, or the load is particularly high, these tests are
> unreliable since the wake latency from the scheduler can be high enough
> to miss the timing window.
>
> Remove the timing related tests from the test suites.  These tests now
> ensure the add/remove callback infrastructure unit tests, but drop the
> waits and reliance on system timing and load.
>
> This avoids FAIL on various testing infrastructures.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
> v2: Drop the timing requirements, but keep the API calls

NAK - I have a v3 for this with some stuff that was missed.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-ci] [PATCH v3] test: remove strict timing requirements from alarm and cycles tests
  2021-05-25  0:16 [dpdk-ci] [PATCH] test: drop 'alarm_autotest' and 'cycles_autotest' from test suite Aaron Conole
  2021-05-26 11:58 ` [dpdk-ci] [PATCH v2] test: remove strict timing requirements from alarm and cycles tests Aaron Conole
@ 2021-06-03 13:46 ` Aaron Conole
  2021-06-03 15:22   ` [dpdk-ci] [PATCH v4] " Aaron Conole
  1 sibling, 1 reply; 6+ messages in thread
From: Aaron Conole @ 2021-06-03 13:46 UTC (permalink / raw)
  To: dev; +Cc: David Marchand, ci, Michael Santana, Ilya Maximets

The tests 'alarm_autotest' and 'cycles_autotest' rely on the underlying
system having very accurate and precise timing.  On systems where the
timing isn't as rigid, or the load is particularly high, these tests are
unreliable since the wake latency from the scheduler can be high enough
to miss the timing window.

Remove the timing related tests from the test suites.  These tests now
ensure the add/remove callback infrastructure unit tests, but drop the
waits and reliance on system timing and load.

This avoids FAIL on various testing infrastructures.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 app/test/autotest_data.py |  12 ---
 app/test/meson.build      |   2 -
 app/test/test_alarm.c     | 176 +-------------------------------------
 app/test/test_cycles.c    |  81 ------------------
 4 files changed, 1 insertion(+), 270 deletions(-)

diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 097638941f..3b1561432b 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -9,12 +9,6 @@
 # groups of tests that can be run in parallel
 # the grouping has been found largely empirically
 parallel_test_list = [
-    {
-        "Name":    "Cycles autotest",
-        "Command": "cycles_autotest",
-        "Func":    default_autotest,
-        "Report":  None,
-    },
     {
         "Name":    "Timer autotest",
         "Command": "timer_autotest",
@@ -183,12 +177,6 @@
         "Func":    default_autotest,
         "Report":  None,
     },
-    {
-        "Name":    "Alarm autotest",
-        "Command": "alarm_autotest",
-        "Func":    default_autotest,
-        "Report":  None,
-    },
     {
         "Name":    "Malloc autotest",
         "Command": "malloc_autotest",
diff --git a/app/test/meson.build b/app/test/meson.build
index 08c82d3d23..8dec48e81c 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -185,14 +185,12 @@ test_deps = [
 # to indicate whether it can run in no-huge mode.
 fast_tests = [
         ['acl_autotest', true],
-        ['alarm_autotest', false],
         ['atomic_autotest', false],
         ['bitops_autotest', true],
         ['byteorder_autotest', true],
         ['cmdline_autotest', true],
         ['common_autotest', true],
         ['cpuflags_autotest', true],
-        ['cycles_autotest', true],
         ['debug_autotest', true],
         ['eal_flags_c_opt_autotest', false],
         ['eal_flags_main_opt_autotest', false],
diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 951b7f07b3..b4034339b8 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -6,19 +6,10 @@
 #include <stdint.h>
 
 #include <rte_common.h>
-#include <rte_cycles.h>
-#include <rte_interrupts.h>
-#include <rte_atomic.h>
 #include <rte_alarm.h>
 
 #include "test.h"
 
-#define US_PER_MS 1000
-
-#define RTE_TEST_ALARM_TIMEOUT 10 /* ms */
-#define RTE_TEST_CHECK_PERIOD   3 /* ms */
-#define RTE_TEST_MAX_REPEAT    20
-
 static volatile int flag;
 
 static void
@@ -28,175 +19,13 @@ test_alarm_callback(void *cb_arg)
 	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
 }
 
-static rte_atomic32_t cb_count;
-
-static void
-test_multi_cb(void *arg)
-{
-	rte_atomic32_inc(&cb_count);
-	printf("In %s - arg = %p\n", __func__, arg);
-}
-
-static volatile int recursive_error = 0;
-
-static void
-test_remove_in_callback(void *arg)
-{
-	printf("In %s - arg = %p\n", __func__, arg);
-	if (rte_eal_alarm_cancel(test_remove_in_callback, arg) ||
-			rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1)) {
-		printf("Error - cancelling callback from within function succeeded!\n");
-		recursive_error = 1;
-	}
-	flag = (int)((uintptr_t)arg);
-}
-
-static volatile int flag_2;
-
-static void
-test_remove_in_callback_2(void *arg)
-{
-	if (rte_eal_alarm_cancel(test_remove_in_callback_2, arg) || rte_eal_alarm_cancel(test_remove_in_callback_2, (void *)-1)) {
-		printf("Error - cancelling callback of test_remove_in_callback_2\n");
-		return;
-	}
-	flag_2 = 1;
-}
-
-static int
-test_multi_alarms(void)
-{
-	int rm_count = 0;
-	int count = 0;
-	cb_count.cnt = 0;
-
-	printf("Expect 6 callbacks in order...\n");
-	/* add two alarms in order */
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_multi_cb, (void *)2);
-
-	/* now add in reverse order */
-	rte_eal_alarm_set(60 * US_PER_MS, test_multi_cb, (void *)6);
-	rte_eal_alarm_set(50 * US_PER_MS, test_multi_cb, (void *)5);
-	rte_eal_alarm_set(40 * US_PER_MS, test_multi_cb, (void *)4);
-	rte_eal_alarm_set(30 * US_PER_MS, test_multi_cb, (void *)3);
-
-	/* wait for expiry */
-	rte_delay_ms(65);
-	if (cb_count.cnt != 6) {
-		printf("Missing callbacks\n");
-		/* remove any callbacks that might remain */
-		rte_eal_alarm_cancel(test_multi_cb, (void *)-1);
-		return -1;
-	}
-
-	cb_count.cnt = 0;
-	printf("Expect only callbacks with args 1 and 3...\n");
-	/* Add 3 flags, then delete one */
-	rte_eal_alarm_set(30 * US_PER_MS, test_multi_cb, (void *)3);
-	rte_eal_alarm_set(20 * US_PER_MS, test_multi_cb, (void *)2);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rm_count = rte_eal_alarm_cancel(test_multi_cb, (void *)2);
-
-	rte_delay_ms(35);
-	if (cb_count.cnt != 2 || rm_count != 1) {
-		printf("Error: invalid flags count or alarm removal failure"
-				" -  flags value = %d, expected = %d\n",
-				(int)cb_count.cnt, 2);
-		/* remove any callbacks that might remain */
-		rte_eal_alarm_cancel(test_multi_cb, (void *)-1);
-		return -1;
-	}
-
-	printf("Testing adding and then removing multiple alarms\n");
-	/* finally test that no callbacks are called if we delete them all*/
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)2);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)3);
-	rm_count = rte_eal_alarm_cancel(test_alarm_callback, (void *)-1);
-	if (rm_count != 0) {
-		printf("Error removing non-existant alarm succeeded\n");
-		rte_eal_alarm_cancel(test_multi_cb, (void *) -1);
-		return -1;
-	}
-	rm_count = rte_eal_alarm_cancel(test_multi_cb, (void *) -1);
-	if (rm_count != 3) {
-		printf("Error removing all pending alarm callbacks\n");
-		return -1;
-	}
-
-	/* Test that we cannot cancel an alarm from within the callback itself
-	 * Also test that we can cancel head-of-line callbacks ok.*/
-	flag = 0;
-	recursive_error = 0;
-	rte_eal_alarm_set(10 * US_PER_MS, test_remove_in_callback, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_remove_in_callback, (void *)2);
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback, (void *)1);
-	if (rm_count != 1) {
-		printf("Error cancelling head-of-list callback\n");
-		return -1;
-	}
-	rte_delay_ms(15);
-	if (flag != 0) {
-		printf("Error, cancelling head-of-list leads to premature callback\n");
-		return -1;
-	}
-
-	while (flag != 2 && count++ < RTE_TEST_MAX_REPEAT)
-		rte_delay_ms(10);
-
-	if (flag != 2) {
-		printf("Error - expected callback not called\n");
-		rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1);
-		return -1;
-	}
-	if (recursive_error == 1)
-		return -1;
-
-	/* Check if it can cancel all for the same callback */
-	printf("Testing canceling all for the same callback\n");
-	flag_2 = 0;
-	rte_eal_alarm_set(10 * US_PER_MS, test_remove_in_callback, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_remove_in_callback_2, (void *)2);
-	rte_eal_alarm_set(30 * US_PER_MS, test_remove_in_callback_2, (void *)3);
-	rte_eal_alarm_set(40 * US_PER_MS, test_remove_in_callback, (void *)4);
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback_2, (void *)-1);
-	if (rm_count != 2) {
-		printf("Error, cannot cancel all for the same callback\n");
-		return -1;
-	}
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1);
-	if (rm_count != 2) {
-		printf("Error, cannot cancel all for the same callback\n");
-		return -1;
-	}
-
-	return 0;
-}
-
 static int
 test_alarm(void)
 {
-	int count = 0;
 #ifdef RTE_EXEC_ENV_FREEBSD
 	printf("The alarm API is not supported on FreeBSD\n");
 	return 0;
 #endif
-	/* check if the callback will be called */
-	printf("check if the callback will be called\n");
-	flag = 0;
-	if (rte_eal_alarm_set(RTE_TEST_ALARM_TIMEOUT * US_PER_MS,
-			test_alarm_callback, NULL) < 0) {
-		printf("fail to set alarm callback\n");
-		return -1;
-	}
-	while (flag == 0 && count++ < RTE_TEST_MAX_REPEAT)
-		rte_delay_ms(RTE_TEST_CHECK_PERIOD);
-
-	if (flag == 0){
-		printf("Callback not called\n");
-		return -1;
-	}
 
 	/* check if it will fail to set alarm with wrong us value */
 	printf("check if it will fail to set alarm with wrong ms values\n");
@@ -213,7 +42,7 @@ test_alarm(void)
 
 	/* check if it will fail to set alarm with null callback parameter */
 	printf("check if it will fail to set alarm with null callback parameter\n");
-	if (rte_eal_alarm_set(RTE_TEST_ALARM_TIMEOUT, NULL, NULL) >= 0) {
+	if (rte_eal_alarm_set(10 /* ms */, NULL, NULL) >= 0) {
 		printf("should not be successful to set alarm with null callback parameter\n");
 		return -1;
 	}
@@ -225,9 +54,6 @@ test_alarm(void)
 		return -1;
 	}
 
-	if (test_multi_alarms() != 0)
-		return -1;
-
 	return 0;
 }
 
diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
index 97d42f3032..66d11e6db8 100644
--- a/app/test/test_cycles.c
+++ b/app/test/test_cycles.c
@@ -10,87 +10,6 @@
 
 #include "test.h"
 
-#define N 10000
-
-/*
- * Cycles test
- * ===========
- *
- * - Loop N times and check that the timer always increments and
- *   never decrements during this loop.
- *
- * - Wait one second using rte_usleep() and check that the increment
- *   of cycles is correct with regard to the frequency of the timer.
- */
-
-static int
-check_wait_one_second(void)
-{
-	uint64_t cycles, prev_cycles;
-	uint64_t hz = rte_get_timer_hz();
-	uint64_t max_inc = (hz / 100); /* 10 ms max between 2 reads */
-
-	/* check that waiting 1 second is precise */
-	prev_cycles = rte_get_timer_cycles();
-	rte_delay_us(1000000);
-	cycles = rte_get_timer_cycles();
-
-	if ((uint64_t)(cycles - prev_cycles) > (hz + max_inc)) {
-		printf("delay_us is not accurate: too long\n");
-		return -1;
-	}
-	if ((uint64_t)(cycles - prev_cycles) < (hz - max_inc)) {
-		printf("delay_us is not accurate: too short\n");
-		return -1;
-	}
-
-	return 0;
-}
-
-static int
-test_cycles(void)
-{
-	unsigned i;
-	uint64_t start_cycles, cycles, prev_cycles;
-	uint64_t hz = rte_get_timer_hz();
-	uint64_t max_inc = (hz / 100); /* 10 ms max between 2 reads */
-
-	/* check that the timer is always incrementing */
-	start_cycles = rte_get_timer_cycles();
-	prev_cycles = start_cycles;
-	for (i=0; i<N; i++) {
-		cycles = rte_get_timer_cycles();
-		if ((uint64_t)(cycles - prev_cycles) > max_inc) {
-			printf("increment too high or going backwards\n");
-			return -1;
-		}
-		prev_cycles = cycles;
-	}
-
-	return check_wait_one_second();
-}
-
-REGISTER_TEST_COMMAND(cycles_autotest, test_cycles);
-
-/*
- * One second precision test with rte_delay_us_sleep.
- */
-
-static int
-test_delay_us_sleep(void)
-{
-	int rv;
-
-	rte_delay_us_callback_register(rte_delay_us_sleep);
-	rv = check_wait_one_second();
-	/* restore original delay function */
-	rte_delay_us_callback_register(rte_delay_us_block);
-
-	return rv;
-}
-
-REGISTER_TEST_COMMAND(delay_us_sleep_autotest, test_delay_us_sleep);
-
 /*
  * rte_delay_us_callback test
  *
-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-ci] [PATCH v4] test: remove strict timing requirements from alarm and cycles tests
  2021-06-03 13:46 ` [dpdk-ci] [PATCH v3] " Aaron Conole
@ 2021-06-03 15:22   ` Aaron Conole
  2021-06-03 16:13     ` David Marchand
  0 siblings, 1 reply; 6+ messages in thread
From: Aaron Conole @ 2021-06-03 15:22 UTC (permalink / raw)
  To: dev; +Cc: David Marchand, ci, Michael Santana, Ilya Maximets

The tests 'alarm_autotest' and 'cycles_autotest' rely on the underlying
system having very accurate and precise timing.  On systems where the
timing isn't as rigid, or the load is particularly high, these tests are
unreliable since the wake latency from the scheduler can be high enough
to miss the timing window.

Remove the timing related tests from the test suites.  These tests now
ensure the add/remove callback infrastructure unit tests, but drop the
waits and reliance on system timing and load.

This avoids FAIL on various testing infrastructures.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
v4: I relied on the github actions test mechanism to do the test, but
    it appears it was successful even though a run manually or in
    interactive mode would have revealed a failure.  Additionally, the
    travis run for my local repo also uncovered the issue.

 app/test/autotest_data.py |  18 ----
 app/test/meson.build      |   3 -
 app/test/test_alarm.c     | 176 +-------------------------------------
 app/test/test_cycles.c    |  81 ------------------
 4 files changed, 1 insertion(+), 277 deletions(-)

diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 097638941f..11f9c8640c 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -9,12 +9,6 @@
 # groups of tests that can be run in parallel
 # the grouping has been found largely empirically
 parallel_test_list = [
-    {
-        "Name":    "Cycles autotest",
-        "Command": "cycles_autotest",
-        "Func":    default_autotest,
-        "Report":  None,
-    },
     {
         "Name":    "Timer autotest",
         "Command": "timer_autotest",
@@ -183,12 +177,6 @@
         "Func":    default_autotest,
         "Report":  None,
     },
-    {
-        "Name":    "Alarm autotest",
-        "Command": "alarm_autotest",
-        "Func":    default_autotest,
-        "Report":  None,
-    },
     {
         "Name":    "Malloc autotest",
         "Command": "malloc_autotest",
@@ -345,12 +333,6 @@
         "Func":    default_autotest,
         "Report":  None,
     },
-    {
-        "Name":    "Sleep delay",
-        "Command": "delay_us_sleep_autotest",
-        "Func":    default_autotest,
-        "Report":  None,
-    },
     {
         "Name":    "Rawdev autotest",
         "Command": "rawdev_autotest",
diff --git a/app/test/meson.build b/app/test/meson.build
index 08c82d3d23..0a5f425578 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -185,14 +185,12 @@ test_deps = [
 # to indicate whether it can run in no-huge mode.
 fast_tests = [
         ['acl_autotest', true],
-        ['alarm_autotest', false],
         ['atomic_autotest', false],
         ['bitops_autotest', true],
         ['byteorder_autotest', true],
         ['cmdline_autotest', true],
         ['common_autotest', true],
         ['cpuflags_autotest', true],
-        ['cycles_autotest', true],
         ['debug_autotest', true],
         ['eal_flags_c_opt_autotest', false],
         ['eal_flags_main_opt_autotest', false],
@@ -255,7 +253,6 @@ fast_tests = [
         ['user_delay_us', true],
         ['version_autotest', true],
         ['crc_autotest', true],
-        ['delay_us_sleep_autotest', true],
         ['distributor_autotest', false],
         ['eventdev_common_autotest', true],
         ['fbarray_autotest', true],
diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 951b7f07b3..b4034339b8 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -6,19 +6,10 @@
 #include <stdint.h>
 
 #include <rte_common.h>
-#include <rte_cycles.h>
-#include <rte_interrupts.h>
-#include <rte_atomic.h>
 #include <rte_alarm.h>
 
 #include "test.h"
 
-#define US_PER_MS 1000
-
-#define RTE_TEST_ALARM_TIMEOUT 10 /* ms */
-#define RTE_TEST_CHECK_PERIOD   3 /* ms */
-#define RTE_TEST_MAX_REPEAT    20
-
 static volatile int flag;
 
 static void
@@ -28,175 +19,13 @@ test_alarm_callback(void *cb_arg)
 	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
 }
 
-static rte_atomic32_t cb_count;
-
-static void
-test_multi_cb(void *arg)
-{
-	rte_atomic32_inc(&cb_count);
-	printf("In %s - arg = %p\n", __func__, arg);
-}
-
-static volatile int recursive_error = 0;
-
-static void
-test_remove_in_callback(void *arg)
-{
-	printf("In %s - arg = %p\n", __func__, arg);
-	if (rte_eal_alarm_cancel(test_remove_in_callback, arg) ||
-			rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1)) {
-		printf("Error - cancelling callback from within function succeeded!\n");
-		recursive_error = 1;
-	}
-	flag = (int)((uintptr_t)arg);
-}
-
-static volatile int flag_2;
-
-static void
-test_remove_in_callback_2(void *arg)
-{
-	if (rte_eal_alarm_cancel(test_remove_in_callback_2, arg) || rte_eal_alarm_cancel(test_remove_in_callback_2, (void *)-1)) {
-		printf("Error - cancelling callback of test_remove_in_callback_2\n");
-		return;
-	}
-	flag_2 = 1;
-}
-
-static int
-test_multi_alarms(void)
-{
-	int rm_count = 0;
-	int count = 0;
-	cb_count.cnt = 0;
-
-	printf("Expect 6 callbacks in order...\n");
-	/* add two alarms in order */
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_multi_cb, (void *)2);
-
-	/* now add in reverse order */
-	rte_eal_alarm_set(60 * US_PER_MS, test_multi_cb, (void *)6);
-	rte_eal_alarm_set(50 * US_PER_MS, test_multi_cb, (void *)5);
-	rte_eal_alarm_set(40 * US_PER_MS, test_multi_cb, (void *)4);
-	rte_eal_alarm_set(30 * US_PER_MS, test_multi_cb, (void *)3);
-
-	/* wait for expiry */
-	rte_delay_ms(65);
-	if (cb_count.cnt != 6) {
-		printf("Missing callbacks\n");
-		/* remove any callbacks that might remain */
-		rte_eal_alarm_cancel(test_multi_cb, (void *)-1);
-		return -1;
-	}
-
-	cb_count.cnt = 0;
-	printf("Expect only callbacks with args 1 and 3...\n");
-	/* Add 3 flags, then delete one */
-	rte_eal_alarm_set(30 * US_PER_MS, test_multi_cb, (void *)3);
-	rte_eal_alarm_set(20 * US_PER_MS, test_multi_cb, (void *)2);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rm_count = rte_eal_alarm_cancel(test_multi_cb, (void *)2);
-
-	rte_delay_ms(35);
-	if (cb_count.cnt != 2 || rm_count != 1) {
-		printf("Error: invalid flags count or alarm removal failure"
-				" -  flags value = %d, expected = %d\n",
-				(int)cb_count.cnt, 2);
-		/* remove any callbacks that might remain */
-		rte_eal_alarm_cancel(test_multi_cb, (void *)-1);
-		return -1;
-	}
-
-	printf("Testing adding and then removing multiple alarms\n");
-	/* finally test that no callbacks are called if we delete them all*/
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)1);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)2);
-	rte_eal_alarm_set(10 * US_PER_MS, test_multi_cb, (void *)3);
-	rm_count = rte_eal_alarm_cancel(test_alarm_callback, (void *)-1);
-	if (rm_count != 0) {
-		printf("Error removing non-existant alarm succeeded\n");
-		rte_eal_alarm_cancel(test_multi_cb, (void *) -1);
-		return -1;
-	}
-	rm_count = rte_eal_alarm_cancel(test_multi_cb, (void *) -1);
-	if (rm_count != 3) {
-		printf("Error removing all pending alarm callbacks\n");
-		return -1;
-	}
-
-	/* Test that we cannot cancel an alarm from within the callback itself
-	 * Also test that we can cancel head-of-line callbacks ok.*/
-	flag = 0;
-	recursive_error = 0;
-	rte_eal_alarm_set(10 * US_PER_MS, test_remove_in_callback, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_remove_in_callback, (void *)2);
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback, (void *)1);
-	if (rm_count != 1) {
-		printf("Error cancelling head-of-list callback\n");
-		return -1;
-	}
-	rte_delay_ms(15);
-	if (flag != 0) {
-		printf("Error, cancelling head-of-list leads to premature callback\n");
-		return -1;
-	}
-
-	while (flag != 2 && count++ < RTE_TEST_MAX_REPEAT)
-		rte_delay_ms(10);
-
-	if (flag != 2) {
-		printf("Error - expected callback not called\n");
-		rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1);
-		return -1;
-	}
-	if (recursive_error == 1)
-		return -1;
-
-	/* Check if it can cancel all for the same callback */
-	printf("Testing canceling all for the same callback\n");
-	flag_2 = 0;
-	rte_eal_alarm_set(10 * US_PER_MS, test_remove_in_callback, (void *)1);
-	rte_eal_alarm_set(20 * US_PER_MS, test_remove_in_callback_2, (void *)2);
-	rte_eal_alarm_set(30 * US_PER_MS, test_remove_in_callback_2, (void *)3);
-	rte_eal_alarm_set(40 * US_PER_MS, test_remove_in_callback, (void *)4);
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback_2, (void *)-1);
-	if (rm_count != 2) {
-		printf("Error, cannot cancel all for the same callback\n");
-		return -1;
-	}
-	rm_count = rte_eal_alarm_cancel(test_remove_in_callback, (void *)-1);
-	if (rm_count != 2) {
-		printf("Error, cannot cancel all for the same callback\n");
-		return -1;
-	}
-
-	return 0;
-}
-
 static int
 test_alarm(void)
 {
-	int count = 0;
 #ifdef RTE_EXEC_ENV_FREEBSD
 	printf("The alarm API is not supported on FreeBSD\n");
 	return 0;
 #endif
-	/* check if the callback will be called */
-	printf("check if the callback will be called\n");
-	flag = 0;
-	if (rte_eal_alarm_set(RTE_TEST_ALARM_TIMEOUT * US_PER_MS,
-			test_alarm_callback, NULL) < 0) {
-		printf("fail to set alarm callback\n");
-		return -1;
-	}
-	while (flag == 0 && count++ < RTE_TEST_MAX_REPEAT)
-		rte_delay_ms(RTE_TEST_CHECK_PERIOD);
-
-	if (flag == 0){
-		printf("Callback not called\n");
-		return -1;
-	}
 
 	/* check if it will fail to set alarm with wrong us value */
 	printf("check if it will fail to set alarm with wrong ms values\n");
@@ -213,7 +42,7 @@ test_alarm(void)
 
 	/* check if it will fail to set alarm with null callback parameter */
 	printf("check if it will fail to set alarm with null callback parameter\n");
-	if (rte_eal_alarm_set(RTE_TEST_ALARM_TIMEOUT, NULL, NULL) >= 0) {
+	if (rte_eal_alarm_set(10 /* ms */, NULL, NULL) >= 0) {
 		printf("should not be successful to set alarm with null callback parameter\n");
 		return -1;
 	}
@@ -225,9 +54,6 @@ test_alarm(void)
 		return -1;
 	}
 
-	if (test_multi_alarms() != 0)
-		return -1;
-
 	return 0;
 }
 
diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
index 97d42f3032..66d11e6db8 100644
--- a/app/test/test_cycles.c
+++ b/app/test/test_cycles.c
@@ -10,87 +10,6 @@
 
 #include "test.h"
 
-#define N 10000
-
-/*
- * Cycles test
- * ===========
- *
- * - Loop N times and check that the timer always increments and
- *   never decrements during this loop.
- *
- * - Wait one second using rte_usleep() and check that the increment
- *   of cycles is correct with regard to the frequency of the timer.
- */
-
-static int
-check_wait_one_second(void)
-{
-	uint64_t cycles, prev_cycles;
-	uint64_t hz = rte_get_timer_hz();
-	uint64_t max_inc = (hz / 100); /* 10 ms max between 2 reads */
-
-	/* check that waiting 1 second is precise */
-	prev_cycles = rte_get_timer_cycles();
-	rte_delay_us(1000000);
-	cycles = rte_get_timer_cycles();
-
-	if ((uint64_t)(cycles - prev_cycles) > (hz + max_inc)) {
-		printf("delay_us is not accurate: too long\n");
-		return -1;
-	}
-	if ((uint64_t)(cycles - prev_cycles) < (hz - max_inc)) {
-		printf("delay_us is not accurate: too short\n");
-		return -1;
-	}
-
-	return 0;
-}
-
-static int
-test_cycles(void)
-{
-	unsigned i;
-	uint64_t start_cycles, cycles, prev_cycles;
-	uint64_t hz = rte_get_timer_hz();
-	uint64_t max_inc = (hz / 100); /* 10 ms max between 2 reads */
-
-	/* check that the timer is always incrementing */
-	start_cycles = rte_get_timer_cycles();
-	prev_cycles = start_cycles;
-	for (i=0; i<N; i++) {
-		cycles = rte_get_timer_cycles();
-		if ((uint64_t)(cycles - prev_cycles) > max_inc) {
-			printf("increment too high or going backwards\n");
-			return -1;
-		}
-		prev_cycles = cycles;
-	}
-
-	return check_wait_one_second();
-}
-
-REGISTER_TEST_COMMAND(cycles_autotest, test_cycles);
-
-/*
- * One second precision test with rte_delay_us_sleep.
- */
-
-static int
-test_delay_us_sleep(void)
-{
-	int rv;
-
-	rte_delay_us_callback_register(rte_delay_us_sleep);
-	rv = check_wait_one_second();
-	/* restore original delay function */
-	rte_delay_us_callback_register(rte_delay_us_block);
-
-	return rv;
-}
-
-REGISTER_TEST_COMMAND(delay_us_sleep_autotest, test_delay_us_sleep);
-
 /*
  * rte_delay_us_callback test
  *
-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-ci] [PATCH v4] test: remove strict timing requirements from alarm and cycles tests
  2021-06-03 15:22   ` [dpdk-ci] [PATCH v4] " Aaron Conole
@ 2021-06-03 16:13     ` David Marchand
  0 siblings, 0 replies; 6+ messages in thread
From: David Marchand @ 2021-06-03 16:13 UTC (permalink / raw)
  To: Aaron Conole; +Cc: dev, ci, Michael Santana, Ilya Maximets

On Thu, Jun 3, 2021 at 5:22 PM Aaron Conole <aconole@redhat.com> wrote:
>
> The tests 'alarm_autotest' and 'cycles_autotest' rely on the underlying
> system having very accurate and precise timing.  On systems where the
> timing isn't as rigid, or the load is particularly high, these tests are
> unreliable since the wake latency from the scheduler can be high enough
> to miss the timing window.
>
> Remove the timing related tests from the test suites.  These tests now
> ensure the add/remove callback infrastructure unit tests, but drop the
> waits and reliance on system timing and load.
>
> This avoids FAIL on various testing infrastructures.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>

Applied, thanks Aaron.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-06-03 16:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25  0:16 [dpdk-ci] [PATCH] test: drop 'alarm_autotest' and 'cycles_autotest' from test suite Aaron Conole
2021-05-26 11:58 ` [dpdk-ci] [PATCH v2] test: remove strict timing requirements from alarm and cycles tests Aaron Conole
2021-05-26 13:06   ` Aaron Conole
2021-06-03 13:46 ` [dpdk-ci] [PATCH v3] " Aaron Conole
2021-06-03 15:22   ` [dpdk-ci] [PATCH v4] " Aaron Conole
2021-06-03 16:13     ` David Marchand

DPDK CI discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/ci/0 ci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ci ci/ https://inbox.dpdk.org/ci \
		ci@dpdk.org
	public-inbox-index ci

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.ci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git