[PATCH 0/5] alarm related patches

[PATCH 0/5] alarm related patches

[Stephen Hemminger]

This is the set of things which stood out as needing fixing when
looking at the rte_eal_alarm tests.

Stephen Hemminger (5):
  eal: add missing parameter check to rte_eal_alarm_set on Windows
  Revert "test/alarm: disable bad time cases on Windows"
  test: support alarm test on FreeBSD
  test/alarm: rewrite the alarm test
  eal: simplify eal alarm cancel by using LIST_FOREACH_SAFE

 app/test/test_alarm.c       | 84 +++++++++++++++++++++----------------
 lib/eal/linux/eal_alarm.c   | 51 +++++++++-------------
 lib/eal/windows/eal_alarm.c |  7 ++++
 3 files changed, 75 insertions(+), 67 deletions(-)

-- 
2.43.0

[PATCH 1/5] eal: add missing parameter check to rte_eal_alarm_set on Windows

[Stephen Hemminger]

Both Linux and FreeBSD, check parameters to rte_eal_alarm_set,
but Windows missed this. And the test was "fixed" instead.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/windows/eal_alarm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
index 052af4b21b..09a02a1cbf 100644
--- a/lib/eal/windows/eal_alarm.c
+++ b/lib/eal/windows/eal_alarm.c
@@ -92,6 +92,13 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	LARGE_INTEGER deadline;
 	int ret;
 
+	/* Check parameters, including that us won't cause a uint64_t overflow */
+	if (us < 1 || us > (UINT64_MAX - US_PER_S)) {
+		EAL_LOG(ERR, "Invalid alarm interval");
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	if (cb_fn == NULL) {
 		EAL_LOG(ERR, "NULL callback");
 		ret = -EINVAL;
-- 
2.43.0

[PATCH 2/5] Revert "test/alarm: disable bad time cases on Windows"

[Stephen Hemminger]

This reverts commit a089d320338d708f5b7126dab5fd6861c82e6347.

Windows EAL should have been fixed rather than papering over
the bug.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_alarm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 70e97a3109..b4034339b8 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -10,7 +10,6 @@
 
 #include "test.h"
 
-#ifndef RTE_EXEC_ENV_WINDOWS
 static volatile int flag;
 
 static void
@@ -19,7 +18,6 @@ test_alarm_callback(void *cb_arg)
 	flag = 1;
 	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
 }
-#endif
 
 static int
 test_alarm(void)
@@ -29,7 +27,6 @@ test_alarm(void)
 	return 0;
 #endif
 
-#ifndef RTE_EXEC_ENV_WINDOWS
 	/* 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");
 	if (rte_eal_alarm_set(0, test_alarm_callback,
@@ -42,7 +39,6 @@ test_alarm(void)
 		printf("should not be successful with (UINT64_MAX-1) us value\n");
 		return -1;
 	}
-#endif
 
 	/* 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");
-- 
2.43.0

[PATCH 3/5] test: support alarm test on FreeBSD

[Stephen Hemminger]

Yes the alarm API is supported on FreeBSD.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_alarm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index b4034339b8..0cdfad1374 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -22,11 +22,6 @@ test_alarm_callback(void *cb_arg)
 static int
 test_alarm(void)
 {
-#ifdef RTE_EXEC_ENV_FREEBSD
-	printf("The alarm API is not supported on FreeBSD\n");
-	return 0;
-#endif
-
 	/* 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");
 	if (rte_eal_alarm_set(0, test_alarm_callback,
-- 
2.43.0

[PATCH 4/5] test/alarm: rewrite the alarm test

[Stephen Hemminger]

The alarm test had several issues:
   - only negative tests were done, no positive test to make sure
     API worked
   - the set/cancel test had too short an interval and the alarm
     might expire
   - the test should have used the standard TEST macros.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_alarm.c | 77 ++++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 27 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 0cdfad1374..67ca30b1fe 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -3,53 +3,76 @@
  */
 
 #include <stdio.h>
+#include <stdbool.h>
 #include <stdint.h>
 
-#include <rte_common.h>
 #include <rte_alarm.h>
+#include <rte_atomic.h>
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_errno.h>
 
 #include "test.h"
 
-static volatile int flag;
+#ifndef US_PER_SEC
+#define US_PER_SEC	1000000u
+#endif
 
 static void
 test_alarm_callback(void *cb_arg)
 {
-	flag = 1;
-	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
+	bool *triggered = cb_arg;
+
+	rte_atomic_store_explicit(triggered, 1, rte_memory_order_release);
 }
 
+
 static int
 test_alarm(void)
 {
-	/* 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");
-	if (rte_eal_alarm_set(0, test_alarm_callback,
-						NULL) >= 0) {
-		printf("should not be successful with 0 us value\n");
-		return -1;
-	}
-	if (rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback,
-						NULL) >= 0) {
-		printf("should not be successful with (UINT64_MAX-1) us value\n");
-		return -1;
-	}
+	bool triggered = false;
+	bool later = false;
+	int ret;
+
+	/* check if it will fail to set alarm with wrong us values */
+	TEST_ASSERT_FAIL(rte_eal_alarm_set(0, test_alarm_callback, NULL),
+			 "Expected rte_eal_alarm_set to fail with 0 us value");
+
+	/* check it if will fail with a very large timeout value */
+	TEST_ASSERT_FAIL(rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback, NULL),
+			 "Expected rte_eal_alarm_set to fail with (UINT64_MAX-1) us value");
 
 	/* 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(10 /* ms */, NULL, NULL) >= 0) {
-		printf("should not be successful to set alarm with null callback parameter\n");
-		return -1;
-	}
+	TEST_ASSERT_FAIL(rte_eal_alarm_set(US_PER_SEC, NULL, NULL),
+			 "Expected rte_eal_alarm_set to fail with null callback parameter");
 
 	/* check if it will fail to remove alarm with null callback parameter */
-	printf("check if it will fail to remove alarm with null callback parameter\n");
-	if (rte_eal_alarm_cancel(NULL, NULL) == 0) {
-		printf("should not be successful to remove alarm with null callback parameter");
-		return -1;
-	}
+	TEST_ASSERT_FAIL(rte_eal_alarm_cancel(NULL, NULL),
+			 "Expected rte_eal_alarm_cancel to fail with null callback parameter");
+
+	/* check if can set a alarm for one second */
+	TEST_ASSERT_SUCCESS(rte_eal_alarm_set(US_PER_SEC, test_alarm_callback, &triggered),
+			    "Setting one second alarm failed");
+
+	/* set a longer alarm that will be canceled. */
+	TEST_ASSERT_SUCCESS(rte_eal_alarm_set(10 * US_PER_SEC, test_alarm_callback, &later),
+			    "Setting ten second alarm failed");
+
+	/* wait for alarm to happen */
+	while (rte_atomic_load_explicit(&triggered, rte_memory_order_acquire) == false)
+		rte_delay_us_sleep(US_PER_SEC / 10);
+
+	TEST_ASSERT(!rte_atomic_load_explicit(&later, rte_memory_order_acquire),
+		    "Only one alarm should have fired.");
+
+	ret = rte_eal_alarm_cancel(test_alarm_callback, &triggered);
+	TEST_ASSERT(ret == 0 && rte_errno == ENOENT,
+		    "Canceling alarm after run ret %d: %s", ret, rte_strerror(rte_errno));
+
+	ret = rte_eal_alarm_cancel(test_alarm_callback, &later);
+	TEST_ASSERT(ret == 1, "Canceling ten second alarm failed %d: %s", ret, rte_strerror(rte_errno));
 
 	return 0;
 }
 
-REGISTER_TEST_COMMAND(alarm_autotest, test_alarm);
+REGISTER_FAST_TEST(alarm_autotest, true, true, test_alarm);
-- 
2.43.0

[PATCH 5/5] eal: simplify eal alarm cancel by using LIST_FOREACH_SAFE

[Stephen Hemminger]

The code for rte_eal_alarm_cancel was using two loops and duplicate
code to handle cancel at the start of the list. Introduce the
LIST_FOREACH_SAFE() macro from FreeBsd which makes writing
the loop cleaner and simpler.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/linux/eal_alarm.c | 51 +++++++++++++++------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/lib/eal/linux/eal_alarm.c b/lib/eal/linux/eal_alarm.c
index eeb096213b..3f3b722421 100644
--- a/lib/eal/linux/eal_alarm.c
+++ b/lib/eal/linux/eal_alarm.c
@@ -53,6 +53,13 @@ static struct rte_intr_handle *intr_handle;
 static int handler_registered = 0;
 static void eal_alarm_callback(void *arg);
 
+#ifndef LIST_FOREACH_SAFE
+#define LIST_FOREACH_SAFE(var, head, field, tvar)			\
+	for ((var) = LIST_FIRST((head));				\
+	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
 void
 rte_eal_alarm_cleanup(void)
 {
@@ -194,7 +201,7 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 int
 rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 {
-	struct alarm_entry *ap, *ap_prev;
+	struct alarm_entry *ap, *tmp;
 	int count = 0;
 	int err = 0;
 	int executing;
@@ -207,46 +214,26 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 	do {
 		executing = 0;
 		rte_spinlock_lock(&alarm_list_lk);
-		/* remove any matches at the start of the list */
-		while ((ap = LIST_FIRST(&alarm_list)) != NULL &&
-				cb_fn == ap->cb_fn &&
-				(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {
-
-			if (ap->executing == 0) {
-				LIST_REMOVE(ap, next);
-				free(ap);
-				count++;
-			} else {
-				/* If calling from other context, mark that alarm is executing
-				 * so loop can spin till it finish. Otherwise we are trying to
-				 * cancel our self - mark it by EINPROGRESS */
-				if (pthread_equal(ap->executing_id, pthread_self()) == 0)
-					executing++;
-				else
-					err = EINPROGRESS;
-
-				break;
-			}
-		}
-		ap_prev = ap;
 
-		/* now go through list, removing entries not at start */
-		LIST_FOREACH(ap, &alarm_list, next) {
+		LIST_FOREACH_SAFE(ap, &alarm_list, next, tmp) {
 			/* this won't be true first time through */
 			if (cb_fn == ap->cb_fn &&
 					(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {
-
 				if (ap->executing == 0) {
 					LIST_REMOVE(ap, next);
 					free(ap);
 					count++;
-					ap = ap_prev;
-				} else if (pthread_equal(ap->executing_id, pthread_self()) == 0)
-					executing++;
-				else
-					err = EINPROGRESS;
+				} else {
+					/* If calling from other context, mark that alarm is executing
+					 * so loop can spin till it finish. Otherwise we are trying to
+					 * cancel our self - mark it by EINPROGRESS
+					 */
+					if (pthread_equal(ap->executing_id, pthread_self()) == 0)
+						executing++;
+					else
+						err = EINPROGRESS;
+				}
 			}
-			ap_prev = ap;
 		}
 		rte_spinlock_unlock(&alarm_list_lk);
 	} while (executing != 0);
-- 
2.43.0

Re: [PATCH 2/5] Revert "test/alarm: disable bad time cases on Windows"

[Dmitry Kozlyuk]

2024-08-08 12:46 (UTC-0700), Stephen Hemminger:
> This reverts commit a089d320338d708f5b7126dab5fd6861c82e6347.
> 
> Windows EAL should have been fixed rather than papering over
> the bug.

Linux and FreeBSD alarm implementations use the same approach
that limits possible timeout range in API.
Test cases in question check that these values are rejected.
Windows EAL can accept any values.
I think the proper fix would be documenting Unix limitations
at API level (it never worked the other way, so no real breakage),
then adding the same checks to Windows EAL only for consistency.

Re: [PATCH 2/5] Revert "test/alarm: disable bad time cases on Windows"

[Dmitry Kozlyuk]

2024-08-09 10:23 (UTC+0300), Dmitry Kozlyuk:
> 2024-08-08 12:46 (UTC-0700), Stephen Hemminger:
> > This reverts commit a089d320338d708f5b7126dab5fd6861c82e6347.
> > 
> > Windows EAL should have been fixed rather than papering over
> > the bug.  
> 
> Linux and FreeBSD alarm implementations use the same approach
> that limits possible timeout range in API.
> Test cases in question check that these values are rejected.
> Windows EAL can accept any values.
> I think the proper fix would be documenting Unix limitations
> at API level (it never worked the other way, so no real breakage),
> then adding the same checks to Windows EAL only for consistency.

Oops, I got patch 1/5 in email later somehow. It is doing just that.
Sorry for the noise.

RE: [PATCH 5/5] eal: simplify eal alarm cancel by using LIST_FOREACH_SAFE

[Morten Brørup]

> +++ b/lib/eal/linux/eal_alarm.c
> @@ -53,6 +53,13 @@ static struct rte_intr_handle *intr_handle;
>  static int handler_registered = 0;
>  static void eal_alarm_callback(void *arg);
> 
> +#ifndef LIST_FOREACH_SAFE
> +#define LIST_FOREACH_SAFE(var, head, field, tvar)			\
> +	for ((var) = LIST_FIRST((head));				\
> +	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
> +	    (var) = (tvar))
> +#endif

This macro is already defined for Windows [1]; isn't it also defined in some Linux/BSD standard header file already?

[1]: https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/windows/include/sys/queue.h#L515

Re: [PATCH 2/5] Revert "test/alarm: disable bad time cases on Windows"

[Stephen Hemminger]

On Fri, 9 Aug 2024 10:23:24 +0300
Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:

> 2024-08-08 12:46 (UTC-0700), Stephen Hemminger:
> > This reverts commit a089d320338d708f5b7126dab5fd6861c82e6347.
> > 
> > Windows EAL should have been fixed rather than papering over
> > the bug.  
> 
> Linux and FreeBSD alarm implementations use the same approach
> that limits possible timeout range in API.
> Test cases in question check that these values are rejected.
> Windows EAL can accept any values.

That was a bug. There should not be different behavior
on different OS.

> I think the proper fix would be documenting Unix limitations
> at API level (it never worked the other way, so no real breakage),
> then adding the same checks to Windows EAL only for consistency.

No. All EAL should behave the same.

Re: [PATCH 5/5] eal: simplify eal alarm cancel by using LIST_FOREACH_SAFE

[Stephen Hemminger]

On Fri, 9 Aug 2024 10:33:49 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > +++ b/lib/eal/linux/eal_alarm.c
> > @@ -53,6 +53,13 @@ static struct rte_intr_handle *intr_handle;
> >  static int handler_registered = 0;
> >  static void eal_alarm_callback(void *arg);
> > 
> > +#ifndef LIST_FOREACH_SAFE
> > +#define LIST_FOREACH_SAFE(var, head, field, tvar)			\
> > +	for ((var) = LIST_FIRST((head));				\
> > +	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
> > +	    (var) = (tvar))
> > +#endif  
> 
> This macro is already defined for Windows [1]; isn't it also defined in some Linux/BSD standard header file already?
> 
> [1]: https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/windows/include/sys/queue.h#L515
> 

Unfortunately, sys/queue.h on Linux is stuck on some old version.
There maybe more complete version on some libbsd headers but don't want
to get into having that dependency.

[PATCH v2 0/3] alarm test fixes

[Stephen Hemminger]

This is the set of things which stood out as needing fixing when
looking at the rte_eal_alarm tests.

v2 - drop cleanup of eal alarm code for later
   - add RTE_ATOMIC() to fix clang build

Stephen Hemminger (3):
  eal: add missing parameter check to rte_eal_alarm_set on Windows
  test: support alarm test on FreeBSD
  test/alarm: rewrite the alarm test

 app/test/test_alarm.c       | 85 ++++++++++++++++++++++---------------
 lib/eal/windows/eal_alarm.c |  7 +++
 2 files changed, 57 insertions(+), 35 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] eal: add missing parameter check to rte_eal_alarm_set on Windows

[Stephen Hemminger]

Both Linux and FreeBSD, check parameters to rte_eal_alarm_set,
but Windows missed this. And the test was "fixed" instead.

This reverts commit a089d320338d708f5b7126dab5fd6861c82e6347.

Windows EAL should have been fixed rather than papering over
the bug.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_alarm.c       | 4 ----
 lib/eal/windows/eal_alarm.c | 7 +++++++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 70e97a3109..b4034339b8 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -10,7 +10,6 @@
 
 #include "test.h"
 
-#ifndef RTE_EXEC_ENV_WINDOWS
 static volatile int flag;
 
 static void
@@ -19,7 +18,6 @@ test_alarm_callback(void *cb_arg)
 	flag = 1;
 	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
 }
-#endif
 
 static int
 test_alarm(void)
@@ -29,7 +27,6 @@ test_alarm(void)
 	return 0;
 #endif
 
-#ifndef RTE_EXEC_ENV_WINDOWS
 	/* 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");
 	if (rte_eal_alarm_set(0, test_alarm_callback,
@@ -42,7 +39,6 @@ test_alarm(void)
 		printf("should not be successful with (UINT64_MAX-1) us value\n");
 		return -1;
 	}
-#endif
 
 	/* 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");
diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
index 052af4b21b..09a02a1cbf 100644
--- a/lib/eal/windows/eal_alarm.c
+++ b/lib/eal/windows/eal_alarm.c
@@ -92,6 +92,13 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	LARGE_INTEGER deadline;
 	int ret;
 
+	/* Check parameters, including that us won't cause a uint64_t overflow */
+	if (us < 1 || us > (UINT64_MAX - US_PER_S)) {
+		EAL_LOG(ERR, "Invalid alarm interval");
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	if (cb_fn == NULL) {
 		EAL_LOG(ERR, "NULL callback");
 		ret = -EINVAL;
-- 
2.43.0

[PATCH v2 2/3] test: support alarm test on FreeBSD

[Stephen Hemminger]

Yes the alarm API is supported on FreeBSD.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_alarm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index b4034339b8..0cdfad1374 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -22,11 +22,6 @@ test_alarm_callback(void *cb_arg)
 static int
 test_alarm(void)
 {
-#ifdef RTE_EXEC_ENV_FREEBSD
-	printf("The alarm API is not supported on FreeBSD\n");
-	return 0;
-#endif
-
 	/* 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");
 	if (rte_eal_alarm_set(0, test_alarm_callback,
-- 
2.43.0

[PATCH v2 3/3] test/alarm: rewrite the alarm test

[Stephen Hemminger]

The alarm test had several issues:
   - only negative tests were done, no positive test to make sure
     API worked
   - the set/cancel test had too short an interval and the alarm
     might expire
   - the test should have used the standard TEST macros.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_alarm.c | 78 ++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 0cdfad1374..9ed8c6f72c 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -3,53 +3,77 @@
  */
 
 #include <stdio.h>
+#include <stdbool.h>
 #include <stdint.h>
 
-#include <rte_common.h>
 #include <rte_alarm.h>
+#include <rte_atomic.h>
+#include <rte_common.h>
+#include <rte_cycles.h>
+#include <rte_errno.h>
 
 #include "test.h"
 
-static volatile int flag;
+#ifndef US_PER_SEC
+#define US_PER_SEC	1000000u
+#endif
 
 static void
 test_alarm_callback(void *cb_arg)
 {
-	flag = 1;
-	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
+	RTE_ATOMIC(bool) *triggered = cb_arg;
+
+	rte_atomic_store_explicit(triggered, 1, rte_memory_order_release);
 }
 
+
 static int
 test_alarm(void)
 {
-	/* 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");
-	if (rte_eal_alarm_set(0, test_alarm_callback,
-						NULL) >= 0) {
-		printf("should not be successful with 0 us value\n");
-		return -1;
-	}
-	if (rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback,
-						NULL) >= 0) {
-		printf("should not be successful with (UINT64_MAX-1) us value\n");
-		return -1;
-	}
+	RTE_ATOMIC(bool) triggered = false;
+	RTE_ATOMIC(bool) later = false;
+	int ret;
+
+	/* check if it will fail to set alarm with wrong us values */
+	TEST_ASSERT_FAIL(rte_eal_alarm_set(0, test_alarm_callback, NULL),
+			 "Expected rte_eal_alarm_set to fail with 0 us value");
+
+	/* check it if will fail with a very large timeout value */
+	TEST_ASSERT_FAIL(rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback, NULL),
+			 "Expected rte_eal_alarm_set to fail with (UINT64_MAX-1) us value");
 
 	/* 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(10 /* ms */, NULL, NULL) >= 0) {
-		printf("should not be successful to set alarm with null callback parameter\n");
-		return -1;
-	}
+	TEST_ASSERT_FAIL(rte_eal_alarm_set(US_PER_SEC, NULL, NULL),
+			 "Expected rte_eal_alarm_set to fail with null callback parameter");
 
 	/* check if it will fail to remove alarm with null callback parameter */
-	printf("check if it will fail to remove alarm with null callback parameter\n");
-	if (rte_eal_alarm_cancel(NULL, NULL) == 0) {
-		printf("should not be successful to remove alarm with null callback parameter");
-		return -1;
-	}
+	TEST_ASSERT_FAIL(rte_eal_alarm_cancel(NULL, NULL),
+			 "Expected rte_eal_alarm_cancel to fail with null callback parameter");
+
+	/* check if can set a alarm for one second */
+	TEST_ASSERT_SUCCESS(rte_eal_alarm_set(US_PER_SEC, test_alarm_callback, &triggered),
+			    "Setting one second alarm failed");
+
+	/* set a longer alarm that will be canceled. */
+	TEST_ASSERT_SUCCESS(rte_eal_alarm_set(10 * US_PER_SEC, test_alarm_callback, &later),
+			    "Setting ten second alarm failed");
+
+	/* wait for alarm to happen */
+	while (rte_atomic_load_explicit(&triggered, rte_memory_order_acquire) == false)
+		rte_delay_us_sleep(US_PER_SEC / 10);
+
+	TEST_ASSERT(!rte_atomic_load_explicit(&later, rte_memory_order_acquire),
+		    "Only one alarm should have fired.");
+
+	ret = rte_eal_alarm_cancel(test_alarm_callback, &triggered);
+	TEST_ASSERT(ret == 0 && rte_errno == ENOENT,
+		    "Canceling alarm after run ret %d: %s", ret, rte_strerror(rte_errno));
+
+	ret = rte_eal_alarm_cancel(test_alarm_callback, &later);
+	TEST_ASSERT(ret == 1, "Canceling ten second alarm failed %d: %s",
+		    ret, rte_strerror(rte_errno));
 
 	return 0;
 }
 
-REGISTER_TEST_COMMAND(alarm_autotest, test_alarm);
+REGISTER_FAST_TEST(alarm_autotest, true, true, test_alarm);
-- 
2.43.0

Re: [PATCH v2 0/3] alarm test fixes

[fengchengwen]

LGTM
Series-acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2024/8/9 23:24, Stephen Hemminger wrote:
> This is the set of things which stood out as needing fixing when
> looking at the rte_eal_alarm tests.
> 
> v2 - drop cleanup of eal alarm code for later
>    - add RTE_ATOMIC() to fix clang build
> 
> Stephen Hemminger (3):
>   eal: add missing parameter check to rte_eal_alarm_set on Windows
>   test: support alarm test on FreeBSD
>   test/alarm: rewrite the alarm test
> 
>  app/test/test_alarm.c       | 85 ++++++++++++++++++++++---------------
>  lib/eal/windows/eal_alarm.c |  7 +++
>  2 files changed, 57 insertions(+), 35 deletions(-)
> 

Re: [PATCH v2 0/3] alarm test fixes

[David Marchand]

On Fri, Aug 9, 2024 at 5:26 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> This is the set of things which stood out as needing fixing when
> looking at the rte_eal_alarm tests.
>
> v2 - drop cleanup of eal alarm code for later
>    - add RTE_ATOMIC() to fix clang build
>
> Stephen Hemminger (3):
>   eal: add missing parameter check to rte_eal_alarm_set on Windows
>   test: support alarm test on FreeBSD
>   test/alarm: rewrite the alarm test
>
>  app/test/test_alarm.c       | 85 ++++++++++++++++++++++---------------
>  lib/eal/windows/eal_alarm.c |  7 +++
>  2 files changed, 57 insertions(+), 35 deletions(-)

Thanks for the cleanup, and reviews.
Series applied.

-- 
David Marchand