[dpdk-dev] [RFC 0/5] Use correct memory ordering in eal functions

[dpdk-dev] [RFC 0/5] Use correct memory ordering in eal functions

[Honnappa Nagarahalli]

rte_eal_remote_launch and rte_eal_wait_lcore need to provide
correct memory ordering to address the data communication from
main core to worker core and vice versa.

There are 2 use cases:
1) All the store operations (meant for worker) by main core
should be visible to worker core before the worker core runs the
assigned function

2) All the store operations by the worker core should be visible
to the main core after rte_eal_wait_lcore returns.

For the data that needs to be communicated to the worker after
the rte_eal_remote_launch returns, load-acquire and store-release
semantics should be used (just like any other writer-reader use case).

For the main to worker communication, the pointer to function
to execute is used as the guard variable. Hence, resetting of
the function pointer is important.

For the worker to main communication, the existing code uses the
lcore state as the guard variable. However, it looks like
the FINISHED state is not really required. Hence the FINISHED state
is removed before using the state as the guard variable. I would like
some feedback on why the FINISHED state is required. I have not
paid attention to what it means for backward compatibility.
If it is decided to remove this state, documentation changes are
required.


Honnappa Nagarahalli (5):
  eal: reset lcore function pointer and argument
  eal: ensure memory operations are visible to worker
  eal: lcore state FINISHED is not required
  eal: ensure memory operations are visible to main
  test/ring: use relaxed barriers for ring stress test

 app/test/test_ring_stress_impl.h              | 18 ++++-----
 drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-
 drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-
 drivers/event/sw/sw_evdev_selftest.c          |  4 +-
 examples/l2fwd-keepalive/main.c               |  2 +-
 lib/librte_eal/common/eal_common_launch.c     | 13 +++---
 lib/librte_eal/freebsd/eal_thread.c           | 31 +++++++++++---
 lib/librte_eal/linux/eal_thread.c             | 40 +++++++++++++------
 lib/librte_eal/windows/eal_thread.c           | 34 +++++++++++-----
 9 files changed, 94 insertions(+), 52 deletions(-)

-- 
2.17.1

[dpdk-dev] [RFC 1/5] eal: reset lcore function pointer and argument

[Honnappa Nagarahalli]

In the rte_eal_remote_launch function, the lcore function
pointer is checked for NULL. However, the pointer is never
reset to NULL. Reset the lcore function pointer and argument
after the worker has completed executing the lcore function.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/librte_eal/freebsd/eal_thread.c | 2 ++
 lib/librte_eal/linux/eal_thread.c   | 2 ++
 lib/librte_eal/windows/eal_thread.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/lib/librte_eal/freebsd/eal_thread.c b/lib/librte_eal/freebsd/eal_thread.c
index 1dce9b04f..bbc3a8e98 100644
--- a/lib/librte_eal/freebsd/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal_thread.c
@@ -126,6 +126,8 @@ eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 		lcore_config[lcore_id].state = FINISHED;
 	}
diff --git a/lib/librte_eal/linux/eal_thread.c b/lib/librte_eal/linux/eal_thread.c
index 83c2034b9..8f3c0dafd 100644
--- a/lib/librte_eal/linux/eal_thread.c
+++ b/lib/librte_eal/linux/eal_thread.c
@@ -126,6 +126,8 @@ eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT
diff --git a/lib/librte_eal/windows/eal_thread.c b/lib/librte_eal/windows/eal_thread.c
index 908e726d1..b69672fe0 100644
--- a/lib/librte_eal/windows/eal_thread.c
+++ b/lib/librte_eal/windows/eal_thread.c
@@ -110,6 +110,8 @@ eal_thread_loop(void *arg __rte_unused)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT
-- 
2.17.1

[dpdk-dev] [RFC 2/5] eal: ensure memory operations are visible to worker

[Honnappa Nagarahalli]

Ensure that the memory operations before the call to
rte_eal_remote_launch are visible to the worker thread.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
---
 lib/librte_eal/freebsd/eal_thread.c | 19 +++++++++++++++----
 lib/librte_eal/linux/eal_thread.c   | 19 +++++++++++++++----
 lib/librte_eal/windows/eal_thread.c | 19 +++++++++++++++----
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal_thread.c b/lib/librte_eal/freebsd/eal_thread.c
index bbc3a8e98..17b8f3996 100644
--- a/lib/librte_eal/freebsd/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal_thread.c
@@ -42,8 +42,12 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned worker_id)
 	if (lcore_config[worker_id].state != WAIT)
 		goto finish;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -100,6 +104,7 @@ eal_thread_loop(__rte_unused void *arg)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -119,12 +124,18 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
diff --git a/lib/librte_eal/linux/eal_thread.c b/lib/librte_eal/linux/eal_thread.c
index 8f3c0dafd..a0a009104 100644
--- a/lib/librte_eal/linux/eal_thread.c
+++ b/lib/librte_eal/linux/eal_thread.c
@@ -42,8 +42,12 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id)
 	if (lcore_config[worker_id].state != WAIT)
 		goto finish;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -100,6 +104,7 @@ eal_thread_loop(__rte_unused void *arg)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -119,12 +124,18 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
diff --git a/lib/librte_eal/windows/eal_thread.c b/lib/librte_eal/windows/eal_thread.c
index b69672fe0..7a9277c51 100644
--- a/lib/librte_eal/windows/eal_thread.c
+++ b/lib/librte_eal/windows/eal_thread.c
@@ -32,8 +32,12 @@ rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int worker_id)
 	if (lcore_config[worker_id].state != WAIT)
 		return -EBUSY;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -84,6 +88,7 @@ eal_thread_loop(void *arg __rte_unused)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -103,12 +108,18 @@ eal_thread_loop(void *arg __rte_unused)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-- 
2.17.1

[dpdk-dev] [RFC 3/5] eal: lcore state FINISHED is not required

[Honnappa Nagarahalli]

FINISHED state seems to be used to indicate that the worker's update
of the 'state' is not visible to other threads. There seems to be no
requirement to have such a state.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
---
 drivers/event/dpaa2/dpaa2_eventdev_selftest.c | 2 +-
 drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
 drivers/event/sw/sw_evdev_selftest.c          | 4 ++--
 examples/l2fwd-keepalive/main.c               | 2 +-
 lib/librte_eal/common/eal_common_launch.c     | 7 ++-----
 lib/librte_eal/freebsd/eal_thread.c           | 2 +-
 lib/librte_eal/linux/eal_thread.c             | 8 +-------
 lib/librte_eal/windows/eal_thread.c           | 8 +-------
 8 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
index cd7311a94..bbbd20951 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
@@ -468,7 +468,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
 	RTE_SET_USED(count);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(lcore) != WAIT) {
 		uint64_t new_cycles = rte_get_timer_cycles();
 
 		if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/octeontx/ssovf_evdev_selftest.c b/drivers/event/octeontx/ssovf_evdev_selftest.c
index 528f99dd8..d7b0d2211 100644
--- a/drivers/event/octeontx/ssovf_evdev_selftest.c
+++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
@@ -579,7 +579,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
 	RTE_SET_USED(count);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(lcore) != WAIT) {
 		uint64_t new_cycles = rte_get_timer_cycles();
 
 		if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index e4bfb3a0f..7847a8645 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -3138,8 +3138,8 @@ worker_loopback(struct test *t, uint8_t disable_implicit_release)
 	rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
-			rte_eal_get_lcore_state(w_lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(p_lcore) != WAIT ||
+			rte_eal_get_lcore_state(w_lcore) != WAIT) {
 
 		rte_service_run_iter_on_app_lcore(t->service_id, 1);
 
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index e4c2b2793..dd777c46a 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -506,7 +506,7 @@ dead_core(__rte_unused void *ptr_data, const int id_core)
 	if (terminate_signal_received)
 		return;
 	printf("Dead core %i - restarting..\n", id_core);
-	if (rte_eal_get_lcore_state(id_core) == FINISHED) {
+	if (rte_eal_get_lcore_state(id_core) == WAIT) {
 		rte_eal_wait_lcore(id_core);
 		rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);
 	} else {
diff --git a/lib/librte_eal/common/eal_common_launch.c b/lib/librte_eal/common/eal_common_launch.c
index 34f854ad8..78fd94026 100644
--- a/lib/librte_eal/common/eal_common_launch.c
+++ b/lib/librte_eal/common/eal_common_launch.c
@@ -26,14 +26,11 @@ rte_eal_wait_lcore(unsigned worker_id)
 	if (lcore_config[worker_id].state == WAIT)
 		return 0;
 
-	while (lcore_config[worker_id].state != WAIT &&
-	       lcore_config[worker_id].state != FINISHED)
+	while (lcore_config[worker_id].state != WAIT)
 		rte_pause();
 
 	rte_rmb();
 
-	/* we are in finished state, go to wait state */
-	lcore_config[worker_id].state = WAIT;
 	return lcore_config[worker_id].ret;
 }
 
@@ -62,7 +59,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
 
 	if (call_main == CALL_MAIN) {
 		lcore_config[main_lcore].ret = f(arg);
-		lcore_config[main_lcore].state = FINISHED;
+		lcore_config[main_lcore].state = WAIT;
 	}
 
 	return 0;
diff --git a/lib/librte_eal/freebsd/eal_thread.c b/lib/librte_eal/freebsd/eal_thread.c
index 17b8f3996..6d6f1e2fd 100644
--- a/lib/librte_eal/freebsd/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal_thread.c
@@ -140,7 +140,7 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
-		lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 
 	/* never reached */
diff --git a/lib/librte_eal/linux/eal_thread.c b/lib/librte_eal/linux/eal_thread.c
index a0a009104..7b9463df3 100644
--- a/lib/librte_eal/linux/eal_thread.c
+++ b/lib/librte_eal/linux/eal_thread.c
@@ -141,13 +141,7 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
-		/* when a service core returns, it should go directly to WAIT
-		 * state, because the application will not lcore_wait() for it.
-		 */
-		if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
-			lcore_config[lcore_id].state = WAIT;
-		else
-			lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 
 	/* never reached */
diff --git a/lib/librte_eal/windows/eal_thread.c b/lib/librte_eal/windows/eal_thread.c
index 7a9277c51..35d059a30 100644
--- a/lib/librte_eal/windows/eal_thread.c
+++ b/lib/librte_eal/windows/eal_thread.c
@@ -125,13 +125,7 @@ eal_thread_loop(void *arg __rte_unused)
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
-		/* when a service core returns, it should go directly to WAIT
-		 * state, because the application will not lcore_wait() for it.
-		 */
-		if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
-			lcore_config[lcore_id].state = WAIT;
-		else
-			lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 }
 
-- 
2.17.1

[dpdk-dev] [RFC 4/5] eal: ensure memory operations are visible to main

[Honnappa Nagarahalli]

Ensure that the memory operations in worker thread, that happen
before it returns the status of the assigned function, are
visible to the main thread.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/librte_eal/common/eal_common_launch.c |  8 ++++----
 lib/librte_eal/freebsd/eal_thread.c       | 10 ++++++++--
 lib/librte_eal/linux/eal_thread.c         | 17 ++++++++++++-----
 lib/librte_eal/windows/eal_thread.c       |  9 +++++++--
 4 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_launch.c b/lib/librte_eal/common/eal_common_launch.c
index 78fd94026..9cc71801a 100644
--- a/lib/librte_eal/common/eal_common_launch.c
+++ b/lib/librte_eal/common/eal_common_launch.c
@@ -23,14 +23,14 @@
 int
 rte_eal_wait_lcore(unsigned worker_id)
 {
-	if (lcore_config[worker_id].state == WAIT)
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) == WAIT)
 		return 0;
 
-	while (lcore_config[worker_id].state != WAIT)
+	while (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		rte_pause();
 
-	rte_rmb();
-
 	return lcore_config[worker_id].ret;
 }
 
diff --git a/lib/librte_eal/freebsd/eal_thread.c b/lib/librte_eal/freebsd/eal_thread.c
index 6d6f1e2fd..58c8502de 100644
--- a/lib/librte_eal/freebsd/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal_thread.c
@@ -139,8 +139,14 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
-		lcore_config[lcore_id].state = WAIT;
+
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 
 	/* never reached */
diff --git a/lib/librte_eal/linux/eal_thread.c b/lib/librte_eal/linux/eal_thread.c
index 7b9463df3..eab6fa652 100644
--- a/lib/librte_eal/linux/eal_thread.c
+++ b/lib/librte_eal/linux/eal_thread.c
@@ -39,13 +39,14 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id)
 	int w2m = lcore_config[worker_id].pipe_worker2main[0];
 	int rc = -EBUSY;
 
-	if (lcore_config[worker_id].state != WAIT)
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		goto finish;
 
 	lcore_config[worker_id].arg = arg;
 	/* Ensure that all the memory operations are completed
 	 * before the worker thread starts running the function.
-	 * Use worker thread function as the guard variable.
+	 * Use worker thread function pointer as the guard variable.
 	 */
 	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
@@ -115,7 +116,8 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n <= 0)
 			rte_panic("cannot read on configuration pipe\n");
 
-		lcore_config[lcore_id].state = RUNNING;
+		__atomic_store_n(&lcore_config[lcore_id].state, RUNNING,
+					__ATOMIC_RELEASE);
 
 		/* send ack */
 		n = 0;
@@ -139,9 +141,14 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
 
-		lcore_config[lcore_id].state = WAIT;
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 
 	/* never reached */
diff --git a/lib/librte_eal/windows/eal_thread.c b/lib/librte_eal/windows/eal_thread.c
index 35d059a30..fb1ec4b4f 100644
--- a/lib/librte_eal/windows/eal_thread.c
+++ b/lib/librte_eal/windows/eal_thread.c
@@ -123,9 +123,14 @@ eal_thread_loop(void *arg __rte_unused)
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
 
-		lcore_config[lcore_id].state = WAIT;
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 }
 
-- 
2.17.1

[dpdk-dev] [RFC 5/5] test/ring: use relaxed barriers for ring stress test

[Honnappa Nagarahalli]

wrk_cmd variable is used to signal the worker thread to start
or stop the stress test loop. Relaxed barriers are used
to achieve the same.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
---
 app/test/test_ring_stress_impl.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/test/test_ring_stress_impl.h b/app/test/test_ring_stress_impl.h
index f9ca63b90..ee8293bb0 100644
--- a/app/test/test_ring_stress_impl.h
+++ b/app/test/test_ring_stress_impl.h
@@ -22,7 +22,7 @@ enum {
 	WRK_CMD_RUN,
 };
 
-static volatile uint32_t wrk_cmd __rte_cache_aligned;
+static volatile uint32_t wrk_cmd __rte_cache_aligned = WRK_CMD_STOP;
 
 /* test run-time in seconds */
 static const uint32_t run_time = 60;
@@ -197,10 +197,12 @@ test_worker(void *arg, const char *fname, int32_t prcs)
 	fill_ring_elm(&def_elm, UINT32_MAX);
 	fill_ring_elm(&loc_elm, lc);
 
-	while (wrk_cmd != WRK_CMD_RUN) {
-		rte_smp_rmb();
+	/* Acquire ordering is not required as the main is not
+	 * really releasing any data through 'wrk_cmd' to
+	 * the worker.
+	 */
+	while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) != WRK_CMD_RUN)
 		rte_pause();
-	}
 
 	cl = rte_rdtsc_precise();
 
@@ -242,7 +244,7 @@ test_worker(void *arg, const char *fname, int32_t prcs)
 
 		lcore_stat_update(&la->stats, 1, num, tm0 + tm1, prcs);
 
-	} while (wrk_cmd == WRK_CMD_RUN);
+	} while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) == WRK_CMD_RUN);
 
 	cl = rte_rdtsc_precise() - cl;
 	if (prcs == 0)
@@ -356,14 +358,12 @@ test_mt1(int (*test)(void *))
 	}
 
 	/* signal worker to start test */
-	wrk_cmd = WRK_CMD_RUN;
-	rte_smp_wmb();
+	__atomic_store_n(&wrk_cmd, WRK_CMD_RUN, __ATOMIC_RELEASE);
 
 	usleep(run_time * US_PER_S);
 
 	/* signal worker to start test */
-	wrk_cmd = WRK_CMD_STOP;
-	rte_smp_wmb();
+	__atomic_store_n(&wrk_cmd, WRK_CMD_STOP, __ATOMIC_RELEASE);
 
 	/* wait for workers and collect stats. */
 	mc = rte_lcore_id();
-- 
2.17.1

[dpdk-dev] 回复: [RFC 3/5] eal: lcore state FINISHED is not required

[Feifei Wang]

Hi, Honnappa

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Thursday, February 25, 2021 5:20 AM
> To: hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; jerinj@marvell.com;
> Harry van Haaren <harry.van.haaren@intel.com>; Bruce Richardson
> <bruce.richardson@intel.com>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>;
> Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>; Dmitry Malloy
> <dmitrym@microsoft.com>; Pallavi Kadam <pallavi.kadam@intel.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Feifei Wang (Arm Technology China)
> <Feifei.Wang@arm.com>; nd <nd@arm.com>
> Subject: [RFC 3/5] eal: lcore state FINISHED is not required
> 
> FINISHED state seems to be used to indicate that the worker's update of the
> 'state' is not visible to other threads. There seems to be no requirement to
> have such a state.

I am not sure "FINISHED" is necessary to be removed, and I propose some of my profiles for discussion.
 There are three states for lcore now:
"WAIT": indicate lcore can start working
"RUNNING": indicate lcore is working
"FINISHED": indicate lcore has finished its working and wait to be reset

From the description above, we can find "FINISHED" is different from "WAIT", it can shows that lcore
has done the work and finished it. Thus, if we remove "FINISHED", maybe we will not know whether
the lcore finishes its work or just doesn't start, because this two state has the same tag "WAIT". 

Furthermore, consider such a scenario:
Core 1 need to monitor Core 2 state, if Core 2 finishes one task, Core 1 can start its working.
However, if there is only  one tag "WAIT", Core 1 maybe  start its work at the wrong time, when
Core 2 still does not start its task at state "WAIT".
This is just my guess, and at present, there is no similar application scenario in dpdk.

On the other hand, if we decide to remove "FINISHED", please consider the following files:
1. lib/librte_eal/linux/eal_thread.c: line 31
    lib/librte_eal/windows/eal_thread.c: line 22
    lib/librte_eal/freebsd/eal_thread.c: line 31
2. lib/librte_eal/include/rte_launch.h: line 24, 44, 121, 123, 131
3. examples/l2fwd-keepalive/main.c: line 510
rte_eal_wait_lcore(id_core) can be removed. Because the core state has been checked as "WAIT",
this is a redundant operation

Best Regards
Feifei
 
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> ---
>  drivers/event/dpaa2/dpaa2_eventdev_selftest.c | 2 +-
> drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
>  drivers/event/sw/sw_evdev_selftest.c          | 4 ++--
>  examples/l2fwd-keepalive/main.c               | 2 +-
>  lib/librte_eal/common/eal_common_launch.c     | 7 ++-----
>  lib/librte_eal/freebsd/eal_thread.c           | 2 +-
>  lib/librte_eal/linux/eal_thread.c             | 8 +-------
>  lib/librte_eal/windows/eal_thread.c           | 8 +-------
>  8 files changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> index cd7311a94..bbbd20951 100644
> --- a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> +++ b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> @@ -468,7 +468,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t
> *count)  RTE_SET_USED(count);
> 
>  print_cycles = cycles = rte_get_timer_cycles(); -while
> (rte_eal_get_lcore_state(lcore) != FINISHED) {
> +while (rte_eal_get_lcore_state(lcore) != WAIT) {
>  uint64_t new_cycles = rte_get_timer_cycles();
> 
>  if (new_cycles - print_cycles > rte_get_timer_hz()) { diff --git
> a/drivers/event/octeontx/ssovf_evdev_selftest.c
> b/drivers/event/octeontx/ssovf_evdev_selftest.c
> index 528f99dd8..d7b0d2211 100644
> --- a/drivers/event/octeontx/ssovf_evdev_selftest.c
> +++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
> @@ -579,7 +579,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t
> *count)  RTE_SET_USED(count);
> 
>  print_cycles = cycles = rte_get_timer_cycles(); -while
> (rte_eal_get_lcore_state(lcore) != FINISHED) {
> +while (rte_eal_get_lcore_state(lcore) != WAIT) {
>  uint64_t new_cycles = rte_get_timer_cycles();
> 
>  if (new_cycles - print_cycles > rte_get_timer_hz()) { diff --git
> a/drivers/event/sw/sw_evdev_selftest.c
> b/drivers/event/sw/sw_evdev_selftest.c
> index e4bfb3a0f..7847a8645 100644
> --- a/drivers/event/sw/sw_evdev_selftest.c
> +++ b/drivers/event/sw/sw_evdev_selftest.c
> @@ -3138,8 +3138,8 @@ worker_loopback(struct test *t, uint8_t
> disable_implicit_release)
> rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
> 
>  print_cycles = cycles = rte_get_timer_cycles(); -while
> (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
> -rte_eal_get_lcore_state(w_lcore) != FINISHED) {
> +while (rte_eal_get_lcore_state(p_lcore) != WAIT ||
> +rte_eal_get_lcore_state(w_lcore) != WAIT) {
> 
>  rte_service_run_iter_on_app_lcore(t->service_id, 1);
> 
> diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-
> keepalive/main.c index e4c2b2793..dd777c46a 100644
> --- a/examples/l2fwd-keepalive/main.c
> +++ b/examples/l2fwd-keepalive/main.c
> @@ -506,7 +506,7 @@ dead_core(__rte_unused void *ptr_data, const int
> id_core)  if (terminate_signal_received)  return;  printf("Dead core %i -
> restarting..\n", id_core); -if (rte_eal_get_lcore_state(id_core) == FINISHED) {
> +if (rte_eal_get_lcore_state(id_core) == WAIT) {
>  rte_eal_wait_lcore(id_core);
>  rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);  } else
> { diff --git a/lib/librte_eal/common/eal_common_launch.c
> b/lib/librte_eal/common/eal_common_launch.c
> index 34f854ad8..78fd94026 100644
> --- a/lib/librte_eal/common/eal_common_launch.c
> +++ b/lib/librte_eal/common/eal_common_launch.c
> @@ -26,14 +26,11 @@ rte_eal_wait_lcore(unsigned worker_id)  if
> (lcore_config[worker_id].state == WAIT)  return 0;
> 
> -while (lcore_config[worker_id].state != WAIT &&
> -       lcore_config[worker_id].state != FINISHED)
> +while (lcore_config[worker_id].state != WAIT)
>  rte_pause();
> 
>  rte_rmb();
> 
> -/* we are in finished state, go to wait state */ -
> lcore_config[worker_id].state = WAIT;  return lcore_config[worker_id].ret;  }
> 
> @@ -62,7 +59,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
> 
>  if (call_main == CALL_MAIN) {
>  lcore_config[main_lcore].ret = f(arg);
> -lcore_config[main_lcore].state = FINISHED;
> +lcore_config[main_lcore].state = WAIT;
>  }
> 
>  return 0;
> diff --git a/lib/librte_eal/freebsd/eal_thread.c
> b/lib/librte_eal/freebsd/eal_thread.c
> index 17b8f3996..6d6f1e2fd 100644
> --- a/lib/librte_eal/freebsd/eal_thread.c
> +++ b/lib/librte_eal/freebsd/eal_thread.c
> @@ -140,7 +140,7 @@ eal_thread_loop(__rte_unused void *arg)
> lcore_config[lcore_id].f = NULL;  lcore_config[lcore_id].arg = NULL;
> rte_wmb(); -lcore_config[lcore_id].state = FINISHED;
> +lcore_config[lcore_id].state = WAIT;
>  }
> 
>  /* never reached */
> diff --git a/lib/librte_eal/linux/eal_thread.c
> b/lib/librte_eal/linux/eal_thread.c
> index a0a009104..7b9463df3 100644
> --- a/lib/librte_eal/linux/eal_thread.c
> +++ b/lib/librte_eal/linux/eal_thread.c
> @@ -141,13 +141,7 @@ eal_thread_loop(__rte_unused void *arg)
> lcore_config[lcore_id].arg = NULL;  rte_wmb();
> 
> -/* when a service core returns, it should go directly to WAIT
> - * state, because the application will not lcore_wait() for it.
> - */
> -if (lcore_config[lcore_id].core_role == ROLE_SERVICE) -
> lcore_config[lcore_id].state = WAIT; -else -lcore_config[lcore_id].state =
> FINISHED;
> +lcore_config[lcore_id].state = WAIT;
>  }
> 
>  /* never reached */
> diff --git a/lib/librte_eal/windows/eal_thread.c
> b/lib/librte_eal/windows/eal_thread.c
> index 7a9277c51..35d059a30 100644
> --- a/lib/librte_eal/windows/eal_thread.c
> +++ b/lib/librte_eal/windows/eal_thread.c
> @@ -125,13 +125,7 @@ eal_thread_loop(void *arg __rte_unused)
> lcore_config[lcore_id].arg = NULL;  rte_wmb();
> 
> -/* when a service core returns, it should go directly to WAIT
> - * state, because the application will not lcore_wait() for it.
> - */
> -if (lcore_config[lcore_id].core_role == ROLE_SERVICE) -
> lcore_config[lcore_id].state = WAIT; -else -lcore_config[lcore_id].state =
> FINISHED;
> +lcore_config[lcore_id].state = WAIT;
>  }
>  }
> 
> --
> 2.17.1
> 

Re: [dpdk-dev] [RFC 3/5] eal: lcore state FINISHED is not required

[Honnappa Nagarahalli]

+Thomas, David, Konstantin for input

<snip>

> > Subject: [RFC 3/5] eal: lcore state FINISHED is not required
> >
> > FINISHED state seems to be used to indicate that the worker's update
> > of the 'state' is not visible to other threads. There seems to be no
> > requirement to have such a state.
> 
> I am not sure "FINISHED" is necessary to be removed, and I propose some of my
> profiles for discussion.
>  There are three states for lcore now:
> "WAIT": indicate lcore can start working
> "RUNNING": indicate lcore is working
> "FINISHED": indicate lcore has finished its working and wait to be reset
If you look at the definitions of "WAIT" and "FINISHED" states, they look similar, except for "wait to be reset" in "FINISHED" state . The code really does not do anything to reset the lcore. It just changes the state to "WAIT".

> 
> From the description above, we can find "FINISHED" is different from "WAIT", it
> can shows that lcore has done the work and finished it. Thus, if we remove
> "FINISHED", maybe we will not know whether the lcore finishes its work or just
> doesn't start, because this two state has the same tag "WAIT".
Looking at "eal_thread_loop", the worker thread sets the state to "RUNNING" before sending the ack back to main core. After that it is guaranteed that the worker will run the assigned function. Only case where it will not run the assigned function is when the 'write' syscall fails, in which case it results in a panic.

> 
> Furthermore, consider such a scenario:
> Core 1 need to monitor Core 2 state, if Core 2 finishes one task, Core 1 can start
> its working.
> However, if there is only  one tag "WAIT", Core 1 maybe  start its work at the
> wrong time, when Core 2 still does not start its task at state "WAIT".
> This is just my guess, and at present, there is no similar application scenario in
> dpdk.
To be able to do this effectively, core 1 needs to observe the state change from WAIT->RUNNING->FINISHED. This requires that core 1 should be calling rte_eal_remote_launch and rte_eal_wait_lcore functions. It is not possible to observe this state transition from a 3rd core (for ex: a worker might go from RUNNING->FINISHED->WAIT->RUNNING which a 3rd core might not be able to observe).

> 
> On the other hand, if we decide to remove "FINISHED", please consider the
> following files:
> 1. lib/librte_eal/linux/eal_thread.c: line 31
>     lib/librte_eal/windows/eal_thread.c: line 22
>     lib/librte_eal/freebsd/eal_thread.c: line 31
I have looked at these lines, they do not capture "why" FINISHED state is required.

 2.
> lib/librte_eal/include/rte_launch.h: line 24, 44, 121, 123, 131 3. examples/l2fwd-
> keepalive/main.c: line 510
> rte_eal_wait_lcore(id_core) can be removed. Because the core state has been
> checked as "WAIT", this is a redundant operation
> 
> Best Regards
> Feifei
> 
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> > ---
> >  drivers/event/dpaa2/dpaa2_eventdev_selftest.c | 2 +-
> > drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
> >  drivers/event/sw/sw_evdev_selftest.c          | 4 ++--
> >  examples/l2fwd-keepalive/main.c               | 2 +-
> >  lib/librte_eal/common/eal_common_launch.c     | 7 ++-----
> >  lib/librte_eal/freebsd/eal_thread.c           | 2 +-
> >  lib/librte_eal/linux/eal_thread.c             | 8 +-------
> >  lib/librte_eal/windows/eal_thread.c           | 8 +-------
> >  8 files changed, 10 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > index cd7311a94..bbbd20951 100644
> > --- a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > +++ b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > @@ -468,7 +468,7 @@ wait_workers_to_join(int lcore, const
> > rte_atomic32_t
> > *count)  RTE_SET_USED(count);
> >
> >  print_cycles = cycles = rte_get_timer_cycles(); -while
> > (rte_eal_get_lcore_state(lcore) != FINISHED) {
> > +while (rte_eal_get_lcore_state(lcore) != WAIT) {
> >  uint64_t new_cycles = rte_get_timer_cycles();
> >
> >  if (new_cycles - print_cycles > rte_get_timer_hz()) { diff --git
> > a/drivers/event/octeontx/ssovf_evdev_selftest.c
> > b/drivers/event/octeontx/ssovf_evdev_selftest.c
> > index 528f99dd8..d7b0d2211 100644
> > --- a/drivers/event/octeontx/ssovf_evdev_selftest.c
> > +++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
> > @@ -579,7 +579,7 @@ wait_workers_to_join(int lcore, const
> > rte_atomic32_t
> > *count)  RTE_SET_USED(count);
> >
> >  print_cycles = cycles = rte_get_timer_cycles(); -while
> > (rte_eal_get_lcore_state(lcore) != FINISHED) {
> > +while (rte_eal_get_lcore_state(lcore) != WAIT) {
> >  uint64_t new_cycles = rte_get_timer_cycles();
> >
> >  if (new_cycles - print_cycles > rte_get_timer_hz()) { diff --git
> > a/drivers/event/sw/sw_evdev_selftest.c
> > b/drivers/event/sw/sw_evdev_selftest.c
> > index e4bfb3a0f..7847a8645 100644
> > --- a/drivers/event/sw/sw_evdev_selftest.c
> > +++ b/drivers/event/sw/sw_evdev_selftest.c
> > @@ -3138,8 +3138,8 @@ worker_loopback(struct test *t, uint8_t
> > disable_implicit_release)
> > rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
> >
> >  print_cycles = cycles = rte_get_timer_cycles(); -while
> > (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
> > -rte_eal_get_lcore_state(w_lcore) != FINISHED) {
> > +while (rte_eal_get_lcore_state(p_lcore) != WAIT ||
> > +rte_eal_get_lcore_state(w_lcore) != WAIT) {
> >
> >  rte_service_run_iter_on_app_lcore(t->service_id, 1);
> >
> > diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-
> > keepalive/main.c index e4c2b2793..dd777c46a 100644
> > --- a/examples/l2fwd-keepalive/main.c
> > +++ b/examples/l2fwd-keepalive/main.c
> > @@ -506,7 +506,7 @@ dead_core(__rte_unused void *ptr_data, const int
> > id_core)  if (terminate_signal_received)  return;  printf("Dead core
> > %i - restarting..\n", id_core); -if (rte_eal_get_lcore_state(id_core)
> > == FINISHED) {
> > +if (rte_eal_get_lcore_state(id_core) == WAIT) {
> >  rte_eal_wait_lcore(id_core);
> >  rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);  } else
> > { diff --git a/lib/librte_eal/common/eal_common_launch.c
> > b/lib/librte_eal/common/eal_common_launch.c
> > index 34f854ad8..78fd94026 100644
> > --- a/lib/librte_eal/common/eal_common_launch.c
> > +++ b/lib/librte_eal/common/eal_common_launch.c
> > @@ -26,14 +26,11 @@ rte_eal_wait_lcore(unsigned worker_id)  if
> > (lcore_config[worker_id].state == WAIT)  return 0;
> >
> > -while (lcore_config[worker_id].state != WAIT &&
> > -       lcore_config[worker_id].state != FINISHED)
> > +while (lcore_config[worker_id].state != WAIT)
> >  rte_pause();
> >
> >  rte_rmb();
> >
> > -/* we are in finished state, go to wait state */ -
> > lcore_config[worker_id].state = WAIT;  return
> > lcore_config[worker_id].ret;  }
> >
> > @@ -62,7 +59,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void
> > *arg,
> >
> >  if (call_main == CALL_MAIN) {
> >  lcore_config[main_lcore].ret = f(arg);
> > -lcore_config[main_lcore].state = FINISHED;
> > +lcore_config[main_lcore].state = WAIT;
> >  }
> >
> >  return 0;
> > diff --git a/lib/librte_eal/freebsd/eal_thread.c
> > b/lib/librte_eal/freebsd/eal_thread.c
> > index 17b8f3996..6d6f1e2fd 100644
> > --- a/lib/librte_eal/freebsd/eal_thread.c
> > +++ b/lib/librte_eal/freebsd/eal_thread.c
> > @@ -140,7 +140,7 @@ eal_thread_loop(__rte_unused void *arg)
> > lcore_config[lcore_id].f = NULL;  lcore_config[lcore_id].arg = NULL;
> > rte_wmb(); -lcore_config[lcore_id].state = FINISHED;
> > +lcore_config[lcore_id].state = WAIT;
> >  }
> >
> >  /* never reached */
> > diff --git a/lib/librte_eal/linux/eal_thread.c
> > b/lib/librte_eal/linux/eal_thread.c
> > index a0a009104..7b9463df3 100644
> > --- a/lib/librte_eal/linux/eal_thread.c
> > +++ b/lib/librte_eal/linux/eal_thread.c
> > @@ -141,13 +141,7 @@ eal_thread_loop(__rte_unused void *arg)
> > lcore_config[lcore_id].arg = NULL;  rte_wmb();
> >
> > -/* when a service core returns, it should go directly to WAIT
> > - * state, because the application will not lcore_wait() for it.
> > - */
> > -if (lcore_config[lcore_id].core_role == ROLE_SERVICE) -
> > lcore_config[lcore_id].state = WAIT; -else
> > -lcore_config[lcore_id].state = FINISHED;
> > +lcore_config[lcore_id].state = WAIT;
> >  }
> >
> >  /* never reached */
> > diff --git a/lib/librte_eal/windows/eal_thread.c
> > b/lib/librte_eal/windows/eal_thread.c
> > index 7a9277c51..35d059a30 100644
> > --- a/lib/librte_eal/windows/eal_thread.c
> > +++ b/lib/librte_eal/windows/eal_thread.c
> > @@ -125,13 +125,7 @@ eal_thread_loop(void *arg __rte_unused)
> > lcore_config[lcore_id].arg = NULL;  rte_wmb();
> >
> > -/* when a service core returns, it should go directly to WAIT
> > - * state, because the application will not lcore_wait() for it.
> > - */
> > -if (lcore_config[lcore_id].core_role == ROLE_SERVICE) -
> > lcore_config[lcore_id].state = WAIT; -else
> > -lcore_config[lcore_id].state = FINISHED;
> > +lcore_config[lcore_id].state = WAIT;
> >  }
> >  }
> >
> > --
> > 2.17.1
> >

Re: [dpdk-dev] [RFC 3/5] eal: lcore state FINISHED is not required

[Thomas Monjalon]

26/02/2021 00:33, Honnappa Nagarahalli:
> +Thomas, David, Konstantin for input
> 
> <snip>
> 
> > > Subject: [RFC 3/5] eal: lcore state FINISHED is not required
> > >
> > > FINISHED state seems to be used to indicate that the worker's update
> > > of the 'state' is not visible to other threads. There seems to be no
> > > requirement to have such a state.
> > 
> > I am not sure "FINISHED" is necessary to be removed, and I propose some of my
> > profiles for discussion.
> >  There are three states for lcore now:
> > "WAIT": indicate lcore can start working
> > "RUNNING": indicate lcore is working
> > "FINISHED": indicate lcore has finished its working and wait to be reset
> If you look at the definitions of "WAIT" and "FINISHED" states, they look similar, except for "wait to be reset" in "FINISHED" state . The code really does not do anything to reset the lcore. It just changes the state to "WAIT".
> 
> > 
> > From the description above, we can find "FINISHED" is different from "WAIT", it
> > can shows that lcore has done the work and finished it. Thus, if we remove
> > "FINISHED", maybe we will not know whether the lcore finishes its work or just
> > doesn't start, because this two state has the same tag "WAIT".
> Looking at "eal_thread_loop", the worker thread sets the state to "RUNNING" before sending the ack back to main core. After that it is guaranteed that the worker will run the assigned function. Only case where it will not run the assigned function is when the 'write' syscall fails, in which case it results in a panic.

Quick note: it should not panic.
We must find a way to return an error
without crashing the whole application.

 
> > Furthermore, consider such a scenario:
> > Core 1 need to monitor Core 2 state, if Core 2 finishes one task, Core 1 can start
> > its working.
> > However, if there is only  one tag "WAIT", Core 1 maybe  start its work at the
> > wrong time, when Core 2 still does not start its task at state "WAIT".
> > This is just my guess, and at present, there is no similar application scenario in
> > dpdk.
> To be able to do this effectively, core 1 needs to observe the state change from WAIT->RUNNING->FINISHED. This requires that core 1 should be calling rte_eal_remote_launch and rte_eal_wait_lcore functions. It is not possible to observe this state transition from a 3rd core (for ex: a worker might go from RUNNING->FINISHED->WAIT->RUNNING which a 3rd core might not be able to observe).
> 
> > 
> > On the other hand, if we decide to remove "FINISHED", please consider the
> > following files:
> > 1. lib/librte_eal/linux/eal_thread.c: line 31
> >     lib/librte_eal/windows/eal_thread.c: line 22
> >     lib/librte_eal/freebsd/eal_thread.c: line 31
> I have looked at these lines, they do not capture "why" FINISHED state is required.
> 
>  2.
> > lib/librte_eal/include/rte_launch.h: line 24, 44, 121, 123, 131 3. examples/l2fwd-
> > keepalive/main.c: line 510
> > rte_eal_wait_lcore(id_core) can be removed. Because the core state has been
> > checked as "WAIT", this is a redundant operation

[dpdk-dev] 回复: [RFC 3/5] eal: lcore state FINISHED is not required

[Feifei Wang]

> -----邮件原件-----
> 发件人: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> 发送时间: 2021年2月26日 7:33
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; hemant.agrawal@nxp.com;
> Nipun.gupta@nxp.com; jerinj@marvell.com; harry.van.haaren@intel.com;
> bruce.richardson@intel.com; dmitry.kozliuk@gmail.com;
> navasile@linux.microsoft.com; dmitrym@microsoft.com;
> pallavi.kadam@intel.com; thomas@monjalon.net;
> david.marchand@redhat.com; konstantin.ananyev@intel.com
> 抄送: dev@dpdk.org; Ruifeng Wang <Ruifeng.Wang@arm.com>; nd
> <nd@arm.com>; nd <nd@arm.com>
> 主题: RE: [RFC 3/5] eal: lcore state FINISHED is not required
> 
> +Thomas, David, Konstantin for input
> 
> <snip>
> 
> > > Subject: [RFC 3/5] eal: lcore state FINISHED is not required
> > >
> > > FINISHED state seems to be used to indicate that the worker's update
> > > of the 'state' is not visible to other threads. There seems to be no
> > > requirement to have such a state.
> >
> > I am not sure "FINISHED" is necessary to be removed, and I propose
> > some of my profiles for discussion.
> >  There are three states for lcore now:
> > "WAIT": indicate lcore can start working
> > "RUNNING": indicate lcore is working
> > "FINISHED": indicate lcore has finished its working and wait to be
> > reset
> If you look at the definitions of "WAIT" and "FINISHED" states, they look
> similar, except for "wait to be reset" in "FINISHED" state . The code really
> does not do anything to reset the lcore. It just changes the state to "WAIT".
> 
> >
> > From the description above, we can find "FINISHED" is different from
> > "WAIT", it can shows that lcore has done the work and finished it.
> > Thus, if we remove "FINISHED", maybe we will not know whether the
> > lcore finishes its work or just doesn't start, because this two state has the
> same tag "WAIT".
> Looking at "eal_thread_loop", the worker thread sets the state to
> "RUNNING" before sending the ack back to main core. After that it is
> guaranteed that the worker will run the assigned function. Only case where it
> will not run the assigned function is when the 'write' syscall fails, in which
> case it results in a panic.
> 

I agree that the worker can be guaranteed to run the assigned function. 
But I means that we cannot know when the worker start or when the worker
finishes its working if "Finished" is removed. Please refer to the following
Example for further explanation.

> >
> > Furthermore, consider such a scenario:
> > Core 1 need to monitor Core 2 state, if Core 2 finishes one task, Core
> > 1 can start its working.
> > However, if there is only  one tag "WAIT", Core 1 maybe  start its
> > work at the wrong time, when Core 2 still does not start its task at state
> "WAIT".
> > This is just my guess, and at present, there is no similar application
> > scenario in dpdk.
> To be able to do this effectively, core 1 needs to observe the state change
> from WAIT->RUNNING->FINISHED. This requires that core 1 should be calling
> rte_eal_remote_launch and rte_eal_wait_lcore functions. It is not possible
> to observe this state transition from a 3rd core (for ex: a worker might go
> from RUNNING->FINISHED->WAIT->RUNNING which a 3rd core might not be
> able to observe).

Time Slot               Core 1                                               Core 2                       Core 3（main core）
      1                                                                          eal_thread_loop     rte_eal_remote_launch
      2                                                                                   WAIT
      3       if(rte_get_lcore_state ==FINISHED)          RUNNING
                                       ^
      4                               |                                               execute f
      5                               v                                              FINISHDED 
      6                   do some operations
      7                                                                                                                rte_eal_wait_lcore

I means that Core 1 is an additional thread which observes Core 2. It can use rte_get_lcore_state API to
know core 2 state. However, I just find "rte_get_lcore_state" API only can be called by the main core. This
is the same as you say that worker can not be observed by each other. 
As a result, I agree with you that "FINISHED" state is  redundant  and it can be removed from this current dpdk version.  

> 
> >
> > On the other hand, if we decide to remove "FINISHED", please consider
> > the following files:
> > 1. lib/librte_eal/linux/eal_thread.c: line 31
> >     lib/librte_eal/windows/eal_thread.c: line 22
> >     lib/librte_eal/freebsd/eal_thread.c: line 31
> I have looked at these lines, they do not capture "why" FINISHED state is
> required.

I mean if we have removed "FINISHED", we should change the description here, "FINISHED"
should be replaced by "WAIT". 

> 
>  2.
> > lib/librte_eal/include/rte_launch.h: line 24, 44, 121, 123, 131 3.
> > examples/l2fwd-
> > keepalive/main.c: line 510
> > rte_eal_wait_lcore(id_core) can be removed. Because the core state has
> > been checked as "WAIT", this is a redundant operation
> >
> > Best Regards
> > Feifei
> >
> > >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> > > ---
> > >  drivers/event/dpaa2/dpaa2_eventdev_selftest.c | 2 +-
> > > drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
> > >  drivers/event/sw/sw_evdev_selftest.c          | 4 ++--
> > >  examples/l2fwd-keepalive/main.c               | 2 +-
> > >  lib/librte_eal/common/eal_common_launch.c     | 7 ++-----
> > >  lib/librte_eal/freebsd/eal_thread.c           | 2 +-
> > >  lib/librte_eal/linux/eal_thread.c             | 8 +-------
> > >  lib/librte_eal/windows/eal_thread.c           | 8 +-------
> > >  8 files changed, 10 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > > b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > > index cd7311a94..bbbd20951 100644
> > > --- a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > > +++ b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
> > > @@ -468,7 +468,7 @@ wait_workers_to_join(int lcore, const
> > > rte_atomic32_t
> > > *count)  RTE_SET_USED(count);
> > >
> > >  print_cycles = cycles = rte_get_timer_cycles(); -while
> > > (rte_eal_get_lcore_state(lcore) != FINISHED) {
> > > +while (rte_eal_get_lcore_state(lcore) != WAIT) {
> > >  uint64_t new_cycles = rte_get_timer_cycles();
> > >
> > >  if (new_cycles - print_cycles > rte_get_timer_hz()) { diff --git
> > > a/drivers/event/octeontx/ssovf_evdev_selftest.c
> > > b/drivers/event/octeontx/ssovf_evdev_selftest.c
> > > index 528f99dd8..d7b0d2211 100644
> > > --- a/drivers/event/octeontx/ssovf_evdev_selftest.c
> > > +++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
> > > @@ -579,7 +579,7 @@ wait_workers_to_join(int lcore, const
> > > rte_atomic32_t
> > > *count)  RTE_SET_USED(count);
> > >
> > >  print_cycles = cycles = rte_get_timer_cycles(); -while
> > > (rte_eal_get_lcore_state(lcore) != FINISHED) {
> > > +while (rte_eal_get_lcore_state(lcore) != WAIT) {
> > >  uint64_t new_cycles = rte_get_timer_cycles();
> > >
> > >  if (new_cycles - print_cycles > rte_get_timer_hz()) { diff --git
> > > a/drivers/event/sw/sw_evdev_selftest.c
> > > b/drivers/event/sw/sw_evdev_selftest.c
> > > index e4bfb3a0f..7847a8645 100644
> > > --- a/drivers/event/sw/sw_evdev_selftest.c
> > > +++ b/drivers/event/sw/sw_evdev_selftest.c
> > > @@ -3138,8 +3138,8 @@ worker_loopback(struct test *t, uint8_t
> > > disable_implicit_release)
> > > rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
> > >
> > >  print_cycles = cycles = rte_get_timer_cycles(); -while
> > > (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
> > > -rte_eal_get_lcore_state(w_lcore) != FINISHED) {
> > > +while (rte_eal_get_lcore_state(p_lcore) != WAIT ||
> > > +rte_eal_get_lcore_state(w_lcore) != WAIT) {
> > >
> > >  rte_service_run_iter_on_app_lcore(t->service_id, 1);
> > >
> > > diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-
> > > keepalive/main.c index e4c2b2793..dd777c46a 100644
> > > --- a/examples/l2fwd-keepalive/main.c
> > > +++ b/examples/l2fwd-keepalive/main.c
> > > @@ -506,7 +506,7 @@ dead_core(__rte_unused void *ptr_data, const
> int
> > > id_core)  if (terminate_signal_received)  return;  printf("Dead core
> > > %i - restarting..\n", id_core); -if
> > > (rte_eal_get_lcore_state(id_core) == FINISHED) {
> > > +if (rte_eal_get_lcore_state(id_core) == WAIT) {
> > >  rte_eal_wait_lcore(id_core);
> > >  rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);  }
> > > else { diff --git a/lib/librte_eal/common/eal_common_launch.c
> > > b/lib/librte_eal/common/eal_common_launch.c
> > > index 34f854ad8..78fd94026 100644
> > > --- a/lib/librte_eal/common/eal_common_launch.c
> > > +++ b/lib/librte_eal/common/eal_common_launch.c
> > > @@ -26,14 +26,11 @@ rte_eal_wait_lcore(unsigned worker_id)  if
> > > (lcore_config[worker_id].state == WAIT)  return 0;
> > >
> > > -while (lcore_config[worker_id].state != WAIT &&
> > > -       lcore_config[worker_id].state != FINISHED)
> > > +while (lcore_config[worker_id].state != WAIT)
> > >  rte_pause();
> > >
> > >  rte_rmb();
> > >
> > > -/* we are in finished state, go to wait state */ -
> > > lcore_config[worker_id].state = WAIT;  return
> > > lcore_config[worker_id].ret;  }
> > >
> > > @@ -62,7 +59,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void
> > > *arg,
> > >
> > >  if (call_main == CALL_MAIN) {
> > >  lcore_config[main_lcore].ret = f(arg);
> > > -lcore_config[main_lcore].state = FINISHED;
> > > +lcore_config[main_lcore].state = WAIT;
> > >  }
> > >
> > >  return 0;
> > > diff --git a/lib/librte_eal/freebsd/eal_thread.c
> > > b/lib/librte_eal/freebsd/eal_thread.c
> > > index 17b8f3996..6d6f1e2fd 100644
> > > --- a/lib/librte_eal/freebsd/eal_thread.c
> > > +++ b/lib/librte_eal/freebsd/eal_thread.c
> > > @@ -140,7 +140,7 @@ eal_thread_loop(__rte_unused void *arg)
> > > lcore_config[lcore_id].f = NULL;  lcore_config[lcore_id].arg = NULL;
> > > rte_wmb(); -lcore_config[lcore_id].state = FINISHED;
> > > +lcore_config[lcore_id].state = WAIT;
> > >  }
> > >
> > >  /* never reached */
> > > diff --git a/lib/librte_eal/linux/eal_thread.c
> > > b/lib/librte_eal/linux/eal_thread.c
> > > index a0a009104..7b9463df3 100644
> > > --- a/lib/librte_eal/linux/eal_thread.c
> > > +++ b/lib/librte_eal/linux/eal_thread.c
> > > @@ -141,13 +141,7 @@ eal_thread_loop(__rte_unused void *arg)
> > > lcore_config[lcore_id].arg = NULL;  rte_wmb();
> > >
> > > -/* when a service core returns, it should go directly to WAIT
> > > - * state, because the application will not lcore_wait() for it.
> > > - */
> > > -if (lcore_config[lcore_id].core_role == ROLE_SERVICE) -
> > > lcore_config[lcore_id].state = WAIT; -else
> > > -lcore_config[lcore_id].state = FINISHED;
> > > +lcore_config[lcore_id].state = WAIT;
> > >  }
> > >
> > >  /* never reached */
> > > diff --git a/lib/librte_eal/windows/eal_thread.c
> > > b/lib/librte_eal/windows/eal_thread.c
> > > index 7a9277c51..35d059a30 100644
> > > --- a/lib/librte_eal/windows/eal_thread.c
> > > +++ b/lib/librte_eal/windows/eal_thread.c
> > > @@ -125,13 +125,7 @@ eal_thread_loop(void *arg __rte_unused)
> > > lcore_config[lcore_id].arg = NULL;  rte_wmb();
> > >
> > > -/* when a service core returns, it should go directly to WAIT
> > > - * state, because the application will not lcore_wait() for it.
> > > - */
> > > -if (lcore_config[lcore_id].core_role == ROLE_SERVICE) -
> > > lcore_config[lcore_id].state = WAIT; -else
> > > -lcore_config[lcore_id].state = FINISHED;
> > > +lcore_config[lcore_id].state = WAIT;
> > >  }
> > >  }
> > >
> > > --
> > > 2.17.1
> > >

Re: [dpdk-dev] [RFC 0/5] Use correct memory ordering in eal functions

[Stephen Hemminger]

On Wed, 24 Feb 2021 15:20:13 -0600
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:

> rte_eal_remote_launch and rte_eal_wait_lcore need to provide
> correct memory ordering to address the data communication from
> main core to worker core and vice versa.
> 
> There are 2 use cases:
> 1) All the store operations (meant for worker) by main core
> should be visible to worker core before the worker core runs the
> assigned function
> 
> 2) All the store operations by the worker core should be visible
> to the main core after rte_eal_wait_lcore returns.
> 
> For the data that needs to be communicated to the worker after
> the rte_eal_remote_launch returns, load-acquire and store-release
> semantics should be used (just like any other writer-reader use case).
> 
> For the main to worker communication, the pointer to function
> to execute is used as the guard variable. Hence, resetting of
> the function pointer is important.
> 
> For the worker to main communication, the existing code uses the
> lcore state as the guard variable. However, it looks like
> the FINISHED state is not really required. Hence the FINISHED state
> is removed before using the state as the guard variable. I would like
> some feedback on why the FINISHED state is required. I have not
> paid attention to what it means for backward compatibility.
> If it is decided to remove this state, documentation changes are
> required.
> 


Most likely all use of volatile in DPDK is suspect.
Perhaps we should re-enable the "volatile considered harmful" warning
in checkpatch

Re: [dpdk-dev] [RFC 3/5] eal: lcore state FINISHED is not required

[Honnappa Nagarahalli]

<snip>

> >
> > > > Subject: [RFC 3/5] eal: lcore state FINISHED is not required
> > > >
> > > > FINISHED state seems to be used to indicate that the worker's
> > > > update of the 'state' is not visible to other threads. There seems
> > > > to be no requirement to have such a state.
> > >
> > > I am not sure "FINISHED" is necessary to be removed, and I propose
> > > some of my profiles for discussion.
> > >  There are three states for lcore now:
> > > "WAIT": indicate lcore can start working
> > > "RUNNING": indicate lcore is working
> > > "FINISHED": indicate lcore has finished its working and wait to be
> > > reset
> > If you look at the definitions of "WAIT" and "FINISHED" states, they look
> similar, except for "wait to be reset" in "FINISHED" state . The code really does
> not do anything to reset the lcore. It just changes the state to "WAIT".
> >
> > >
> > > From the description above, we can find "FINISHED" is different from
> > > "WAIT", it can shows that lcore has done the work and finished it.
> > > Thus, if we remove "FINISHED", maybe we will not know whether the
> > > lcore finishes its work or just doesn't start, because this two state has the
> same tag "WAIT".
> > Looking at "eal_thread_loop", the worker thread sets the state to "RUNNING"
> before sending the ack back to main core. After that it is guaranteed that the
> worker will run the assigned function. Only case where it will not run the
> assigned function is when the 'write' syscall fails, in which case it results in a
> panic.
> 
> Quick note: it should not panic.
> We must find a way to return an error
> without crashing the whole application.
The syscalls are being used to communicate the status back to the main thread. If they fail, it is not possible to communicate the status. May be it is better to panic.
We could change the implementation using shared variables, but it would require polling the memory. May be the syscalls are being used to avoid polling. However, this polling would happen during init time (or similar) for a short duration.

> 
> 
> > > Furthermore, consider such a scenario:
> > > Core 1 need to monitor Core 2 state, if Core 2 finishes one task,
> > > Core 1 can start its working.
> > > However, if there is only  one tag "WAIT", Core 1 maybe  start its
> > > work at the wrong time, when Core 2 still does not start its task at state
> "WAIT".
> > > This is just my guess, and at present, there is no similar
> > > application scenario in dpdk.
> > To be able to do this effectively, core 1 needs to observe the state change
> from WAIT->RUNNING->FINISHED. This requires that core 1 should be calling
> rte_eal_remote_launch and rte_eal_wait_lcore functions. It is not possible to
> observe this state transition from a 3rd core (for ex: a worker might go from
> RUNNING->FINISHED->WAIT->RUNNING which a 3rd core might not be able to
> observe).
> >
> > >
> > > On the other hand, if we decide to remove "FINISHED", please
> > > consider the following files:
> > > 1. lib/librte_eal/linux/eal_thread.c: line 31
> > >     lib/librte_eal/windows/eal_thread.c: line 22
> > >     lib/librte_eal/freebsd/eal_thread.c: line 31
> > I have looked at these lines, they do not capture "why" FINISHED state is
> required.
> >
> >  2.
> > > lib/librte_eal/include/rte_launch.h: line 24, 44, 121, 123, 131 3.
> > > examples/l2fwd-
> > > keepalive/main.c: line 510
> > > rte_eal_wait_lcore(id_core) can be removed. Because the core state
> > > has been checked as "WAIT", this is a redundant operation
> 
> 

Re: [dpdk-dev] [RFC 0/5] Use correct memory ordering in eal functions

[Honnappa Nagarahalli]

<snip>

> 
> > rte_eal_remote_launch and rte_eal_wait_lcore need to provide correct
> > memory ordering to address the data communication from main core to
> > worker core and vice versa.
> >
> > There are 2 use cases:
> > 1) All the store operations (meant for worker) by main core should be
> > visible to worker core before the worker core runs the assigned
> > function
> >
> > 2) All the store operations by the worker core should be visible to
> > the main core after rte_eal_wait_lcore returns.
> >
> > For the data that needs to be communicated to the worker after the
> > rte_eal_remote_launch returns, load-acquire and store-release
> > semantics should be used (just like any other writer-reader use case).
> >
> > For the main to worker communication, the pointer to function to
> > execute is used as the guard variable. Hence, resetting of the
> > function pointer is important.
> >
> > For the worker to main communication, the existing code uses the lcore
> > state as the guard variable. However, it looks like the FINISHED state
> > is not really required. Hence the FINISHED state is removed before
> > using the state as the guard variable. I would like some feedback on
> > why the FINISHED state is required. I have not paid attention to what
> > it means for backward compatibility.
> > If it is decided to remove this state, documentation changes are
> > required.
> >
> 
> 
> Most likely all use of volatile in DPDK is suspect.
> Perhaps we should re-enable the "volatile considered harmful" warning in
> checkpatch
Agree.

Re: [dpdk-dev] [RFC 3/5] eal: lcore state FINISHED is not required

[Ananyev, Konstantin]

Hi everyone,

> <snip>
> 
> > >
> > > > > Subject: [RFC 3/5] eal: lcore state FINISHED is not required
> > > > >
> > > > > FINISHED state seems to be used to indicate that the worker's
> > > > > update of the 'state' is not visible to other threads. There seems
> > > > > to be no requirement to have such a state.
> > > >
> > > > I am not sure "FINISHED" is necessary to be removed, and I propose
> > > > some of my profiles for discussion.
> > > >  There are three states for lcore now:
> > > > "WAIT": indicate lcore can start working
> > > > "RUNNING": indicate lcore is working
> > > > "FINISHED": indicate lcore has finished its working and wait to be
> > > > reset
> > > If you look at the definitions of "WAIT" and "FINISHED" states, they look
> > similar, except for "wait to be reset" in "FINISHED" state . The code really does
> > not do anything to reset the lcore. It just changes the state to "WAIT".


I agree that 3 states here seems excessive.
Just 2 (RUNNING/IDLE) seems enough.
Though we can't just remove FINISHED here - it will be an Abi breakage.
Might be deprecate FINISHED now and remove in 21.11.

Also need to decide what rte_eal_wait_lcore() should return in that case?
Always zero, or always status of last function called?

> > >
> > > >
> > > > From the description above, we can find "FINISHED" is different from
> > > > "WAIT", it can shows that lcore has done the work and finished it.
> > > > Thus, if we remove "FINISHED", maybe we will not know whether the
> > > > lcore finishes its work or just doesn't start, because this two state has the
> > same tag "WAIT".
> > > Looking at "eal_thread_loop", the worker thread sets the state to "RUNNING"
> > before sending the ack back to main core. After that it is guaranteed that the
> > worker will run the assigned function. Only case where it will not run the
> > assigned function is when the 'write' syscall fails, in which case it results in a
> > panic.
> >
> > Quick note: it should not panic.
> > We must find a way to return an error
> > without crashing the whole application.
> The syscalls are being used to communicate the status back to the main thread. If they fail, it is not possible to communicate the status.
> May be it is better to panic.
> We could change the implementation using shared variables, but it would require polling the memory. May be the syscalls are being used to
> avoid polling. However, this polling would happen during init time (or similar) for a short duration.

AFAIK we use read and write not for status communication, but sort of sleep/ack point.
Though I agree if we can't do read/write from the system pipe then something is totally wrong,
and probably there is no much point to continue. 
 
> >
> >
> > > > Furthermore, consider such a scenario:
> > > > Core 1 need to monitor Core 2 state, if Core 2 finishes one task,
> > > > Core 1 can start its working.
> > > > However, if there is only  one tag "WAIT", Core 1 maybe  start its
> > > > work at the wrong time, when Core 2 still does not start its task at state
> > "WAIT".
> > > > This is just my guess, and at present, there is no similar
> > > > application scenario in dpdk.
> > > To be able to do this effectively, core 1 needs to observe the state change
> > from WAIT->RUNNING->FINISHED. This requires that core 1 should be calling
> > rte_eal_remote_launch and rte_eal_wait_lcore functions. It is not possible to
> > observe this state transition from a 3rd core (for ex: a worker might go from
> > RUNNING->FINISHED->WAIT->RUNNING which a 3rd core might not be able to
> > observe).
> > >
> > > >
> > > > On the other hand, if we decide to remove "FINISHED", please
> > > > consider the following files:
> > > > 1. lib/librte_eal/linux/eal_thread.c: line 31
> > > >     lib/librte_eal/windows/eal_thread.c: line 22
> > > >     lib/librte_eal/freebsd/eal_thread.c: line 31
> > > I have looked at these lines, they do not capture "why" FINISHED state is
> > required.
> > >
> > >  2.
> > > > lib/librte_eal/include/rte_launch.h: line 24, 44, 121, 123, 131 3.
> > > > examples/l2fwd-
> > > > keepalive/main.c: line 510
> > > > rte_eal_wait_lcore(id_core) can be removed. Because the core state
> > > > has been checked as "WAIT", this is a redundant operation
> >
> >

Re: [dpdk-dev] [RFC 3/5] eal: lcore state FINISHED is not required

[Honnappa Nagarahalli]

<snip>

> 
> Hi everyone,
Thanks Konstantin for the review.

> 
> > <snip>
> >
> > > >
> > > > > > Subject: [RFC 3/5] eal: lcore state FINISHED is not required
> > > > > >
> > > > > > FINISHED state seems to be used to indicate that the worker's
> > > > > > update of the 'state' is not visible to other threads. There
> > > > > > seems to be no requirement to have such a state.
> > > > >
> > > > > I am not sure "FINISHED" is necessary to be removed, and I
> > > > > propose some of my profiles for discussion.
> > > > >  There are three states for lcore now:
> > > > > "WAIT": indicate lcore can start working
> > > > > "RUNNING": indicate lcore is working
> > > > > "FINISHED": indicate lcore has finished its working and wait to
> > > > > be reset
> > > > If you look at the definitions of "WAIT" and "FINISHED" states,
> > > > they look
> > > similar, except for "wait to be reset" in "FINISHED" state . The
> > > code really does not do anything to reset the lcore. It just changes the
> state to "WAIT".
> 
> 
> I agree that 3 states here seems excessive.
> Just 2 (RUNNING/IDLE) seems enough.
> Though we can't just remove FINISHED here - it will be an Abi breakage.
> Might be deprecate FINISHED now and remove in 21.11.
Agree, will add a new patch to deprecate the FINISHED state. Also, does the deprecation notice need to go into 20.08 release notes?

> 
> Also need to decide what rte_eal_wait_lcore() should return in that case?
> Always zero, or always status of last function called?
I am not sure why ' rte_eal_wait_lcore' has the following code:

if (lcore_config[worker_id].state == WAIT)
                return 0;

This indicates that the caller has called 'rte_eal_wait_lcore' function earlier. May be there is a use case where there are multiple threads waiting for the lcores to complete?
Anyway, IMO, returning the status of the last function always is better for this API.

> 
> > > >
> > > > >
> > > > > From the description above, we can find "FINISHED" is different
> > > > > from "WAIT", it can shows that lcore has done the work and finished
> it.
> > > > > Thus, if we remove "FINISHED", maybe we will not know whether
> > > > > the lcore finishes its work or just doesn't start, because this
> > > > > two state has the
> > > same tag "WAIT".
> > > > Looking at "eal_thread_loop", the worker thread sets the state to
> "RUNNING"
> > > before sending the ack back to main core. After that it is
> > > guaranteed that the worker will run the assigned function. Only case
> > > where it will not run the assigned function is when the 'write'
> > > syscall fails, in which case it results in a panic.
> > >
> > > Quick note: it should not panic.
> > > We must find a way to return an error without crashing the whole
> > > application.
> > The syscalls are being used to communicate the status back to the main
> thread. If they fail, it is not possible to communicate the status.
> > May be it is better to panic.
> > We could change the implementation using shared variables, but it
> > would require polling the memory. May be the syscalls are being used to
> avoid polling. However, this polling would happen during init time (or similar)
> for a short duration.
> 
> AFAIK we use read and write not for status communication, but sort of
> sleep/ack point.
> Though I agree if we can't do read/write from the system pipe then
> something is totally wrong, and probably there is no much point to continue.
> 
> > >
> > >
> > > > > Furthermore, consider such a scenario:
> > > > > Core 1 need to monitor Core 2 state, if Core 2 finishes one
> > > > > task, Core 1 can start its working.
> > > > > However, if there is only  one tag "WAIT", Core 1 maybe  start
> > > > > its work at the wrong time, when Core 2 still does not start its
> > > > > task at state
> > > "WAIT".
> > > > > This is just my guess, and at present, there is no similar
> > > > > application scenario in dpdk.
> > > > To be able to do this effectively, core 1 needs to observe the
> > > > state change
> > > from WAIT->RUNNING->FINISHED. This requires that core 1 should be
> > > calling rte_eal_remote_launch and rte_eal_wait_lcore functions. It
> > > is not possible to observe this state transition from a 3rd core
> > > (for ex: a worker might go from
> > > RUNNING->FINISHED->WAIT->RUNNING which a 3rd core might not be
> able
> > > RUNNING->FINISHED->WAIT->to
> > > observe).
> > > >
> > > > >
> > > > > On the other hand, if we decide to remove "FINISHED", please
> > > > > consider the following files:
> > > > > 1. lib/librte_eal/linux/eal_thread.c: line 31
> > > > >     lib/librte_eal/windows/eal_thread.c: line 22
> > > > >     lib/librte_eal/freebsd/eal_thread.c: line 31
> > > > I have looked at these lines, they do not capture "why" FINISHED
> > > > state is
> > > required.
> > > >
> > > >  2.
> > > > > lib/librte_eal/include/rte_launch.h: line 24, 44, 121, 123, 131 3.
> > > > > examples/l2fwd-
> > > > > keepalive/main.c: line 510
> > > > > rte_eal_wait_lcore(id_core) can be removed. Because the core
> > > > > state has been checked as "WAIT", this is a redundant operation
> > >
> > >

[dpdk-dev] [PATCH v2 0/6] Use correct memory ordering in eal functions

[Honnappa Nagarahalli]

rte_eal_remote_launch and rte_eal_wait_lcore need to provide
correct memory ordering to address the data communication from
main core to worker core.

There are 2 use cases:
1) All the store operations (meant for worker) by main core
should be visible to worker core before the worker core runs the
assigned function

2) All the store operations by the worker core should be visible
to the main core after rte_eal_wait_lcore returns.

For the main to worker communication, the pointer to function
to execute is used as the guard variable. Hence, resetting of
the function pointer is important.

For the worker to main communication, the existing code uses the
lcore state as the guard variable. However, the state is also
modified by the main (where it sets the state to FINISHED) which
breaks the reader-writer semantics. It looks like the FINISHED
state is not really required. Hence the FINISHED state
is removed before using the state as the guard variable.

(For the data that needs to be communicated to the worker after
the rte_eal_remote_launch returns, load-acquire and store-release
semantics should be used)

v2:
1) Added documentation changes
2) Changed rte_eal_wait_lcore API definition to reflect
   the removal of FINISHED state.

Honnappa Nagarahalli (6):
  eal: reset lcore function pointer and argument
  eal: ensure memory operations are visible to worker
  eal: lcore state FINISHED is not required
  eal: update rte_eal_wait_lcore definition
  eal: ensure memory operations are visible to main
  test/ring: use relaxed barriers for ring stress test

 app/test/test_ring_stress_impl.h              | 18 +++----
 drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-
 drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-
 drivers/event/sw/sw_evdev_selftest.c          |  4 +-
 examples/l2fwd-keepalive/main.c               |  3 +-
 lib/eal/common/eal_common_launch.c            | 13 ++---
 lib/eal/freebsd/eal_thread.c                  | 45 +++++++++++++----
 lib/eal/include/rte_launch.h                  | 21 ++++----
 lib/eal/include/rte_service.h                 |  4 +-
 lib/eal/linux/eal_thread.c                    | 48 +++++++++++++------
 lib/eal/windows/eal_thread.c                  | 48 +++++++++++++------
 11 files changed, 132 insertions(+), 76 deletions(-)

-- 
2.25.1

[dpdk-dev] [PATCH v2 1/6] eal: reset lcore function pointer and argument

[Honnappa Nagarahalli]

In the rte_eal_remote_launch function, the lcore function
pointer is checked for NULL. However, the pointer is never
reset to NULL. Reset the lcore function pointer and argument
after the worker has completed executing the lcore function.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 lib/eal/freebsd/eal_thread.c | 2 ++
 lib/eal/linux/eal_thread.c   | 2 ++
 lib/eal/windows/eal_thread.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index 1dce9b04f2..bbc3a8e985 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -126,6 +126,8 @@ eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 		lcore_config[lcore_id].state = FINISHED;
 	}
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index 83c2034b93..8f3c0dafd6 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -126,6 +126,8 @@ eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index 9c3f6d69fd..df1df5d02c 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -110,6 +110,8 @@ eal_thread_loop(void *arg __rte_unused)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT
-- 
2.25.1

[dpdk-dev] [PATCH v2 2/6] eal: ensure memory operations are visible to worker

[Honnappa Nagarahalli]

Ensure that the memory operations before the call to
rte_eal_remote_launch are visible to the worker thread.
Use the function pointer to execute in worker thread
as the guard variable.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 lib/eal/freebsd/eal_thread.c | 19 +++++++++++++++----
 lib/eal/linux/eal_thread.c   | 19 +++++++++++++++----
 lib/eal/windows/eal_thread.c | 19 +++++++++++++++----
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index bbc3a8e985..17b8f39966 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -42,8 +42,12 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned worker_id)
 	if (lcore_config[worker_id].state != WAIT)
 		goto finish;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -100,6 +104,7 @@ eal_thread_loop(__rte_unused void *arg)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -119,12 +124,18 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index 8f3c0dafd6..a0a0091040 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -42,8 +42,12 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id)
 	if (lcore_config[worker_id].state != WAIT)
 		goto finish;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -100,6 +104,7 @@ eal_thread_loop(__rte_unused void *arg)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -119,12 +124,18 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index df1df5d02c..e08abcf21f 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -32,8 +32,12 @@ rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int worker_id)
 	if (lcore_config[worker_id].state != WAIT)
 		return -EBUSY;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -84,6 +88,7 @@ eal_thread_loop(void *arg __rte_unused)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -103,12 +108,18 @@ eal_thread_loop(void *arg __rte_unused)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-- 
2.25.1

[dpdk-dev] [PATCH v2 3/6] eal: lcore state FINISHED is not required

[Honnappa Nagarahalli]

FINISHED state seems to be used to indicate that the worker's update
of the 'state' is not visible to other threads. There seems to be no
requirement to have such a state.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-
 drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-
 drivers/event/sw/sw_evdev_selftest.c          |  4 ++--
 examples/l2fwd-keepalive/main.c               |  3 +--
 lib/eal/common/eal_common_launch.c            |  7 ++-----
 lib/eal/freebsd/eal_thread.c                  |  4 ++--
 lib/eal/include/rte_launch.h                  |  9 +++++----
 lib/eal/include/rte_service.h                 |  4 +---
 lib/eal/linux/eal_thread.c                    | 10 ++--------
 lib/eal/windows/eal_thread.c                  | 10 ++--------
 10 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
index cd7311a94d..bbbd20951f 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
@@ -468,7 +468,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
 	RTE_SET_USED(count);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(lcore) != WAIT) {
 		uint64_t new_cycles = rte_get_timer_cycles();
 
 		if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/octeontx/ssovf_evdev_selftest.c b/drivers/event/octeontx/ssovf_evdev_selftest.c
index 528f99dd84..d7b0d22111 100644
--- a/drivers/event/octeontx/ssovf_evdev_selftest.c
+++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
@@ -579,7 +579,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
 	RTE_SET_USED(count);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(lcore) != WAIT) {
 		uint64_t new_cycles = rte_get_timer_cycles();
 
 		if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index d53e903129..9768d3a0c7 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -3140,8 +3140,8 @@ worker_loopback(struct test *t, uint8_t disable_implicit_release)
 	rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
-			rte_eal_get_lcore_state(w_lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(p_lcore) != WAIT ||
+			rte_eal_get_lcore_state(w_lcore) != WAIT) {
 
 		rte_service_run_iter_on_app_lcore(t->service_id, 1);
 
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index 4e1a17cfe4..b4f9d7334b 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -507,8 +507,7 @@ dead_core(__rte_unused void *ptr_data, const int id_core)
 	if (terminate_signal_received)
 		return;
 	printf("Dead core %i - restarting..\n", id_core);
-	if (rte_eal_get_lcore_state(id_core) == FINISHED) {
-		rte_eal_wait_lcore(id_core);
+	if (rte_eal_get_lcore_state(id_core) == WAIT) {
 		rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);
 	} else {
 		printf("..false positive!\n");
diff --git a/lib/eal/common/eal_common_launch.c b/lib/eal/common/eal_common_launch.c
index 34f854ad80..78fd940267 100644
--- a/lib/eal/common/eal_common_launch.c
+++ b/lib/eal/common/eal_common_launch.c
@@ -26,14 +26,11 @@ rte_eal_wait_lcore(unsigned worker_id)
 	if (lcore_config[worker_id].state == WAIT)
 		return 0;
 
-	while (lcore_config[worker_id].state != WAIT &&
-	       lcore_config[worker_id].state != FINISHED)
+	while (lcore_config[worker_id].state != WAIT)
 		rte_pause();
 
 	rte_rmb();
 
-	/* we are in finished state, go to wait state */
-	lcore_config[worker_id].state = WAIT;
 	return lcore_config[worker_id].ret;
 }
 
@@ -62,7 +59,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
 
 	if (call_main == CALL_MAIN) {
 		lcore_config[main_lcore].ret = f(arg);
-		lcore_config[main_lcore].state = FINISHED;
+		lcore_config[main_lcore].state = WAIT;
 	}
 
 	return 0;
diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index 17b8f39966..e5e3edf79b 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -28,7 +28,7 @@
 /*
  * Send a message to a worker lcore identified by worker_id to call a
  * function f with argument arg. Once the execution is done, the
- * remote lcore switch in FINISHED state.
+ * remote lcore switches to WAIT state.
  */
 int
 rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned worker_id)
@@ -140,7 +140,7 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
-		lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 
 	/* never reached */
diff --git a/lib/eal/include/rte_launch.h b/lib/eal/include/rte_launch.h
index 22a901ce62..ed0bb4762a 100644
--- a/lib/eal/include/rte_launch.h
+++ b/lib/eal/include/rte_launch.h
@@ -19,9 +19,10 @@ extern "C" {
  * State of an lcore.
  */
 enum rte_lcore_state_t {
-	WAIT,       /**< waiting a new command */
-	RUNNING,    /**< executing command */
-	FINISHED,   /**< command executed */
+	WAIT,
+	/**< waiting for new command */
+	RUNNING,
+	/**< executing command */
 };
 
 /**
@@ -41,7 +42,7 @@ typedef int (lcore_function_t)(void *);
  *
  * When the remote lcore receives the message, it switches to
  * the RUNNING state, then calls the function f with argument arg. Once the
- * execution is done, the remote lcore switches to a FINISHED state and
+ * execution is done, the remote lcore switches to WAIT state and
  * the return value of f is stored in a local variable to be read using
  * rte_eal_wait_lcore().
  *
diff --git a/lib/eal/include/rte_service.h b/lib/eal/include/rte_service.h
index c7d037d862..646533e871 100644
--- a/lib/eal/include/rte_service.h
+++ b/lib/eal/include/rte_service.h
@@ -321,9 +321,7 @@ int32_t rte_service_lcore_count(void);
  * from duty, just unmaps all services / cores, and stops() the service cores.
  * The runstate of services is not modified.
  *
- * The cores that are stopped with this call, are in FINISHED state and
- * the application must take care of bringing them back to a launchable state:
- * e.g. call *rte_eal_lcore_wait* on the lcore_id.
+ * The cores that are stopped with this call, are in WAIT state.
  *
  * @retval 0 Success
  */
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index a0a0091040..c79e09c9ad 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -28,7 +28,7 @@
 /*
  * Send a message to a worker lcore identified by worker_id to call a
  * function f with argument arg. Once the execution is done, the
- * remote lcore switch in FINISHED state.
+ * remote lcore switches to WAIT state.
  */
 int
 rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id)
@@ -141,13 +141,7 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
-		/* when a service core returns, it should go directly to WAIT
-		 * state, because the application will not lcore_wait() for it.
-		 */
-		if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
-			lcore_config[lcore_id].state = WAIT;
-		else
-			lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 
 	/* never reached */
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index e08abcf21f..977911905e 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -19,7 +19,7 @@
 /*
  * Send a message to a worker lcore identified by worker_id to call a
  * function f with argument arg. Once the execution is done, the
- * remote lcore switch in FINISHED state.
+ * remote lcore switches to WAIT state.
  */
 int
 rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int worker_id)
@@ -125,13 +125,7 @@ eal_thread_loop(void *arg __rte_unused)
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
-		/* when a service core returns, it should go directly to WAIT
-		 * state, because the application will not lcore_wait() for it.
-		 */
-		if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
-			lcore_config[lcore_id].state = WAIT;
-		else
-			lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 }
 
-- 
2.25.1

[dpdk-dev] [PATCH v2 4/6] eal: update rte_eal_wait_lcore definition

[Honnappa Nagarahalli]

Since the FINISHED state is removed, the API rte_eal_wait_lcore
is updated to always return the status of the last function that
ran in the worker core.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 lib/eal/common/eal_common_launch.c |  6 ++----
 lib/eal/include/rte_launch.h       | 12 +++++-------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/lib/eal/common/eal_common_launch.c b/lib/eal/common/eal_common_launch.c
index 78fd940267..4bc842417a 100644
--- a/lib/eal/common/eal_common_launch.c
+++ b/lib/eal/common/eal_common_launch.c
@@ -23,10 +23,8 @@
 int
 rte_eal_wait_lcore(unsigned worker_id)
 {
-	if (lcore_config[worker_id].state == WAIT)
-		return 0;
-
-	while (lcore_config[worker_id].state != WAIT)
+	while (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		rte_pause();
 
 	rte_rmb();
diff --git a/lib/eal/include/rte_launch.h b/lib/eal/include/rte_launch.h
index ed0bb4762a..f2d386e6e2 100644
--- a/lib/eal/include/rte_launch.h
+++ b/lib/eal/include/rte_launch.h
@@ -119,18 +119,16 @@ enum rte_lcore_state_t rte_eal_get_lcore_state(unsigned int worker_id);
  *
  * To be executed on the MAIN lcore only.
  *
- * If the worker lcore identified by the worker_id is in a FINISHED state,
- * switch to the WAIT state. If the lcore is in RUNNING state, wait until
- * the lcore finishes its job and moves to the FINISHED state.
+ * If the lcore identified by the worker_id is in RUNNING state, wait until
+ * the lcore finishes its job and moves to the WAIT state.
  *
  * @param worker_id
  *   The identifier of the lcore.
  * @return
- *   - 0: If the lcore identified by the worker_id is in a WAIT state.
+ *   - 0: If the remote launch function was never called on the lcore
+ *     identified by the worker_id.
  *   - The value that was returned by the previous remote launch
- *     function call if the lcore identified by the worker_id was in a
- *     FINISHED or RUNNING state. In this case, it changes the state
- *     of the lcore to WAIT.
+ *     function call.
  */
 int rte_eal_wait_lcore(unsigned worker_id);
 
-- 
2.25.1

[dpdk-dev] [PATCH v2 5/6] eal: ensure memory operations are visible to main

[Honnappa Nagarahalli]

Ensure that the memory operations in worker thread, that happen
before it returns the status of the assigned function, are
visible to the main thread.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 lib/eal/common/eal_common_launch.c |  2 --
 lib/eal/freebsd/eal_thread.c       | 22 ++++++++++++++++++----
 lib/eal/linux/eal_thread.c         | 23 ++++++++++++++++++-----
 lib/eal/windows/eal_thread.c       | 21 +++++++++++++++++----
 4 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/lib/eal/common/eal_common_launch.c b/lib/eal/common/eal_common_launch.c
index 4bc842417a..e95dadffb3 100644
--- a/lib/eal/common/eal_common_launch.c
+++ b/lib/eal/common/eal_common_launch.c
@@ -27,8 +27,6 @@ rte_eal_wait_lcore(unsigned worker_id)
 					__ATOMIC_ACQUIRE) != WAIT)
 		rte_pause();
 
-	rte_rmb();
-
 	return lcore_config[worker_id].ret;
 }
 
diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index e5e3edf79b..3b18030d73 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -39,7 +39,11 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned worker_id)
 	int w2m = lcore_config[worker_id].pipe_worker2main[0];
 	int rc = -EBUSY;
 
-	if (lcore_config[worker_id].state != WAIT)
+	/* Check if the worker is in 'WAIT' state. Use acquire order
+	 * since 'state' variable is used as the guard variable.
+	 */
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		goto finish;
 
 	lcore_config[worker_id].arg = arg;
@@ -115,7 +119,11 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n <= 0)
 			rte_panic("cannot read on configuration pipe\n");
 
-		lcore_config[lcore_id].state = RUNNING;
+		/* Set the state to 'RUNNING'. Use release order
+		 * since 'state' variable is used as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, RUNNING,
+					__ATOMIC_RELEASE);
 
 		/* send ack */
 		n = 0;
@@ -139,8 +147,14 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
-		lcore_config[lcore_id].state = WAIT;
+
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 
 	/* never reached */
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index c79e09c9ad..c7f0f9b2f7 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -39,13 +39,17 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id)
 	int w2m = lcore_config[worker_id].pipe_worker2main[0];
 	int rc = -EBUSY;
 
-	if (lcore_config[worker_id].state != WAIT)
+	/* Check if the worker is in 'WAIT' state. Use acquire order
+	 * since 'state' variable is used as the guard variable.
+	 */
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		goto finish;
 
 	lcore_config[worker_id].arg = arg;
 	/* Ensure that all the memory operations are completed
 	 * before the worker thread starts running the function.
-	 * Use worker thread function as the guard variable.
+	 * Use worker thread function pointer as the guard variable.
 	 */
 	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
@@ -115,7 +119,11 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n <= 0)
 			rte_panic("cannot read on configuration pipe\n");
 
-		lcore_config[lcore_id].state = RUNNING;
+		/* Set the state to 'RUNNING'. Use release order
+		 * since 'state' variable is used as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, RUNNING,
+					__ATOMIC_RELEASE);
 
 		/* send ack */
 		n = 0;
@@ -139,9 +147,14 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
 
-		lcore_config[lcore_id].state = WAIT;
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 
 	/* never reached */
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index 977911905e..54fa93fa62 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -29,7 +29,11 @@ rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int worker_id)
 	int m2w = lcore_config[worker_id].pipe_main2worker[1];
 	int w2m = lcore_config[worker_id].pipe_worker2main[0];
 
-	if (lcore_config[worker_id].state != WAIT)
+	/* Check if the worker is in 'WAIT' state. Use acquire order
+	 * since 'state' variable is used as the guard variable.
+	 */
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		return -EBUSY;
 
 	lcore_config[worker_id].arg = arg;
@@ -99,7 +103,11 @@ eal_thread_loop(void *arg __rte_unused)
 		if (n <= 0)
 			rte_panic("cannot read on configuration pipe\n");
 
-		lcore_config[lcore_id].state = RUNNING;
+		/* Set the state to 'RUNNING'. Use release order
+		 * since 'state' variable is used as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, RUNNING,
+					__ATOMIC_RELEASE);
 
 		/* send ack */
 		n = 0;
@@ -123,9 +131,14 @@ eal_thread_loop(void *arg __rte_unused)
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
 
-		lcore_config[lcore_id].state = WAIT;
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 }
 
-- 
2.25.1

[dpdk-dev] [PATCH v2 6/6] test/ring: use relaxed barriers for ring stress test

[Honnappa Nagarahalli]

wrk_cmd variable is used to signal the worker thread to start
or stop the stress test loop. Relaxed barriers are used
to achieve the same.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 app/test/test_ring_stress_impl.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/test/test_ring_stress_impl.h b/app/test/test_ring_stress_impl.h
index f9ca63b908..ee8293bb04 100644
--- a/app/test/test_ring_stress_impl.h
+++ b/app/test/test_ring_stress_impl.h
@@ -22,7 +22,7 @@ enum {
 	WRK_CMD_RUN,
 };
 
-static volatile uint32_t wrk_cmd __rte_cache_aligned;
+static volatile uint32_t wrk_cmd __rte_cache_aligned = WRK_CMD_STOP;
 
 /* test run-time in seconds */
 static const uint32_t run_time = 60;
@@ -197,10 +197,12 @@ test_worker(void *arg, const char *fname, int32_t prcs)
 	fill_ring_elm(&def_elm, UINT32_MAX);
 	fill_ring_elm(&loc_elm, lc);
 
-	while (wrk_cmd != WRK_CMD_RUN) {
-		rte_smp_rmb();
+	/* Acquire ordering is not required as the main is not
+	 * really releasing any data through 'wrk_cmd' to
+	 * the worker.
+	 */
+	while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) != WRK_CMD_RUN)
 		rte_pause();
-	}
 
 	cl = rte_rdtsc_precise();
 
@@ -242,7 +244,7 @@ test_worker(void *arg, const char *fname, int32_t prcs)
 
 		lcore_stat_update(&la->stats, 1, num, tm0 + tm1, prcs);
 
-	} while (wrk_cmd == WRK_CMD_RUN);
+	} while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) == WRK_CMD_RUN);
 
 	cl = rte_rdtsc_precise() - cl;
 	if (prcs == 0)
@@ -356,14 +358,12 @@ test_mt1(int (*test)(void *))
 	}
 
 	/* signal worker to start test */
-	wrk_cmd = WRK_CMD_RUN;
-	rte_smp_wmb();
+	__atomic_store_n(&wrk_cmd, WRK_CMD_RUN, __ATOMIC_RELEASE);
 
 	usleep(run_time * US_PER_S);
 
 	/* signal worker to start test */
-	wrk_cmd = WRK_CMD_STOP;
-	rte_smp_wmb();
+	__atomic_store_n(&wrk_cmd, WRK_CMD_STOP, __ATOMIC_RELEASE);
 
 	/* wait for workers and collect stats. */
 	mc = rte_lcore_id();
-- 
2.25.1

Re: [dpdk-dev] [PATCH v2 4/6] eal: update rte_eal_wait_lcore definition

[Honnappa Nagarahalli]

+ Techboard.

This is the API (not ABI as I mentioned) compatibility breakage that I mentioned during the last Techboard meeting.

The removal of the FINISHED state itself was announced. This is a change related to that. However, the specific API change was not called out in the deprecation notice.


> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Thursday, September 9, 2021 6:13 PM
> To: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; konstantin.ananyev@intel.com;
> david.marchand@redhat.com; Feifei Wang <Feifei.Wang2@arm.com>
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; Feifei
> Wang (Arm Technology China) <Feifei.Wang@arm.com>
> Subject: [PATCH v2 4/6] eal: update rte_eal_wait_lcore definition
> 
> Since the FINISHED state is removed, the API rte_eal_wait_lcore is updated to
> always return the status of the last function that ran in the worker core.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Feifei Wang <feifei.wang@arm.com>
> ---
>  lib/eal/common/eal_common_launch.c |  6 ++----
>  lib/eal/include/rte_launch.h       | 12 +++++-------
>  2 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_launch.c
> b/lib/eal/common/eal_common_launch.c
> index 78fd940267..4bc842417a 100644
> --- a/lib/eal/common/eal_common_launch.c
> +++ b/lib/eal/common/eal_common_launch.c
> @@ -23,10 +23,8 @@
>  int
>  rte_eal_wait_lcore(unsigned worker_id)
>  {
> -	if (lcore_config[worker_id].state == WAIT)
> -		return 0;
> -
> -	while (lcore_config[worker_id].state != WAIT)
> +	while (__atomic_load_n(&lcore_config[worker_id].state,
> +					__ATOMIC_ACQUIRE) != WAIT)
>  		rte_pause();
> 
>  	rte_rmb();
> diff --git a/lib/eal/include/rte_launch.h b/lib/eal/include/rte_launch.h index
> ed0bb4762a..f2d386e6e2 100644
> --- a/lib/eal/include/rte_launch.h
> +++ b/lib/eal/include/rte_launch.h
> @@ -119,18 +119,16 @@ enum rte_lcore_state_t
> rte_eal_get_lcore_state(unsigned int worker_id);
>   *
>   * To be executed on the MAIN lcore only.
>   *
> - * If the worker lcore identified by the worker_id is in a FINISHED state,
> - * switch to the WAIT state. If the lcore is in RUNNING state, wait until
> - * the lcore finishes its job and moves to the FINISHED state.
> + * If the lcore identified by the worker_id is in RUNNING state, wait
> + until
> + * the lcore finishes its job and moves to the WAIT state.
>   *
>   * @param worker_id
>   *   The identifier of the lcore.
>   * @return
> - *   - 0: If the lcore identified by the worker_id is in a WAIT state.
> + *   - 0: If the remote launch function was never called on the lcore
> + *     identified by the worker_id.
>   *   - The value that was returned by the previous remote launch
> - *     function call if the lcore identified by the worker_id was in a
> - *     FINISHED or RUNNING state. In this case, it changes the state
> - *     of the lcore to WAIT.
> + *     function call.
>   */
>  int rte_eal_wait_lcore(unsigned worker_id);
> 
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v2 1/6] eal: reset lcore function pointer and argument

[Bruce Richardson]

On Thu, Sep 09, 2021 at 06:13:07PM -0500, Honnappa Nagarahalli wrote:
> In the rte_eal_remote_launch function, the lcore function pointer is
> checked for NULL. However, the pointer is never reset to NULL. Reset the
> lcore function pointer and argument after the worker has completed
> executing the lcore function.
> 
No problems with this patch, but just in general observation.  It would be
good if we had test cases to cover these sorts of problems. If a test were
added to the autotests for this issue we could observe the issue reproduced
and easily verify it were fixed by this patch, giving us a high degree of
confidence in the fix.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH v2 1/6] eal: reset lcore function pointer and argument

[David Marchand]

On Fri, Sep 10, 2021 at 9:49 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Thu, Sep 09, 2021 at 06:13:07PM -0500, Honnappa Nagarahalli wrote:
> > In the rte_eal_remote_launch function, the lcore function pointer is
> > checked for NULL. However, the pointer is never reset to NULL. Reset the
> > lcore function pointer and argument after the worker has completed
> > executing the lcore function.
> >
> No problems with this patch, but just in general observation.  It would be
> good if we had test cases to cover these sorts of problems. If a test were
> added to the autotests for this issue we could observe the issue reproduced
> and easily verify it were fixed by this patch, giving us a high degree of
> confidence in the fix.

+1


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2 1/6] eal: reset lcore function pointer and argument

[Honnappa Nagarahalli]

<snip>

> 
> On Fri, Sep 10, 2021 at 9:49 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Thu, Sep 09, 2021 at 06:13:07PM -0500, Honnappa Nagarahalli wrote:
> > > In the rte_eal_remote_launch function, the lcore function pointer is
> > > checked for NULL. However, the pointer is never reset to NULL. Reset
> > > the lcore function pointer and argument after the worker has
> > > completed executing the lcore function.
> > >
> > No problems with this patch, but just in general observation.  It
> > would be good if we had test cases to cover these sorts of problems.
> > If a test were added to the autotests for this issue we could observe
> > the issue reproduced and easily verify it were fixed by this patch,
> > giving us a high degree of confidence in the fix.
> 
> +1
Thanks for the comments.
Currently, the eal_thread_loop function calls rte_panic if the function pointer is set to NULL. This needs to be changed to return an error code. However, we need to distinguish between the error codes from EAL and the error codes from the function the lcore would run. Currently, there is no mechanism to return the error codes from EAL.

We could add a new member to " struct lcore_config" to capture the error codes from EAL. We need to add a new API to return the EAL error code to the application.

With this we should be able to add a new test case.

> 
> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH v2 6/6] test/ring: use relaxed barriers for ring stress test

[Ananyev, Konstantin]

 
> wrk_cmd variable is used to signal the worker thread to start
> or stop the stress test loop. Relaxed barriers are used
> to achieve the same.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> Reviewed-by: Feifei Wang <feifei.wang@arm.com>
> ---
>  app/test/test_ring_stress_impl.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/app/test/test_ring_stress_impl.h b/app/test/test_ring_stress_impl.h
> index f9ca63b908..ee8293bb04 100644
> --- a/app/test/test_ring_stress_impl.h
> +++ b/app/test/test_ring_stress_impl.h
> @@ -22,7 +22,7 @@ enum {
>  	WRK_CMD_RUN,
>  };
> 
> -static volatile uint32_t wrk_cmd __rte_cache_aligned;
> +static volatile uint32_t wrk_cmd __rte_cache_aligned = WRK_CMD_STOP;

If we switch to using atomic load/store for 'wrk_cmd',
then we can get remove 'volatile' classifier in the 'wrk_cmd' definition above?  

> 
>  /* test run-time in seconds */
>  static const uint32_t run_time = 60;
> @@ -197,10 +197,12 @@ test_worker(void *arg, const char *fname, int32_t prcs)
>  	fill_ring_elm(&def_elm, UINT32_MAX);
>  	fill_ring_elm(&loc_elm, lc);
> 
> -	while (wrk_cmd != WRK_CMD_RUN) {
> -		rte_smp_rmb();
> +	/* Acquire ordering is not required as the main is not
> +	 * really releasing any data through 'wrk_cmd' to
> +	 * the worker.
> +	 */
> +	while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) != WRK_CMD_RUN)
>  		rte_pause();
> -	}
> 
>  	cl = rte_rdtsc_precise();
> 
> @@ -242,7 +244,7 @@ test_worker(void *arg, const char *fname, int32_t prcs)
> 
>  		lcore_stat_update(&la->stats, 1, num, tm0 + tm1, prcs);
> 
> -	} while (wrk_cmd == WRK_CMD_RUN);
> +	} while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) == WRK_CMD_RUN);
> 
>  	cl = rte_rdtsc_precise() - cl;
>  	if (prcs == 0)
> @@ -356,14 +358,12 @@ test_mt1(int (*test)(void *))
>  	}
> 
>  	/* signal worker to start test */
> -	wrk_cmd = WRK_CMD_RUN;
> -	rte_smp_wmb();
> +	__atomic_store_n(&wrk_cmd, WRK_CMD_RUN, __ATOMIC_RELEASE);
> 
>  	usleep(run_time * US_PER_S);
> 
>  	/* signal worker to start test */
> -	wrk_cmd = WRK_CMD_STOP;
> -	rte_smp_wmb();
> +	__atomic_store_n(&wrk_cmd, WRK_CMD_STOP, __ATOMIC_RELEASE);
> 
>  	/* wait for workers and collect stats. */
>  	mc = rte_lcore_id();
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v2 6/6] test/ring: use relaxed barriers for ring stress test

[Honnappa Nagarahalli]

<snip>
> 
> 
> > wrk_cmd variable is used to signal the worker thread to start or stop
> > the stress test loop. Relaxed barriers are used to achieve the same.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> > Reviewed-by: Feifei Wang <feifei.wang@arm.com>
> > ---
> >  app/test/test_ring_stress_impl.h | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/app/test/test_ring_stress_impl.h
> > b/app/test/test_ring_stress_impl.h
> > index f9ca63b908..ee8293bb04 100644
> > --- a/app/test/test_ring_stress_impl.h
> > +++ b/app/test/test_ring_stress_impl.h
> > @@ -22,7 +22,7 @@ enum {
> >  	WRK_CMD_RUN,
> >  };
> >
> > -static volatile uint32_t wrk_cmd __rte_cache_aligned;
> > +static volatile uint32_t wrk_cmd __rte_cache_aligned = WRK_CMD_STOP;
> 
> If we switch to using atomic load/store for 'wrk_cmd', then we can get remove
> 'volatile' classifier in the 'wrk_cmd' definition above?
Agree, will remove

> 
> >
> >  /* test run-time in seconds */
> >  static const uint32_t run_time = 60;
<snip>

[dpdk-dev] [PATCH v3 0/4] Use correct memory ordering in eal functions

[Honnappa Nagarahalli]

v3:
a) Added Fixes, Cc:stable#dpdk.org in 1/6
b) Merged 3/6 & 4/6 and moved after the first commit in the series
c) Merged 2/6 & 5/6 as they need to be in a single commit
d) Removed use of volatile in 6/6 (Konstantin)

rte_eal_remote_launch and rte_eal_wait_lcore need to provide
correct memory ordering to address the data communication from
main core to worker core.

There are 2 use cases:
1) All the store operations (meant for worker) by main core
should be visible to worker core before the worker core runs the
assigned function

2) All the store operations by the worker core should be visible
to the main core after rte_eal_wait_lcore returns.

For the data that needs to be communicated to the worker after
the rte_eal_remote_launch returns, load-acquire and store-release
semantics should be used.

For the main to worker communication, the pointer to function
to execute is used as the guard variable. Hence, resetting of
the function pointer is important.

For the worker to main communication, the existing code uses the
lcore state as the guard variable. However, it looks like
the FINISHED state is not really required. Hence the FINISHED state
is removed before using the state as the guard variable.

Honnappa Nagarahalli (4):
  eal: reset lcore function pointer and argument
  eal: lcore state FINISHED is not required
  eal: ensure correct memory ordering
  test/ring: use relaxed barriers for ring stress test

 app/test/test_ring_stress_impl.h              | 18 +++----
 drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-
 drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-
 drivers/event/sw/sw_evdev_selftest.c          |  4 +-
 examples/l2fwd-keepalive/main.c               |  3 +-
 lib/eal/common/eal_common_launch.c            | 13 ++---
 lib/eal/freebsd/eal_thread.c                  | 45 +++++++++++++----
 lib/eal/include/rte_launch.h                  | 21 ++++----
 lib/eal/include/rte_service.h                 |  4 +-
 lib/eal/linux/eal_thread.c                    | 48 +++++++++++++------
 lib/eal/windows/eal_thread.c                  | 48 +++++++++++++------
 11 files changed, 132 insertions(+), 76 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v3 1/4] eal: reset lcore function pointer and argument

[Honnappa Nagarahalli]

In the rte_eal_remote_launch function, the lcore function
pointer is checked for NULL. However, the pointer is never
reset to NULL. Reset the lcore function pointer and argument
after the worker has completed executing the lcore function.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 lib/eal/freebsd/eal_thread.c | 2 ++
 lib/eal/linux/eal_thread.c   | 2 ++
 lib/eal/windows/eal_thread.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index 1dce9b04f2..bbc3a8e985 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -126,6 +126,8 @@ eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 		lcore_config[lcore_id].state = FINISHED;
 	}
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index 83c2034b93..8f3c0dafd6 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -126,6 +126,8 @@ eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index 9c3f6d69fd..df1df5d02c 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -110,6 +110,8 @@ eal_thread_loop(void *arg __rte_unused)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT
-- 
2.17.1

[dpdk-dev] [PATCH v3 2/4] eal: lcore state FINISHED is not required

[Honnappa Nagarahalli]

FINISHED state seems to be used to indicate that the worker's update
of the 'state' is not visible to other threads. There seems to be no
requirement to have such a state.

Since the FINISHED state is removed, the API rte_eal_wait_lcore
is updated to always return the status of the last function that
ran in the worker core.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-
 drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-
 drivers/event/sw/sw_evdev_selftest.c          |  4 ++--
 examples/l2fwd-keepalive/main.c               |  3 +--
 lib/eal/common/eal_common_launch.c            |  7 ++-----
 lib/eal/freebsd/eal_thread.c                  |  4 ++--
 lib/eal/include/rte_launch.h                  | 21 +++++++++----------
 lib/eal/include/rte_service.h                 |  4 +---
 lib/eal/linux/eal_thread.c                    | 10 ++-------
 lib/eal/windows/eal_thread.c                  | 10 ++-------
 10 files changed, 24 insertions(+), 43 deletions(-)

diff --git a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
index cd7311a94d..bbbd20951f 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev_selftest.c
@@ -468,7 +468,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
 	RTE_SET_USED(count);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(lcore) != WAIT) {
 		uint64_t new_cycles = rte_get_timer_cycles();
 
 		if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/octeontx/ssovf_evdev_selftest.c b/drivers/event/octeontx/ssovf_evdev_selftest.c
index 528f99dd84..d7b0d22111 100644
--- a/drivers/event/octeontx/ssovf_evdev_selftest.c
+++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
@@ -579,7 +579,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
 	RTE_SET_USED(count);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(lcore) != WAIT) {
 		uint64_t new_cycles = rte_get_timer_cycles();
 
 		if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index d53e903129..9768d3a0c7 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -3140,8 +3140,8 @@ worker_loopback(struct test *t, uint8_t disable_implicit_release)
 	rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
 
 	print_cycles = cycles = rte_get_timer_cycles();
-	while (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
-			rte_eal_get_lcore_state(w_lcore) != FINISHED) {
+	while (rte_eal_get_lcore_state(p_lcore) != WAIT ||
+			rte_eal_get_lcore_state(w_lcore) != WAIT) {
 
 		rte_service_run_iter_on_app_lcore(t->service_id, 1);
 
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index 4e1a17cfe4..b4f9d7334b 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -507,8 +507,7 @@ dead_core(__rte_unused void *ptr_data, const int id_core)
 	if (terminate_signal_received)
 		return;
 	printf("Dead core %i - restarting..\n", id_core);
-	if (rte_eal_get_lcore_state(id_core) == FINISHED) {
-		rte_eal_wait_lcore(id_core);
+	if (rte_eal_get_lcore_state(id_core) == WAIT) {
 		rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);
 	} else {
 		printf("..false positive!\n");
diff --git a/lib/eal/common/eal_common_launch.c b/lib/eal/common/eal_common_launch.c
index 34f854ad80..78fd940267 100644
--- a/lib/eal/common/eal_common_launch.c
+++ b/lib/eal/common/eal_common_launch.c
@@ -26,14 +26,11 @@ rte_eal_wait_lcore(unsigned worker_id)
 	if (lcore_config[worker_id].state == WAIT)
 		return 0;
 
-	while (lcore_config[worker_id].state != WAIT &&
-	       lcore_config[worker_id].state != FINISHED)
+	while (lcore_config[worker_id].state != WAIT)
 		rte_pause();
 
 	rte_rmb();
 
-	/* we are in finished state, go to wait state */
-	lcore_config[worker_id].state = WAIT;
 	return lcore_config[worker_id].ret;
 }
 
@@ -62,7 +59,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
 
 	if (call_main == CALL_MAIN) {
 		lcore_config[main_lcore].ret = f(arg);
-		lcore_config[main_lcore].state = FINISHED;
+		lcore_config[main_lcore].state = WAIT;
 	}
 
 	return 0;
diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index bbc3a8e985..ce314c1dd4 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -28,7 +28,7 @@
 /*
  * Send a message to a worker lcore identified by worker_id to call a
  * function f with argument arg. Once the execution is done, the
- * remote lcore switch in FINISHED state.
+ * remote lcore switches to WAIT state.
  */
 int
 rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned worker_id)
@@ -129,7 +129,7 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
-		lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 
 	/* never reached */
diff --git a/lib/eal/include/rte_launch.h b/lib/eal/include/rte_launch.h
index 22a901ce62..f2d386e6e2 100644
--- a/lib/eal/include/rte_launch.h
+++ b/lib/eal/include/rte_launch.h
@@ -19,9 +19,10 @@ extern "C" {
  * State of an lcore.
  */
 enum rte_lcore_state_t {
-	WAIT,       /**< waiting a new command */
-	RUNNING,    /**< executing command */
-	FINISHED,   /**< command executed */
+	WAIT,
+	/**< waiting for new command */
+	RUNNING,
+	/**< executing command */
 };
 
 /**
@@ -41,7 +42,7 @@ typedef int (lcore_function_t)(void *);
  *
  * When the remote lcore receives the message, it switches to
  * the RUNNING state, then calls the function f with argument arg. Once the
- * execution is done, the remote lcore switches to a FINISHED state and
+ * execution is done, the remote lcore switches to WAIT state and
  * the return value of f is stored in a local variable to be read using
  * rte_eal_wait_lcore().
  *
@@ -118,18 +119,16 @@ enum rte_lcore_state_t rte_eal_get_lcore_state(unsigned int worker_id);
  *
  * To be executed on the MAIN lcore only.
  *
- * If the worker lcore identified by the worker_id is in a FINISHED state,
- * switch to the WAIT state. If the lcore is in RUNNING state, wait until
- * the lcore finishes its job and moves to the FINISHED state.
+ * If the lcore identified by the worker_id is in RUNNING state, wait until
+ * the lcore finishes its job and moves to the WAIT state.
  *
  * @param worker_id
  *   The identifier of the lcore.
  * @return
- *   - 0: If the lcore identified by the worker_id is in a WAIT state.
+ *   - 0: If the remote launch function was never called on the lcore
+ *     identified by the worker_id.
  *   - The value that was returned by the previous remote launch
- *     function call if the lcore identified by the worker_id was in a
- *     FINISHED or RUNNING state. In this case, it changes the state
- *     of the lcore to WAIT.
+ *     function call.
  */
 int rte_eal_wait_lcore(unsigned worker_id);
 
diff --git a/lib/eal/include/rte_service.h b/lib/eal/include/rte_service.h
index c7d037d862..646533e871 100644
--- a/lib/eal/include/rte_service.h
+++ b/lib/eal/include/rte_service.h
@@ -321,9 +321,7 @@ int32_t rte_service_lcore_count(void);
  * from duty, just unmaps all services / cores, and stops() the service cores.
  * The runstate of services is not modified.
  *
- * The cores that are stopped with this call, are in FINISHED state and
- * the application must take care of bringing them back to a launchable state:
- * e.g. call *rte_eal_lcore_wait* on the lcore_id.
+ * The cores that are stopped with this call, are in WAIT state.
  *
  * @retval 0 Success
  */
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index 8f3c0dafd6..e2c9e5a3f7 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -28,7 +28,7 @@
 /*
  * Send a message to a worker lcore identified by worker_id to call a
  * function f with argument arg. Once the execution is done, the
- * remote lcore switch in FINISHED state.
+ * remote lcore switches to WAIT state.
  */
 int
 rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id)
@@ -130,13 +130,7 @@ eal_thread_loop(__rte_unused void *arg)
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
-		/* when a service core returns, it should go directly to WAIT
-		 * state, because the application will not lcore_wait() for it.
-		 */
-		if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
-			lcore_config[lcore_id].state = WAIT;
-		else
-			lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 
 	/* never reached */
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index df1df5d02c..0028123c95 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -19,7 +19,7 @@
 /*
  * Send a message to a worker lcore identified by worker_id to call a
  * function f with argument arg. Once the execution is done, the
- * remote lcore switch in FINISHED state.
+ * remote lcore switches to WAIT state.
  */
 int
 rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int worker_id)
@@ -114,13 +114,7 @@ eal_thread_loop(void *arg __rte_unused)
 		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
-		/* when a service core returns, it should go directly to WAIT
-		 * state, because the application will not lcore_wait() for it.
-		 */
-		if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
-			lcore_config[lcore_id].state = WAIT;
-		else
-			lcore_config[lcore_id].state = FINISHED;
+		lcore_config[lcore_id].state = WAIT;
 	}
 }
 
-- 
2.17.1

[dpdk-dev] [PATCH v3 3/4] eal: use correct memory ordering

[Honnappa Nagarahalli]

Ensure that the memory operations before the call to
rte_eal_remote_launch are visible to the worker thread.
Use the function pointer to execute in worker thread
as the guard variable.

Ensure that the memory operations in worker thread, that happen
before it returns the status of the assigned function, are
visible to the main thread. Use the variable containing the
lcore's state as the guard variable.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 lib/eal/common/eal_common_launch.c |  8 ++----
 lib/eal/freebsd/eal_thread.c       | 41 ++++++++++++++++++++++++------
 lib/eal/linux/eal_thread.c         | 40 +++++++++++++++++++++++------
 lib/eal/windows/eal_thread.c       | 40 +++++++++++++++++++++++------
 4 files changed, 99 insertions(+), 30 deletions(-)

diff --git a/lib/eal/common/eal_common_launch.c b/lib/eal/common/eal_common_launch.c
index 78fd940267..e95dadffb3 100644
--- a/lib/eal/common/eal_common_launch.c
+++ b/lib/eal/common/eal_common_launch.c
@@ -23,14 +23,10 @@
 int
 rte_eal_wait_lcore(unsigned worker_id)
 {
-	if (lcore_config[worker_id].state == WAIT)
-		return 0;
-
-	while (lcore_config[worker_id].state != WAIT)
+	while (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		rte_pause();
 
-	rte_rmb();
-
 	return lcore_config[worker_id].ret;
 }
 
diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index ce314c1dd4..3b18030d73 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -39,11 +39,19 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned worker_id)
 	int w2m = lcore_config[worker_id].pipe_worker2main[0];
 	int rc = -EBUSY;
 
-	if (lcore_config[worker_id].state != WAIT)
+	/* Check if the worker is in 'WAIT' state. Use acquire order
+	 * since 'state' variable is used as the guard variable.
+	 */
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		goto finish;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -100,6 +108,7 @@ eal_thread_loop(__rte_unused void *arg)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -110,7 +119,11 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n <= 0)
 			rte_panic("cannot read on configuration pipe\n");
 
-		lcore_config[lcore_id].state = RUNNING;
+		/* Set the state to 'RUNNING'. Use release order
+		 * since 'state' variable is used as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, RUNNING,
+					__ATOMIC_RELEASE);
 
 		/* send ack */
 		n = 0;
@@ -119,17 +132,29 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
-		lcore_config[lcore_id].state = WAIT;
+
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 
 	/* never reached */
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index e2c9e5a3f7..c7f0f9b2f7 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -39,11 +39,19 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id)
 	int w2m = lcore_config[worker_id].pipe_worker2main[0];
 	int rc = -EBUSY;
 
-	if (lcore_config[worker_id].state != WAIT)
+	/* Check if the worker is in 'WAIT' state. Use acquire order
+	 * since 'state' variable is used as the guard variable.
+	 */
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		goto finish;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function pointer as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -100,6 +108,7 @@ eal_thread_loop(__rte_unused void *arg)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -110,7 +119,11 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n <= 0)
 			rte_panic("cannot read on configuration pipe\n");
 
-		lcore_config[lcore_id].state = RUNNING;
+		/* Set the state to 'RUNNING'. Use release order
+		 * since 'state' variable is used as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, RUNNING,
+					__ATOMIC_RELEASE);
 
 		/* send ack */
 		n = 0;
@@ -119,18 +132,29 @@ eal_thread_loop(__rte_unused void *arg)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
 
-		lcore_config[lcore_id].state = WAIT;
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 
 	/* never reached */
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index 0028123c95..54fa93fa62 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -29,11 +29,19 @@ rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int worker_id)
 	int m2w = lcore_config[worker_id].pipe_main2worker[1];
 	int w2m = lcore_config[worker_id].pipe_worker2main[0];
 
-	if (lcore_config[worker_id].state != WAIT)
+	/* Check if the worker is in 'WAIT' state. Use acquire order
+	 * since 'state' variable is used as the guard variable.
+	 */
+	if (__atomic_load_n(&lcore_config[worker_id].state,
+					__ATOMIC_ACQUIRE) != WAIT)
 		return -EBUSY;
 
-	lcore_config[worker_id].f = f;
 	lcore_config[worker_id].arg = arg;
+	/* Ensure that all the memory operations are completed
+	 * before the worker thread starts running the function.
+	 * Use worker thread function as the guard variable.
+	 */
+	__atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE);
 
 	/* send message */
 	n = 0;
@@ -84,6 +92,7 @@ eal_thread_loop(void *arg __rte_unused)
 
 	/* read on our pipe to get commands */
 	while (1) {
+		lcore_function_t *f;
 		void *fct_arg;
 
 		/* wait command */
@@ -94,7 +103,11 @@ eal_thread_loop(void *arg __rte_unused)
 		if (n <= 0)
 			rte_panic("cannot read on configuration pipe\n");
 
-		lcore_config[lcore_id].state = RUNNING;
+		/* Set the state to 'RUNNING'. Use release order
+		 * since 'state' variable is used as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, RUNNING,
+					__ATOMIC_RELEASE);
 
 		/* send ack */
 		n = 0;
@@ -103,18 +116,29 @@ eal_thread_loop(void *arg __rte_unused)
 		if (n < 0)
 			rte_panic("cannot write on configuration pipe\n");
 
-		if (lcore_config[lcore_id].f == NULL)
-			rte_panic("NULL function pointer\n");
+		/* Load 'f' with acquire order to ensure that
+		 * the memory operations from the main thread
+		 * are accessed only after update to 'f' is visible.
+		 * Wait till the update to 'f' is visible to the worker.
+		 */
+		while ((f = __atomic_load_n(&lcore_config[lcore_id].f,
+			__ATOMIC_ACQUIRE)) == NULL)
+			rte_pause();
 
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
-		ret = lcore_config[lcore_id].f(fct_arg);
+		ret = f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
 		lcore_config[lcore_id].f = NULL;
 		lcore_config[lcore_id].arg = NULL;
-		rte_wmb();
 
-		lcore_config[lcore_id].state = WAIT;
+		/* Store the state with release order to ensure that
+		 * the memory operations from the worker thread
+		 * are completed before the state is updated.
+		 * Use 'state' as the guard variable.
+		 */
+		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
+					__ATOMIC_RELEASE);
 	}
 }
 
-- 
2.17.1

[dpdk-dev] [PATCH v3 4/4] test/ring: use relaxed barriers for ring stress test

[Honnappa Nagarahalli]

wrk_cmd variable is used to signal the worker thread to start
or stop the stress test loop. Relaxed barriers are used
to achieve the same.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Feifei Wang <feifei.wang@arm.com>
---
 app/test/test_ring_stress_impl.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/test/test_ring_stress_impl.h b/app/test/test_ring_stress_impl.h
index f9ca63b908..2825a9dce6 100644
--- a/app/test/test_ring_stress_impl.h
+++ b/app/test/test_ring_stress_impl.h
@@ -22,7 +22,7 @@ enum {
 	WRK_CMD_RUN,
 };
 
-static volatile uint32_t wrk_cmd __rte_cache_aligned;
+static uint32_t wrk_cmd __rte_cache_aligned = WRK_CMD_STOP;
 
 /* test run-time in seconds */
 static const uint32_t run_time = 60;
@@ -197,10 +197,12 @@ test_worker(void *arg, const char *fname, int32_t prcs)
 	fill_ring_elm(&def_elm, UINT32_MAX);
 	fill_ring_elm(&loc_elm, lc);
 
-	while (wrk_cmd != WRK_CMD_RUN) {
-		rte_smp_rmb();
+	/* Acquire ordering is not required as the main is not
+	 * really releasing any data through 'wrk_cmd' to
+	 * the worker.
+	 */
+	while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) != WRK_CMD_RUN)
 		rte_pause();
-	}
 
 	cl = rte_rdtsc_precise();
 
@@ -242,7 +244,7 @@ test_worker(void *arg, const char *fname, int32_t prcs)
 
 		lcore_stat_update(&la->stats, 1, num, tm0 + tm1, prcs);
 
-	} while (wrk_cmd == WRK_CMD_RUN);
+	} while (__atomic_load_n(&wrk_cmd, __ATOMIC_RELAXED) == WRK_CMD_RUN);
 
 	cl = rte_rdtsc_precise() - cl;
 	if (prcs == 0)
@@ -356,14 +358,12 @@ test_mt1(int (*test)(void *))
 	}
 
 	/* signal worker to start test */
-	wrk_cmd = WRK_CMD_RUN;
-	rte_smp_wmb();
+	__atomic_store_n(&wrk_cmd, WRK_CMD_RUN, __ATOMIC_RELEASE);
 
 	usleep(run_time * US_PER_S);
 
 	/* signal worker to start test */
-	wrk_cmd = WRK_CMD_STOP;
-	rte_smp_wmb();
+	__atomic_store_n(&wrk_cmd, WRK_CMD_STOP, __ATOMIC_RELEASE);
 
 	/* wait for workers and collect stats. */
 	mc = rte_lcore_id();
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 0/4] Use correct memory ordering in eal functions

[David Marchand]

On Mon, Oct 25, 2021 at 6:53 AM Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> wrote:
>
> v3:
> a) Added Fixes, Cc:stable#dpdk.org in 1/6
> b) Merged 3/6 & 4/6 and moved after the first commit in the series
> c) Merged 2/6 & 5/6 as they need to be in a single commit
> d) Removed use of volatile in 6/6 (Konstantin)
>
> rte_eal_remote_launch and rte_eal_wait_lcore need to provide
> correct memory ordering to address the data communication from
> main core to worker core.
>
> There are 2 use cases:
> 1) All the store operations (meant for worker) by main core
> should be visible to worker core before the worker core runs the
> assigned function
>
> 2) All the store operations by the worker core should be visible
> to the main core after rte_eal_wait_lcore returns.
>
> For the data that needs to be communicated to the worker after
> the rte_eal_remote_launch returns, load-acquire and store-release
> semantics should be used.
>
> For the main to worker communication, the pointer to function
> to execute is used as the guard variable. Hence, resetting of
> the function pointer is important.
>
> For the worker to main communication, the existing code uses the
> lcore state as the guard variable. However, it looks like
> the FINISHED state is not really required. Hence the FINISHED state
> is removed before using the state as the guard variable.
>
> Honnappa Nagarahalli (4):
>   eal: reset lcore function pointer and argument
>   eal: lcore state FINISHED is not required
>   eal: ensure correct memory ordering
>   test/ring: use relaxed barriers for ring stress test
>
>  app/test/test_ring_stress_impl.h              | 18 +++----
>  drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-
>  drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-
>  drivers/event/sw/sw_evdev_selftest.c          |  4 +-
>  examples/l2fwd-keepalive/main.c               |  3 +-
>  lib/eal/common/eal_common_launch.c            | 13 ++---
>  lib/eal/freebsd/eal_thread.c                  | 45 +++++++++++++----
>  lib/eal/include/rte_launch.h                  | 21 ++++----
>  lib/eal/include/rte_service.h                 |  4 +-
>  lib/eal/linux/eal_thread.c                    | 48 +++++++++++++------
>  lib/eal/windows/eal_thread.c                  | 48 +++++++++++++------
>  11 files changed, 132 insertions(+), 76 deletions(-)

Tweaked commit titles, removed deprecation notice and updated release
notes in patch 2.
Series applied, thanks.


-- 
David Marchand