[PATCH 0/4] pdump: fix capture on ports with hairpin queues

[PATCH 0/4] pdump: fix capture on ports with hairpin queues

[Stephen Hemminger]

This was reported on dpdk slack channel.

dpdk-dumpcap fails on a port with hairpin queues. The first attempt
reports "Unknown error -22" and every later attempt fails with EEXIST
until the application is restarted.

Several bugs combine: pdump tries to add callbacks to hairpin queues,
the error is returned with the wrong sign, and a partially enabled
capture is not undone. Also the first Rx callback API was missing the
hairpin check the other callback APIs have.

Stephen Hemminger (4):
  ethdev: reject hairpin queue in add first Rx callback
  pdump: fix error code when adding callback fails
  pdump: skip hairpin queues
  pdump: undo partial enable on failure

 lib/ethdev/rte_ethdev.c |   5 ++
 lib/pdump/rte_pdump.c   | 112 +++++++++++++++++++++++++++++++++-------
 2 files changed, 98 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH 1/4] ethdev: reject hairpin queue in add first Rx callback

[Stephen Hemminger]

rte_eth_add_rx_callback() and rte_eth_add_tx_callback() refuse
hairpin queues, but rte_eth_add_first_rx_callback() was missed.
A callback installed on a hairpin queue is never invoked.

Fixes: cf5516696d77 ("ethdev: add hairpin queue")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ethdev/rte_ethdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index f843e8808a..562a24d559 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6173,6 +6173,11 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 		return NULL;
 	}
 
+	if (rte_eth_dev_is_rx_hairpin_queue(&rte_eth_devices[port_id], queue_id)) {
+		rte_errno = EINVAL;
+		return NULL;
+	}
+
 	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
 
 	if (cb == NULL) {
-- 
2.53.0

[PATCH 2/4] pdump: fix error code when adding callback fails

[Stephen Hemminger]

The register functions returned positive rte_errno on failure.
The caller only checks for negative values, so a failed Rx enable
fell through to Tx, and the positive value sent back to the client
was negated into rte_errno, giving messages like "Unknown error -22".

Fixes: 278f945402c5 ("pdump: add new library for packet capture")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/pdump/rte_pdump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index ac94efe7ff..9c368bcfd7 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -283,7 +283,7 @@ pdump_register_rx_callbacks(enum pdump_version ver,
 				PDUMP_LOG_LINE(ERR,
 					"failed to add rx callback, errno=%d",
 					rte_errno);
-				return rte_errno;
+				return -rte_errno;
 			}
 
 			memset(&pdump_stats->rx[port][qid], 0, sizeof(struct rte_pdump_stats));
@@ -345,7 +345,7 @@ pdump_register_tx_callbacks(enum pdump_version ver,
 				PDUMP_LOG_LINE(ERR,
 					"failed to add tx callback, errno=%d",
 					rte_errno);
-				return rte_errno;
+				return -rte_errno;
 			}
 			memset(&pdump_stats->tx[port][qid], 0, sizeof(struct rte_pdump_stats));
 		} else if (operation == DISABLE) {
-- 
2.53.0

[PATCH 3/4] pdump: skip hairpin queues

[Stephen Hemminger]

When capturing on all queues, pdump walks nb_rx_queues and
nb_tx_queues, which include hairpin queues. Adding a Tx callback
to a hairpin queue fails, so capture could not be enabled on any
port with hairpin queues configured.

Skip hairpin queues when capturing all queues, and reject an
explicit request for a hairpin queue.

Fixes: cf5516696d77 ("ethdev: add hairpin queue")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/pdump/rte_pdump.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index 9c368bcfd7..3261876d29 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -8,6 +8,7 @@
 #include <rte_alarm.h>
 #include <rte_mbuf.h>
 #include <rte_ethdev.h>
+#include <ethdev_driver.h>
 #include <rte_lcore.h>
 #include <rte_log.h>
 #include <rte_memzone.h>
@@ -263,6 +264,16 @@ pdump_register_rx_callbacks(enum pdump_version ver,
 	for (; qid < end_q; qid++) {
 		struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
 
+		/* hairpin queues do not pass through rx burst */
+		if (rte_eth_dev_is_rx_hairpin_queue(&rte_eth_devices[port], qid)) {
+			if (queue == RTE_PDUMP_ALL_QUEUES)
+				continue;
+			PDUMP_LOG_LINE(ERR,
+				"cannot capture on hairpin rx queue port=%d queue=%d",
+				port, qid);
+			return -EINVAL;
+		}
+
 		if (operation == ENABLE) {
 			if (cbs->cb) {
 				PDUMP_LOG_LINE(ERR,
@@ -325,6 +336,16 @@ pdump_register_tx_callbacks(enum pdump_version ver,
 	for (; qid < end_q; qid++) {
 		struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
 
+		/* hairpin queues do not pass through tx burst */
+		if (rte_eth_dev_is_tx_hairpin_queue(&rte_eth_devices[port], qid)) {
+			if (queue == RTE_PDUMP_ALL_QUEUES)
+				continue;
+			PDUMP_LOG_LINE(ERR,
+				"cannot capture on hairpin tx queue port=%d queue=%d",
+				port, qid);
+			return -EINVAL;
+		}
+
 		if (operation == ENABLE) {
 			if (cbs->cb) {
 				PDUMP_LOG_LINE(ERR,
-- 
2.53.0

[PATCH 4/4] pdump: undo partial enable on failure

[Stephen Hemminger]

If enabling capture failed partway, the callbacks already installed
were left in place. They still reference the ring and mempool of the
client that requested capture, and every later enable on that port
fails with EEXIST until the application is restarted.

Remove callbacks installed by the failed request, on the failing
direction and on Rx when Tx fails. Also release the BPF filter on
the error paths.

Fixes: 278f945402c5 ("pdump: add new library for packet capture")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/pdump/rte_pdump.c | 91 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 72 insertions(+), 19 deletions(-)

diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index 3261876d29..644c6bee68 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -251,6 +251,39 @@ pdump_tx(uint16_t port, uint16_t queue,
 }
 
 
+/* Remove callbacks on queues [first, last) after a failed enable */
+static void
+pdump_unwind_rx_callbacks(uint16_t port, uint16_t first, uint16_t last)
+{
+	uint16_t qid;
+
+	for (qid = first; qid < last; qid++) {
+		struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
+
+		if (cbs->cb == NULL)
+			continue;
+		rte_eth_remove_rx_callback(port, qid, cbs->cb);
+		pdump_cb_wait(cbs);
+		cbs->cb = NULL;
+	}
+}
+
+static void
+pdump_unwind_tx_callbacks(uint16_t port, uint16_t first, uint16_t last)
+{
+	uint16_t qid;
+
+	for (qid = first; qid < last; qid++) {
+		struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
+
+		if (cbs->cb == NULL)
+			continue;
+		rte_eth_remove_tx_callback(port, qid, cbs->cb);
+		pdump_cb_wait(cbs);
+		cbs->cb = NULL;
+	}
+}
+
 static int
 pdump_register_rx_callbacks(enum pdump_version ver,
 			    uint16_t end_q, uint16_t port, uint16_t queue,
@@ -258,10 +291,11 @@ pdump_register_rx_callbacks(enum pdump_version ver,
 			    struct rte_bpf *filter,
 			    uint16_t operation, uint32_t snaplen)
 {
-	uint16_t qid;
+	uint16_t first, qid;
+	int ret;
 
-	qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
-	for (; qid < end_q; qid++) {
+	first = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
+	for (qid = first; qid < end_q; qid++) {
 		struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];
 
 		/* hairpin queues do not pass through rx burst */
@@ -279,6 +313,7 @@ pdump_register_rx_callbacks(enum pdump_version ver,
 				PDUMP_LOG_LINE(ERR,
 					"rx callback for port=%d queue=%d, already exists",
 					port, qid);
+				pdump_unwind_rx_callbacks(port, first, qid);
 				return -EEXIST;
 			}
 			cbs->use_count = 0;
@@ -294,13 +329,13 @@ pdump_register_rx_callbacks(enum pdump_version ver,
 				PDUMP_LOG_LINE(ERR,
 					"failed to add rx callback, errno=%d",
 					rte_errno);
-				return -rte_errno;
+				ret = -rte_errno;
+				pdump_unwind_rx_callbacks(port, first, qid);
+				return ret;
 			}
 
 			memset(&pdump_stats->rx[port][qid], 0, sizeof(struct rte_pdump_stats));
 		} else if (operation == DISABLE) {
-			int ret;
-
 			if (cbs->cb == NULL) {
 				PDUMP_LOG_LINE(ERR,
 					"no existing rx callback for port=%d queue=%d",
@@ -330,10 +365,11 @@ pdump_register_tx_callbacks(enum pdump_version ver,
 			    uint16_t operation, uint32_t snaplen)
 {
 
-	uint16_t qid;
+	uint16_t first, qid;
+	int ret;
 
-	qid = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
-	for (; qid < end_q; qid++) {
+	first = (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue;
+	for (qid = first; qid < end_q; qid++) {
 		struct pdump_rxtx_cbs *cbs = &tx_cbs[port][qid];
 
 		/* hairpin queues do not pass through tx burst */
@@ -351,6 +387,7 @@ pdump_register_tx_callbacks(enum pdump_version ver,
 				PDUMP_LOG_LINE(ERR,
 					"tx callback for port=%d queue=%d, already exists",
 					port, qid);
+				pdump_unwind_tx_callbacks(port, first, qid);
 				return -EEXIST;
 			}
 			cbs->use_count = 0;
@@ -366,12 +403,12 @@ pdump_register_tx_callbacks(enum pdump_version ver,
 				PDUMP_LOG_LINE(ERR,
 					"failed to add tx callback, errno=%d",
 					rte_errno);
-				return -rte_errno;
+				ret = -rte_errno;
+				pdump_unwind_tx_callbacks(port, first, qid);
+				return ret;
 			}
 			memset(&pdump_stats->tx[port][qid], 0, sizeof(struct rte_pdump_stats));
 		} else if (operation == DISABLE) {
-			int ret;
-
 			if (cbs->cb == NULL) {
 				PDUMP_LOG_LINE(ERR,
 					"no existing tx callback for port=%d queue=%d",
@@ -440,7 +477,8 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
 		PDUMP_LOG_LINE(ERR,
 			  "failed to get port id for device id=%s",
 			  p->device);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto free_filter;
 	}
 
 	/* validation if packet capture is for all queues */
@@ -452,7 +490,7 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
 			PDUMP_LOG_LINE(ERR,
 				"Error during getting device (port %u) info: %s",
 				port, strerror(-ret));
-			return ret;
+			goto free_filter;
 		}
 
 		nb_rx_q = dev_info.nb_rx_queues;
@@ -460,18 +498,21 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
 		if (nb_rx_q == 0 && flags & RTE_PDUMP_FLAG_RX) {
 			PDUMP_LOG_LINE(ERR,
 				"number of rx queues cannot be 0");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto free_filter;
 		}
 		if (nb_tx_q == 0 && flags & RTE_PDUMP_FLAG_TX) {
 			PDUMP_LOG_LINE(ERR,
 				"number of tx queues cannot be 0");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto free_filter;
 		}
 		if ((nb_tx_q == 0 || nb_rx_q == 0) &&
 			flags == RTE_PDUMP_FLAG_RXTX) {
 			PDUMP_LOG_LINE(ERR,
 				"both tx&rx queues must be non zero");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto free_filter;
 		}
 	}
 
@@ -482,7 +523,7 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
 						  ring, mp, filter,
 						  operation, p->snaplen);
 		if (ret < 0)
-			return ret;
+			goto free_filter;
 	}
 
 	/* register TX callback */
@@ -492,8 +533,20 @@ set_pdump_rxtx_cbs(const struct pdump_request *p)
 						  ring, mp, filter,
 						  operation, p->snaplen);
 		if (ret < 0)
-			return ret;
+			goto unwind_rx;
+	}
+
+	return 0;
+
+unwind_rx:
+	if (operation == ENABLE && (flags & RTE_PDUMP_FLAG_RX)) {
+		end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_rx_q : queue + 1;
+		pdump_unwind_rx_callbacks(port,
+			(queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue, end_q);
 	}
+free_filter:
+	if (filter != NULL)
+		rte_bpf_destroy(filter);
 
 	return ret;
 }
-- 
2.53.0