[dpdk-dev] [PATCH 0/5] ixgbe: fix mbuf release on RX and TX

[dpdk-dev] [PATCH 0/5] ixgbe: fix mbuf release on RX and TX

[Bruce Richardson]

Konstantin has correctly pointed out that the previously applied fix:
 b35d0d80f0a8 ("ixgbe: check mbuf refcnt when clearing a ring")
is not a proper fix for the reported issue at all.
Ref: http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/21932

This patch set reverts the original fix, and applies a better fix for the
issue, as well as performing other cleanups in the code in question, to
try and avoid future issues.

Bruce Richardson (5):
  Revert "ixgbe: check mbuf refcnt when clearing a ring"
  ixgbe: fix comments on rx_queue fields
  ixgbe: fix bug on release of mbufs from queue
  ixgbe: rename tx queue release function for consistency
  ixgbe: remove awkward typecasts from ixgbe SSE PMD

 drivers/net/ixgbe/ixgbe_rxtx.c     | 19 +++++++++-
 drivers/net/ixgbe/ixgbe_rxtx.h     | 11 ++++--
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 76 +++++++++++++++++++++-----------------
 3 files changed, 68 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/5] Revert "ixgbe: check mbuf refcnt when clearing a ring"

[Bruce Richardson]

This reverts commit b35d0d80f0a809939549ddde99c1a76b7e38bff3.
The bug fix was incorrect as it did not take account of the fact that
the mbufs that were previously freed may have since be re-allocated.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c     | 3 +--
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 8 +-------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index af7e222..75c5555 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2272,8 +2272,7 @@ ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
 
 	if (rxq->sw_ring != NULL) {
 		for (i = 0; i < rxq->nb_rx_desc; i++) {
-			if (rxq->sw_ring[i].mbuf != NULL &&
-					rte_mbuf_refcnt_read(rxq->sw_ring[i].mbuf)) {
+			if (rxq->sw_ring[i].mbuf != NULL) {
 				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
 				rxq->sw_ring[i].mbuf = NULL;
 			}
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index d3ac74a..47ff990 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -736,13 +736,7 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 		     nb_free < max_desc && i != txq->tx_tail;
 		     i = (i + 1) & max_desc) {
 			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
-			/*
-			 * Check for already freed packets.
-			 * Note: ixgbe_tx_free_bufs does not NULL after free,
-			 * so we actually have to check the reference count.
-			 */
-			if (txe->mbuf != NULL &&
-					rte_mbuf_refcnt_read(txe->mbuf) != 0)
+			if (txe->mbuf != NULL)
 				rte_pktmbuf_free_seg(txe->mbuf);
 		}
 		/* reset tx_entry */
-- 
1.8.3.1

[dpdk-dev] [PATCH 2/5] ixgbe: fix comments on rx_queue fields

[Bruce Richardson]

The two fields for vector RX rearming in the rx queue structure were
incorrectly labelled. Switching the comments on each around makes things
clearer.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 0e6ad93..64e6bb9 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -122,8 +122,8 @@ struct ixgbe_rx_queue {
 	uint16_t rx_free_trigger; /**< triggers rx buffer allocation */
 #endif
 #ifdef RTE_IXGBE_INC_VECTOR
-	uint16_t            rxrearm_nb; /**< the idx we start the re-arming from */
-	uint16_t            rxrearm_start;  /**< number of remaining to be re-armed */
+	uint16_t            rxrearm_nb;     /**< number of remaining to be re-armed */
+	uint16_t            rxrearm_start;  /**< the idx we start the re-arming from */
 #endif
 	uint16_t            rx_free_thresh; /**< max free RX desc to hold. */
 	uint16_t            queue_id; /**< RX queue index. */
-- 
1.8.3.1

[dpdk-dev] [PATCH 3/5] ixgbe: fix bug on release of mbufs from queue

[Bruce Richardson]

The calculations of what mbufs were valid in the RX and TX queues were
incorrect when freeing the mbufs for the vector PMD. This led to crashes
due to invalid reference counts when mbuf debugging was turned on, and
possibly other more subtle problems (such as mbufs being freed when in use)
in other cases.

To fix this, the following changes were made:
* correct counts and post-loop values in the TX release function for the
  vector code.
* create a new separate RX release function for the RX vector code, since the
  tracking of what mbufs are valid or not is different for that code path

Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c     | 16 ++++++++++++
 drivers/net/ixgbe/ixgbe_rxtx.h     |  2 ++
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 52 +++++++++++++++++++++++++-------------
 3 files changed, 53 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 75c5555..a8bccab 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2270,6 +2270,14 @@ ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
 {
 	unsigned i;
 
+#ifdef RTE_IXGBE_INC_VECTOR
+	/* SSE Vector driver has a different way of releasing mbufs. */
+	if (rxq->rx_using_sse) {
+		ixgbe_rx_queue_release_mbufs_vec(rxq);
+		return;
+	}
+#endif
+
 	if (rxq->sw_ring != NULL) {
 		for (i = 0; i < rxq->nb_rx_desc; i++) {
 			if (rxq->sw_ring[i].mbuf != NULL) {
@@ -4013,6 +4021,14 @@ ixgbe_set_rx_function(struct rte_eth_dev *dev)
 
 		dev->rx_pkt_burst = ixgbe_recv_pkts;
 	}
+
+	if (adapter->rx_vec_allowed) {
+		unsigned i;
+		for (i = 0; i < dev->data->nb_rx_queues; i++) {
+			struct ixgbe_rx_queue *rxq = dev->data->rx_queues[i];
+			rxq->rx_using_sse = 1;
+		}
+	}
 }
 
 /**
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 64e6bb9..2ed6816 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -121,6 +121,7 @@ struct ixgbe_rx_queue {
 	uint16_t rx_next_avail; /**< idx of next staged pkt to ret to app */
 	uint16_t rx_free_trigger; /**< triggers rx buffer allocation */
 #endif
+	uint16_t            rx_using_sse;   /**< indicates that vector RX is in use */
 #ifdef RTE_IXGBE_INC_VECTOR
 	uint16_t            rxrearm_nb;     /**< number of remaining to be re-armed */
 	uint16_t            rxrearm_start;  /**< the idx we start the re-arming from */
@@ -284,6 +285,7 @@ uint16_t ixgbe_recv_scattered_pkts_vec(void *rx_queue,
 		struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
 int ixgbe_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev);
 int ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq);
+void ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq);
 
 #ifdef RTE_IXGBE_INC_VECTOR
 
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index 47ff990..a6a506f 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -726,25 +726,43 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 {
 	unsigned i;
 	struct ixgbe_tx_entry_v *txe;
-	uint16_t nb_free, max_desc;
+	const uint16_t max_desc = (uint16_t)(txq->nb_tx_desc - 1);
 
-	if (txq->sw_ring != NULL) {
-		/* release the used mbufs in sw_ring */
-		nb_free = txq->nb_tx_free;
-		max_desc = (uint16_t)(txq->nb_tx_desc - 1);
-		for (i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
-		     nb_free < max_desc && i != txq->tx_tail;
-		     i = (i + 1) & max_desc) {
-			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
-			if (txe->mbuf != NULL)
-				rte_pktmbuf_free_seg(txe->mbuf);
-		}
-		/* reset tx_entry */
-		for (i = 0; i < txq->nb_tx_desc; i++) {
-			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
-			txe->mbuf = NULL;
-		}
+	if (txq->sw_ring == NULL || txq->nb_tx_free == max_desc)
+		return;
+	
+	/* release the used mbufs in sw_ring */
+	for (i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
+	     i != txq->tx_tail;
+	     i = (i + 1) & max_desc) {
+		txe = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[i];
+		rte_pktmbuf_free_seg(txe->mbuf);
 	}
+	txq->nb_tx_free = max_desc;
+
+	/* reset tx_entry */
+	for (i = 0; i < txq->nb_tx_desc; i++) {
+		txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
+		txe->mbuf = NULL;
+	}
+}
+
+void __attribute__((cold))
+ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
+{
+	const unsigned mask = rxq->nb_rx_desc - 1;
+	unsigned i;
+
+	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
+		return;
+
+	/* free all mbufs that are valid in the ring */
+	for (i = rxq->rx_tail; i != rxq->rxrearm_start; i = (i+1) & mask)
+		rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
+	rxq->rxrearm_nb = rxq->nb_rx_desc;
+
+	/* set all entries to NULL */
+	memset(rxq->sw_ring, 0, sizeof(rxq->sw_ring[0]) * rxq->nb_rx_desc);
 }
 
 static void __attribute__((cold))
-- 
1.8.3.1

[dpdk-dev] [PATCH 4/5] ixgbe: rename tx queue release function for consistency

[Bruce Richardson]

The function inside the vector/SSE poll-mode driver for releasing
the mbufs on the TX queues had the same name as another function
inside the regular PMD. To keep consistency and avoid confusion,
rename the vector PMD version to have a "_vec" suffix.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index a6a506f..7d57b63 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -722,7 +722,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 }
 
 static void __attribute__((cold))
-ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
+ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 {
 	unsigned i;
 	struct ixgbe_tx_entry_v *txe;
@@ -812,7 +812,7 @@ ixgbe_reset_tx_queue(struct ixgbe_tx_queue *txq)
 }
 
 static const struct ixgbe_txq_ops vec_txq_ops = {
-	.release_mbufs = ixgbe_tx_queue_release_mbufs,
+	.release_mbufs = ixgbe_tx_queue_release_mbufs_vec,
 	.free_swring = ixgbe_tx_free_swring,
 	.reset = ixgbe_reset_tx_queue,
 };
-- 
1.8.3.1

[dpdk-dev] [PATCH 5/5] ixgbe: remove awkward typecasts from ixgbe SSE PMD

[Bruce Richardson]

The vector/SSE pmd used a different element type for the tx queue sw_ring
entries. This led to lots of typecasts in the code which required specific
use of bracketing, leading to subtle errors.
For example, in the original code:
	txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
instead needs to be written as:
	txe = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[i];

We can eliminate this problem, by having two software ring pointers in the
structure for the two different element types.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.h     |  5 ++++-
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 18 ++++++++----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 2ed6816..36388ec 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -192,7 +192,10 @@ struct ixgbe_tx_queue {
 	/** TX ring virtual address. */
 	volatile union ixgbe_adv_tx_desc *tx_ring;
 	uint64_t            tx_ring_phys_addr; /**< TX ring DMA address. */
-	struct ixgbe_tx_entry *sw_ring;      /**< virtual address of SW ring. */
+	union {
+		struct ixgbe_tx_entry *sw_ring; /**< virtual address of SW ring. */
+		struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW ring for vector PMD */
+	};
 	volatile uint32_t   *tdt_reg_addr; /**< Address of TDT register. */
 	uint16_t            nb_tx_desc;    /**< number of TX descriptors. */
 	uint16_t            tx_tail;       /**< current value of TDT reg. */
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index 7d57b63..0c659b7 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -608,8 +608,7 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
 	 * first buffer to free from S/W ring is at index
 	 * tx_next_dd - (tx_rs_thresh-1)
 	 */
-	txep = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[txq->tx_next_dd -
-			(n - 1)];
+	txep = &txq->sw_ring_v[txq->tx_next_dd - (n - 1)];
 	m = __rte_pktmbuf_prefree_seg(txep[0].mbuf);
 	if (likely(m != NULL)) {
 		free[0] = m;
@@ -678,7 +677,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 	tx_id = txq->tx_tail;
 	txdp = &txq->tx_ring[tx_id];
-	txep = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[tx_id];
+	txep = &txq->sw_ring_v[tx_id];
 
 	txq->nb_tx_free = (uint16_t)(txq->nb_tx_free - nb_pkts);
 
@@ -699,7 +698,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 		/* avoid reach the end of ring */
 		txdp = &(txq->tx_ring[tx_id]);
-		txep = &(((struct ixgbe_tx_entry_v *)txq->sw_ring)[tx_id]);
+		txep = &txq->sw_ring_v[tx_id];
 	}
 
 	tx_backlog_entry(txep, tx_pkts, nb_commit);
@@ -735,14 +734,14 @@ ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 	for (i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
 	     i != txq->tx_tail;
 	     i = (i + 1) & max_desc) {
-		txe = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[i];
+		txe = &txq->sw_ring_v[i];
 		rte_pktmbuf_free_seg(txe->mbuf);
 	}
 	txq->nb_tx_free = max_desc;
 
 	/* reset tx_entry */
 	for (i = 0; i < txq->nb_tx_desc; i++) {
-		txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
+		txe = &txq->sw_ring_v[i];
 		txe->mbuf = NULL;
 	}
 }
@@ -781,7 +780,7 @@ static void __attribute__((cold))
 ixgbe_reset_tx_queue(struct ixgbe_tx_queue *txq)
 {
 	static const union ixgbe_adv_tx_desc zeroed_desc = {{0}};
-	struct ixgbe_tx_entry_v *txe = (struct ixgbe_tx_entry_v *)txq->sw_ring;
+	struct ixgbe_tx_entry_v *txe = txq->sw_ring_v;
 	uint16_t i;
 
 	/* Zero out HW ring memory */
@@ -838,12 +837,11 @@ ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq)
 int __attribute__((cold))
 ixgbe_txq_vec_setup(struct ixgbe_tx_queue *txq)
 {
-	if (txq->sw_ring == NULL)
+	if (txq->sw_ring_v == NULL)
 		return -1;
 
 	/* leave the first one for overflow */
-	txq->sw_ring = (struct ixgbe_tx_entry *)
-		((struct ixgbe_tx_entry_v *)txq->sw_ring + 1);
+	txq->sw_ring_v = txq->sw_ring_v + 1;
 	txq->ops = &vec_txq_ops;
 
 	return 0;
-- 
1.8.3.1

[dpdk-dev] [PATCHv2 0/5] ixgbe: fix mbuf release on RX and TX

[Konstantin Ananyev]

Konstantin has correctly pointed out that the previously applied fix:
b35d0d80f0a8 ("ixgbe: check mbuf refcnt when clearing a ring")
is not a proper fix for the reported issue at all.
Ref: http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/21932

This patch set reverts the original fix, and applies a better fix for the
issue, as well as performing other cleanups in the code in question, to
try and avoid future issues.

v2 chages:
- Make sure that rx_using_sse is reset to zero if scalar RX function was chosen.
- fix checkpatch.pl errors.
- fix remaining wrong typecast.

Konstantin Ananyev (5):
  Revert "ixgbe: check mbuf refcnt when clearing a ring"
  ixgbe: fix comments on rx_queue fields
  ixgbe: fix bug on release of mbufs from queue
  ixgbe: rename tx queue release function for consistency
  ixgbe: remove awkward typecasts from ixgbe SSE PMD

 drivers/net/ixgbe/ixgbe_rxtx.c     | 23 ++++++++++-
 drivers/net/ixgbe/ixgbe_rxtx.h     | 12 ++++--
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 80 +++++++++++++++++++++-----------------
 3 files changed, 75 insertions(+), 40 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCHv2 1/5] Revert "ixgbe: check mbuf refcnt when clearing a ring"

[Konstantin Ananyev]

This reverts commit b35d0d80f0a809939549ddde99c1a76b7e38bff3.
The bug fix was incorrect as it did not take account of the fact that
the mbufs that were previously freed may have since be re-allocated.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c     | 3 +--
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 8 +-------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index af7e222..75c5555 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2272,8 +2272,7 @@ ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
 
 	if (rxq->sw_ring != NULL) {
 		for (i = 0; i < rxq->nb_rx_desc; i++) {
-			if (rxq->sw_ring[i].mbuf != NULL &&
-					rte_mbuf_refcnt_read(rxq->sw_ring[i].mbuf)) {
+			if (rxq->sw_ring[i].mbuf != NULL) {
 				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
 				rxq->sw_ring[i].mbuf = NULL;
 			}
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index d3ac74a..47ff990 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -736,13 +736,7 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 		     nb_free < max_desc && i != txq->tx_tail;
 		     i = (i + 1) & max_desc) {
 			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
-			/*
-			 * Check for already freed packets.
-			 * Note: ixgbe_tx_free_bufs does not NULL after free,
-			 * so we actually have to check the reference count.
-			 */
-			if (txe->mbuf != NULL &&
-					rte_mbuf_refcnt_read(txe->mbuf) != 0)
+			if (txe->mbuf != NULL)
 				rte_pktmbuf_free_seg(txe->mbuf);
 		}
 		/* reset tx_entry */
-- 
1.8.3.1

[dpdk-dev] [PATCHv2 2/5] ixgbe: fix comments on rx_queue fields

[Konstantin Ananyev]

The two fields for vector RX rearming in the rx queue structure were
incorrectly labelled. Switching the comments on each around makes things
clearer.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 0e6ad93..64e6bb9 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -122,8 +122,8 @@ struct ixgbe_rx_queue {
 	uint16_t rx_free_trigger; /**< triggers rx buffer allocation */
 #endif
 #ifdef RTE_IXGBE_INC_VECTOR
-	uint16_t            rxrearm_nb; /**< the idx we start the re-arming from */
-	uint16_t            rxrearm_start;  /**< number of remaining to be re-armed */
+	uint16_t            rxrearm_nb;     /**< number of remaining to be re-armed */
+	uint16_t            rxrearm_start;  /**< the idx we start the re-arming from */
 #endif
 	uint16_t            rx_free_thresh; /**< max free RX desc to hold. */
 	uint16_t            queue_id; /**< RX queue index. */
-- 
1.8.3.1

[dpdk-dev] [PATCHv2 3/5] ixgbe: fix bug on release of mbufs from queue

[Konstantin Ananyev]

The calculations of what mbufs were valid in the RX and TX queues were
incorrect when freeing the mbufs for the vector PMD. This led to crashes
due to invalid reference counts when mbuf debugging was turned on, and
possibly other more subtle problems (such as mbufs being freed when in use)
in other cases.

To fix this, the following changes were made:
* correct counts and post-loop values in the TX release function for the
  vector code.
* create a new separate RX release function for the RX vector code, since the
  tracking of what mbufs are valid or not is different for that code path

Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")

v2 chages:
- Make sure that rx_using_sse is reset to zero if scalar RX function was chosen.
- fix checkpatch.pl errors.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c     | 20 +++++++++++++++
 drivers/net/ixgbe/ixgbe_rxtx.h     |  3 +++
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 52 +++++++++++++++++++++++++-------------
 3 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 75c5555..db2454c 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2270,6 +2270,14 @@ ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
 {
 	unsigned i;
 
+#ifdef RTE_IXGBE_INC_VECTOR
+	/* SSE Vector driver has a different way of releasing mbufs. */
+	if (rxq->rx_using_sse) {
+		ixgbe_rx_queue_release_mbufs_vec(rxq);
+		return;
+	}
+#endif
+
 	if (rxq->sw_ring != NULL) {
 		for (i = 0; i < rxq->nb_rx_desc; i++) {
 			if (rxq->sw_ring[i].mbuf != NULL) {
@@ -3925,6 +3933,7 @@ ixgbe_set_ivar(struct rte_eth_dev *dev, u8 entry, u8 vector, s8 type)
 void __attribute__((cold))
 ixgbe_set_rx_function(struct rte_eth_dev *dev)
 {
+	uint16_t i, rx_using_sse;
 	struct ixgbe_adapter *adapter =
 		(struct ixgbe_adapter *)dev->data->dev_private;
 
@@ -4013,6 +4022,17 @@ ixgbe_set_rx_function(struct rte_eth_dev *dev)
 
 		dev->rx_pkt_burst = ixgbe_recv_pkts;
 	}
+
+	/* Propagate information about RX function choice through all queues. */
+
+	rx_using_sse =
+		(dev->rx_pkt_burst == ixgbe_recv_scattered_pkts_vec ||
+		dev->rx_pkt_burst == ixgbe_recv_pkts_vec);
+
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		struct ixgbe_rx_queue *rxq = dev->data->rx_queues[i];
+		rxq->rx_using_sse = rx_using_sse;
+	}
 }
 
 /**
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 64e6bb9..befdc3a 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -121,6 +121,8 @@ struct ixgbe_rx_queue {
 	uint16_t rx_next_avail; /**< idx of next staged pkt to ret to app */
 	uint16_t rx_free_trigger; /**< triggers rx buffer allocation */
 #endif
+	uint16_t            rx_using_sse;
+	/**< indicates that vector RX is in use */
 #ifdef RTE_IXGBE_INC_VECTOR
 	uint16_t            rxrearm_nb;     /**< number of remaining to be re-armed */
 	uint16_t            rxrearm_start;  /**< the idx we start the re-arming from */
@@ -284,6 +286,7 @@ uint16_t ixgbe_recv_scattered_pkts_vec(void *rx_queue,
 		struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
 int ixgbe_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev);
 int ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq);
+void ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq);
 
 #ifdef RTE_IXGBE_INC_VECTOR
 
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index 47ff990..c01da7b 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -726,27 +726,45 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 {
 	unsigned i;
 	struct ixgbe_tx_entry_v *txe;
-	uint16_t nb_free, max_desc;
+	const uint16_t max_desc = (uint16_t)(txq->nb_tx_desc - 1);
 
-	if (txq->sw_ring != NULL) {
-		/* release the used mbufs in sw_ring */
-		nb_free = txq->nb_tx_free;
-		max_desc = (uint16_t)(txq->nb_tx_desc - 1);
-		for (i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
-		     nb_free < max_desc && i != txq->tx_tail;
-		     i = (i + 1) & max_desc) {
-			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
-			if (txe->mbuf != NULL)
-				rte_pktmbuf_free_seg(txe->mbuf);
-		}
-		/* reset tx_entry */
-		for (i = 0; i < txq->nb_tx_desc; i++) {
-			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
-			txe->mbuf = NULL;
-		}
+	if (txq->sw_ring == NULL || txq->nb_tx_free == max_desc)
+		return;
+
+	/* release the used mbufs in sw_ring */
+	for (i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
+	     i != txq->tx_tail;
+	     i = (i + 1) & max_desc) {
+		txe = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[i];
+		rte_pktmbuf_free_seg(txe->mbuf);
+	}
+	txq->nb_tx_free = max_desc;
+
+	/* reset tx_entry */
+	for (i = 0; i < txq->nb_tx_desc; i++) {
+		txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
+		txe->mbuf = NULL;
 	}
 }
 
+void __attribute__((cold))
+ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
+{
+	const unsigned mask = rxq->nb_rx_desc - 1;
+	unsigned i;
+
+	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
+		return;
+
+	/* free all mbufs that are valid in the ring */
+	for (i = rxq->rx_tail; i != rxq->rxrearm_start; i = (i + 1) & mask)
+		rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
+	rxq->rxrearm_nb = rxq->nb_rx_desc;
+
+	/* set all entries to NULL */
+	memset(rxq->sw_ring, 0, sizeof(rxq->sw_ring[0]) * rxq->nb_rx_desc);
+}
+
 static void __attribute__((cold))
 ixgbe_tx_free_swring(struct ixgbe_tx_queue *txq)
 {
-- 
1.8.3.1

[dpdk-dev] [PATCHv2 4/5] ixgbe: rename tx queue release function for consistency

[Konstantin Ananyev]

The function inside the vector/SSE poll-mode driver for releasing
the mbufs on the TX queues had the same name as another function
inside the regular PMD. To keep consistency and avoid confusion,
rename the vector PMD version to have a "_vec" suffix.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index c01da7b..d3da31d 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -722,7 +722,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 }
 
 static void __attribute__((cold))
-ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
+ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 {
 	unsigned i;
 	struct ixgbe_tx_entry_v *txe;
@@ -812,7 +812,7 @@ ixgbe_reset_tx_queue(struct ixgbe_tx_queue *txq)
 }
 
 static const struct ixgbe_txq_ops vec_txq_ops = {
-	.release_mbufs = ixgbe_tx_queue_release_mbufs,
+	.release_mbufs = ixgbe_tx_queue_release_mbufs_vec,
 	.free_swring = ixgbe_tx_free_swring,
 	.reset = ixgbe_reset_tx_queue,
 };
-- 
1.8.3.1

[dpdk-dev] [PATCHv2 5/5] ixgbe: remove awkward typecasts from ixgbe SSE PMD

[Konstantin Ananyev]

The vector/SSE pmd used a different element type for the tx queue sw_ring
entries. This led to lots of typecasts in the code which required specific
use of bracketing, leading to subtle errors.
For example, in the original code:
	txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
instead needs to be written as:
	txe = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[i];

We can eliminate this problem, by having two software ring pointers in the
structure for the two different element types.

v2 changes:
- fix remaining wrong typecast.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.h     |  5 ++++-
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 22 ++++++++++------------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index befdc3a..1557438 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -193,7 +193,10 @@ struct ixgbe_tx_queue {
 	/** TX ring virtual address. */
 	volatile union ixgbe_adv_tx_desc *tx_ring;
 	uint64_t            tx_ring_phys_addr; /**< TX ring DMA address. */
-	struct ixgbe_tx_entry *sw_ring;      /**< virtual address of SW ring. */
+	union {
+		struct ixgbe_tx_entry *sw_ring; /**< address of SW ring for scalar PMD. */
+		struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW ring for vector PMD */
+	};
 	volatile uint32_t   *tdt_reg_addr; /**< Address of TDT register. */
 	uint16_t            nb_tx_desc;    /**< number of TX descriptors. */
 	uint16_t            tx_tail;       /**< current value of TDT reg. */
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index d3da31d..9c5390e 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -608,8 +608,7 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
 	 * first buffer to free from S/W ring is at index
 	 * tx_next_dd - (tx_rs_thresh-1)
 	 */
-	txep = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[txq->tx_next_dd -
-			(n - 1)];
+	txep = &txq->sw_ring_v[txq->tx_next_dd - (n - 1)];
 	m = __rte_pktmbuf_prefree_seg(txep[0].mbuf);
 	if (likely(m != NULL)) {
 		free[0] = m;
@@ -678,7 +677,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 	tx_id = txq->tx_tail;
 	txdp = &txq->tx_ring[tx_id];
-	txep = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[tx_id];
+	txep = &txq->sw_ring_v[tx_id];
 
 	txq->nb_tx_free = (uint16_t)(txq->nb_tx_free - nb_pkts);
 
@@ -699,7 +698,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 		/* avoid reach the end of ring */
 		txdp = &(txq->tx_ring[tx_id]);
-		txep = &(((struct ixgbe_tx_entry_v *)txq->sw_ring)[tx_id]);
+		txep = &txq->sw_ring_v[tx_id];
 	}
 
 	tx_backlog_entry(txep, tx_pkts, nb_commit);
@@ -735,14 +734,14 @@ ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 	for (i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
 	     i != txq->tx_tail;
 	     i = (i + 1) & max_desc) {
-		txe = &((struct ixgbe_tx_entry_v *)txq->sw_ring)[i];
+		txe = &txq->sw_ring_v[i];
 		rte_pktmbuf_free_seg(txe->mbuf);
 	}
 	txq->nb_tx_free = max_desc;
 
 	/* reset tx_entry */
 	for (i = 0; i < txq->nb_tx_desc; i++) {
-		txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
+		txe = &txq->sw_ring_v[i];
 		txe->mbuf = NULL;
 	}
 }
@@ -772,8 +771,8 @@ ixgbe_tx_free_swring(struct ixgbe_tx_queue *txq)
 		return;
 
 	if (txq->sw_ring != NULL) {
-		rte_free((struct ixgbe_rx_entry *)txq->sw_ring - 1);
-		txq->sw_ring = NULL;
+		rte_free(txq->sw_ring_v - 1);
+		txq->sw_ring_v = NULL;
 	}
 }
 
@@ -781,7 +780,7 @@ static void __attribute__((cold))
 ixgbe_reset_tx_queue(struct ixgbe_tx_queue *txq)
 {
 	static const union ixgbe_adv_tx_desc zeroed_desc = {{0}};
-	struct ixgbe_tx_entry_v *txe = (struct ixgbe_tx_entry_v *)txq->sw_ring;
+	struct ixgbe_tx_entry_v *txe = txq->sw_ring_v;
 	uint16_t i;
 
 	/* Zero out HW ring memory */
@@ -838,12 +837,11 @@ ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq)
 int __attribute__((cold))
 ixgbe_txq_vec_setup(struct ixgbe_tx_queue *txq)
 {
-	if (txq->sw_ring == NULL)
+	if (txq->sw_ring_v == NULL)
 		return -1;
 
 	/* leave the first one for overflow */
-	txq->sw_ring = (struct ixgbe_tx_entry *)
-		((struct ixgbe_tx_entry_v *)txq->sw_ring + 1);
+	txq->sw_ring_v = txq->sw_ring_v + 1;
 	txq->ops = &vec_txq_ops;
 
 	return 0;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCHv2 1/5] Revert "ixgbe: check mbuf refcnt when clearing a ring"

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin Ananyev
> Sent: Friday, July 24, 2015 6:58 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCHv2 1/5] Revert "ixgbe: check mbuf refcnt when
> clearing a ring"
> 
> This reverts commit b35d0d80f0a809939549ddde99c1a76b7e38bff3.
> The bug fix was incorrect as it did not take account of the fact that the mbufs that
> were previously freed may have since be re-allocated.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

Re: [dpdk-dev] [PATCHv2 4/5] ixgbe: rename tx queue release function for consistency

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin Ananyev
> Sent: Friday, July 24, 2015 6:58 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCHv2 4/5] ixgbe: rename tx queue release function for
> consistency
> 
> The function inside the vector/SSE poll-mode driver for releasing the mbufs on
> the TX queues had the same name as another function inside the regular PMD.
> To keep consistency and avoid confusion, rename the vector PMD version to
> have a "_vec" suffix.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

Re: [dpdk-dev] [PATCHv2 0/5] ixgbe: fix mbuf release on RX and TX

[Thomas Monjalon]

2015-07-24 14:58, Konstantin Ananyev:
> Konstantin has correctly pointed out that the previously applied fix:
> b35d0d80f0a8 ("ixgbe: check mbuf refcnt when clearing a ring")
> is not a proper fix for the reported issue at all.
> Ref: http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/21932
> 
> This patch set reverts the original fix, and applies a better fix for the
> issue, as well as performing other cleanups in the code in question, to
> try and avoid future issues.
> 
> v2 chages:
> - Make sure that rx_using_sse is reset to zero if scalar RX function was chosen.
> - fix checkpatch.pl errors.
> - fix remaining wrong typecast.

Applied, thanks