diff mbox series

[v2,03/12] test/timer: use compiler atomic builtins for sync

Message ID 20211116094205.750359-4-joyce.kong@arm.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers show
Series use compiler atomic builtins for app modules | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Joyce Kong Nov. 16, 2021, 9:41 a.m. UTC
Convert rte_atomic usages to compiler atomic
built-ins for lcore_state and collisions sync.

Also, move 'main_init_workers' outside of
'timer_stress2_main_loop' to guarantee lcore_state
initialized correctly before the threads launched.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test/test_timer.c           | 30 +++++++++++++-----------------
 app/test/test_timer_secondary.c |  1 -
 2 files changed, 13 insertions(+), 18 deletions(-)

Comments

Honnappa Nagarahalli Nov. 16, 2021, 7:52 p.m. UTC | #1
<snip>

> 
> Convert rte_atomic usages to compiler atomic built-ins for lcore_state and
> collisions sync.
> 
> Also, move 'main_init_workers' outside of 'timer_stress2_main_loop' to
> guarantee lcore_state initialized correctly before the threads launched.
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> ---
>  app/test/test_timer.c           | 30 +++++++++++++-----------------
>  app/test/test_timer_secondary.c |  1 -
>  2 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/app/test/test_timer.c b/app/test/test_timer.c index
> a10b2fe9da..c97e5c891c 100644
> --- a/app/test/test_timer.c
> +++ b/app/test/test_timer.c
> @@ -102,7 +102,6 @@
>  #include <rte_eal.h>
>  #include <rte_per_lcore.h>
>  #include <rte_lcore.h>
> -#include <rte_atomic.h>
>  #include <rte_timer.h>
>  #include <rte_random.h>
>  #include <rte_malloc.h>
> @@ -203,7 +202,7 @@ timer_stress_main_loop(__rte_unused void *arg)
> 
>  /* Need to synchronize worker lcores through multiple steps. */  enum {
> WORKER_WAITING = 1, WORKER_RUN_SIGNAL, WORKER_RUNNING,
> WORKER_FINISHED }; -static rte_atomic16_t lcore_state[RTE_MAX_LCORE];
> +static uint16_t lcore_state[RTE_MAX_LCORE];
> 
>  static void
>  main_init_workers(void)
> @@ -211,7 +210,7 @@ main_init_workers(void)
>  	unsigned i;
> 
>  	RTE_LCORE_FOREACH_WORKER(i) {
> -		rte_atomic16_set(&lcore_state[i], WORKER_WAITING);
> +		__atomic_store_n(&lcore_state[i], WORKER_WAITING,
> __ATOMIC_RELAXED);
>  	}
>  }
> 
> @@ -221,11 +220,10 @@ main_start_workers(void)
>  	unsigned i;
> 
>  	RTE_LCORE_FOREACH_WORKER(i) {
> -		rte_atomic16_set(&lcore_state[i], WORKER_RUN_SIGNAL);
> +		__atomic_store_n(&lcore_state[i], WORKER_RUN_SIGNAL,
> +__ATOMIC_RELEASE);
>  	}
>  	RTE_LCORE_FOREACH_WORKER(i) {
> -		while (rte_atomic16_read(&lcore_state[i]) !=
> WORKER_RUNNING)
> -			rte_pause();
> +		rte_wait_until_equal_16(&lcore_state[i], WORKER_RUNNING,
> +__ATOMIC_ACQUIRE);
>  	}
>  }
> 
> @@ -235,8 +233,7 @@ main_wait_for_workers(void)
>  	unsigned i;
> 
>  	RTE_LCORE_FOREACH_WORKER(i) {
> -		while (rte_atomic16_read(&lcore_state[i]) !=
> WORKER_FINISHED)
> -			rte_pause();
> +		rte_wait_until_equal_16(&lcore_state[i], WORKER_FINISHED,
> +__ATOMIC_ACQUIRE);
>  	}
>  }
> 
> @@ -245,9 +242,8 @@ worker_wait_to_start(void)  {
>  	unsigned lcore_id = rte_lcore_id();
> 
> -	while (rte_atomic16_read(&lcore_state[lcore_id]) !=
> WORKER_RUN_SIGNAL)
> -		rte_pause();
> -	rte_atomic16_set(&lcore_state[lcore_id], WORKER_RUNNING);
> +	rte_wait_until_equal_16(&lcore_state[lcore_id],
> WORKER_RUN_SIGNAL, __ATOMIC_ACQUIRE);
> +	__atomic_store_n(&lcore_state[lcore_id], WORKER_RUNNING,
> +__ATOMIC_RELEASE);
>  }
> 
>  static void
> @@ -255,7 +251,7 @@ worker_finish(void)
>  {
>  	unsigned lcore_id = rte_lcore_id();
> 
> -	rte_atomic16_set(&lcore_state[lcore_id], WORKER_FINISHED);
> +	__atomic_store_n(&lcore_state[lcore_id], WORKER_FINISHED,
> +__ATOMIC_RELEASE);
>  }
> 
> 
> @@ -281,13 +277,12 @@ timer_stress2_main_loop(__rte_unused void *arg)
>  	unsigned int lcore_id = rte_lcore_id();
>  	unsigned int main_lcore = rte_get_main_lcore();
>  	int32_t my_collisions = 0;
> -	static rte_atomic32_t collisions;
> +	static uint32_t collisions;
> 
>  	if (lcore_id == main_lcore) {
>  		cb_count = 0;
>  		test_failed = 0;
> -		rte_atomic32_set(&collisions, 0);
> -		main_init_workers();
> +		__atomic_store_n(&collisions, 0, __ATOMIC_RELAXED);
>  		timers = rte_malloc(NULL, sizeof(*timers) *
> NB_STRESS2_TIMERS, 0);
>  		if (timers == NULL) {
>  			printf("Test Failed\n");
> @@ -315,7 +310,7 @@ timer_stress2_main_loop(__rte_unused void *arg)
>  			my_collisions++;
>  	}
>  	if (my_collisions != 0)
> -		rte_atomic32_add(&collisions, my_collisions);
> +		__atomic_fetch_add(&collisions, my_collisions,
> __ATOMIC_RELAXED);
> 
>  	/* wait long enough for timers to expire */
>  	rte_delay_ms(100);
> @@ -329,7 +324,7 @@ timer_stress2_main_loop(__rte_unused void *arg)
> 
>  	/* now check that we get the right number of callbacks */
>  	if (lcore_id == main_lcore) {
> -		my_collisions = rte_atomic32_read(&collisions);
> +		my_collisions = __atomic_load_n(&collisions,
> __ATOMIC_RELAXED);
>  		if (my_collisions != 0)
>  			printf("- %d timer reset collisions (OK)\n",
> my_collisions);
>  		rte_timer_manage();
> @@ -573,6 +568,7 @@ test_timer(void)
>  	/* run a second, slightly different set of stress tests */
>  	printf("\nStart timer stress tests 2\n");
>  	test_failed = 0;
> +	main_init_workers();
>  	rte_eal_mp_remote_launch(timer_stress2_main_loop, NULL,
> CALL_MAIN);
>  	rte_eal_mp_wait_lcore();
>  	if (test_failed)
> diff --git a/app/test/test_timer_secondary.c b/app/test/test_timer_secondary.c
> index 16a9f1878b..5795c97f07 100644
> --- a/app/test/test_timer_secondary.c
> +++ b/app/test/test_timer_secondary.c
> @@ -9,7 +9,6 @@
>  #include <rte_lcore.h>
>  #include <rte_debug.h>
>  #include <rte_memzone.h>
> -#include <rte_atomic.h>
>  #include <rte_timer.h>
>  #include <rte_cycles.h>
>  #include <rte_mempool.h>
> --
> 2.25.1
David Marchand Nov. 16, 2021, 8:20 p.m. UTC | #2
Joyce, Honnappa,

On Tue, Nov 16, 2021 at 10:43 AM Joyce Kong <joyce.kong@arm.com> wrote:
>
> Convert rte_atomic usages to compiler atomic
> built-ins for lcore_state and collisions sync.
>
> Also, move 'main_init_workers' outside of
> 'timer_stress2_main_loop' to guarantee lcore_state
> initialized correctly before the threads launched.

Is this "also" part actually related to the change?
Or is it a separate fix?


>
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Honnappa Nagarahalli Nov. 16, 2021, 9:21 p.m. UTC | #3
<snip>

> 
> Joyce, Honnappa,
> 
> On Tue, Nov 16, 2021 at 10:43 AM Joyce Kong <joyce.kong@arm.com> wrote:
> >
> > Convert rte_atomic usages to compiler atomic built-ins for lcore_state
> > and collisions sync.
> >
> > Also, move 'main_init_workers' outside of 'timer_stress2_main_loop' to
> > guarantee lcore_state initialized correctly before the threads
> > launched.
> 
> Is this "also" part actually related to the change?
> Or is it a separate fix?
'Also' part is not fixing a different problem (i.e. the code earlier was not having any issues). This 'also' part just helps to keep the code simple.

> 
> 
> >
> > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> 
> 
> 
> --
> David Marchand
David Marchand Nov. 17, 2021, 9:29 a.m. UTC | #4
On Tue, Nov 16, 2021 at 10:21 PM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
> > Joyce, Honnappa,
> >
> > On Tue, Nov 16, 2021 at 10:43 AM Joyce Kong <joyce.kong@arm.com> wrote:
> > >
> > > Convert rte_atomic usages to compiler atomic built-ins for lcore_state
> > > and collisions sync.
> > >
> > > Also, move 'main_init_workers' outside of 'timer_stress2_main_loop' to
> > > guarantee lcore_state initialized correctly before the threads
> > > launched.
> >
> > Is this "also" part actually related to the change?
> > Or is it a separate fix?
> 'Also' part is not fixing a different problem (i.e. the code earlier was not having any issues). This 'also' part just helps to keep the code simple.

This is indeed better this way.
Thanks.
diff mbox series

Patch

diff --git a/app/test/test_timer.c b/app/test/test_timer.c
index a10b2fe9da..c97e5c891c 100644
--- a/app/test/test_timer.c
+++ b/app/test/test_timer.c
@@ -102,7 +102,6 @@ 
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_timer.h>
 #include <rte_random.h>
 #include <rte_malloc.h>
@@ -203,7 +202,7 @@  timer_stress_main_loop(__rte_unused void *arg)
 
 /* Need to synchronize worker lcores through multiple steps. */
 enum { WORKER_WAITING = 1, WORKER_RUN_SIGNAL, WORKER_RUNNING, WORKER_FINISHED };
-static rte_atomic16_t lcore_state[RTE_MAX_LCORE];
+static uint16_t lcore_state[RTE_MAX_LCORE];
 
 static void
 main_init_workers(void)
@@ -211,7 +210,7 @@  main_init_workers(void)
 	unsigned i;
 
 	RTE_LCORE_FOREACH_WORKER(i) {
-		rte_atomic16_set(&lcore_state[i], WORKER_WAITING);
+		__atomic_store_n(&lcore_state[i], WORKER_WAITING, __ATOMIC_RELAXED);
 	}
 }
 
@@ -221,11 +220,10 @@  main_start_workers(void)
 	unsigned i;
 
 	RTE_LCORE_FOREACH_WORKER(i) {
-		rte_atomic16_set(&lcore_state[i], WORKER_RUN_SIGNAL);
+		__atomic_store_n(&lcore_state[i], WORKER_RUN_SIGNAL, __ATOMIC_RELEASE);
 	}
 	RTE_LCORE_FOREACH_WORKER(i) {
-		while (rte_atomic16_read(&lcore_state[i]) != WORKER_RUNNING)
-			rte_pause();
+		rte_wait_until_equal_16(&lcore_state[i], WORKER_RUNNING, __ATOMIC_ACQUIRE);
 	}
 }
 
@@ -235,8 +233,7 @@  main_wait_for_workers(void)
 	unsigned i;
 
 	RTE_LCORE_FOREACH_WORKER(i) {
-		while (rte_atomic16_read(&lcore_state[i]) != WORKER_FINISHED)
-			rte_pause();
+		rte_wait_until_equal_16(&lcore_state[i], WORKER_FINISHED, __ATOMIC_ACQUIRE);
 	}
 }
 
@@ -245,9 +242,8 @@  worker_wait_to_start(void)
 {
 	unsigned lcore_id = rte_lcore_id();
 
-	while (rte_atomic16_read(&lcore_state[lcore_id]) != WORKER_RUN_SIGNAL)
-		rte_pause();
-	rte_atomic16_set(&lcore_state[lcore_id], WORKER_RUNNING);
+	rte_wait_until_equal_16(&lcore_state[lcore_id], WORKER_RUN_SIGNAL, __ATOMIC_ACQUIRE);
+	__atomic_store_n(&lcore_state[lcore_id], WORKER_RUNNING, __ATOMIC_RELEASE);
 }
 
 static void
@@ -255,7 +251,7 @@  worker_finish(void)
 {
 	unsigned lcore_id = rte_lcore_id();
 
-	rte_atomic16_set(&lcore_state[lcore_id], WORKER_FINISHED);
+	__atomic_store_n(&lcore_state[lcore_id], WORKER_FINISHED, __ATOMIC_RELEASE);
 }
 
 
@@ -281,13 +277,12 @@  timer_stress2_main_loop(__rte_unused void *arg)
 	unsigned int lcore_id = rte_lcore_id();
 	unsigned int main_lcore = rte_get_main_lcore();
 	int32_t my_collisions = 0;
-	static rte_atomic32_t collisions;
+	static uint32_t collisions;
 
 	if (lcore_id == main_lcore) {
 		cb_count = 0;
 		test_failed = 0;
-		rte_atomic32_set(&collisions, 0);
-		main_init_workers();
+		__atomic_store_n(&collisions, 0, __ATOMIC_RELAXED);
 		timers = rte_malloc(NULL, sizeof(*timers) * NB_STRESS2_TIMERS, 0);
 		if (timers == NULL) {
 			printf("Test Failed\n");
@@ -315,7 +310,7 @@  timer_stress2_main_loop(__rte_unused void *arg)
 			my_collisions++;
 	}
 	if (my_collisions != 0)
-		rte_atomic32_add(&collisions, my_collisions);
+		__atomic_fetch_add(&collisions, my_collisions, __ATOMIC_RELAXED);
 
 	/* wait long enough for timers to expire */
 	rte_delay_ms(100);
@@ -329,7 +324,7 @@  timer_stress2_main_loop(__rte_unused void *arg)
 
 	/* now check that we get the right number of callbacks */
 	if (lcore_id == main_lcore) {
-		my_collisions = rte_atomic32_read(&collisions);
+		my_collisions = __atomic_load_n(&collisions, __ATOMIC_RELAXED);
 		if (my_collisions != 0)
 			printf("- %d timer reset collisions (OK)\n", my_collisions);
 		rte_timer_manage();
@@ -573,6 +568,7 @@  test_timer(void)
 	/* run a second, slightly different set of stress tests */
 	printf("\nStart timer stress tests 2\n");
 	test_failed = 0;
+	main_init_workers();
 	rte_eal_mp_remote_launch(timer_stress2_main_loop, NULL, CALL_MAIN);
 	rte_eal_mp_wait_lcore();
 	if (test_failed)
diff --git a/app/test/test_timer_secondary.c b/app/test/test_timer_secondary.c
index 16a9f1878b..5795c97f07 100644
--- a/app/test/test_timer_secondary.c
+++ b/app/test/test_timer_secondary.c
@@ -9,7 +9,6 @@ 
 #include <rte_lcore.h>
 #include <rte_debug.h>
 #include <rte_memzone.h>
-#include <rte_atomic.h>
 #include <rte_timer.h>
 #include <rte_cycles.h>
 #include <rte_mempool.h>