Hi, Pavan

Sorry for my late reply and thanks very much for your review.

-----Original Message-----
From: Pavan Nikhilesh Bhagavatula <redacted>
Sent: 2020年12月22日 18:33
To: Feifei Wang <redacted>; jerinj@marvell.com; Harry van
Haaren [off-list ref]; Pavan Nikhilesh
[off-list ref]
Cc: dev@dpdk.org; nd <redacted>; Honnappa Nagarahalli
[off-list ref]; stable@dpdk.org; Phil Yang
[off-list ref]
Subject: RE: [RFC PATCH v1 4/6] app/eventdev: add release barriers for
pipeline test

quoted

Add release barriers before updating the processed packets for worker
lcores to ensure the worker lcore has really finished data processing
and then it can update the processed packets number.

I believe we can live with minor inaccuracies in stats being presented as
atomics are pretty heavy when scheduler is limited to burst size as 1.

One option is to move it before a pipeline operation (pipeline_event_tx,
pipeline_fwd_event etc.) as they imply implicit release barrier (as all the
changes done to the event should be visible to the next core).

If I understand correctly, your meaning is that move release barriers before
pipeline_event_tx or pipeline_fwd_event. This can ensure the event has been
processed before the next core begins to tx/fwd. For example:

if (ev.sched_type == RTE_SCHED_TYPE_ATOMIC) {
		+	__atomic_thread_fence(__ATOMIC_RELEASE); 
			pipeline_event_tx(dev, port, &ev);
			w->processed_pkts++;
		} else {
			ev.queue_id++;
		+	__atomic_thread_fence(__ATOMIC_RELEASE);
			pipeline_fwd_event(&ev, RTE_SCHED_TYPE_ATOMIC);
			pipeline_event_enqueue(dev, port, &ev);

However, there are two reasons to prevent this:

First, compare with other tests in app/eventdev, for example, the eventdev perf test,
the wmb is after event operation to ensure operation has been finished and then w->processed_pkts++.
So, if we move release barriers before tx/fwd, it may cause that the tests of app/eventdev
become  inconsistent.This may reduce the maintainability of the code and make it difficult to understand.

Second, it is a test case, though heavy thread may cause performance degradation, it can ensure that
the operation process and the test result are correct. And maybe for a test case, correctness is more important
than performance.

So, due to two reasons above, I'm ambivalent about how we should do in the next step. 

Best Regards
Feifei

quoted

Fixes: 314bcf58ca8f ("app/eventdev: add pipeline queue worker
functions")
Cc: pbhagavatula@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Phil Yang <redacted>
Signed-off-by: Feifei Wang <redacted>
Reviewed-by: Ruifeng Wang <redacted>
---
app/test-eventdev/test_pipeline_queue.c | 64
+++++++++++++++++++++----
1 file changed, 56 insertions(+), 8 deletions(-)

diff --git a/app/test-eventdev/test_pipeline_queue.c b/app/test-

eventdev/test_pipeline_queue.c index 7bebac34f..0c0ec0ceb 100644

--- a/app/test-eventdev/test_pipeline_queue.c
+++ b/app/test-eventdev/test_pipeline_queue.c

@@ -30,7 +30,13 @@ pipeline_queue_worker_single_stage_tx(void

*arg)

		if (ev.sched_type == RTE_SCHED_TYPE_ATOMIC) {
			pipeline_event_tx(dev, port, &ev);
-			w->processed_pkts++;
+
+			/* release barrier here ensures stored operation
+			 * of the event completes before the number of
+			 * processed pkts is visible to the main core
+			 */
+			__atomic_fetch_add(&(w->processed_pkts), 1,
+					__ATOMIC_RELEASE);
		} else {
			ev.queue_id++;
			pipeline_fwd_event(&ev,
RTE_SCHED_TYPE_ATOMIC);

@@ -59,7 +65,13 @@ pipeline_queue_worker_single_stage_fwd(void

*arg)
		rte_event_eth_tx_adapter_txq_set(ev.mbuf, 0);
		pipeline_fwd_event(&ev, RTE_SCHED_TYPE_ATOMIC);
		pipeline_event_enqueue(dev, port, &ev);
-		w->processed_pkts++;
+
+		/* release barrier here ensures stored operation
+		 * of the event completes before the number of
+		 * processed pkts is visible to the main core
+		 */
+		__atomic_fetch_add(&(w->processed_pkts), 1,
+				__ATOMIC_RELEASE);
	}

	return 0;

@@ -84,7 +96,13 @@

pipeline_queue_worker_single_stage_burst_tx(void *arg)
			if (ev[i].sched_type ==
RTE_SCHED_TYPE_ATOMIC) {
				pipeline_event_tx(dev, port, &ev[i]);
				ev[i].op = RTE_EVENT_OP_RELEASE;
-				w->processed_pkts++;
+
+				/* release barrier here ensures stored
operation
+				 * of the event completes before the
number of
+				 * processed pkts is visible to the main
core
+				 */
+				__atomic_fetch_add(&(w-

quoted

processed_pkts), 1,

+						__ATOMIC_RELEASE);
			} else {
				ev[i].queue_id++;
				pipeline_fwd_event(&ev[i],

@@ -121,7 +139,13 @@

pipeline_queue_worker_single_stage_burst_fwd(void *arg)
		}

		pipeline_event_enqueue_burst(dev, port, ev, nb_rx);
-		w->processed_pkts += nb_rx;
+
+		/* release barrier here ensures stored operation
+		 * of the event completes before the number of
+		 * processed pkts is visible to the main core
+		 */
+		__atomic_fetch_add(&(w->processed_pkts), nb_rx,
+				__ATOMIC_RELEASE);
	}

	return 0;

@@ -146,7 +170,13 @@ pipeline_queue_worker_multi_stage_tx(void

*arg)

		if (ev.queue_id == tx_queue[ev.mbuf->port]) {
			pipeline_event_tx(dev, port, &ev);
-			w->processed_pkts++;
+
+			/* release barrier here ensures stored operation
+			 * of the event completes before the number of
+			 * processed pkts is visible to the main core
+			 */
+			__atomic_fetch_add(&(w->processed_pkts), 1,
+					__ATOMIC_RELEASE);
			continue;
		}

@@ -180,7 +210,13 @@

pipeline_queue_worker_multi_stage_fwd(void *arg)
			ev.queue_id = tx_queue[ev.mbuf->port];
			rte_event_eth_tx_adapter_txq_set(ev.mbuf, 0);
			pipeline_fwd_event(&ev,
RTE_SCHED_TYPE_ATOMIC);
-			w->processed_pkts++;
+
+			/* release barrier here ensures stored operation
+			 * of the event completes before the number of
+			 * processed pkts is visible to the main core
+			 */
+			__atomic_fetch_add(&(w->processed_pkts), 1,
+					__ATOMIC_RELEASE);
		} else {
			ev.queue_id++;
			pipeline_fwd_event(&ev,
sched_type_list[cq_id]);

@@ -214,7 +250,13 @@

pipeline_queue_worker_multi_stage_burst_tx(void *arg)
			if (ev[i].queue_id == tx_queue[ev[i].mbuf-

quoted

port]) {

				pipeline_event_tx(dev, port, &ev[i]);
				ev[i].op = RTE_EVENT_OP_RELEASE;
-				w->processed_pkts++;
+
+				/* release barrier here ensures stored
operation
+				 * of the event completes before the
number of
+				 * processed pkts is visible to the main
core
+				 */
+				__atomic_fetch_add(&(w-

quoted

processed_pkts), 1,

+						__ATOMIC_RELEASE);
				continue;
			}

@@ -254,7 +296,13 @@

pipeline_queue_worker_multi_stage_burst_fwd(void *arg)

rte_event_eth_tx_adapter_txq_set(ev[i].mbuf, 0);
				pipeline_fwd_event(&ev[i],

RTE_SCHED_TYPE_ATOMIC);
-				w->processed_pkts++;
+
+				/* release barrier here ensures stored
operation
+				 * of the event completes before the
number of
+				 * processed pkts is visible to the main
core
+				 */
+				__atomic_fetch_add(&(w-

quoted

processed_pkts), 1,

+						__ATOMIC_RELEASE);
			} else {
				ev[i].queue_id++;
				pipeline_fwd_event(&ev[i],
--
2.17.1