<snip>

Hi Feifei,

quoted

The variable "wrk_cmd" is a signal to control threads from running and
stopping. When worker lcores load "wrk_cmd == WRK_CMD_RUN", they

start

quoted

running and when worker lcores load "wrk_cmd == WRK_CMD_STOP",

they

quoted

stop.

For the wmb in test_mt1, no storing operations must keep the order
after storing "wrk_cmd". Thus the wmb is unnecessary.

I think there is a bug in my original code, we should do smp_wmb() *before*
setting wrk_cmd, not after:

        /* launch on all workers */
        RTE_LCORE_FOREACH_WORKER(lc) {
                arg[lc].rng = r;
                arg[lc].stats = init_stat;
                rte_eal_remote_launch(test, &arg[lc], lc);
        }

        /* signal worker to start test */
+      rte_smp_wmb();
        wrk_cmd = WRK_CMD_RUN;
-       rte_smp_wmb();

        usleep(run_time * US_PER_S);


I still think we'd better have some synchronisation here.
Otherwise what would prevent compiler and/or cpu to update wrk_cmd out
of order (before _init_ phase is completed)?
We probably can safely assume no reordering from the compiler here, as we
have function calls straight before and after 'wrk_cmd = WRK_CMD_RUN;'
But for consistency and easier maintenance, I still think it is better to have
something here, after all it is not performance critical pass.

Agree that this is not performance critical.

This is more about correctness (as usually people refer to code to understand the concepts). You can refer to video [1]. Essentially, the pthread_create has 'happens-before' behavior. i.e. all the memory operations before the pthread_create are visible to the new thread. The rte_smp_rmb() barrier in the thread function is not required as it reads the data that was set before the thread was launched.

I do not know why rte_smp_wmb is required. The update to 'wrk_cmd' is seen by the thread eventually. rte_smp_wmb does not result in update being seen sooner/immediately. 

[1] https://www.youtube.com/watch?t=4170&v=KeLBd2EJLOU&feature=youtu.be

quoted

For the rmb in test_worker, the parameters have been prepared when
worker lcores call "test_worker". It is unnessary to wait wrk_cmd to
be loaded, then the parameters can be loaded, So the rmb can be

removed.

It is not only about parameters loading,  it is to prevent worker core to start
too early.

Because 'pthread_launch' provides the 'happens-before' behavior, the worker core will see the updates that happened before the worker was launched.

I suggest changing the commit log to provide the reasoning around pthread_create.

As I understand, your goal is to get rid of rte_smp_*() calls.
Might be better to replace such places here with _atomic_ semantics.
Then, as I can see, we also can get rid of 'volatile' fo wrk_cmd.

quoted

In the meanwhile, fix a typo. The note above storing "stop" into
"wrk_cmd" should be "stop test" rather than "start test".

Signed-off-by: Feifei Wang <redacted>
Reviewed-by: Honnappa Nagarahalli <redacted>
Reviewed-by: Ruifeng Wang <redacted>
---
 app/test/test_ring_stress_impl.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/test/test_ring_stress_impl.h

b/app/test/test_ring_stress_impl.h
index f9ca63b90..384555ef9 100644

--- a/app/test/test_ring_stress_impl.h
+++ b/app/test/test_ring_stress_impl.h

@@ -198,7 +198,6 @@ test_worker(void *arg, const char *fname, int32_t

prcs)

quoted

 	fill_ring_elm(&loc_elm, lc);

 	while (wrk_cmd != WRK_CMD_RUN) {
-		rte_smp_rmb();
 		rte_pause();
 	}

@@ -357,13 +356,11 @@ test_mt1(int (*test)(void *))

 	/* signal worker to start test */
 	wrk_cmd = WRK_CMD_RUN;
-	rte_smp_wmb();

 	usleep(run_time * US_PER_S);

-	/* signal worker to start test */
+	/* signal worker to stop test */
 	wrk_cmd = WRK_CMD_STOP;
-	rte_smp_wmb();

 	/* wait for workers and collect stats. */
 	mc = rte_lcore_id();
--

2.17.1