patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH v1 1/6] app/test: fix deadlock in distributor test
       [not found] ` <CGME20200915193457eucas1p2adbe25c41a0e4ef16c029e7bff104503@eucas1p2.samsung.com>
@ 2020-09-15 19:34   ` Lukasz Wojciechowski
  2020-09-17 11:21     ` David Hunt
  0 siblings, 1 reply; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-15 19:34 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

The sanity test with worker shutdown delegates all bufs
to be processed by a single lcore worker, then it freezes
one of the lcore workers and continues to send more bufs.

Problem occurred if freezed lcore is the same as the one
that is processing the mbufs. The lcore processing mbufs
might be different every time test is launched.
This is caused by keeping the value of wkr static variable
in rte_distributor_process function between running test cases.

Test freezed always lcore with 0 id. The patch changes avoids
possible collision by freezing lcore with zero_idx. The lcore
that receives the data updates the zero_idx, so it is not freezed
itself.

To reproduce the issue fixed by this patch, please run
distributor_autotest command in test app several times in a row.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index ba1f81cf8..35b25463a 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -28,6 +28,7 @@ struct worker_params worker_params;
 static volatile int quit;      /**< general quit variable for all threads */
 static volatile int zero_quit; /**< var for when we just want thr0 to quit*/
 static volatile unsigned worker_idx;
+static volatile unsigned zero_idx;
 
 struct worker_stats {
 	volatile unsigned handled_packets;
@@ -346,27 +347,43 @@ handle_work_for_shutdown_test(void *arg)
 	unsigned int total = 0;
 	unsigned int i;
 	unsigned int returned = 0;
+	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
 
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 
+	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+	if (id == zero_id && num > 0) {
+		zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+			__ATOMIC_ACQUIRE);
+		__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+	}
+
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
-	while (!quit && !(id == 0 && zero_quit)) {
+	while (!quit && !(id == zero_id && zero_quit)) {
 		worker_stats[id].handled_packets += num;
 		count += num;
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
+
+		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+		if (id == zero_id && num > 0) {
+			zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+				__ATOMIC_ACQUIRE);
+			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+		}
+
 		total += num;
 	}
 	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
-	if (id == 0) {
+	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
 		 */
@@ -586,6 +603,7 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_eal_mp_wait_lcore();
 	quit = 0;
 	worker_idx = 0;
+	zero_idx = 0;
 }
 
 static int
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v1 2/6] app/test: synchronize statistics between lcores
       [not found] ` <CGME20200915193457eucas1p2321d28b6abf69f244cd7c1e61ed0620e@eucas1p2.samsung.com>
@ 2020-09-15 19:34   ` Lukasz Wojciechowski
  2020-09-17 11:50     ` David Hunt
  0 siblings, 1 reply; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-15 19:34 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Statistics of handled packets are cleared and read on main lcore,
while they are increased in workers handlers on different lcores.

Without synchronization occasionally showed invalid values.
This patch uses atomic acquire/release mechanisms to synchronize.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 39 ++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 35b25463a..0e49e3714 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -43,7 +43,8 @@ total_packet_count(void)
 {
 	unsigned i, count = 0;
 	for (i = 0; i < worker_idx; i++)
-		count += worker_stats[i].handled_packets;
+		count += __atomic_load_n(&worker_stats[i].handled_packets,
+				__ATOMIC_ACQUIRE);
 	return count;
 }
 
@@ -52,6 +53,7 @@ static inline void
 clear_packet_count(void)
 {
 	memset(&worker_stats, 0, sizeof(worker_stats));
+	rte_atomic_thread_fence(__ATOMIC_RELEASE);
 }
 
 /* this is the basic worker function for sanity test
@@ -72,13 +74,13 @@ handle_work(void *arg)
 	num = rte_distributor_get_pkt(db, id, buf, buf, num);
 	while (!quit) {
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-				__ATOMIC_RELAXED);
+				__ATOMIC_ACQ_REL);
 		count += num;
 		num = rte_distributor_get_pkt(db, id,
 				buf, buf, num);
 	}
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-			__ATOMIC_RELAXED);
+			__ATOMIC_ACQ_REL);
 	count += num;
 	rte_distributor_return_pkt(db, id, buf, num);
 	return 0;
@@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with all zero hashes done.\n");
 
 	/* pick two flows and check they go correctly */
@@ -159,7 +162,9 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 		for (i = 0; i < rte_lcore_count() - 1; i++)
 			printf("Worker %u handled %u packets\n", i,
-					worker_stats[i].handled_packets);
+				__atomic_load_n(
+					&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 		printf("Sanity test with two hash values done\n");
 	}
 
@@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with non-zero hashes done\n");
 
 	rte_mempool_put_bulk(p, (void *)bufs, BURST);
@@ -280,15 +286,17 @@ handle_work_with_free_mbufs(void *arg)
 		buf[i] = NULL;
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 	while (!quit) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
@@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
@@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg)
 
 		total += num;
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
@@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg)
 				id, buf, buf, num);
 
 		while (!quit) {
-			worker_stats[id].handled_packets += num;
 			count += num;
 			rte_pktmbuf_free(pkt);
 			num = rte_distributor_get_pkt(d, id, buf, buf, num);
+			__atomic_fetch_add(&worker_stats[id].handled_packets,
+					num, __ATOMIC_ACQ_REL);
 		}
 		returned = rte_distributor_return_pkt(d,
 				id, buf, num);
@@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct worker_params *wp,
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST * 2) {
 		printf("Line %d: Error, not all packets flushed. "
@@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct worker_params *wp,
 	zero_quit = 0;
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST) {
 		printf("Line %d: Error, not all packets flushed. "
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests
       [not found] ` <CGME20200915193458eucas1p1d9308e63063eda28f96eedba3a361a2b@eucas1p1.samsung.com>
@ 2020-09-15 19:34   ` Lukasz Wojciechowski
  2020-09-17 12:34     ` David Hunt
  2020-09-22 12:42     ` [dpdk-stable] [dpdk-dev] " David Marchand
  0 siblings, 2 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-15 19:34 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Sanity tests with mbuf alloc and shutdown tests assume that
mbufs passed to worker cores are freed in handlers.
Such packets should not be returned to the distributor's main
core. The only packets that should be returned are the packets
send after completion of the tests in quit_workers function.

This patch fixes freeing mbufs, stops returning them
to distributor's core and cleans up unused variables.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 0e49e3714..da13a9a3f 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -277,24 +277,21 @@ handle_work_with_free_mbufs(void *arg)
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int i;
 	unsigned int num = 0;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
 
 	for (i = 0; i < 8; i++)
 		buf[i] = NULL;
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 	while (!quit) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 	}
-	count += num;
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
@@ -322,7 +319,6 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 			rte_distributor_process(d, NULL, 0);
 		for (j = 0; j < BURST; j++) {
 			bufs[j]->hash.usr = (i+j) << 1;
-			rte_mbuf_refcnt_set(bufs[j], 1);
 		}
 
 		rte_distributor_process(d, bufs, BURST);
@@ -346,20 +342,15 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 static int
 handle_work_for_shutdown_test(void *arg)
 {
-	struct rte_mbuf *pkt = NULL;
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int num = 0;
-	unsigned int total = 0;
 	unsigned int i;
-	unsigned int returned = 0;
 	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
-
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 
 	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 	if (id == zero_id && num > 0) {
@@ -371,13 +362,12 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 		if (id == zero_id && num > 0) {
@@ -385,12 +375,7 @@ handle_work_for_shutdown_test(void *arg)
 				__ATOMIC_ACQUIRE);
 			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
 		}
-
-		total += num;
 	}
-	count += num;
-	returned = rte_distributor_return_pkt(d, id, buf, num);
-
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
@@ -400,20 +385,20 @@ handle_work_for_shutdown_test(void *arg)
 		while (zero_quit)
 			usleep(100);
 
+		for (i = 0; i < num; i++)
+			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		while (!quit) {
-			count += num;
-			rte_pktmbuf_free(pkt);
-			num = rte_distributor_get_pkt(d, id, buf, buf, num);
 			__atomic_fetch_add(&worker_stats[id].handled_packets,
 					num, __ATOMIC_ACQ_REL);
+			for (i = 0; i < num; i++)
+				rte_pktmbuf_free(buf[i]);
+			num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 		}
-		returned = rte_distributor_return_pkt(d,
-				id, buf, num);
-		printf("Num returned = %d\n", returned);
 	}
+	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v1 4/6] app/test: collect return mbufs in distributor test
       [not found] ` <CGME20200915193459eucas1p19f5d1cbea87d7dc3bbd2638cdb96a31b@eucas1p1.samsung.com>
@ 2020-09-15 19:34   ` Lukasz Wojciechowski
  2020-09-17 12:37     ` David Hunt
  0 siblings, 1 reply; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-15 19:34 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

During quit_workers function distributor's main core processes
some packets to wake up pending worker cores so they can quit.
As quit_workers acts also as a cleanup procedure for next test
case it should also collect these packages returned by workers'
handlers, so the cyclic buffer with returned packets
in distributor remains empty.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index da13a9a3f..13c6397cc 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -599,6 +599,10 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_distributor_process(d, NULL, 0);
 	rte_distributor_flush(d);
 	rte_eal_mp_wait_lcore();
+
+	while (rte_distributor_returned_pkts(d, bufs, RTE_MAX_LCORE))
+		;
+
 	quit = 0;
 	worker_idx = 0;
 	zero_idx = 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v1 5/6] distributor: fix missing handshake synchronization
       [not found] ` <CGME20200915193500eucas1p2b079e1dcfd2d54e01a5630609b82b370@eucas1p2.samsung.com>
@ 2020-09-15 19:34   ` Lukasz Wojciechowski
  2020-09-17 13:22     ` David Hunt
  0 siblings, 1 reply; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-15 19:34 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

rte_distributor_return_pkt function which is run on worker cores
must wait for distributor core to clear handshake on retptr64
before using those buffers. While the handshake is set distributor
core controls buffers and any operations on worker side might overwrite
buffers which are unread yet.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 1c047f065..89493c331 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -160,6 +160,7 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 {
 	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
 	unsigned int i;
+	volatile int64_t *retptr64;
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (num == 1)
@@ -169,6 +170,19 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 			return -EINVAL;
 	}
 
+	retptr64 = &(buf->retptr64[0]);
+	/* Spin while handshake bits are set (scheduler clears it).
+	 * Sync with worker on GET_BUF flag.
+	 */
+	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
+			& RTE_DISTRIB_GET_BUF)) {
+		rte_pause();
+		uint64_t t = rte_rdtsc()+100;
+
+		while (rte_rdtsc() < t)
+			rte_pause();
+	}
+
 	/* Sync with distributor to acquire retptrs */
 	__atomic_thread_fence(__ATOMIC_ACQUIRE);
 	for (i = 0; i < RTE_DIST_BURST_SIZE; i++)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v1 6/6] distributor: fix handshake deadlock
       [not found] ` <CGME20200915193501eucas1p2333f0b08077c06ba04b89ce192072f9a@eucas1p2.samsung.com>
@ 2020-09-15 19:34   ` Lukasz Wojciechowski
  2020-09-17 13:28     ` David Hunt
  0 siblings, 1 reply; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-15 19:34 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Synchronization of data exchange between distributor and worker cores
is based on 2 handshakes: retptr64 for returning mbufs from workers
to distributor and bufptr64 for passing mbufs to workers.

Without proper order of verifying those 2 handshakes a deadlock may
occur. This can happen when worker core want to return back mbufs
and waits for retptr handshake to be cleared and distributor core
wait for bufptr to send mbufs to worker.

This can happen as worker core first returns mbufs to distributor
and later gets new mbufs, while distributor first release mbufs
to worker and later handle returning packets.

This patch fixes possibility of the deadlock by always taking care
of returning packets first on the distributor side and handling
packets while waiting to release new.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 89493c331..12b3db33c 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -321,12 +321,14 @@ release(struct rte_distributor *d, unsigned int wkr)
 	struct rte_distributor_buffer *buf = &(d->bufs[wkr]);
 	unsigned int i;
 
+	handle_returns(d, wkr);
+
 	/* Sync with worker on GET_BUF flag */
 	while (!(__atomic_load_n(&(d->bufs[wkr].bufptr64[0]), __ATOMIC_ACQUIRE)
-		& RTE_DISTRIB_GET_BUF))
+		& RTE_DISTRIB_GET_BUF)) {
+		handle_returns(d, wkr);
 		rte_pause();
-
-	handle_returns(d, wkr);
+	}
 
 	buf->count = 0;
 
@@ -376,6 +378,7 @@ rte_distributor_process(struct rte_distributor *d,
 		/* Flush out all non-full cache-lines to workers. */
 		for (wid = 0 ; wid < d->num_workers; wid++) {
 			/* Sync with worker on GET_BUF flag. */
+			handle_returns(d, wid);
 			if (__atomic_load_n(&(d->bufs[wid].bufptr64[0]),
 				__ATOMIC_ACQUIRE) & RTE_DISTRIB_GET_BUF) {
 				release(d, wid);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [PATCH v1 1/6] app/test: fix deadlock in distributor test
  2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 1/6] app/test: fix deadlock in distributor test Lukasz Wojciechowski
@ 2020-09-17 11:21     ` David Hunt
  2020-09-17 14:01       ` Lukasz Wojciechowski
  0 siblings, 1 reply; 41+ messages in thread
From: David Hunt @ 2020-09-17 11:21 UTC (permalink / raw)
  To: Lukasz Wojciechowski, Bruce Richardson; +Cc: dev, stable

Hi Lukasz,

On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> The sanity test with worker shutdown delegates all bufs
> to be processed by a single lcore worker, then it freezes
> one of the lcore workers and continues to send more bufs.
>
> Problem occurred if freezed lcore is the same as the one
> that is processing the mbufs. The lcore processing mbufs
> might be different every time test is launched.
> This is caused by keeping the value of wkr static variable
> in rte_distributor_process function between running test cases.
>
> Test freezed always lcore with 0 id. The patch changes avoids
> possible collision by freezing lcore with zero_idx. The lcore
> that receives the data updates the zero_idx, so it is not freezed
> itself.
>
> To reproduce the issue fixed by this patch, please run
> distributor_autotest command in test app several times in a row.
>
> Fixes: c3eabff124e6 ("distributor: add unit tests")
> Cc: bruce.richardson@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   app/test/test_distributor.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index ba1f81cf8..35b25463a 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -28,6 +28,7 @@ struct worker_params worker_params;
>   static volatile int quit;      /**< general quit variable for all threads */
>   static volatile int zero_quit; /**< var for when we just want thr0 to quit*/
>   static volatile unsigned worker_idx;
> +static volatile unsigned zero_idx;
>   
>   struct worker_stats {
>   	volatile unsigned handled_packets;
> @@ -346,27 +347,43 @@ handle_work_for_shutdown_test(void *arg)
>   	unsigned int total = 0;
>   	unsigned int i;
>   	unsigned int returned = 0;
> +	unsigned int zero_id = 0;
>   	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
>   			__ATOMIC_RELAXED);
>   
>   	num = rte_distributor_get_pkt(d, id, buf, buf, num);
>   
> +	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
> +	if (id == zero_id && num > 0) {
> +		zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
> +			__ATOMIC_ACQUIRE);
> +		__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
> +	}
> +
>   	/* wait for quit single globally, or for worker zero, wait
>   	 * for zero_quit */
> -	while (!quit && !(id == 0 && zero_quit)) {
> +	while (!quit && !(id == zero_id && zero_quit)) {
>   		worker_stats[id].handled_packets += num;
>   		count += num;
>   		for (i = 0; i < num; i++)
>   			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
>   				id, buf, buf, num);
> +
> +		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
> +		if (id == zero_id && num > 0) {
> +			zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
> +				__ATOMIC_ACQUIRE);
> +			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
> +		}
> +
>   		total += num;
>   	}
>   	worker_stats[id].handled_packets += num;
>   	count += num;
>   	returned = rte_distributor_return_pkt(d, id, buf, num);
>   
> -	if (id == 0) {
> +	if (id == zero_id) {
>   		/* for worker zero, allow it to restart to pick up last packet
>   		 * when all workers are shutting down.
>   		 */
> @@ -586,6 +603,7 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
>   	rte_eal_mp_wait_lcore();
>   	quit = 0;
>   	worker_idx = 0;
> +	zero_idx = 0;
>   }
>   
>   static int


Lockup reproducable if you run the distributor_autotest 19 times in 
succesion. I was able to run the test many times more than that with the 
patch applied. Thanks.

Tested-by: David Hunt <david.hunt@intel.com>





^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [PATCH v1 2/6] app/test: synchronize statistics between lcores
  2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 2/6] app/test: synchronize statistics between lcores Lukasz Wojciechowski
@ 2020-09-17 11:50     ` David Hunt
  0 siblings, 0 replies; 41+ messages in thread
From: David Hunt @ 2020-09-17 11:50 UTC (permalink / raw)
  To: Lukasz Wojciechowski, Bruce Richardson; +Cc: dev, stable


On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> Statistics of handled packets are cleared and read on main lcore,
> while they are increased in workers handlers on different lcores.
>
> Without synchronization occasionally showed invalid values.
> This patch uses atomic acquire/release mechanisms to synchronize.
>
> Fixes: c3eabff124e6 ("distributor: add unit tests")
> Cc: bruce.richardson@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   app/test/test_distributor.c | 39 ++++++++++++++++++++++++-------------
>   1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index 35b25463a..0e49e3714 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -43,7 +43,8 @@ total_packet_count(void)
>   {
>   	unsigned i, count = 0;
>   	for (i = 0; i < worker_idx; i++)
> -		count += worker_stats[i].handled_packets;
> +		count += __atomic_load_n(&worker_stats[i].handled_packets,
> +				__ATOMIC_ACQUIRE);
>   	return count;
>   }
>   
> @@ -52,6 +53,7 @@ static inline void
>   clear_packet_count(void)
>   {
>   	memset(&worker_stats, 0, sizeof(worker_stats));
> +	rte_atomic_thread_fence(__ATOMIC_RELEASE);
>   }
>   
>   /* this is the basic worker function for sanity test
> @@ -72,13 +74,13 @@ handle_work(void *arg)
>   	num = rte_distributor_get_pkt(db, id, buf, buf, num);
>   	while (!quit) {
>   		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> -				__ATOMIC_RELAXED);
> +				__ATOMIC_ACQ_REL);
>   		count += num;
>   		num = rte_distributor_get_pkt(db, id,
>   				buf, buf, num);
>   	}
>   	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> -			__ATOMIC_RELAXED);
> +			__ATOMIC_ACQ_REL);
>   	count += num;
>   	rte_distributor_return_pkt(db, id, buf, num);
>   	return 0;
> @@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   
>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>   		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>   	printf("Sanity test with all zero hashes done.\n");
>   
>   	/* pick two flows and check they go correctly */
> @@ -159,7 +162,9 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   
>   		for (i = 0; i < rte_lcore_count() - 1; i++)
>   			printf("Worker %u handled %u packets\n", i,
> -					worker_stats[i].handled_packets);
> +				__atomic_load_n(
> +					&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>   		printf("Sanity test with two hash values done\n");
>   	}
>   
> @@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>   
>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>   		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>   	printf("Sanity test with non-zero hashes done\n");
>   
>   	rte_mempool_put_bulk(p, (void *)bufs, BURST);
> @@ -280,15 +286,17 @@ handle_work_with_free_mbufs(void *arg)
>   		buf[i] = NULL;
>   	num = rte_distributor_get_pkt(d, id, buf, buf, num);
>   	while (!quit) {
> -		worker_stats[id].handled_packets += num;
>   		count += num;
> +		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> +				__ATOMIC_ACQ_REL);
>   		for (i = 0; i < num; i++)
>   			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
>   				id, buf, buf, num);
>   	}
> -	worker_stats[id].handled_packets += num;
>   	count += num;
> +	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> +			__ATOMIC_ACQ_REL);
>   	rte_distributor_return_pkt(d, id, buf, num);
>   	return 0;
>   }
> @@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg)
>   	/* wait for quit single globally, or for worker zero, wait
>   	 * for zero_quit */
>   	while (!quit && !(id == zero_id && zero_quit)) {
> -		worker_stats[id].handled_packets += num;
>   		count += num;
> +		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> +				__ATOMIC_ACQ_REL);
>   		for (i = 0; i < num; i++)
>   			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
> @@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg)
>   
>   		total += num;
>   	}
> -	worker_stats[id].handled_packets += num;
>   	count += num;
>   	returned = rte_distributor_return_pkt(d, id, buf, num);
>   
> +	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> +			__ATOMIC_ACQ_REL);
>   	if (id == zero_id) {
>   		/* for worker zero, allow it to restart to pick up last packet
>   		 * when all workers are shutting down.
> @@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg)
>   				id, buf, buf, num);
>   
>   		while (!quit) {
> -			worker_stats[id].handled_packets += num;
>   			count += num;
>   			rte_pktmbuf_free(pkt);
>   			num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +			__atomic_fetch_add(&worker_stats[id].handled_packets,
> +					num, __ATOMIC_ACQ_REL);
>   		}
>   		returned = rte_distributor_return_pkt(d,
>   				id, buf, num);
> @@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct worker_params *wp,
>   
>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>   		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>   
>   	if (total_packet_count() != BURST * 2) {
>   		printf("Line %d: Error, not all packets flushed. "
> @@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct worker_params *wp,
>   	zero_quit = 0;
>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>   		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>   
>   	if (total_packet_count() != BURST) {
>   		printf("Line %d: Error, not all packets flushed. "


Thanks.

Acked-by: David Hunt <david.hunt@intel.com>



^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests
  2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests Lukasz Wojciechowski
@ 2020-09-17 12:34     ` David Hunt
  2020-09-22 12:42     ` [dpdk-stable] [dpdk-dev] " David Marchand
  1 sibling, 0 replies; 41+ messages in thread
From: David Hunt @ 2020-09-17 12:34 UTC (permalink / raw)
  To: Lukasz Wojciechowski, Bruce Richardson; +Cc: dev, stable

Hi Lukasz,

On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> Sanity tests with mbuf alloc and shutdown tests assume that
> mbufs passed to worker cores are freed in handlers.
> Such packets should not be returned to the distributor's main
> core. The only packets that should be returned are the packets
> send after completion of the tests in quit_workers function.
>
> This patch fixes freeing mbufs, stops returning them
> to distributor's core and cleans up unused variables.
>
> Fixes: c0de0eb82e40 ("distributor: switch over to new API")
> Cc: david.hunt@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   app/test/test_distributor.c | 37 +++++++++++--------------------------
>   1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index 0e49e3714..da13a9a3f 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -277,24 +277,21 @@ handle_work_with_free_mbufs(void *arg)
>   	struct rte_mbuf *buf[8] __rte_cache_aligned;
>   	struct worker_params *wp = arg;
>   	struct rte_distributor *d = wp->dist;
> -	unsigned int count = 0;
>   	unsigned int i;
>   	unsigned int num = 0;
>   	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
>   
>   	for (i = 0; i < 8; i++)
>   		buf[i] = NULL;
> -	num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
>   	while (!quit) {
> -		count += num;
>   		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   				__ATOMIC_ACQ_REL);
>   		for (i = 0; i < num; i++)
>   			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
> -				id, buf, buf, num);
> +				id, buf, buf, 0);
>   	}
> -	count += num;
>   	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   			__ATOMIC_ACQ_REL);
>   	rte_distributor_return_pkt(d, id, buf, num);
> @@ -322,7 +319,6 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
>   			rte_distributor_process(d, NULL, 0);
>   		for (j = 0; j < BURST; j++) {
>   			bufs[j]->hash.usr = (i+j) << 1;
> -			rte_mbuf_refcnt_set(bufs[j], 1);
>   		}
>   
>   		rte_distributor_process(d, bufs, BURST);
> @@ -346,20 +342,15 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
>   static int
>   handle_work_for_shutdown_test(void *arg)
>   {
> -	struct rte_mbuf *pkt = NULL;
>   	struct rte_mbuf *buf[8] __rte_cache_aligned;
>   	struct worker_params *wp = arg;
>   	struct rte_distributor *d = wp->dist;
> -	unsigned int count = 0;
>   	unsigned int num = 0;
> -	unsigned int total = 0;
>   	unsigned int i;
> -	unsigned int returned = 0;
>   	unsigned int zero_id = 0;
>   	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
>   			__ATOMIC_RELAXED);
> -
> -	num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
>   
>   	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
>   	if (id == zero_id && num > 0) {
> @@ -371,13 +362,12 @@ handle_work_for_shutdown_test(void *arg)
>   	/* wait for quit single globally, or for worker zero, wait
>   	 * for zero_quit */
>   	while (!quit && !(id == zero_id && zero_quit)) {
> -		count += num;
>   		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   				__ATOMIC_ACQ_REL);
>   		for (i = 0; i < num; i++)
>   			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
> -				id, buf, buf, num);
> +				id, buf, buf, 0);
>   
>   		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
>   		if (id == zero_id && num > 0) {
> @@ -385,12 +375,7 @@ handle_work_for_shutdown_test(void *arg)
>   				__ATOMIC_ACQUIRE);
>   			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
>   		}
> -
> -		total += num;
>   	}
> -	count += num;
> -	returned = rte_distributor_return_pkt(d, id, buf, num);
> -
>   	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>   			__ATOMIC_ACQ_REL);
>   	if (id == zero_id) {
> @@ -400,20 +385,20 @@ handle_work_for_shutdown_test(void *arg)
>   		while (zero_quit)
>   			usleep(100);
>   
> +		for (i = 0; i < num; i++)
> +			rte_pktmbuf_free(buf[i]);
>   		num = rte_distributor_get_pkt(d,
> -				id, buf, buf, num);
> +				id, buf, buf, 0);
>   
>   		while (!quit) {
> -			count += num;
> -			rte_pktmbuf_free(pkt);
> -			num = rte_distributor_get_pkt(d, id, buf, buf, num);
>   			__atomic_fetch_add(&worker_stats[id].handled_packets,
>   					num, __ATOMIC_ACQ_REL);
> +			for (i = 0; i < num; i++)
> +				rte_pktmbuf_free(buf[i]);
> +			num = rte_distributor_get_pkt(d, id, buf, buf, 0);
>   		}
> -		returned = rte_distributor_return_pkt(d,
> -				id, buf, num);
> -		printf("Num returned = %d\n", returned);
>   	}
> +	rte_distributor_return_pkt(d, id, buf, num);
>   	return 0;
>   }
>   

Nice cleanup, Thanks.

Acked-by: David Hunt <david.hunt@intel.com>






^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [PATCH v1 4/6] app/test: collect return mbufs in distributor test
  2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 4/6] app/test: collect return mbufs in distributor test Lukasz Wojciechowski
@ 2020-09-17 12:37     ` David Hunt
  0 siblings, 0 replies; 41+ messages in thread
From: David Hunt @ 2020-09-17 12:37 UTC (permalink / raw)
  To: Lukasz Wojciechowski, Bruce Richardson; +Cc: dev, stable

Hi Lukasz,

On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> During quit_workers function distributor's main core processes
> some packets to wake up pending worker cores so they can quit.
> As quit_workers acts also as a cleanup procedure for next test
> case it should also collect these packages returned by workers'
> handlers, so the cyclic buffer with returned packets
> in distributor remains empty.
>
> Fixes: c3eabff124e6 ("distributor: add unit tests")
> Cc: bruce.richardson@intel.com
> Fixes: c0de0eb82e40 ("distributor: switch over to new API")
> Cc: david.hunt@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   app/test/test_distributor.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index da13a9a3f..13c6397cc 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -599,6 +599,10 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
>   	rte_distributor_process(d, NULL, 0);
>   	rte_distributor_flush(d);
>   	rte_eal_mp_wait_lcore();
> +
> +	while (rte_distributor_returned_pkts(d, bufs, RTE_MAX_LCORE))
> +		;
> +
>   	quit = 0;
>   	worker_idx = 0;
>   	zero_idx = 0;


Acked-by: David Hunt <david.hunt@intel.com>




^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [PATCH v1 5/6] distributor: fix missing handshake synchronization
  2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 5/6] distributor: fix missing handshake synchronization Lukasz Wojciechowski
@ 2020-09-17 13:22     ` David Hunt
  0 siblings, 0 replies; 41+ messages in thread
From: David Hunt @ 2020-09-17 13:22 UTC (permalink / raw)
  To: Lukasz Wojciechowski, Bruce Richardson; +Cc: dev, stable

Hi Lukasz,

On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> rte_distributor_return_pkt function which is run on worker cores
> must wait for distributor core to clear handshake on retptr64
> before using those buffers. While the handshake is set distributor
> core controls buffers and any operations on worker side might overwrite
> buffers which are unread yet.
>
> Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
> Cc: david.hunt@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   lib/librte_distributor/rte_distributor.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
> index 1c047f065..89493c331 100644
> --- a/lib/librte_distributor/rte_distributor.c
> +++ b/lib/librte_distributor/rte_distributor.c
> @@ -160,6 +160,7 @@ rte_distributor_return_pkt(struct rte_distributor *d,
>   {
>   	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
>   	unsigned int i;
> +	volatile int64_t *retptr64;
>   
>   	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
>   		if (num == 1)
> @@ -169,6 +170,19 @@ rte_distributor_return_pkt(struct rte_distributor *d,
>   			return -EINVAL;
>   	}
>   
> +	retptr64 = &(buf->retptr64[0]);
> +	/* Spin while handshake bits are set (scheduler clears it).
> +	 * Sync with worker on GET_BUF flag.
> +	 */
> +	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
> +			& RTE_DISTRIB_GET_BUF)) {
> +		rte_pause();
> +		uint64_t t = rte_rdtsc()+100;
> +
> +		while (rte_rdtsc() < t)
> +			rte_pause();
> +	}
> +


The 'unlikely' is appropriate, but when it does occur, this looks to be 
a necessary addition.
And I've confirmed no loss in performance on my system.

Acked-by: David Hunt <david.hunt@intel.com>





^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [PATCH v1 6/6] distributor: fix handshake deadlock
  2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 6/6] distributor: fix handshake deadlock Lukasz Wojciechowski
@ 2020-09-17 13:28     ` David Hunt
  0 siblings, 0 replies; 41+ messages in thread
From: David Hunt @ 2020-09-17 13:28 UTC (permalink / raw)
  To: Lukasz Wojciechowski, Bruce Richardson; +Cc: dev, stable

Hi Lukasz,

On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
> Synchronization of data exchange between distributor and worker cores
> is based on 2 handshakes: retptr64 for returning mbufs from workers
> to distributor and bufptr64 for passing mbufs to workers.
>
> Without proper order of verifying those 2 handshakes a deadlock may
> occur. This can happen when worker core want to return back mbufs
> and waits for retptr handshake to be cleared and distributor core
> wait for bufptr to send mbufs to worker.
>
> This can happen as worker core first returns mbufs to distributor
> and later gets new mbufs, while distributor first release mbufs
> to worker and later handle returning packets.
>
> This patch fixes possibility of the deadlock by always taking care
> of returning packets first on the distributor side and handling
> packets while waiting to release new.
>
> Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
> Cc: david.hunt@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>   lib/librte_distributor/rte_distributor.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
> index 89493c331..12b3db33c 100644
> --- a/lib/librte_distributor/rte_distributor.c
> +++ b/lib/librte_distributor/rte_distributor.c
> @@ -321,12 +321,14 @@ release(struct rte_distributor *d, unsigned int wkr)
>   	struct rte_distributor_buffer *buf = &(d->bufs[wkr]);
>   	unsigned int i;
>   
> +	handle_returns(d, wkr);
> +
>   	/* Sync with worker on GET_BUF flag */
>   	while (!(__atomic_load_n(&(d->bufs[wkr].bufptr64[0]), __ATOMIC_ACQUIRE)
> -		& RTE_DISTRIB_GET_BUF))
> +		& RTE_DISTRIB_GET_BUF)) {
> +		handle_returns(d, wkr);
>   		rte_pause();
> -
> -	handle_returns(d, wkr);
> +	}
>   
>   	buf->count = 0;
>   
> @@ -376,6 +378,7 @@ rte_distributor_process(struct rte_distributor *d,
>   		/* Flush out all non-full cache-lines to workers. */
>   		for (wid = 0 ; wid < d->num_workers; wid++) {
>   			/* Sync with worker on GET_BUF flag. */
> +			handle_returns(d, wid);
>   			if (__atomic_load_n(&(d->bufs[wid].bufptr64[0]),
>   				__ATOMIC_ACQUIRE) & RTE_DISTRIB_GET_BUF) {
>   				release(d, wid);

Makes sense. Thanks for the series.  Again, no degradation in 
performance on my systems.

Acked-by: David Hunt <david.hunt@intel.com>




^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [PATCH v1 1/6] app/test: fix deadlock in distributor test
  2020-09-17 11:21     ` David Hunt
@ 2020-09-17 14:01       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-17 14:01 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson
  Cc: dev, stable, \"'Lukasz Wojciechowski'\",

Hi David,

W dniu 17.09.2020 o 13:21, David Hunt pisze:
> Hi Lukasz,
>
> On 15/9/2020 8:34 PM, Lukasz Wojciechowski wrote:
>> The sanity test with worker shutdown delegates all bufs
>> to be processed by a single lcore worker, then it freezes
>> one of the lcore workers and continues to send more bufs.
>>
>> Problem occurred if freezed lcore is the same as the one
>> that is processing the mbufs. The lcore processing mbufs
>> might be different every time test is launched.
>> This is caused by keeping the value of wkr static variable
>> in rte_distributor_process function between running test cases.
>>
>> Test freezed always lcore with 0 id. The patch changes avoids
>> possible collision by freezing lcore with zero_idx. The lcore
>> that receives the data updates the zero_idx, so it is not freezed
>> itself.
>>
>> To reproduce the issue fixed by this patch, please run
>> distributor_autotest command in test app several times in a row.
>>
>> Fixes: c3eabff124e6 ("distributor: add unit tests")
>> Cc: bruce.richardson@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> ---
>>   app/test/test_distributor.c | 22 ++++++++++++++++++++--
>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
>> index ba1f81cf8..35b25463a 100644
>> --- a/app/test/test_distributor.c
>> +++ b/app/test/test_distributor.c
>> @@ -28,6 +28,7 @@ struct worker_params worker_params;
>>   static volatile int quit;      /**< general quit variable for all 
>> threads */
>>   static volatile int zero_quit; /**< var for when we just want thr0 
>> to quit*/
>>   static volatile unsigned worker_idx;
>> +static volatile unsigned zero_idx;
>>     struct worker_stats {
>>       volatile unsigned handled_packets;
>> @@ -346,27 +347,43 @@ handle_work_for_shutdown_test(void *arg)
>>       unsigned int total = 0;
>>       unsigned int i;
>>       unsigned int returned = 0;
>> +    unsigned int zero_id = 0;
>>       const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
>>               __ATOMIC_RELAXED);
>>         num = rte_distributor_get_pkt(d, id, buf, buf, num);
>>   +    zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
>> +    if (id == zero_id && num > 0) {
>> +        zero_id = (zero_id + 1) % __atomic_load_n(&worker_idx,
>> +            __ATOMIC_ACQUIRE);
>> +        __atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
>> +    }
>> +
>>       /* wait for quit single globally, or for worker zero, wait
>>        * for zero_quit */
>> -    while (!quit && !(id == 0 && zero_quit)) {
>> +    while (!quit && !(id == zero_id && zero_quit)) {
>>           worker_stats[id].handled_packets += num;
>>           count += num;
>>           for (i = 0; i < num; i++)
>>               rte_pktmbuf_free(buf[i]);
>>           num = rte_distributor_get_pkt(d,
>>                   id, buf, buf, num);
>> +
>> +        zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
>> +        if (id == zero_id && num > 0) {
>> +            zero_id = (zero_id + 1) % __atomic_load_n(&worker_idx,
>> +                __ATOMIC_ACQUIRE);
>> +            __atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
>> +        }
>> +
>>           total += num;
>>       }
>>       worker_stats[id].handled_packets += num;
>>       count += num;
>>       returned = rte_distributor_return_pkt(d, id, buf, num);
>>   -    if (id == 0) {
>> +    if (id == zero_id) {
>>           /* for worker zero, allow it to restart to pick up last packet
>>            * when all workers are shutting down.
>>            */
>> @@ -586,6 +603,7 @@ quit_workers(struct worker_params *wp, struct 
>> rte_mempool *p)
>>       rte_eal_mp_wait_lcore();
>>       quit = 0;
>>       worker_idx = 0;
>> +    zero_idx = 0;
>>   }
>>     static int
>
>
> Lockup reproducable if you run the distributor_autotest 19 times in 
> succesion. I was able to run the test many times more than that with 
> the patch applied. Thanks.
The number depends on number of lcores on your test environment.
>
> Tested-by: David Hunt <david.hunt@intel.com>


Thank you very much for reviewing and testing whole series.

>
>
>
-- 
Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests
  2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests Lukasz Wojciechowski
  2020-09-17 12:34     ` David Hunt
@ 2020-09-22 12:42     ` " David Marchand
  2020-09-23  1:55       ` Lukasz Wojciechowski
  1 sibling, 1 reply; 41+ messages in thread
From: David Marchand @ 2020-09-22 12:42 UTC (permalink / raw)
  To: Lukasz Wojciechowski, David Hunt; +Cc: Bruce Richardson, dev, dpdk stable

Hello Lukasz, David,


On Tue, Sep 15, 2020 at 9:35 PM Lukasz Wojciechowski
<l.wojciechow@partner.samsung.com> wrote:
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index 0e49e3714..da13a9a3f 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -277,24 +277,21 @@ handle_work_with_free_mbufs(void *arg)
>         struct rte_mbuf *buf[8] __rte_cache_aligned;
>         struct worker_params *wp = arg;
>         struct rte_distributor *d = wp->dist;
> -       unsigned int count = 0;
>         unsigned int i;
>         unsigned int num = 0;
>         unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
>
>         for (i = 0; i < 8; i++)
>                 buf[i] = NULL;
> -       num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +       num = rte_distributor_get_pkt(d, id, buf, buf, 0);

For my understanding, we pass an array even if we return 0 packet. Is
this necessary?


>         while (!quit) {
> -               count += num;
>                 __atomic_fetch_add(&worker_stats[id].handled_packets, num,
>                                 __ATOMIC_ACQ_REL);
>                 for (i = 0; i < num; i++)
>                         rte_pktmbuf_free(buf[i]);
>                 num = rte_distributor_get_pkt(d,
> -                               id, buf, buf, num);
> +                               id, buf, buf, 0);

Here, it gives the impression we have some potential use-after-free on
buf[] content.
And trying to pass NULL, I can see the distributor library
dereferences oldpkt[] without checking retcount != 0.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 1/8] app/test: fix deadlock in distributor test
       [not found]   ` <CGME20200923014718eucas1p11fdcd774fef7b9e077e14e01c9f951d5@eucas1p1.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

The sanity test with worker shutdown delegates all bufs
to be processed by a single lcore worker, then it freezes
one of the lcore workers and continues to send more bufs.

Problem occurred if freezed lcore is the same as the one
that is processing the mbufs. The lcore processing mbufs
might be different every time test is launched.
This is caused by keeping the value of wkr static variable
in rte_distributor_process function between running test cases.

Test freezed always lcore with 0 id. The patch changes avoids
possible collision by freezing lcore with zero_idx. The lcore
that receives the data updates the zero_idx, so it is not freezed
itself.

To reproduce the issue fixed by this patch, please run
distributor_autotest command in test app several times in a row.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index ba1f81cf8..35b25463a 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -28,6 +28,7 @@ struct worker_params worker_params;
 static volatile int quit;      /**< general quit variable for all threads */
 static volatile int zero_quit; /**< var for when we just want thr0 to quit*/
 static volatile unsigned worker_idx;
+static volatile unsigned zero_idx;
 
 struct worker_stats {
 	volatile unsigned handled_packets;
@@ -346,27 +347,43 @@ handle_work_for_shutdown_test(void *arg)
 	unsigned int total = 0;
 	unsigned int i;
 	unsigned int returned = 0;
+	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
 
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 
+	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+	if (id == zero_id && num > 0) {
+		zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+			__ATOMIC_ACQUIRE);
+		__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+	}
+
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
-	while (!quit && !(id == 0 && zero_quit)) {
+	while (!quit && !(id == zero_id && zero_quit)) {
 		worker_stats[id].handled_packets += num;
 		count += num;
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
+
+		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+		if (id == zero_id && num > 0) {
+			zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+				__ATOMIC_ACQUIRE);
+			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+		}
+
 		total += num;
 	}
 	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
-	if (id == 0) {
+	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
 		 */
@@ -586,6 +603,7 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_eal_mp_wait_lcore();
 	quit = 0;
 	worker_idx = 0;
+	zero_idx = 0;
 }
 
 static int
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 2/8] app/test: synchronize statistics between lcores
       [not found]   ` <CGME20200923014719eucas1p2f26000109e86a649796e902c30e58bf0@eucas1p2.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  2020-09-23  4:30       ` [dpdk-stable] [dpdk-dev] " Honnappa Nagarahalli
  0 siblings, 1 reply; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Statistics of handled packets are cleared and read on main lcore,
while they are increased in workers handlers on different lcores.

Without synchronization occasionally showed invalid values.
This patch uses atomic acquire/release mechanisms to synchronize.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 39 ++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 35b25463a..0e49e3714 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -43,7 +43,8 @@ total_packet_count(void)
 {
 	unsigned i, count = 0;
 	for (i = 0; i < worker_idx; i++)
-		count += worker_stats[i].handled_packets;
+		count += __atomic_load_n(&worker_stats[i].handled_packets,
+				__ATOMIC_ACQUIRE);
 	return count;
 }
 
@@ -52,6 +53,7 @@ static inline void
 clear_packet_count(void)
 {
 	memset(&worker_stats, 0, sizeof(worker_stats));
+	rte_atomic_thread_fence(__ATOMIC_RELEASE);
 }
 
 /* this is the basic worker function for sanity test
@@ -72,13 +74,13 @@ handle_work(void *arg)
 	num = rte_distributor_get_pkt(db, id, buf, buf, num);
 	while (!quit) {
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-				__ATOMIC_RELAXED);
+				__ATOMIC_ACQ_REL);
 		count += num;
 		num = rte_distributor_get_pkt(db, id,
 				buf, buf, num);
 	}
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-			__ATOMIC_RELAXED);
+			__ATOMIC_ACQ_REL);
 	count += num;
 	rte_distributor_return_pkt(db, id, buf, num);
 	return 0;
@@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with all zero hashes done.\n");
 
 	/* pick two flows and check they go correctly */
@@ -159,7 +162,9 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 		for (i = 0; i < rte_lcore_count() - 1; i++)
 			printf("Worker %u handled %u packets\n", i,
-					worker_stats[i].handled_packets);
+				__atomic_load_n(
+					&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 		printf("Sanity test with two hash values done\n");
 	}
 
@@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with non-zero hashes done\n");
 
 	rte_mempool_put_bulk(p, (void *)bufs, BURST);
@@ -280,15 +286,17 @@ handle_work_with_free_mbufs(void *arg)
 		buf[i] = NULL;
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 	while (!quit) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
@@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
@@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg)
 
 		total += num;
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
@@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg)
 				id, buf, buf, num);
 
 		while (!quit) {
-			worker_stats[id].handled_packets += num;
 			count += num;
 			rte_pktmbuf_free(pkt);
 			num = rte_distributor_get_pkt(d, id, buf, buf, num);
+			__atomic_fetch_add(&worker_stats[id].handled_packets,
+					num, __ATOMIC_ACQ_REL);
 		}
 		returned = rte_distributor_return_pkt(d,
 				id, buf, num);
@@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct worker_params *wp,
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST * 2) {
 		printf("Line %d: Error, not all packets flushed. "
@@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct worker_params *wp,
 	zero_quit = 0;
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST) {
 		printf("Line %d: Error, not all packets flushed. "
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 3/8] app/test: fix freeing mbufs in distributor tests
       [not found]   ` <CGME20200923014719eucas1p165c419cff4f265cff8add8cc818210ff@eucas1p1.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Sanity tests with mbuf alloc and shutdown tests assume that
mbufs passed to worker cores are freed in handlers.
Such packets should not be returned to the distributor's main
core. The only packets that should be returned are the packets
send after completion of the tests in quit_workers function.

This patch fixes freeing mbufs, stops returning them
to distributor's core and cleans up unused variables.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 50 +++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 0e49e3714..94b65b382 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -277,24 +277,23 @@ handle_work_with_free_mbufs(void *arg)
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int i;
 	unsigned int num = 0;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
 
 	for (i = 0; i < 8; i++)
 		buf[i] = NULL;
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 	while (!quit) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num; i++) {
 			rte_pktmbuf_free(buf[i]);
+			buf[i] = NULL;
+		}
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 	}
-	count += num;
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
@@ -322,7 +321,6 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 			rte_distributor_process(d, NULL, 0);
 		for (j = 0; j < BURST; j++) {
 			bufs[j]->hash.usr = (i+j) << 1;
-			rte_mbuf_refcnt_set(bufs[j], 1);
 		}
 
 		rte_distributor_process(d, bufs, BURST);
@@ -346,20 +344,18 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 static int
 handle_work_for_shutdown_test(void *arg)
 {
-	struct rte_mbuf *pkt = NULL;
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int num = 0;
-	unsigned int total = 0;
 	unsigned int i;
-	unsigned int returned = 0;
 	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
+	for (i = 0; i < 8; i++)
+		buf[i] = NULL;
 
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 
 	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 	if (id == zero_id && num > 0) {
@@ -371,13 +367,14 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num; i++) {
 			rte_pktmbuf_free(buf[i]);
+			buf[i] = NULL;
+		}
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 		if (id == zero_id && num > 0) {
@@ -385,12 +382,7 @@ handle_work_for_shutdown_test(void *arg)
 				__ATOMIC_ACQUIRE);
 			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
 		}
-
-		total += num;
 	}
-	count += num;
-	returned = rte_distributor_return_pkt(d, id, buf, num);
-
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
@@ -400,20 +392,24 @@ handle_work_for_shutdown_test(void *arg)
 		while (zero_quit)
 			usleep(100);
 
+		for (i = 0; i < num; i++) {
+			rte_pktmbuf_free(buf[i]);
+			buf[i] = NULL;
+		}
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		while (!quit) {
-			count += num;
-			rte_pktmbuf_free(pkt);
-			num = rte_distributor_get_pkt(d, id, buf, buf, num);
 			__atomic_fetch_add(&worker_stats[id].handled_packets,
 					num, __ATOMIC_ACQ_REL);
+			for (i = 0; i < num; i++) {
+				rte_pktmbuf_free(buf[i]);
+				buf[i] = NULL;
+			}
+			num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 		}
-		returned = rte_distributor_return_pkt(d,
-				id, buf, num);
-		printf("Num returned = %d\n", returned);
 	}
+	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 4/8] app/test: collect return mbufs in distributor test
       [not found]   ` <CGME20200923014720eucas1p2bd5887c96c24839f364810a1bbe840da@eucas1p2.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

During quit_workers function distributor's main core processes
some packets to wake up pending worker cores so they can quit.
As quit_workers acts also as a cleanup procedure for next test
case it should also collect these packages returned by workers'
handlers, so the cyclic buffer with returned packets
in distributor remains empty.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 94b65b382..f31b54edf 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -610,6 +610,10 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_distributor_process(d, NULL, 0);
 	rte_distributor_flush(d);
 	rte_eal_mp_wait_lcore();
+
+	while (rte_distributor_returned_pkts(d, bufs, RTE_MAX_LCORE))
+		;
+
 	quit = 0;
 	worker_idx = 0;
 	zero_idx = 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 5/8] distributor: fix missing handshake synchronization
       [not found]   ` <CGME20200923014721eucas1p1d22ac56c9b9e4fb49ac73d72d51a7a23@eucas1p1.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

rte_distributor_return_pkt function which is run on worker cores
must wait for distributor core to clear handshake on retptr64
before using those buffers. While the handshake is set distributor
core controls buffers and any operations on worker side might overwrite
buffers which are unread yet.
Same situation appears in the legacy single distributor. Function
rte_distributor_return_pkt_single shouldn't modify the bufptr64 until
handshake on it is cleared by distributor lcore.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.c        | 14 ++++++++++++++
 lib/librte_distributor/rte_distributor_single.c |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 1c047f065..89493c331 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -160,6 +160,7 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 {
 	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
 	unsigned int i;
+	volatile int64_t *retptr64;
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (num == 1)
@@ -169,6 +170,19 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 			return -EINVAL;
 	}
 
+	retptr64 = &(buf->retptr64[0]);
+	/* Spin while handshake bits are set (scheduler clears it).
+	 * Sync with worker on GET_BUF flag.
+	 */
+	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
+			& RTE_DISTRIB_GET_BUF)) {
+		rte_pause();
+		uint64_t t = rte_rdtsc()+100;
+
+		while (rte_rdtsc() < t)
+			rte_pause();
+	}
+
 	/* Sync with distributor to acquire retptrs */
 	__atomic_thread_fence(__ATOMIC_ACQUIRE);
 	for (i = 0; i < RTE_DIST_BURST_SIZE; i++)
diff --git a/lib/librte_distributor/rte_distributor_single.c b/lib/librte_distributor/rte_distributor_single.c
index abaf7730c..f4725b1d0 100644
--- a/lib/librte_distributor/rte_distributor_single.c
+++ b/lib/librte_distributor/rte_distributor_single.c
@@ -74,6 +74,10 @@ rte_distributor_return_pkt_single(struct rte_distributor_single *d,
 	union rte_distributor_buffer_single *buf = &d->bufs[worker_id];
 	uint64_t req = (((int64_t)(uintptr_t)oldpkt) << RTE_DISTRIB_FLAG_BITS)
 			| RTE_DISTRIB_RETURN_BUF;
+	while (unlikely(__atomic_load_n(&buf->bufptr64, __ATOMIC_RELAXED)
+			& RTE_DISTRIB_FLAGS_MASK))
+		rte_pause();
+
 	/* Sync with distributor on RETURN_BUF flag. */
 	__atomic_store_n(&(buf->bufptr64), req, __ATOMIC_RELEASE);
 	return 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 6/8] distributor: fix handshake deadlock
       [not found]   ` <CGME20200923014722eucas1p2c2ef63759f4b800c1b5a80094e07e384@eucas1p2.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Synchronization of data exchange between distributor and worker cores
is based on 2 handshakes: retptr64 for returning mbufs from workers
to distributor and bufptr64 for passing mbufs to workers.

Without proper order of verifying those 2 handshakes a deadlock may
occur. This can happen when worker core want to return back mbufs
and waits for retptr handshake to be cleared and distributor core
wait for bufptr to send mbufs to worker.

This can happen as worker core first returns mbufs to distributor
and later gets new mbufs, while distributor first release mbufs
to worker and later handle returning packets.

This patch fixes possibility of the deadlock by always taking care
of returning packets first on the distributor side and handling
packets while waiting to release new.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 89493c331..12b3db33c 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -321,12 +321,14 @@ release(struct rte_distributor *d, unsigned int wkr)
 	struct rte_distributor_buffer *buf = &(d->bufs[wkr]);
 	unsigned int i;
 
+	handle_returns(d, wkr);
+
 	/* Sync with worker on GET_BUF flag */
 	while (!(__atomic_load_n(&(d->bufs[wkr].bufptr64[0]), __ATOMIC_ACQUIRE)
-		& RTE_DISTRIB_GET_BUF))
+		& RTE_DISTRIB_GET_BUF)) {
+		handle_returns(d, wkr);
 		rte_pause();
-
-	handle_returns(d, wkr);
+	}
 
 	buf->count = 0;
 
@@ -376,6 +378,7 @@ rte_distributor_process(struct rte_distributor *d,
 		/* Flush out all non-full cache-lines to workers. */
 		for (wid = 0 ; wid < d->num_workers; wid++) {
 			/* Sync with worker on GET_BUF flag. */
+			handle_returns(d, wid);
 			if (__atomic_load_n(&(d->bufs[wid].bufptr64[0]),
 				__ATOMIC_ACQUIRE) & RTE_DISTRIB_GET_BUF) {
 				release(d, wid);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 7/8] distributor: do not use oldpkt when not needed
       [not found]   ` <CGME20200923014723eucas1p2a7c7210a55289b3739faff4f5ed72e30@eucas1p2.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

rte_distributor_request_pkt and rte_distributor_get_pkt dereferenced
oldpkt parameter when in RTE_DIST_ALG_SINGLE even if number
of returned buffers from worker to distributor was 0.

This patch passes NULL to the legacy API when number of returned
buffers is 0. This allows passing NULL as oldpkt parameter.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 12b3db33c..b720abe03 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -42,7 +42,7 @@ rte_distributor_request_pkt(struct rte_distributor *d,
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		rte_distributor_request_pkt_single(d->d_single,
-			worker_id, oldpkt[0]);
+			worker_id, count ? oldpkt[0] : NULL);
 		return;
 	}
 
@@ -134,7 +134,7 @@ rte_distributor_get_pkt(struct rte_distributor *d,
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (return_count <= 1) {
 			pkts[0] = rte_distributor_get_pkt_single(d->d_single,
-				worker_id, oldpkt[0]);
+				worker_id, return_count ? oldpkt[0] : NULL);
 			return (pkts[0]) ? 1 : 0;
 		} else
 			return -EINVAL;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v2 8/8] distributor: align API documentation with code
       [not found]   ` <CGME20200923014724eucas1p13d3c0428a15bea26def7a4343251e4e4@eucas1p1.samsung.com>
@ 2020-09-23  1:47     ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:47 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

After introducing burst API there were some artefacts in the
API documentation from legacy single API.
Also the rte_distributor_poll_pkt() function return values
mismatched the implementation.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.h | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.h b/lib/librte_distributor/rte_distributor.h
index 327c0c4ab..a073e6461 100644
--- a/lib/librte_distributor/rte_distributor.h
+++ b/lib/librte_distributor/rte_distributor.h
@@ -155,7 +155,7 @@ rte_distributor_clear_returns(struct rte_distributor *d);
  * @param pkts
  *   The mbufs pointer array to be filled in (up to 8 packets)
  * @param oldpkt
- *   The previous packet, if any, being processed by the worker
+ *   The previous packets, if any, being processed by the worker
  * @param retcount
  *   The number of packets being returned
  *
@@ -187,15 +187,15 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 
 /**
  * API called by a worker to request a new packet to process.
- * Any previous packet given to the worker is assumed to have completed
+ * Any previous packets given to the worker are assumed to have completed
  * processing, and may be optionally returned to the distributor via
  * the oldpkt parameter.
- * Unlike rte_distributor_get_pkt_burst(), this function does not wait for a
- * new packet to be provided by the distributor.
+ * Unlike rte_distributor_get_pkt(), this function does not wait for
+ * new packets to be provided by the distributor.
  *
- * NOTE: after calling this function, rte_distributor_poll_pkt_burst() should
- * be used to poll for the packet requested. The rte_distributor_get_pkt_burst()
- * API should *not* be used to try and retrieve the new packet.
+ * NOTE: after calling this function, rte_distributor_poll_pkt() should
+ * be used to poll for the packets requested. The rte_distributor_get_pkt()
+ * API should *not* be used to try and retrieve the new packets.
  *
  * @param d
  *   The distributor instance to be used
@@ -213,9 +213,9 @@ rte_distributor_request_pkt(struct rte_distributor *d,
 		unsigned int count);
 
 /**
- * API called by a worker to check for a new packet that was previously
+ * API called by a worker to check for new packets that were previously
  * requested by a call to rte_distributor_request_pkt(). It does not wait
- * for the new packet to be available, but returns NULL if the request has
+ * for the new packets to be available, but returns if the request has
  * not yet been fulfilled by the distributor.
  *
  * @param d
@@ -227,8 +227,9 @@ rte_distributor_request_pkt(struct rte_distributor *d,
  *   The array of mbufs being given to the worker
  *
  * @return
- *   The number of packets being given to the worker thread, zero if no
- *   packet is yet available.
+ *   The number of packets being given to the worker thread,
+ *   -1 if no packets are yet available (burst API - RTE_DIST_ALG_BURST)
+ *   0 if no packets are yet available (legacy single API - RTE_DIST_ALG_SINGLE)
  */
 int
 rte_distributor_poll_pkt(struct rte_distributor *d,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests
  2020-09-22 12:42     ` [dpdk-stable] [dpdk-dev] " David Marchand
@ 2020-09-23  1:55       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23  1:55 UTC (permalink / raw)
  To: David Marchand, David Hunt; +Cc: Bruce Richardson, dev, dpdk stable

Hello David,

W dniu 22.09.2020 o 14:42, David Marchand pisze:
> Hello Lukasz, David,
>
>
> On Tue, Sep 15, 2020 at 9:35 PM Lukasz Wojciechowski
> <l.wojciechow@partner.samsung.com> wrote:
>> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
>> index 0e49e3714..da13a9a3f 100644
>> --- a/app/test/test_distributor.c
>> +++ b/app/test/test_distributor.c
>> @@ -277,24 +277,21 @@ handle_work_with_free_mbufs(void *arg)
>>          struct rte_mbuf *buf[8] __rte_cache_aligned;
>>          struct worker_params *wp = arg;
>>          struct rte_distributor *d = wp->dist;
>> -       unsigned int count = 0;
>>          unsigned int i;
>>          unsigned int num = 0;
>>          unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
>>
>>          for (i = 0; i < 8; i++)
>>                  buf[i] = NULL;
>> -       num = rte_distributor_get_pkt(d, id, buf, buf, num);
>> +       num = rte_distributor_get_pkt(d, id, buf, buf, 0);
> For my understanding, we pass an array even if we return 0 packet. Is
> this necessary?

The short answer is: yes.

That's because in case of using old legacy API (single distributor), it 
is required to pass a pointer to mbuf (might be NULL however). The new 
burst API functions call the old API dereferencing the first element of 
the array passed. So there must be a valid array containing at least 1 elem.

I pushed the v2 version of the patchset, which contains 2 new patches. 
Patch #7 fixes this issue in librte_distributor by passing NULL mbuf 
pointer to legacy API if number of return buffers is zero.

>
>
>>          while (!quit) {
>> -               count += num;
>>                  __atomic_fetch_add(&worker_stats[id].handled_packets, num,
>>                                  __ATOMIC_ACQ_REL);
>>                  for (i = 0; i < num; i++)
>>                          rte_pktmbuf_free(buf[i]);
>>                  num = rte_distributor_get_pkt(d,
>> -                               id, buf, buf, num);
>> +                               id, buf, buf, 0);
> Here, it gives the impression we have some potential use-after-free on
> buf[] content.
Nice catch! I missed it.
I fixed it in v2 by assigning NULL values to the bufs, so they won't be 
used after free.
> And trying to pass NULL, I can see the distributor library
> dereferences oldpkt[] without checking retcount != 0.

That's fixed in new patch v2 7/8


Best regards

Lukasz

>
>
-- 
Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2 2/8] app/test: synchronize statistics between lcores
  2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 2/8] app/test: synchronize statistics between lcores Lukasz Wojciechowski
@ 2020-09-23  4:30       ` " Honnappa Nagarahalli
  2020-09-23 12:47         ` Lukasz Wojciechowski
  0 siblings, 1 reply; 41+ messages in thread
From: Honnappa Nagarahalli @ 2020-09-23  4:30 UTC (permalink / raw)
  To: Lukasz Wojciechowski, David Hunt, Bruce Richardson
  Cc: dev, stable, nd, Honnappa Nagarahalli, nd

<snip>

> 
> Statistics of handled packets are cleared and read on main lcore, while they
> are increased in workers handlers on different lcores.
> 
> Without synchronization occasionally showed invalid values.
What exactly do you mean by invalid values? Can you elaborate?

> This patch uses atomic acquire/release mechanisms to synchronize.
> 
> Fixes: c3eabff124e6 ("distributor: add unit tests")
> Cc: bruce.richardson@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
>  app/test/test_distributor.c | 39 ++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c index
> 35b25463a..0e49e3714 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -43,7 +43,8 @@ total_packet_count(void)  {
>  	unsigned i, count = 0;
>  	for (i = 0; i < worker_idx; i++)
> -		count += worker_stats[i].handled_packets;
> +		count +=
> __atomic_load_n(&worker_stats[i].handled_packets,
> +				__ATOMIC_ACQUIRE);
>  	return count;
>  }
> 
> @@ -52,6 +53,7 @@ static inline void
>  clear_packet_count(void)
>  {
>  	memset(&worker_stats, 0, sizeof(worker_stats));
> +	rte_atomic_thread_fence(__ATOMIC_RELEASE);
>  }
> 
>  /* this is the basic worker function for sanity test @@ -72,13 +74,13 @@
> handle_work(void *arg)
>  	num = rte_distributor_get_pkt(db, id, buf, buf, num);
>  	while (!quit) {
>  		__atomic_fetch_add(&worker_stats[id].handled_packets,
> num,
> -				__ATOMIC_RELAXED);
> +				__ATOMIC_ACQ_REL);
>  		count += num;
>  		num = rte_distributor_get_pkt(db, id,
>  				buf, buf, num);
>  	}
>  	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> -			__ATOMIC_RELAXED);
> +			__ATOMIC_ACQ_REL);
>  	count += num;
>  	rte_distributor_return_pkt(db, id, buf, num);
>  	return 0;
> @@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct
> rte_mempool *p)
> 
>  	for (i = 0; i < rte_lcore_count() - 1; i++)
>  		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>  	printf("Sanity test with all zero hashes done.\n");
> 
>  	/* pick two flows and check they go correctly */ @@ -159,7 +162,9
> @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
> 
>  		for (i = 0; i < rte_lcore_count() - 1; i++)
>  			printf("Worker %u handled %u packets\n", i,
> -					worker_stats[i].handled_packets);
> +				__atomic_load_n(
> +					&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>  		printf("Sanity test with two hash values done\n");
>  	}
> 
> @@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct
> rte_mempool *p)
> 
>  	for (i = 0; i < rte_lcore_count() - 1; i++)
>  		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
>  	printf("Sanity test with non-zero hashes done\n");
> 
>  	rte_mempool_put_bulk(p, (void *)bufs, BURST); @@ -280,15
> +286,17 @@ handle_work_with_free_mbufs(void *arg)
>  		buf[i] = NULL;
>  	num = rte_distributor_get_pkt(d, id, buf, buf, num);
>  	while (!quit) {
> -		worker_stats[id].handled_packets += num;
>  		count += num;
> +		__atomic_fetch_add(&worker_stats[id].handled_packets,
> num,
> +				__ATOMIC_ACQ_REL);
>  		for (i = 0; i < num; i++)
>  			rte_pktmbuf_free(buf[i]);
>  		num = rte_distributor_get_pkt(d,
>  				id, buf, buf, num);
>  	}
> -	worker_stats[id].handled_packets += num;
>  	count += num;
> +	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> +			__ATOMIC_ACQ_REL);
>  	rte_distributor_return_pkt(d, id, buf, num);
>  	return 0;
>  }
> @@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg)
>  	/* wait for quit single globally, or for worker zero, wait
>  	 * for zero_quit */
>  	while (!quit && !(id == zero_id && zero_quit)) {
> -		worker_stats[id].handled_packets += num;
>  		count += num;
> +		__atomic_fetch_add(&worker_stats[id].handled_packets,
> num,
> +				__ATOMIC_ACQ_REL);
>  		for (i = 0; i < num; i++)
>  			rte_pktmbuf_free(buf[i]);
>  		num = rte_distributor_get_pkt(d,
> @@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg)
> 
>  		total += num;
>  	}
> -	worker_stats[id].handled_packets += num;
>  	count += num;
>  	returned = rte_distributor_return_pkt(d, id, buf, num);
> 
> +	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
> +			__ATOMIC_ACQ_REL);
>  	if (id == zero_id) {
>  		/* for worker zero, allow it to restart to pick up last packet
>  		 * when all workers are shutting down.
> @@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg)
>  				id, buf, buf, num);
> 
>  		while (!quit) {
> -			worker_stats[id].handled_packets += num;
>  			count += num;
>  			rte_pktmbuf_free(pkt);
>  			num = rte_distributor_get_pkt(d, id, buf, buf, num);
> +
> 	__atomic_fetch_add(&worker_stats[id].handled_packets,
> +					num, __ATOMIC_ACQ_REL);
>  		}
>  		returned = rte_distributor_return_pkt(d,
>  				id, buf, num);
> @@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct
> worker_params *wp,
> 
>  	for (i = 0; i < rte_lcore_count() - 1; i++)
>  		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
> 
>  	if (total_packet_count() != BURST * 2) {
>  		printf("Line %d: Error, not all packets flushed. "
> @@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct
> worker_params *wp,
>  	zero_quit = 0;
>  	for (i = 0; i < rte_lcore_count() - 1; i++)
>  		printf("Worker %u handled %u packets\n", i,
> -				worker_stats[i].handled_packets);
> +			__atomic_load_n(&worker_stats[i].handled_packets,
> +					__ATOMIC_ACQUIRE));
> 
>  	if (total_packet_count() != BURST) {
>  		printf("Line %d: Error, not all packets flushed. "
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2 2/8] app/test: synchronize statistics between lcores
  2020-09-23  4:30       ` [dpdk-stable] [dpdk-dev] " Honnappa Nagarahalli
@ 2020-09-23 12:47         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 12:47 UTC (permalink / raw)
  To: Honnappa Nagarahalli
  Cc: David Hunt, Bruce Richardson, dev, stable, nd,
	\"'Lukasz Wojciechowski'\",


W dniu 23.09.2020 o 06:30, Honnappa Nagarahalli pisze:
> <snip>
>
>> Statistics of handled packets are cleared and read on main lcore, while they
>> are increased in workers handlers on different lcores.
>>
>> Without synchronization occasionally showed invalid values.
> What exactly do you mean by invalid values? Can you elaborate?

I mean values that shouldn't be there which are obviously not related to 
number of packets handled by workers.

I reverted the patch and run stress tests. Failures without this patch 
look like these:

=== Test flush fn with worker shutdown (burst) ===
Worker 0 handled 0 packets
Worker 1 handled 0 packets
Worker 2 handled 0 packets
Worker 3 handled 0 packets
Worker 4 handled 32 packets
Worker 5 handled 0 packets
Worker 6 handled 6 packets
Line 519: Error, not all packets flushed. Expected 32, got 38
Test Failed

or:

=== Sanity test of worker shutdown ===
Worker 0 handled 0 packets
Worker 1 handled 0 packets
Worker 2 handled 0 packets
Worker 3 handled 0 packets
Worker 4 handled 0 packets
Worker 5 handled 64 packets
Worker 6 handled 149792 packets
Line 466: Error, not all packets flushed. Expected 64, got 149856
Test Failed

The 6 or 149792 packets reported by worker 6 were never sent to or 
processed by the workers.

>> This patch uses atomic acquire/release mechanisms to synchronize.
>>
>> Fixes: c3eabff124e6 ("distributor: add unit tests")
>> Cc: bruce.richardson@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> ---
>>   app/test/test_distributor.c | 39 ++++++++++++++++++++++++-------------
>>   1 file changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c index
>> 35b25463a..0e49e3714 100644
>> --- a/app/test/test_distributor.c
>> +++ b/app/test/test_distributor.c
>> @@ -43,7 +43,8 @@ total_packet_count(void)  {
>>   	unsigned i, count = 0;
>>   	for (i = 0; i < worker_idx; i++)
>> -		count += worker_stats[i].handled_packets;
>> +		count +=
>> __atomic_load_n(&worker_stats[i].handled_packets,
>> +				__ATOMIC_ACQUIRE);
>>   	return count;
>>   }
>>
>> @@ -52,6 +53,7 @@ static inline void
>>   clear_packet_count(void)
>>   {
>>   	memset(&worker_stats, 0, sizeof(worker_stats));
>> +	rte_atomic_thread_fence(__ATOMIC_RELEASE);
>>   }
>>
>>   /* this is the basic worker function for sanity test @@ -72,13 +74,13 @@
>> handle_work(void *arg)
>>   	num = rte_distributor_get_pkt(db, id, buf, buf, num);
>>   	while (!quit) {
>>   		__atomic_fetch_add(&worker_stats[id].handled_packets,
>> num,
>> -				__ATOMIC_RELAXED);
>> +				__ATOMIC_ACQ_REL);
>>   		count += num;
>>   		num = rte_distributor_get_pkt(db, id,
>>   				buf, buf, num);
>>   	}
>>   	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>> -			__ATOMIC_RELAXED);
>> +			__ATOMIC_ACQ_REL);
>>   	count += num;
>>   	rte_distributor_return_pkt(db, id, buf, num);
>>   	return 0;
>> @@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct
>> rte_mempool *p)
>>
>>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>>   		printf("Worker %u handled %u packets\n", i,
>> -				worker_stats[i].handled_packets);
>> +			__atomic_load_n(&worker_stats[i].handled_packets,
>> +					__ATOMIC_ACQUIRE));
>>   	printf("Sanity test with all zero hashes done.\n");
>>
>>   	/* pick two flows and check they go correctly */ @@ -159,7 +162,9
>> @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
>>
>>   		for (i = 0; i < rte_lcore_count() - 1; i++)
>>   			printf("Worker %u handled %u packets\n", i,
>> -					worker_stats[i].handled_packets);
>> +				__atomic_load_n(
>> +					&worker_stats[i].handled_packets,
>> +					__ATOMIC_ACQUIRE));
>>   		printf("Sanity test with two hash values done\n");
>>   	}
>>
>> @@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct
>> rte_mempool *p)
>>
>>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>>   		printf("Worker %u handled %u packets\n", i,
>> -				worker_stats[i].handled_packets);
>> +			__atomic_load_n(&worker_stats[i].handled_packets,
>> +					__ATOMIC_ACQUIRE));
>>   	printf("Sanity test with non-zero hashes done\n");
>>
>>   	rte_mempool_put_bulk(p, (void *)bufs, BURST); @@ -280,15
>> +286,17 @@ handle_work_with_free_mbufs(void *arg)
>>   		buf[i] = NULL;
>>   	num = rte_distributor_get_pkt(d, id, buf, buf, num);
>>   	while (!quit) {
>> -		worker_stats[id].handled_packets += num;
>>   		count += num;
>> +		__atomic_fetch_add(&worker_stats[id].handled_packets,
>> num,
>> +				__ATOMIC_ACQ_REL);
>>   		for (i = 0; i < num; i++)
>>   			rte_pktmbuf_free(buf[i]);
>>   		num = rte_distributor_get_pkt(d,
>>   				id, buf, buf, num);
>>   	}
>> -	worker_stats[id].handled_packets += num;
>>   	count += num;
>> +	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>> +			__ATOMIC_ACQ_REL);
>>   	rte_distributor_return_pkt(d, id, buf, num);
>>   	return 0;
>>   }
>> @@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg)
>>   	/* wait for quit single globally, or for worker zero, wait
>>   	 * for zero_quit */
>>   	while (!quit && !(id == zero_id && zero_quit)) {
>> -		worker_stats[id].handled_packets += num;
>>   		count += num;
>> +		__atomic_fetch_add(&worker_stats[id].handled_packets,
>> num,
>> +				__ATOMIC_ACQ_REL);
>>   		for (i = 0; i < num; i++)
>>   			rte_pktmbuf_free(buf[i]);
>>   		num = rte_distributor_get_pkt(d,
>> @@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg)
>>
>>   		total += num;
>>   	}
>> -	worker_stats[id].handled_packets += num;
>>   	count += num;
>>   	returned = rte_distributor_return_pkt(d, id, buf, num);
>>
>> +	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
>> +			__ATOMIC_ACQ_REL);
>>   	if (id == zero_id) {
>>   		/* for worker zero, allow it to restart to pick up last packet
>>   		 * when all workers are shutting down.
>> @@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg)
>>   				id, buf, buf, num);
>>
>>   		while (!quit) {
>> -			worker_stats[id].handled_packets += num;
>>   			count += num;
>>   			rte_pktmbuf_free(pkt);
>>   			num = rte_distributor_get_pkt(d, id, buf, buf, num);
>> +
>> 	__atomic_fetch_add(&worker_stats[id].handled_packets,
>> +					num, __ATOMIC_ACQ_REL);
>>   		}
>>   		returned = rte_distributor_return_pkt(d,
>>   				id, buf, num);
>> @@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct
>> worker_params *wp,
>>
>>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>>   		printf("Worker %u handled %u packets\n", i,
>> -				worker_stats[i].handled_packets);
>> +			__atomic_load_n(&worker_stats[i].handled_packets,
>> +					__ATOMIC_ACQUIRE));
>>
>>   	if (total_packet_count() != BURST * 2) {
>>   		printf("Line %d: Error, not all packets flushed. "
>> @@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct
>> worker_params *wp,
>>   	zero_quit = 0;
>>   	for (i = 0; i < rte_lcore_count() - 1; i++)
>>   		printf("Worker %u handled %u packets\n", i,
>> -				worker_stats[i].handled_packets);
>> +			__atomic_load_n(&worker_stats[i].handled_packets,
>> +					__ATOMIC_ACQUIRE));
>>
>>   	if (total_packet_count() != BURST) {
>>   		printf("Line %d: Error, not all packets flushed. "
>> --
>> 2.17.1

-- 
Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 1/8] app/test: fix deadlock in distributor test
       [not found]     ` <CGME20200923132545eucas1p10db12d91121c9afdbab338bb60c8ed37@eucas1p1.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

The sanity test with worker shutdown delegates all bufs
to be processed by a single lcore worker, then it freezes
one of the lcore workers and continues to send more bufs.

Problem occurred if freezed lcore is the same as the one
that is processing the mbufs. The lcore processing mbufs
might be different every time test is launched.
This is caused by keeping the value of wkr static variable
in rte_distributor_process function between running test cases.

Test freezed always lcore with 0 id. The patch changes avoids
possible collision by freezing lcore with zero_idx. The lcore
that receives the data updates the zero_idx, so it is not freezed
itself.

To reproduce the issue fixed by this patch, please run
distributor_autotest command in test app several times in a row.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Tested-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index ba1f81cf8..35b25463a 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -28,6 +28,7 @@ struct worker_params worker_params;
 static volatile int quit;      /**< general quit variable for all threads */
 static volatile int zero_quit; /**< var for when we just want thr0 to quit*/
 static volatile unsigned worker_idx;
+static volatile unsigned zero_idx;
 
 struct worker_stats {
 	volatile unsigned handled_packets;
@@ -346,27 +347,43 @@ handle_work_for_shutdown_test(void *arg)
 	unsigned int total = 0;
 	unsigned int i;
 	unsigned int returned = 0;
+	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
 
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 
+	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+	if (id == zero_id && num > 0) {
+		zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+			__ATOMIC_ACQUIRE);
+		__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+	}
+
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
-	while (!quit && !(id == 0 && zero_quit)) {
+	while (!quit && !(id == zero_id && zero_quit)) {
 		worker_stats[id].handled_packets += num;
 		count += num;
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
+
+		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+		if (id == zero_id && num > 0) {
+			zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+				__ATOMIC_ACQUIRE);
+			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+		}
+
 		total += num;
 	}
 	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
-	if (id == 0) {
+	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
 		 */
@@ -586,6 +603,7 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_eal_mp_wait_lcore();
 	quit = 0;
 	worker_idx = 0;
+	zero_idx = 0;
 }
 
 static int
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 2/8] app/test: synchronize statistics between lcores
       [not found]     ` <CGME20200923132546eucas1p212b6eede801514b544d82d41f5b7e4b8@eucas1p2.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Statistics of handled packets are cleared and read on main lcore,
while they are increased in workers handlers on different lcores.

Without synchronization occasionally showed invalid values.
This patch uses atomic acquire/release mechanisms to synchronize.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 39 ++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 35b25463a..0e49e3714 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -43,7 +43,8 @@ total_packet_count(void)
 {
 	unsigned i, count = 0;
 	for (i = 0; i < worker_idx; i++)
-		count += worker_stats[i].handled_packets;
+		count += __atomic_load_n(&worker_stats[i].handled_packets,
+				__ATOMIC_ACQUIRE);
 	return count;
 }
 
@@ -52,6 +53,7 @@ static inline void
 clear_packet_count(void)
 {
 	memset(&worker_stats, 0, sizeof(worker_stats));
+	rte_atomic_thread_fence(__ATOMIC_RELEASE);
 }
 
 /* this is the basic worker function for sanity test
@@ -72,13 +74,13 @@ handle_work(void *arg)
 	num = rte_distributor_get_pkt(db, id, buf, buf, num);
 	while (!quit) {
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-				__ATOMIC_RELAXED);
+				__ATOMIC_ACQ_REL);
 		count += num;
 		num = rte_distributor_get_pkt(db, id,
 				buf, buf, num);
 	}
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-			__ATOMIC_RELAXED);
+			__ATOMIC_ACQ_REL);
 	count += num;
 	rte_distributor_return_pkt(db, id, buf, num);
 	return 0;
@@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with all zero hashes done.\n");
 
 	/* pick two flows and check they go correctly */
@@ -159,7 +162,9 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 		for (i = 0; i < rte_lcore_count() - 1; i++)
 			printf("Worker %u handled %u packets\n", i,
-					worker_stats[i].handled_packets);
+				__atomic_load_n(
+					&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 		printf("Sanity test with two hash values done\n");
 	}
 
@@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with non-zero hashes done\n");
 
 	rte_mempool_put_bulk(p, (void *)bufs, BURST);
@@ -280,15 +286,17 @@ handle_work_with_free_mbufs(void *arg)
 		buf[i] = NULL;
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 	while (!quit) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
@@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
@@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg)
 
 		total += num;
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
@@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg)
 				id, buf, buf, num);
 
 		while (!quit) {
-			worker_stats[id].handled_packets += num;
 			count += num;
 			rte_pktmbuf_free(pkt);
 			num = rte_distributor_get_pkt(d, id, buf, buf, num);
+			__atomic_fetch_add(&worker_stats[id].handled_packets,
+					num, __ATOMIC_ACQ_REL);
 		}
 		returned = rte_distributor_return_pkt(d,
 				id, buf, num);
@@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct worker_params *wp,
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST * 2) {
 		printf("Line %d: Error, not all packets flushed. "
@@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct worker_params *wp,
 	zero_quit = 0;
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST) {
 		printf("Line %d: Error, not all packets flushed. "
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 3/8] app/test: fix freeing mbufs in distributor tests
       [not found]     ` <CGME20200923132547eucas1p130620b0d5f3080a7a57234838a992e0e@eucas1p1.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Sanity tests with mbuf alloc and shutdown tests assume that
mbufs passed to worker cores are freed in handlers.
Such packets should not be returned to the distributor's main
core. The only packets that should be returned are the packets
send after completion of the tests in quit_workers function.

This patch fixes freeing mbufs, stops returning them
to distributor's core and cleans up unused variables.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 50 +++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 0e49e3714..94b65b382 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -277,24 +277,23 @@ handle_work_with_free_mbufs(void *arg)
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int i;
 	unsigned int num = 0;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
 
 	for (i = 0; i < 8; i++)
 		buf[i] = NULL;
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 	while (!quit) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num; i++) {
 			rte_pktmbuf_free(buf[i]);
+			buf[i] = NULL;
+		}
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 	}
-	count += num;
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
@@ -322,7 +321,6 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 			rte_distributor_process(d, NULL, 0);
 		for (j = 0; j < BURST; j++) {
 			bufs[j]->hash.usr = (i+j) << 1;
-			rte_mbuf_refcnt_set(bufs[j], 1);
 		}
 
 		rte_distributor_process(d, bufs, BURST);
@@ -346,20 +344,18 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 static int
 handle_work_for_shutdown_test(void *arg)
 {
-	struct rte_mbuf *pkt = NULL;
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int num = 0;
-	unsigned int total = 0;
 	unsigned int i;
-	unsigned int returned = 0;
 	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
+	for (i = 0; i < 8; i++)
+		buf[i] = NULL;
 
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 
 	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 	if (id == zero_id && num > 0) {
@@ -371,13 +367,14 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num; i++) {
 			rte_pktmbuf_free(buf[i]);
+			buf[i] = NULL;
+		}
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 		if (id == zero_id && num > 0) {
@@ -385,12 +382,7 @@ handle_work_for_shutdown_test(void *arg)
 				__ATOMIC_ACQUIRE);
 			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
 		}
-
-		total += num;
 	}
-	count += num;
-	returned = rte_distributor_return_pkt(d, id, buf, num);
-
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
@@ -400,20 +392,24 @@ handle_work_for_shutdown_test(void *arg)
 		while (zero_quit)
 			usleep(100);
 
+		for (i = 0; i < num; i++) {
+			rte_pktmbuf_free(buf[i]);
+			buf[i] = NULL;
+		}
 		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+				id, buf, buf, 0);
 
 		while (!quit) {
-			count += num;
-			rte_pktmbuf_free(pkt);
-			num = rte_distributor_get_pkt(d, id, buf, buf, num);
 			__atomic_fetch_add(&worker_stats[id].handled_packets,
 					num, __ATOMIC_ACQ_REL);
+			for (i = 0; i < num; i++) {
+				rte_pktmbuf_free(buf[i]);
+				buf[i] = NULL;
+			}
+			num = rte_distributor_get_pkt(d, id, buf, buf, 0);
 		}
-		returned = rte_distributor_return_pkt(d,
-				id, buf, num);
-		printf("Num returned = %d\n", returned);
 	}
+	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 4/8] app/test: collect return mbufs in distributor test
       [not found]     ` <CGME20200923132548eucas1p2a54328cddb79ae5e876eb104217d585f@eucas1p2.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

During quit_workers function distributor's main core processes
some packets to wake up pending worker cores so they can quit.
As quit_workers acts also as a cleanup procedure for next test
case it should also collect these packages returned by workers'
handlers, so the cyclic buffer with returned packets
in distributor remains empty.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 94b65b382..f31b54edf 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -610,6 +610,10 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_distributor_process(d, NULL, 0);
 	rte_distributor_flush(d);
 	rte_eal_mp_wait_lcore();
+
+	while (rte_distributor_returned_pkts(d, bufs, RTE_MAX_LCORE))
+		;
+
 	quit = 0;
 	worker_idx = 0;
 	zero_idx = 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 5/8] distributor: fix missing handshake synchronization
       [not found]     ` <CGME20200923132549eucas1p29fc391c3f236fa704ff800774ab851f0@eucas1p2.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

rte_distributor_return_pkt function which is run on worker cores
must wait for distributor core to clear handshake on retptr64
before using those buffers. While the handshake is set distributor
core controls buffers and any operations on worker side might overwrite
buffers which are unread yet.
Same situation appears in the legacy single distributor. Function
rte_distributor_return_pkt_single shouldn't modify the bufptr64 until
handshake on it is cleared by distributor lcore.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_distributor/rte_distributor.c        | 14 ++++++++++++++
 lib/librte_distributor/rte_distributor_single.c |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 1c047f065..89493c331 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -160,6 +160,7 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 {
 	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
 	unsigned int i;
+	volatile int64_t *retptr64;
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (num == 1)
@@ -169,6 +170,19 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 			return -EINVAL;
 	}
 
+	retptr64 = &(buf->retptr64[0]);
+	/* Spin while handshake bits are set (scheduler clears it).
+	 * Sync with worker on GET_BUF flag.
+	 */
+	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
+			& RTE_DISTRIB_GET_BUF)) {
+		rte_pause();
+		uint64_t t = rte_rdtsc()+100;
+
+		while (rte_rdtsc() < t)
+			rte_pause();
+	}
+
 	/* Sync with distributor to acquire retptrs */
 	__atomic_thread_fence(__ATOMIC_ACQUIRE);
 	for (i = 0; i < RTE_DIST_BURST_SIZE; i++)
diff --git a/lib/librte_distributor/rte_distributor_single.c b/lib/librte_distributor/rte_distributor_single.c
index abaf7730c..f4725b1d0 100644
--- a/lib/librte_distributor/rte_distributor_single.c
+++ b/lib/librte_distributor/rte_distributor_single.c
@@ -74,6 +74,10 @@ rte_distributor_return_pkt_single(struct rte_distributor_single *d,
 	union rte_distributor_buffer_single *buf = &d->bufs[worker_id];
 	uint64_t req = (((int64_t)(uintptr_t)oldpkt) << RTE_DISTRIB_FLAG_BITS)
 			| RTE_DISTRIB_RETURN_BUF;
+	while (unlikely(__atomic_load_n(&buf->bufptr64, __ATOMIC_RELAXED)
+			& RTE_DISTRIB_FLAGS_MASK))
+		rte_pause();
+
 	/* Sync with distributor on RETURN_BUF flag. */
 	__atomic_store_n(&(buf->bufptr64), req, __ATOMIC_RELEASE);
 	return 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 6/8] distributor: fix handshake deadlock
       [not found]     ` <CGME20200923132550eucas1p2ce158dd81ccc04abcab4130d8cb391f4@eucas1p2.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Synchronization of data exchange between distributor and worker cores
is based on 2 handshakes: retptr64 for returning mbufs from workers
to distributor and bufptr64 for passing mbufs to workers.

Without proper order of verifying those 2 handshakes a deadlock may
occur. This can happen when worker core want to return back mbufs
and waits for retptr handshake to be cleared and distributor core
wait for bufptr to send mbufs to worker.

This can happen as worker core first returns mbufs to distributor
and later gets new mbufs, while distributor first release mbufs
to worker and later handle returning packets.

This patch fixes possibility of the deadlock by always taking care
of returning packets first on the distributor side and handling
packets while waiting to release new.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_distributor/rte_distributor.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 89493c331..12b3db33c 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -321,12 +321,14 @@ release(struct rte_distributor *d, unsigned int wkr)
 	struct rte_distributor_buffer *buf = &(d->bufs[wkr]);
 	unsigned int i;
 
+	handle_returns(d, wkr);
+
 	/* Sync with worker on GET_BUF flag */
 	while (!(__atomic_load_n(&(d->bufs[wkr].bufptr64[0]), __ATOMIC_ACQUIRE)
-		& RTE_DISTRIB_GET_BUF))
+		& RTE_DISTRIB_GET_BUF)) {
+		handle_returns(d, wkr);
 		rte_pause();
-
-	handle_returns(d, wkr);
+	}
 
 	buf->count = 0;
 
@@ -376,6 +378,7 @@ rte_distributor_process(struct rte_distributor *d,
 		/* Flush out all non-full cache-lines to workers. */
 		for (wid = 0 ; wid < d->num_workers; wid++) {
 			/* Sync with worker on GET_BUF flag. */
+			handle_returns(d, wid);
 			if (__atomic_load_n(&(d->bufs[wid].bufptr64[0]),
 				__ATOMIC_ACQUIRE) & RTE_DISTRIB_GET_BUF) {
 				release(d, wid);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 7/8] distributor: do not use oldpkt when not needed
       [not found]     ` <CGME20200923132550eucas1p1ce21011562d0a00cccfd4ae3f0be4ff9@eucas1p1.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

rte_distributor_request_pkt and rte_distributor_get_pkt dereferenced
oldpkt parameter when in RTE_DIST_ALG_SINGLE even if number
of returned buffers from worker to distributor was 0.

This patch passes NULL to the legacy API when number of returned
buffers is 0. This allows passing NULL as oldpkt parameter.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 12b3db33c..b720abe03 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -42,7 +42,7 @@ rte_distributor_request_pkt(struct rte_distributor *d,
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		rte_distributor_request_pkt_single(d->d_single,
-			worker_id, oldpkt[0]);
+			worker_id, count ? oldpkt[0] : NULL);
 		return;
 	}
 
@@ -134,7 +134,7 @@ rte_distributor_get_pkt(struct rte_distributor *d,
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (return_count <= 1) {
 			pkts[0] = rte_distributor_get_pkt_single(d->d_single,
-				worker_id, oldpkt[0]);
+				worker_id, return_count ? oldpkt[0] : NULL);
 			return (pkts[0]) ? 1 : 0;
 		} else
 			return -EINVAL;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v3 8/8] distributor: align API documentation with code
       [not found]     ` <CGME20200923132551eucas1p214a5f78c61e891c5e7b6cddc038d0e2e@eucas1p2.samsung.com>
@ 2020-09-23 13:25       ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-23 13:25 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

After introducing burst API there were some artefacts in the
API documentation from legacy single API.
Also the rte_distributor_poll_pkt() function return values
mismatched the implementation.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.h | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.h b/lib/librte_distributor/rte_distributor.h
index 327c0c4ab..a073e6461 100644
--- a/lib/librte_distributor/rte_distributor.h
+++ b/lib/librte_distributor/rte_distributor.h
@@ -155,7 +155,7 @@ rte_distributor_clear_returns(struct rte_distributor *d);
  * @param pkts
  *   The mbufs pointer array to be filled in (up to 8 packets)
  * @param oldpkt
- *   The previous packet, if any, being processed by the worker
+ *   The previous packets, if any, being processed by the worker
  * @param retcount
  *   The number of packets being returned
  *
@@ -187,15 +187,15 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 
 /**
  * API called by a worker to request a new packet to process.
- * Any previous packet given to the worker is assumed to have completed
+ * Any previous packets given to the worker are assumed to have completed
  * processing, and may be optionally returned to the distributor via
  * the oldpkt parameter.
- * Unlike rte_distributor_get_pkt_burst(), this function does not wait for a
- * new packet to be provided by the distributor.
+ * Unlike rte_distributor_get_pkt(), this function does not wait for
+ * new packets to be provided by the distributor.
  *
- * NOTE: after calling this function, rte_distributor_poll_pkt_burst() should
- * be used to poll for the packet requested. The rte_distributor_get_pkt_burst()
- * API should *not* be used to try and retrieve the new packet.
+ * NOTE: after calling this function, rte_distributor_poll_pkt() should
+ * be used to poll for the packets requested. The rte_distributor_get_pkt()
+ * API should *not* be used to try and retrieve the new packets.
  *
  * @param d
  *   The distributor instance to be used
@@ -213,9 +213,9 @@ rte_distributor_request_pkt(struct rte_distributor *d,
 		unsigned int count);
 
 /**
- * API called by a worker to check for a new packet that was previously
+ * API called by a worker to check for new packets that were previously
  * requested by a call to rte_distributor_request_pkt(). It does not wait
- * for the new packet to be available, but returns NULL if the request has
+ * for the new packets to be available, but returns if the request has
  * not yet been fulfilled by the distributor.
  *
  * @param d
@@ -227,8 +227,9 @@ rte_distributor_request_pkt(struct rte_distributor *d,
  *   The array of mbufs being given to the worker
  *
  * @return
- *   The number of packets being given to the worker thread, zero if no
- *   packet is yet available.
+ *   The number of packets being given to the worker thread,
+ *   -1 if no packets are yet available (burst API - RTE_DIST_ALG_BURST)
+ *   0 if no packets are yet available (legacy single API - RTE_DIST_ALG_SINGLE)
  */
 int
 rte_distributor_poll_pkt(struct rte_distributor *d,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 1/8] test/distributor: fix deadlock with freezed worker
       [not found]       ` <CGME20200925224216eucas1p1e8e1d0ecab4bbbf6e43b117c1d210649@eucas1p1.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

The sanity test with worker shutdown delegates all bufs
to be processed by a single lcore worker, then it freezes
one of the lcore workers and continues to send more bufs.

Problem occurred if freezed lcore is the same as the one
that is processing the mbufs. The lcore processing mbufs
might be different every time test is launched.
This is caused by keeping the value of wkr static variable
in rte_distributor_process function between running test cases.

Test freezed always lcore with 0 id. The patch changes avoids
possible collision by freezing lcore with zero_idx. The lcore
that receives the data updates the zero_idx, so it is not freezed
itself.

To reproduce the issue fixed by this patch, please run
distributor_autotest command in test app several times in a row.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Tested-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index ba1f81cf8..35b25463a 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -28,6 +28,7 @@ struct worker_params worker_params;
 static volatile int quit;      /**< general quit variable for all threads */
 static volatile int zero_quit; /**< var for when we just want thr0 to quit*/
 static volatile unsigned worker_idx;
+static volatile unsigned zero_idx;
 
 struct worker_stats {
 	volatile unsigned handled_packets;
@@ -346,27 +347,43 @@ handle_work_for_shutdown_test(void *arg)
 	unsigned int total = 0;
 	unsigned int i;
 	unsigned int returned = 0;
+	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
 
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 
+	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+	if (id == zero_id && num > 0) {
+		zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+			__ATOMIC_ACQUIRE);
+		__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+	}
+
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
-	while (!quit && !(id == 0 && zero_quit)) {
+	while (!quit && !(id == zero_id && zero_quit)) {
 		worker_stats[id].handled_packets += num;
 		count += num;
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
+
+		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
+		if (id == zero_id && num > 0) {
+			zero_id = (zero_id + 1) %  __atomic_load_n(&worker_idx,
+				__ATOMIC_ACQUIRE);
+			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
+		}
+
 		total += num;
 	}
 	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
-	if (id == 0) {
+	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
 		 */
@@ -586,6 +603,7 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_eal_mp_wait_lcore();
 	quit = 0;
 	worker_idx = 0;
+	zero_idx = 0;
 }
 
 static int
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 2/8] test/distributor: synchronize lcores statistics
       [not found]       ` <CGME20200925224217eucas1p1bb5f73109b4aeed8f2badf311fa8dfb5@eucas1p1.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Statistics of handled packets are cleared and read on main lcore,
while they are increased in workers handlers on different lcores.

Without synchronization occasionally showed invalid values.
This patch uses atomic acquire/release mechanisms to synchronize.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 39 ++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 35b25463a..0e49e3714 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -43,7 +43,8 @@ total_packet_count(void)
 {
 	unsigned i, count = 0;
 	for (i = 0; i < worker_idx; i++)
-		count += worker_stats[i].handled_packets;
+		count += __atomic_load_n(&worker_stats[i].handled_packets,
+				__ATOMIC_ACQUIRE);
 	return count;
 }
 
@@ -52,6 +53,7 @@ static inline void
 clear_packet_count(void)
 {
 	memset(&worker_stats, 0, sizeof(worker_stats));
+	rte_atomic_thread_fence(__ATOMIC_RELEASE);
 }
 
 /* this is the basic worker function for sanity test
@@ -72,13 +74,13 @@ handle_work(void *arg)
 	num = rte_distributor_get_pkt(db, id, buf, buf, num);
 	while (!quit) {
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-				__ATOMIC_RELAXED);
+				__ATOMIC_ACQ_REL);
 		count += num;
 		num = rte_distributor_get_pkt(db, id,
 				buf, buf, num);
 	}
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
-			__ATOMIC_RELAXED);
+			__ATOMIC_ACQ_REL);
 	count += num;
 	rte_distributor_return_pkt(db, id, buf, num);
 	return 0;
@@ -134,7 +136,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with all zero hashes done.\n");
 
 	/* pick two flows and check they go correctly */
@@ -159,7 +162,9 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 		for (i = 0; i < rte_lcore_count() - 1; i++)
 			printf("Worker %u handled %u packets\n", i,
-					worker_stats[i].handled_packets);
+				__atomic_load_n(
+					&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 		printf("Sanity test with two hash values done\n");
 	}
 
@@ -185,7 +190,8 @@ sanity_test(struct worker_params *wp, struct rte_mempool *p)
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 	printf("Sanity test with non-zero hashes done\n");
 
 	rte_mempool_put_bulk(p, (void *)bufs, BURST);
@@ -280,15 +286,17 @@ handle_work_with_free_mbufs(void *arg)
 		buf[i] = NULL;
 	num = rte_distributor_get_pkt(d, id, buf, buf, num);
 	while (!quit) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
 				id, buf, buf, num);
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
@@ -363,8 +371,9 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		worker_stats[id].handled_packets += num;
 		count += num;
+		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d,
@@ -379,10 +388,11 @@ handle_work_for_shutdown_test(void *arg)
 
 		total += num;
 	}
-	worker_stats[id].handled_packets += num;
 	count += num;
 	returned = rte_distributor_return_pkt(d, id, buf, num);
 
+	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
+			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
 		/* for worker zero, allow it to restart to pick up last packet
 		 * when all workers are shutting down.
@@ -394,10 +404,11 @@ handle_work_for_shutdown_test(void *arg)
 				id, buf, buf, num);
 
 		while (!quit) {
-			worker_stats[id].handled_packets += num;
 			count += num;
 			rte_pktmbuf_free(pkt);
 			num = rte_distributor_get_pkt(d, id, buf, buf, num);
+			__atomic_fetch_add(&worker_stats[id].handled_packets,
+					num, __ATOMIC_ACQ_REL);
 		}
 		returned = rte_distributor_return_pkt(d,
 				id, buf, num);
@@ -461,7 +472,8 @@ sanity_test_with_worker_shutdown(struct worker_params *wp,
 
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST * 2) {
 		printf("Line %d: Error, not all packets flushed. "
@@ -514,7 +526,8 @@ test_flush_with_worker_shutdown(struct worker_params *wp,
 	zero_quit = 0;
 	for (i = 0; i < rte_lcore_count() - 1; i++)
 		printf("Worker %u handled %u packets\n", i,
-				worker_stats[i].handled_packets);
+			__atomic_load_n(&worker_stats[i].handled_packets,
+					__ATOMIC_ACQUIRE));
 
 	if (total_packet_count() != BURST) {
 		printf("Line %d: Error, not all packets flushed. "
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 3/8] distributor: do not use oldpkt when not needed
       [not found]       ` <CGME20200925224218eucas1p2383ff0ebdaee18b581f5f731476f05ab@eucas1p2.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

rte_distributor_request_pkt and rte_distributor_get_pkt dereferenced
oldpkt parameter when in RTE_DIST_ALG_SINGLE even if number
of returned buffers from worker to distributor was 0.

This patch passes NULL to the legacy API when number of returned
buffers is 0. This allows passing NULL as oldpkt parameter.

Distribor tests were also updated passing NULL as oldpkt and
0 as number of returned packets, where packets are not returned.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_distributor.c              | 28 +++++++++---------------
 lib/librte_distributor/rte_distributor.c |  4 ++--
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 0e49e3714..0e3ab0c4f 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -65,13 +65,10 @@ handle_work(void *arg)
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *db = wp->dist;
-	unsigned int count = 0, num = 0;
+	unsigned int count = 0, num;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
-	int i;
 
-	for (i = 0; i < 8; i++)
-		buf[i] = NULL;
-	num = rte_distributor_get_pkt(db, id, buf, buf, num);
+	num = rte_distributor_get_pkt(db, id, buf, NULL, 0);
 	while (!quit) {
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
@@ -279,20 +276,17 @@ handle_work_with_free_mbufs(void *arg)
 	struct rte_distributor *d = wp->dist;
 	unsigned int count = 0;
 	unsigned int i;
-	unsigned int num = 0;
+	unsigned int num;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
 
-	for (i = 0; i < 8; i++)
-		buf[i] = NULL;
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 	while (!quit) {
 		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
-		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+		num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 	}
 	count += num;
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
@@ -351,7 +345,7 @@ handle_work_for_shutdown_test(void *arg)
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
 	unsigned int count = 0;
-	unsigned int num = 0;
+	unsigned int num;
 	unsigned int total = 0;
 	unsigned int i;
 	unsigned int returned = 0;
@@ -359,7 +353,7 @@ handle_work_for_shutdown_test(void *arg)
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
 
-	num = rte_distributor_get_pkt(d, id, buf, buf, num);
+	num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 
 	zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 	if (id == zero_id && num > 0) {
@@ -376,8 +370,7 @@ handle_work_for_shutdown_test(void *arg)
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
-		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+		num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 
 		zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
 		if (id == zero_id && num > 0) {
@@ -400,13 +393,12 @@ handle_work_for_shutdown_test(void *arg)
 		while (zero_quit)
 			usleep(100);
 
-		num = rte_distributor_get_pkt(d,
-				id, buf, buf, num);
+		num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 
 		while (!quit) {
 			count += num;
 			rte_pktmbuf_free(pkt);
-			num = rte_distributor_get_pkt(d, id, buf, buf, num);
+			num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 			__atomic_fetch_add(&worker_stats[id].handled_packets,
 					num, __ATOMIC_ACQ_REL);
 		}
diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 1c047f065..8a12bf856 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -42,7 +42,7 @@ rte_distributor_request_pkt(struct rte_distributor *d,
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		rte_distributor_request_pkt_single(d->d_single,
-			worker_id, oldpkt[0]);
+			worker_id, count ? oldpkt[0] : NULL);
 		return;
 	}
 
@@ -134,7 +134,7 @@ rte_distributor_get_pkt(struct rte_distributor *d,
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (return_count <= 1) {
 			pkts[0] = rte_distributor_get_pkt_single(d->d_single,
-				worker_id, oldpkt[0]);
+				worker_id, return_count ? oldpkt[0] : NULL);
 			return (pkts[0]) ? 1 : 0;
 		} else
 			return -EINVAL;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 4/8] test/distributor: fix freeing mbufs
       [not found]       ` <CGME20200925224218eucas1p221c1af87b0e4547547106503cd336afd@eucas1p2.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Sanity tests with mbuf alloc and shutdown tests assume that
mbufs passed to worker cores are freed in handlers.
Such packets should not be returned to the distributor's main
core. The only packets that should be returned are the packets
send after completion of the tests in quit_workers function.

This patch fixes freeing mbufs, stops returning them
to distributor's core and cleans up unused variables.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index 0e3ab0c4f..b302ed118 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -65,20 +65,18 @@ handle_work(void *arg)
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *db = wp->dist;
-	unsigned int count = 0, num;
+	unsigned int num;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
 
 	num = rte_distributor_get_pkt(db, id, buf, NULL, 0);
 	while (!quit) {
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
-		count += num;
 		num = rte_distributor_get_pkt(db, id,
 				buf, buf, num);
 	}
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
-	count += num;
 	rte_distributor_return_pkt(db, id, buf, num);
 	return 0;
 }
@@ -274,21 +272,18 @@ handle_work_with_free_mbufs(void *arg)
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int i;
 	unsigned int num;
 	unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
 
 	num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 	while (!quit) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
 			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 	}
-	count += num;
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	rte_distributor_return_pkt(d, id, buf, num);
@@ -316,7 +311,6 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 			rte_distributor_process(d, NULL, 0);
 		for (j = 0; j < BURST; j++) {
 			bufs[j]->hash.usr = (i+j) << 1;
-			rte_mbuf_refcnt_set(bufs[j], 1);
 		}
 
 		rte_distributor_process(d, bufs, BURST);
@@ -340,15 +334,11 @@ sanity_test_with_mbuf_alloc(struct worker_params *wp, struct rte_mempool *p)
 static int
 handle_work_for_shutdown_test(void *arg)
 {
-	struct rte_mbuf *pkt = NULL;
 	struct rte_mbuf *buf[8] __rte_cache_aligned;
 	struct worker_params *wp = arg;
 	struct rte_distributor *d = wp->dist;
-	unsigned int count = 0;
 	unsigned int num;
-	unsigned int total = 0;
 	unsigned int i;
-	unsigned int returned = 0;
 	unsigned int zero_id = 0;
 	const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
 			__ATOMIC_RELAXED);
@@ -365,7 +355,6 @@ handle_work_for_shutdown_test(void *arg)
 	/* wait for quit single globally, or for worker zero, wait
 	 * for zero_quit */
 	while (!quit && !(id == zero_id && zero_quit)) {
-		count += num;
 		__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 				__ATOMIC_ACQ_REL);
 		for (i = 0; i < num; i++)
@@ -378,12 +367,7 @@ handle_work_for_shutdown_test(void *arg)
 				__ATOMIC_ACQUIRE);
 			__atomic_store_n(&zero_idx, zero_id, __ATOMIC_RELEASE);
 		}
-
-		total += num;
 	}
-	count += num;
-	returned = rte_distributor_return_pkt(d, id, buf, num);
-
 	__atomic_fetch_add(&worker_stats[id].handled_packets, num,
 			__ATOMIC_ACQ_REL);
 	if (id == zero_id) {
@@ -393,19 +377,19 @@ handle_work_for_shutdown_test(void *arg)
 		while (zero_quit)
 			usleep(100);
 
+		for (i = 0; i < num; i++)
+			rte_pktmbuf_free(buf[i]);
 		num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 
 		while (!quit) {
-			count += num;
-			rte_pktmbuf_free(pkt);
-			num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 			__atomic_fetch_add(&worker_stats[id].handled_packets,
 					num, __ATOMIC_ACQ_REL);
+			for (i = 0; i < num; i++)
+				rte_pktmbuf_free(buf[i]);
+			num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
 		}
-		returned = rte_distributor_return_pkt(d,
-				id, buf, num);
-		printf("Num returned = %d\n", returned);
 	}
+	rte_distributor_return_pkt(d, id, buf, num);
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 5/8] test/distributor: collect return mbufs
       [not found]       ` <CGME20200925224219eucas1p2d61447fef421573d653d2376423ecce0@eucas1p2.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

During quit_workers function distributor's main core processes
some packets to wake up pending worker cores so they can quit.
As quit_workers acts also as a cleanup procedure for next test
case it should also collect these packages returned by workers'
handlers, so the cyclic buffer with returned packets
in distributor remains empty.

Fixes: c3eabff124e6 ("distributor: add unit tests")
Cc: bruce.richardson@intel.com
Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 app/test/test_distributor.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index b302ed118..1fbdf6fd1 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -590,6 +590,10 @@ quit_workers(struct worker_params *wp, struct rte_mempool *p)
 	rte_distributor_process(d, NULL, 0);
 	rte_distributor_flush(d);
 	rte_eal_mp_wait_lcore();
+
+	while (rte_distributor_returned_pkts(d, bufs, RTE_MAX_LCORE))
+		;
+
 	quit = 0;
 	worker_idx = 0;
 	zero_idx = 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 6/8] distributor: fix missing handshake synchronization
       [not found]       ` <CGME20200925224220eucas1p1a44e99a1d7750d37d5aefa61f329209b@eucas1p1.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

rte_distributor_return_pkt function which is run on worker cores
must wait for distributor core to clear handshake on retptr64
before using those buffers. While the handshake is set distributor
core controls buffers and any operations on worker side might overwrite
buffers which are unread yet.
Same situation appears in the legacy single distributor. Function
rte_distributor_return_pkt_single shouldn't modify the bufptr64 until
handshake on it is cleared by distributor lcore.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_distributor/rte_distributor.c        | 14 ++++++++++++++
 lib/librte_distributor/rte_distributor_single.c |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 8a12bf856..dd68bc233 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -160,6 +160,7 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 {
 	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
 	unsigned int i;
+	volatile int64_t *retptr64;
 
 	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
 		if (num == 1)
@@ -169,6 +170,19 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 			return -EINVAL;
 	}
 
+	retptr64 = &(buf->retptr64[0]);
+	/* Spin while handshake bits are set (scheduler clears it).
+	 * Sync with worker on GET_BUF flag.
+	 */
+	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
+			& RTE_DISTRIB_GET_BUF)) {
+		rte_pause();
+		uint64_t t = rte_rdtsc()+100;
+
+		while (rte_rdtsc() < t)
+			rte_pause();
+	}
+
 	/* Sync with distributor to acquire retptrs */
 	__atomic_thread_fence(__ATOMIC_ACQUIRE);
 	for (i = 0; i < RTE_DIST_BURST_SIZE; i++)
diff --git a/lib/librte_distributor/rte_distributor_single.c b/lib/librte_distributor/rte_distributor_single.c
index abaf7730c..f4725b1d0 100644
--- a/lib/librte_distributor/rte_distributor_single.c
+++ b/lib/librte_distributor/rte_distributor_single.c
@@ -74,6 +74,10 @@ rte_distributor_return_pkt_single(struct rte_distributor_single *d,
 	union rte_distributor_buffer_single *buf = &d->bufs[worker_id];
 	uint64_t req = (((int64_t)(uintptr_t)oldpkt) << RTE_DISTRIB_FLAG_BITS)
 			| RTE_DISTRIB_RETURN_BUF;
+	while (unlikely(__atomic_load_n(&buf->bufptr64, __ATOMIC_RELAXED)
+			& RTE_DISTRIB_FLAGS_MASK))
+		rte_pause();
+
 	/* Sync with distributor on RETURN_BUF flag. */
 	__atomic_store_n(&(buf->bufptr64), req, __ATOMIC_RELEASE);
 	return 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 7/8] distributor: fix handshake deadlock
       [not found]       ` <CGME20200925224221eucas1p151297834da32a0f7cfdffc120f57ab3a@eucas1p1.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

Synchronization of data exchange between distributor and worker cores
is based on 2 handshakes: retptr64 for returning mbufs from workers
to distributor and bufptr64 for passing mbufs to workers.

Without proper order of verifying those 2 handshakes a deadlock may
occur. This can happen when worker core want to return back mbufs
and waits for retptr handshake to be cleared and distributor core
wait for bufptr to send mbufs to worker.

This can happen as worker core first returns mbufs to distributor
and later gets new mbufs, while distributor first release mbufs
to worker and later handle returning packets.

This patch fixes possibility of the deadlock by always taking care
of returning packets first on the distributor side and handling
packets while waiting to release new.

Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_distributor/rte_distributor.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index dd68bc233..b720abe03 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -321,12 +321,14 @@ release(struct rte_distributor *d, unsigned int wkr)
 	struct rte_distributor_buffer *buf = &(d->bufs[wkr]);
 	unsigned int i;
 
+	handle_returns(d, wkr);
+
 	/* Sync with worker on GET_BUF flag */
 	while (!(__atomic_load_n(&(d->bufs[wkr].bufptr64[0]), __ATOMIC_ACQUIRE)
-		& RTE_DISTRIB_GET_BUF))
+		& RTE_DISTRIB_GET_BUF)) {
+		handle_returns(d, wkr);
 		rte_pause();
-
-	handle_returns(d, wkr);
+	}
 
 	buf->count = 0;
 
@@ -376,6 +378,7 @@ rte_distributor_process(struct rte_distributor *d,
 		/* Flush out all non-full cache-lines to workers. */
 		for (wid = 0 ; wid < d->num_workers; wid++) {
 			/* Sync with worker on GET_BUF flag. */
+			handle_returns(d, wid);
 			if (__atomic_load_n(&(d->bufs[wid].bufptr64[0]),
 				__ATOMIC_ACQUIRE) & RTE_DISTRIB_GET_BUF) {
 				release(d, wid);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dpdk-stable] [PATCH v4 8/8] distributor: align API documentation with code
       [not found]       ` <CGME20200925224222eucas1p1b10891c21bfef6784777526af4443dde@eucas1p1.samsung.com>
@ 2020-09-25 22:42         ` Lukasz Wojciechowski
  0 siblings, 0 replies; 41+ messages in thread
From: Lukasz Wojciechowski @ 2020-09-25 22:42 UTC (permalink / raw)
  To: David Hunt, Bruce Richardson; +Cc: dev, l.wojciechow, stable

After introducing burst API there were some artefacts in the
API documentation from legacy single API.
Also the rte_distributor_poll_pkt() function return values
mismatched the implementation.

Fixes: c0de0eb82e40 ("distributor: switch over to new API")
Cc: david.hunt@intel.com
Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_distributor/rte_distributor.h | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/lib/librte_distributor/rte_distributor.h b/lib/librte_distributor/rte_distributor.h
index 327c0c4ab..a073e6461 100644
--- a/lib/librte_distributor/rte_distributor.h
+++ b/lib/librte_distributor/rte_distributor.h
@@ -155,7 +155,7 @@ rte_distributor_clear_returns(struct rte_distributor *d);
  * @param pkts
  *   The mbufs pointer array to be filled in (up to 8 packets)
  * @param oldpkt
- *   The previous packet, if any, being processed by the worker
+ *   The previous packets, if any, being processed by the worker
  * @param retcount
  *   The number of packets being returned
  *
@@ -187,15 +187,15 @@ rte_distributor_return_pkt(struct rte_distributor *d,
 
 /**
  * API called by a worker to request a new packet to process.
- * Any previous packet given to the worker is assumed to have completed
+ * Any previous packets given to the worker are assumed to have completed
  * processing, and may be optionally returned to the distributor via
  * the oldpkt parameter.
- * Unlike rte_distributor_get_pkt_burst(), this function does not wait for a
- * new packet to be provided by the distributor.
+ * Unlike rte_distributor_get_pkt(), this function does not wait for
+ * new packets to be provided by the distributor.
  *
- * NOTE: after calling this function, rte_distributor_poll_pkt_burst() should
- * be used to poll for the packet requested. The rte_distributor_get_pkt_burst()
- * API should *not* be used to try and retrieve the new packet.
+ * NOTE: after calling this function, rte_distributor_poll_pkt() should
+ * be used to poll for the packets requested. The rte_distributor_get_pkt()
+ * API should *not* be used to try and retrieve the new packets.
  *
  * @param d
  *   The distributor instance to be used
@@ -213,9 +213,9 @@ rte_distributor_request_pkt(struct rte_distributor *d,
 		unsigned int count);
 
 /**
- * API called by a worker to check for a new packet that was previously
+ * API called by a worker to check for new packets that were previously
  * requested by a call to rte_distributor_request_pkt(). It does not wait
- * for the new packet to be available, but returns NULL if the request has
+ * for the new packets to be available, but returns if the request has
  * not yet been fulfilled by the distributor.
  *
  * @param d
@@ -227,8 +227,9 @@ rte_distributor_request_pkt(struct rte_distributor *d,
  *   The array of mbufs being given to the worker
  *
  * @return
- *   The number of packets being given to the worker thread, zero if no
- *   packet is yet available.
+ *   The number of packets being given to the worker thread,
+ *   -1 if no packets are yet available (burst API - RTE_DIST_ALG_BURST)
+ *   0 if no packets are yet available (legacy single API - RTE_DIST_ALG_SINGLE)
  */
 int
 rte_distributor_poll_pkt(struct rte_distributor *d,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, back to index

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200915193449.13310-1-l.wojciechow@partner.samsung.com>
     [not found] ` <CGME20200915193457eucas1p2adbe25c41a0e4ef16c029e7bff104503@eucas1p2.samsung.com>
2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 1/6] app/test: fix deadlock in distributor test Lukasz Wojciechowski
2020-09-17 11:21     ` David Hunt
2020-09-17 14:01       ` Lukasz Wojciechowski
     [not found] ` <CGME20200915193457eucas1p2321d28b6abf69f244cd7c1e61ed0620e@eucas1p2.samsung.com>
2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 2/6] app/test: synchronize statistics between lcores Lukasz Wojciechowski
2020-09-17 11:50     ` David Hunt
     [not found] ` <CGME20200915193458eucas1p1d9308e63063eda28f96eedba3a361a2b@eucas1p1.samsung.com>
2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 3/6] app/test: fix freeing mbufs in distributor tests Lukasz Wojciechowski
2020-09-17 12:34     ` David Hunt
2020-09-22 12:42     ` [dpdk-stable] [dpdk-dev] " David Marchand
2020-09-23  1:55       ` Lukasz Wojciechowski
     [not found] ` <CGME20200915193459eucas1p19f5d1cbea87d7dc3bbd2638cdb96a31b@eucas1p1.samsung.com>
2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 4/6] app/test: collect return mbufs in distributor test Lukasz Wojciechowski
2020-09-17 12:37     ` David Hunt
     [not found] ` <CGME20200915193500eucas1p2b079e1dcfd2d54e01a5630609b82b370@eucas1p2.samsung.com>
2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 5/6] distributor: fix missing handshake synchronization Lukasz Wojciechowski
2020-09-17 13:22     ` David Hunt
     [not found] ` <CGME20200915193501eucas1p2333f0b08077c06ba04b89ce192072f9a@eucas1p2.samsung.com>
2020-09-15 19:34   ` [dpdk-stable] [PATCH v1 6/6] distributor: fix handshake deadlock Lukasz Wojciechowski
2020-09-17 13:28     ` David Hunt
     [not found] ` <20200923014713.16932-1-l.wojciechow@partner.samsung.com>
     [not found]   ` <CGME20200923014718eucas1p11fdcd774fef7b9e077e14e01c9f951d5@eucas1p1.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 1/8] app/test: fix deadlock in distributor test Lukasz Wojciechowski
     [not found]   ` <CGME20200923014719eucas1p2f26000109e86a649796e902c30e58bf0@eucas1p2.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 2/8] app/test: synchronize statistics between lcores Lukasz Wojciechowski
2020-09-23  4:30       ` [dpdk-stable] [dpdk-dev] " Honnappa Nagarahalli
2020-09-23 12:47         ` Lukasz Wojciechowski
     [not found]   ` <CGME20200923014719eucas1p165c419cff4f265cff8add8cc818210ff@eucas1p1.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 3/8] app/test: fix freeing mbufs in distributor tests Lukasz Wojciechowski
     [not found]   ` <CGME20200923014720eucas1p2bd5887c96c24839f364810a1bbe840da@eucas1p2.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 4/8] app/test: collect return mbufs in distributor test Lukasz Wojciechowski
     [not found]   ` <CGME20200923014721eucas1p1d22ac56c9b9e4fb49ac73d72d51a7a23@eucas1p1.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 5/8] distributor: fix missing handshake synchronization Lukasz Wojciechowski
     [not found]   ` <CGME20200923014722eucas1p2c2ef63759f4b800c1b5a80094e07e384@eucas1p2.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 6/8] distributor: fix handshake deadlock Lukasz Wojciechowski
     [not found]   ` <CGME20200923014723eucas1p2a7c7210a55289b3739faff4f5ed72e30@eucas1p2.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 7/8] distributor: do not use oldpkt when not needed Lukasz Wojciechowski
     [not found]   ` <CGME20200923014724eucas1p13d3c0428a15bea26def7a4343251e4e4@eucas1p1.samsung.com>
2020-09-23  1:47     ` [dpdk-stable] [PATCH v2 8/8] distributor: align API documentation with code Lukasz Wojciechowski
     [not found]   ` <20200923132541.21417-1-l.wojciechow@partner.samsung.com>
     [not found]     ` <CGME20200923132545eucas1p10db12d91121c9afdbab338bb60c8ed37@eucas1p1.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 1/8] app/test: fix deadlock in distributor test Lukasz Wojciechowski
     [not found]     ` <CGME20200923132546eucas1p212b6eede801514b544d82d41f5b7e4b8@eucas1p2.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 2/8] app/test: synchronize statistics between lcores Lukasz Wojciechowski
     [not found]     ` <CGME20200923132547eucas1p130620b0d5f3080a7a57234838a992e0e@eucas1p1.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 3/8] app/test: fix freeing mbufs in distributor tests Lukasz Wojciechowski
     [not found]     ` <CGME20200923132548eucas1p2a54328cddb79ae5e876eb104217d585f@eucas1p2.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 4/8] app/test: collect return mbufs in distributor test Lukasz Wojciechowski
     [not found]     ` <CGME20200923132549eucas1p29fc391c3f236fa704ff800774ab851f0@eucas1p2.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 5/8] distributor: fix missing handshake synchronization Lukasz Wojciechowski
     [not found]     ` <CGME20200923132550eucas1p2ce158dd81ccc04abcab4130d8cb391f4@eucas1p2.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 6/8] distributor: fix handshake deadlock Lukasz Wojciechowski
     [not found]     ` <CGME20200923132550eucas1p1ce21011562d0a00cccfd4ae3f0be4ff9@eucas1p1.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 7/8] distributor: do not use oldpkt when not needed Lukasz Wojciechowski
     [not found]     ` <CGME20200923132551eucas1p214a5f78c61e891c5e7b6cddc038d0e2e@eucas1p2.samsung.com>
2020-09-23 13:25       ` [dpdk-stable] [PATCH v3 8/8] distributor: align API documentation with code Lukasz Wojciechowski
     [not found]     ` <20200925224209.12173-1-l.wojciechow@partner.samsung.com>
     [not found]       ` <CGME20200925224216eucas1p1e8e1d0ecab4bbbf6e43b117c1d210649@eucas1p1.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 1/8] test/distributor: fix deadlock with freezed worker Lukasz Wojciechowski
     [not found]       ` <CGME20200925224217eucas1p1bb5f73109b4aeed8f2badf311fa8dfb5@eucas1p1.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 2/8] test/distributor: synchronize lcores statistics Lukasz Wojciechowski
     [not found]       ` <CGME20200925224218eucas1p2383ff0ebdaee18b581f5f731476f05ab@eucas1p2.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 3/8] distributor: do not use oldpkt when not needed Lukasz Wojciechowski
     [not found]       ` <CGME20200925224218eucas1p221c1af87b0e4547547106503cd336afd@eucas1p2.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 4/8] test/distributor: fix freeing mbufs Lukasz Wojciechowski
     [not found]       ` <CGME20200925224219eucas1p2d61447fef421573d653d2376423ecce0@eucas1p2.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 5/8] test/distributor: collect return mbufs Lukasz Wojciechowski
     [not found]       ` <CGME20200925224220eucas1p1a44e99a1d7750d37d5aefa61f329209b@eucas1p1.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 6/8] distributor: fix missing handshake synchronization Lukasz Wojciechowski
     [not found]       ` <CGME20200925224221eucas1p151297834da32a0f7cfdffc120f57ab3a@eucas1p1.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 7/8] distributor: fix handshake deadlock Lukasz Wojciechowski
     [not found]       ` <CGME20200925224222eucas1p1b10891c21bfef6784777526af4443dde@eucas1p1.samsung.com>
2020-09-25 22:42         ` [dpdk-stable] [PATCH v4 8/8] distributor: align API documentation with code Lukasz Wojciechowski

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox