| 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 |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
<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
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>
<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
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 --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>