[PATCH 0/4] Fix i40e/iavf queue reconfig and restarting

[PATCH 0/4] Fix i40e/iavf queue reconfig and restarting

[Bruce Richardson]

Fixes for a number of issues found when looking at stopping and
restarting individual queues, sometimes with a reconfig in the middle.
For i40e, it already claims to support runtime reconfig, while for
iavf this support is not claimed, but appears to work ok once the
bugs in the cleanup code are fixed, therefore the last patch enables
it, by setting the appropriate capability flag.

Bruce Richardson (4):
  net/i40e: fix buffer leak on Rx reconfiguration
  net/iavf: fix buffer leak on Tx queue stop
  net/iavf: fix restart of Rx queue on reconfigure
  net/iavf: add support for runtime queue reconfiguration

 doc/guides/nics/features/iavf.ini       |  2 ++
 drivers/net/i40e/i40e_rxtx.c            |  6 ++++++
 drivers/net/i40e/i40e_rxtx_vec_common.h |  1 +
 drivers/net/iavf/iavf_ethdev.c          |  4 +++-
 drivers/net/iavf/iavf_rxtx.c            |  6 ++++++
 drivers/net/iavf/iavf_rxtx_vec_avx512.c | 17 ++++++++---------
 drivers/net/iavf/iavf_rxtx_vec_common.h | 11 +++++------
 7 files changed, 31 insertions(+), 16 deletions(-)

--
2.39.2

[PATCH 1/4] net/i40e: fix buffer leak on Rx reconfiguration

[Bruce Richardson]

When reconfiguring a single queue on a device, the mbuf initializer
value was not getting set, and remained at zero. This lead to mbuf leaks
as the refcount was incorrect (0), so on free it wrapped around to
UINT16_MAX. When setting up the mbuf initializer, also ensure that the
queue is explicitly marked as using a vector function by setting the
"rx_using_sse" flag.

Fixes: a3c83a2527e1 ("net/i40e: enable runtime queue setup")
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c            | 6 ++++++
 drivers/net/i40e/i40e_rxtx_vec_common.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index b4f65b58fa..d96bbbb677 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1918,6 +1918,12 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
 		if (use_def_burst_func)
 			ad->rx_bulk_alloc_allowed = false;
 		i40e_set_rx_function(dev);
+
+		if (ad->rx_vec_allowed && i40e_rxq_vec_setup(rxq)) {
+			PMD_DRV_LOG(ERR, "Failed vector rx setup.");
+			return -EINVAL;
+		}
+
 		return 0;
 	} else if (ad->rx_vec_allowed && !rte_is_power_of_2(rxq->nb_rx_desc)) {
 		PMD_DRV_LOG(ERR, "Vector mode is allowed, but descriptor"
diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index fe1a6ec75e..8b745630e4 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -201,6 +201,7 @@ i40e_rxq_vec_setup_default(struct i40e_rx_queue *rxq)
 	rte_compiler_barrier();
 	p = (uintptr_t)&mb_def.rearm_data;
 	rxq->mbuf_initializer = *(uint64_t *)p;
+	rxq->rx_using_sse = 1;
 	return 0;
 }
 
-- 
2.39.2

[PATCH 2/4] net/iavf: fix buffer leak on Tx queue stop

[Bruce Richardson]

When a queue is stopped all buffers are meant to released from it, and
then a new set of buffers reallocated on start. For the iavf code when
using vector Tx, some buffers were left in the ring, and so those
buffers were leaked. The buffers were missed as the release code only
handled one side of a wrap-around case in the ring.

Fix the issue by doing a single iteration of the ring freeing all
buffers in it, handling wraparound through a simple post-increment
check.

Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
Fixes: 9ab9514c150e ("net/iavf: enable AVX512 for Tx")
Cc: wenzhuo.lu@intel.com
Cc: jingjing.wu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/iavf/iavf_rxtx_vec_avx512.c | 17 ++++++++---------
 drivers/net/iavf/iavf_rxtx_vec_common.h | 11 +++++------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
index 3e66df5341..48337d5e03 100644
--- a/drivers/net/iavf/iavf_rxtx_vec_avx512.c
+++ b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
@@ -2460,20 +2460,19 @@ iavf_tx_queue_release_mbufs_avx512(struct iavf_tx_queue *txq)
 {
 	unsigned int i;
 	const uint16_t max_desc = (uint16_t)(txq->nb_tx_desc - 1);
+	const uint16_t end_desc = txq->tx_tail >> txq->use_ctx; /* next empty slot */
+	const uint16_t wrap_point = txq->nb_tx_desc >> txq->use_ctx;  /* end of SW ring */
 	struct iavf_tx_vec_entry *swr = (void *)txq->sw_ring;
 
 	if (!txq->sw_ring || txq->nb_free == max_desc)
 		return;
 
-	i = (txq->next_dd >> txq->use_ctx) + 1 -
-			(txq->rs_thresh >> txq->use_ctx);
-
-	if (txq->tx_tail < i) {
-		for (; i < (unsigned int)(txq->nb_tx_desc >> txq->use_ctx); i++) {
-			rte_pktmbuf_free_seg(swr[i].mbuf);
-			swr[i].mbuf = NULL;
-		}
-		i = 0;
+	i = (txq->next_dd - txq->rs_thresh + 1) >> txq->use_ctx;
+	while (i != end_desc) {
+		rte_pktmbuf_free_seg(swr[i].mbuf);
+		swr[i].mbuf = NULL;
+		if (++i == wrap_point)
+			i = 0;
 	}
 }
 
diff --git a/drivers/net/iavf/iavf_rxtx_vec_common.h b/drivers/net/iavf/iavf_rxtx_vec_common.h
index ddb13ce8c3..1fac957fe1 100644
--- a/drivers/net/iavf/iavf_rxtx_vec_common.h
+++ b/drivers/net/iavf/iavf_rxtx_vec_common.h
@@ -186,12 +186,11 @@ _iavf_tx_queue_release_mbufs_vec(struct iavf_tx_queue *txq)
 		return;
 
 	i = txq->next_dd - txq->rs_thresh + 1;
-	if (txq->tx_tail < i) {
-		for (; i < txq->nb_tx_desc; i++) {
-			rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf);
-			txq->sw_ring[i].mbuf = NULL;
-		}
-		i = 0;
+	while (i != txq->tx_tail) {
+		rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf);
+		txq->sw_ring[i].mbuf = NULL;
+		if (++i == txq->nb_tx_desc)
+			i = 0;
 	}
 }
 
-- 
2.39.2

[PATCH 3/4] net/iavf: fix restart of Rx queue on reconfigure

[Bruce Richardson]

After reconfiguring an RX queue the mbuf_initializer value was not being
correctly set. Fix this by calling the appropriate function if vector
processing is enabled. This mirrors the behaviour by the i40e driver.

Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
Cc: jingjing.wu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/iavf/iavf_rxtx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index f7df4665d1..347f686831 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -755,6 +755,12 @@ iavf_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 	if (check_rx_vec_allow(rxq) == false)
 		ad->rx_vec_allowed = false;
 
+	/* check vector conflict */
+	if (ad->rx_vec_allowed && iavf_rxq_vec_setup(rxq)) {
+		PMD_DRV_LOG(ERR, "Failed vector rx setup.");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.39.2

[PATCH 4/4] net/iavf: add support for runtime queue reconfiguration

[Bruce Richardson]

Unlike the i40e driver, the iavf driver does not advertise support for
runtime reconfiguration, meaning that application using the same
hardware may get different behaviour when using a PF vs a VF. On testing
with a 40G NIC, the only blocker to reconfiguring an RX queue on the fly
is the fact that this support is not advertised by the driver.

Add support for runtime reconfig by reporting it in the device info
flags.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/nics/features/iavf.ini | 2 ++
 drivers/net/iavf/iavf_ethdev.c    | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/doc/guides/nics/features/iavf.ini b/doc/guides/nics/features/iavf.ini
index b72cd98484..db4f92ce71 100644
--- a/doc/guides/nics/features/iavf.ini
+++ b/doc/guides/nics/features/iavf.ini
@@ -11,6 +11,8 @@ Speed capabilities   = Y
 Link status          = Y
 Rx interrupt         = Y
 Queue start/stop     = Y
+Runtime Rx queue setup = Y
+Runtime Tx queue setup = Y
 Power mgmt address monitor = Y
 MTU update           = Y
 Scattered Rx         = Y
diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index f2fc5a5621..22fbd7d6b2 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -1127,7 +1127,9 @@ iavf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->reta_size = vf->vf_res->rss_lut_size;
 	dev_info->flow_type_rss_offloads = IAVF_RSS_OFFLOAD_ALL;
 	dev_info->max_mac_addrs = IAVF_NUM_MACADDR_MAX;
-	dev_info->dev_capa &= ~RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP;
+	dev_info->dev_capa =
+		RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP |
+		RTE_ETH_DEV_CAPA_RUNTIME_TX_QUEUE_SETUP;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_VLAN_STRIP |
 		RTE_ETH_RX_OFFLOAD_QINQ_STRIP |
-- 
2.39.2

[PATCH v2 0/4] Fix i40e/iavf queue reconfig and restarting

[Bruce Richardson]

Fixes for a number of issues found when looking at stopping and
restarting individual queues, sometimes with a reconfig in the middle.
For i40e, it already claims to support runtime reconfig, while for
iavf this support is not claimed, but appears to work ok once the
bugs in the cleanup code are fixed, therefore the last patch enables
it, by setting the appropriate capability flag.

---
V2: Fix build issues with non-x86, non-arm architectures reported
    by the CI.

Bruce Richardson (4):
  net/i40e: fix buffer leak on Rx reconfiguration
  net/iavf: fix buffer leak on Tx queue stop
  net/iavf: fix restart of Rx queue on reconfigure
  net/iavf: add support for runtime queue reconfiguration

 doc/guides/nics/features/iavf.ini       |  2 ++
 drivers/net/i40e/i40e_rxtx.c            |  6 ++++++
 drivers/net/i40e/i40e_rxtx_vec_common.h |  1 +
 drivers/net/iavf/iavf_ethdev.c          |  4 +++-
 drivers/net/iavf/iavf_rxtx.c            |  7 +++++++
 drivers/net/iavf/iavf_rxtx_vec_avx512.c | 17 ++++++++---------
 drivers/net/iavf/iavf_rxtx_vec_common.h | 11 +++++------
 7 files changed, 32 insertions(+), 16 deletions(-)

--
2.39.2

[PATCH v2 1/4] net/i40e: fix buffer leak on Rx reconfiguration

[Bruce Richardson]

When reconfiguring a single queue on a device, the mbuf initializer
value was not getting set, and remained at zero. This lead to mbuf leaks
as the refcount was incorrect (0), so on free it wrapped around to
UINT16_MAX. When setting up the mbuf initializer, also ensure that the
queue is explicitly marked as using a vector function by setting the
"rx_using_sse" flag.

Fixes: a3c83a2527e1 ("net/i40e: enable runtime queue setup")
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c            | 6 ++++++
 drivers/net/i40e/i40e_rxtx_vec_common.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index b4f65b58fa..d96bbbb677 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1918,6 +1918,12 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
 		if (use_def_burst_func)
 			ad->rx_bulk_alloc_allowed = false;
 		i40e_set_rx_function(dev);
+
+		if (ad->rx_vec_allowed && i40e_rxq_vec_setup(rxq)) {
+			PMD_DRV_LOG(ERR, "Failed vector rx setup.");
+			return -EINVAL;
+		}
+
 		return 0;
 	} else if (ad->rx_vec_allowed && !rte_is_power_of_2(rxq->nb_rx_desc)) {
 		PMD_DRV_LOG(ERR, "Vector mode is allowed, but descriptor"
diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index fe1a6ec75e..8b745630e4 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -201,6 +201,7 @@ i40e_rxq_vec_setup_default(struct i40e_rx_queue *rxq)
 	rte_compiler_barrier();
 	p = (uintptr_t)&mb_def.rearm_data;
 	rxq->mbuf_initializer = *(uint64_t *)p;
+	rxq->rx_using_sse = 1;
 	return 0;
 }
 
-- 
2.39.2

RE: [PATCH v2 1/4] net/i40e: fix buffer leak on Rx reconfiguration

[Zhang, Qi Z]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday, August 31, 2023 8:34 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH v2 1/4] net/i40e: fix buffer leak on Rx reconfiguration
> 
> When reconfiguring a single queue on a device, the mbuf initializer value was
> not getting set, and remained at zero. This lead to mbuf leaks as the refcount
> was incorrect (0), so on free it wrapped around to UINT16_MAX. When
> setting up the mbuf initializer, also ensure that the queue is explicitly marked
> as using a vector function by setting the "rx_using_sse" flag.
> 
> Fixes: a3c83a2527e1 ("net/i40e: enable runtime queue setup")
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c            | 6 ++++++
>  drivers/net/i40e/i40e_rxtx_vec_common.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index
> b4f65b58fa..d96bbbb677 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1918,6 +1918,12 @@ i40e_dev_rx_queue_setup_runtime(struct
> rte_eth_dev *dev,
>  		if (use_def_burst_func)
>  			ad->rx_bulk_alloc_allowed = false;
>  		i40e_set_rx_function(dev);
> +
> +		if (ad->rx_vec_allowed && i40e_rxq_vec_setup(rxq)) {
> +			PMD_DRV_LOG(ERR, "Failed vector rx setup.");
> +			return -EINVAL;
> +		}
> +
>  		return 0;
>  	} else if (ad->rx_vec_allowed && !rte_is_power_of_2(rxq-
> >nb_rx_desc)) {
>  		PMD_DRV_LOG(ERR, "Vector mode is allowed, but
> descriptor"
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index fe1a6ec75e..8b745630e4 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -201,6 +201,7 @@ i40e_rxq_vec_setup_default(struct i40e_rx_queue
> *rxq)
>  	rte_compiler_barrier();
>  	p = (uintptr_t)&mb_def.rearm_data;
>  	rxq->mbuf_initializer = *(uint64_t *)p;
> +	rxq->rx_using_sse = 1;

I saw some further code clean can be done by leveraging this change.

>  	return 0;
>  }
> 
> --
> 2.39.2

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

[PATCH v2 2/4] net/iavf: fix buffer leak on Tx queue stop

[Bruce Richardson]

When a queue is stopped all buffers are meant to released from it, and
then a new set of buffers reallocated on start. For the iavf code when
using vector Tx, some buffers were left in the ring, and so those
buffers were leaked. The buffers were missed as the release code only
handled one side of a wrap-around case in the ring.

Fix the issue by doing a single iteration of the ring freeing all
buffers in it, handling wraparound through a simple post-increment
check.

Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
Fixes: 9ab9514c150e ("net/iavf: enable AVX512 for Tx")
Cc: wenzhuo.lu@intel.com
Cc: jingjing.wu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/iavf/iavf_rxtx_vec_avx512.c | 17 ++++++++---------
 drivers/net/iavf/iavf_rxtx_vec_common.h | 11 +++++------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
index 3e66df5341..48337d5e03 100644
--- a/drivers/net/iavf/iavf_rxtx_vec_avx512.c
+++ b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
@@ -2460,20 +2460,19 @@ iavf_tx_queue_release_mbufs_avx512(struct iavf_tx_queue *txq)
 {
 	unsigned int i;
 	const uint16_t max_desc = (uint16_t)(txq->nb_tx_desc - 1);
+	const uint16_t end_desc = txq->tx_tail >> txq->use_ctx; /* next empty slot */
+	const uint16_t wrap_point = txq->nb_tx_desc >> txq->use_ctx;  /* end of SW ring */
 	struct iavf_tx_vec_entry *swr = (void *)txq->sw_ring;
 
 	if (!txq->sw_ring || txq->nb_free == max_desc)
 		return;
 
-	i = (txq->next_dd >> txq->use_ctx) + 1 -
-			(txq->rs_thresh >> txq->use_ctx);
-
-	if (txq->tx_tail < i) {
-		for (; i < (unsigned int)(txq->nb_tx_desc >> txq->use_ctx); i++) {
-			rte_pktmbuf_free_seg(swr[i].mbuf);
-			swr[i].mbuf = NULL;
-		}
-		i = 0;
+	i = (txq->next_dd - txq->rs_thresh + 1) >> txq->use_ctx;
+	while (i != end_desc) {
+		rte_pktmbuf_free_seg(swr[i].mbuf);
+		swr[i].mbuf = NULL;
+		if (++i == wrap_point)
+			i = 0;
 	}
 }
 
diff --git a/drivers/net/iavf/iavf_rxtx_vec_common.h b/drivers/net/iavf/iavf_rxtx_vec_common.h
index ddb13ce8c3..1fac957fe1 100644
--- a/drivers/net/iavf/iavf_rxtx_vec_common.h
+++ b/drivers/net/iavf/iavf_rxtx_vec_common.h
@@ -186,12 +186,11 @@ _iavf_tx_queue_release_mbufs_vec(struct iavf_tx_queue *txq)
 		return;
 
 	i = txq->next_dd - txq->rs_thresh + 1;
-	if (txq->tx_tail < i) {
-		for (; i < txq->nb_tx_desc; i++) {
-			rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf);
-			txq->sw_ring[i].mbuf = NULL;
-		}
-		i = 0;
+	while (i != txq->tx_tail) {
+		rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf);
+		txq->sw_ring[i].mbuf = NULL;
+		if (++i == txq->nb_tx_desc)
+			i = 0;
 	}
 }
 
-- 
2.39.2

RE: [PATCH v2 2/4] net/iavf: fix buffer leak on Tx queue stop

[Lu, Wenzhuo]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday, August 31, 2023 8:34 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> stable@dpdk.org
> Subject: [PATCH v2 2/4] net/iavf: fix buffer leak on Tx queue stop
> 
> When a queue is stopped all buffers are meant to released from it, and then a
> new set of buffers reallocated on start. For the iavf code when using vector Tx,
> some buffers were left in the ring, and so those buffers were leaked. The
> buffers were missed as the release code only handled one side of a
> wrap-around case in the ring.
> 
> Fix the issue by doing a single iteration of the ring freeing all buffers in it,
> handling wraparound through a simple post-increment check.
> 
> Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
> Fixes: 9ab9514c150e ("net/iavf: enable AVX512 for Tx")
> Cc: wenzhuo.lu@intel.com
> Cc: jingjing.wu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

RE: [PATCH v2 2/4] net/iavf: fix buffer leak on Tx queue stop

[Zhang, Qi Z]

> -----Original Message-----
> From: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Sent: Monday, September 4, 2023 10:18 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH v2 2/4] net/iavf: fix buffer leak on Tx queue stop
> 
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce <bruce.richardson@intel.com>
> > Sent: Thursday, August 31, 2023 8:34 PM
> > To: dev@dpdk.org
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> > stable@dpdk.org
> > Subject: [PATCH v2 2/4] net/iavf: fix buffer leak on Tx queue stop
> >
> > When a queue is stopped all buffers are meant to released from it, and
> > then a new set of buffers reallocated on start. For the iavf code when
> > using vector Tx, some buffers were left in the ring, and so those
> > buffers were leaked. The buffers were missed as the release code only
> > handled one side of a wrap-around case in the ring.
> >
> > Fix the issue by doing a single iteration of the ring freeing all
> > buffers in it, handling wraparound through a simple post-increment check.
> >
> > Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
> > Fixes: 9ab9514c150e ("net/iavf: enable AVX512 for Tx")
> > Cc: wenzhuo.lu@intel.com
> > Cc: jingjing.wu@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

[PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure

[Bruce Richardson]

After reconfiguring an RX queue the mbuf_initializer value was not being
correctly set. Fix this by calling the appropriate function if vector
processing is enabled. This mirrors the behaviour by the i40e driver.

Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
Cc: jingjing.wu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/iavf/iavf_rxtx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index f7df4665d1..797cdda4b2 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -755,6 +755,13 @@ iavf_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 	if (check_rx_vec_allow(rxq) == false)
 		ad->rx_vec_allowed = false;
 
+#if defined RTE_ARCH_X86 || defined RTE_ARCH_ARM
+	/* check vector conflict */
+	if (ad->rx_vec_allowed && iavf_rxq_vec_setup(rxq)) {
+		PMD_DRV_LOG(ERR, "Failed vector rx setup.");
+		return -EINVAL;
+	}
+#endif
 	return 0;
 }
 
-- 
2.39.2

RE: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure

[Zhang, Qi Z]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, August 31, 2023 8:34 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure
> 
> After reconfiguring an RX queue the mbuf_initializer value was not being
> correctly set. Fix this by calling the appropriate function if vector processing is
> enabled. This mirrors the behaviour by the i40e driver.
> 
> Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
> Cc: jingjing.wu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/iavf/iavf_rxtx.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c index
> f7df4665d1..797cdda4b2 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -755,6 +755,13 @@ iavf_dev_rx_queue_setup(struct rte_eth_dev *dev,
> uint16_t queue_idx,
>  	if (check_rx_vec_allow(rxq) == false)
>  		ad->rx_vec_allowed = false;
> 
> +#if defined RTE_ARCH_X86 || defined RTE_ARCH_ARM
> +	/* check vector conflict */
> +	if (ad->rx_vec_allowed && iavf_rxq_vec_setup(rxq)) {
> +		PMD_DRV_LOG(ERR, "Failed vector rx setup.");
> +		return -EINVAL;
> +	}
> +#endif

Bruce:

	May I know more details about how to reproduce this issue?
	As the iavf PMD does not support RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP (i40e does)
	Which indicates before we call rte_eth_rx_queue_setup, the device must be stopped (the flag will be checked in rte_eth_rx_queue_setup)

	So if we do below steps

	rte_eth_dev_stop ..
	rte_eth_rx_queue_setup
	rte_eth_dev_start

	the iavf_rxq_vec_setup should be invoked in rte_eth_dev_start -> iavf_set_rx_function 

	anything I missed?

Thanks
Qi

>  	return 0;
>  }
> 
> --
> 2.39.2

RE: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, September 4, 2023 9:15 AM
> To: Bruce Richardson <bruce.richardson@intel.com>; dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure
> 
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Thursday, August 31, 2023 8:34 PM
> > To: dev@dpdk.org
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; stable@dpdk.org
> > Subject: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on
> > reconfigure
> >
> > After reconfiguring an RX queue the mbuf_initializer value was not
> > being correctly set. Fix this by calling the appropriate function if
> > vector processing is enabled. This mirrors the behaviour by the i40e driver.
> >
> > Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
> > Cc: jingjing.wu@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  drivers/net/iavf/iavf_rxtx.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/iavf/iavf_rxtx.c
> > b/drivers/net/iavf/iavf_rxtx.c index
> > f7df4665d1..797cdda4b2 100644
> > --- a/drivers/net/iavf/iavf_rxtx.c
> > +++ b/drivers/net/iavf/iavf_rxtx.c
> > @@ -755,6 +755,13 @@ iavf_dev_rx_queue_setup(struct rte_eth_dev *dev,
> > uint16_t queue_idx,
> >  	if (check_rx_vec_allow(rxq) == false)
> >  		ad->rx_vec_allowed = false;
> >
> > +#if defined RTE_ARCH_X86 || defined RTE_ARCH_ARM
> > +	/* check vector conflict */
> > +	if (ad->rx_vec_allowed && iavf_rxq_vec_setup(rxq)) {
> > +		PMD_DRV_LOG(ERR, "Failed vector rx setup.");
> > +		return -EINVAL;
> > +	}
> > +#endif
> 
> Bruce:
> 
> 	May I know more details about how to reproduce this issue?
> 	As the iavf PMD does not support
> RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP (i40e does)

OK, not sure if the patch 4/4 answered my question 😊

should I squash patch 3, 4 into one? , for my understanding patch 3 doesn't appear to be a bug fix unless we announce RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP.



> 	Which indicates before we call rte_eth_rx_queue_setup, the device
> must be stopped (the flag will be checked in rte_eth_rx_queue_setup)
> 
> 	So if we do below steps
> 
> 	rte_eth_dev_stop ..
> 	rte_eth_rx_queue_setup
> 	rte_eth_dev_start
> 
> 	the iavf_rxq_vec_setup should be invoked in rte_eth_dev_start ->
> iavf_set_rx_function
> 
> 	anything I missed?
> 
> Thanks
> Qi
> 
> >  	return 0;
> >  }
> >
> > --
> > 2.39.2

Re: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure

[Bruce Richardson]

On Mon, Sep 04, 2023 at 02:30:32AM +0100, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Monday, September 4, 2023 9:15 AM
> > To: Bruce Richardson <bruce.richardson@intel.com>; dev@dpdk.org
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; stable@dpdk.org
> > Subject: RE: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > Sent: Thursday, August 31, 2023 8:34 PM
> > > To: dev@dpdk.org
> > > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; stable@dpdk.org
> > > Subject: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on
> > > reconfigure
> > >
> > > After reconfiguring an RX queue the mbuf_initializer value was not
> > > being correctly set. Fix this by calling the appropriate function if
> > > vector processing is enabled. This mirrors the behaviour by the i40e driver.
> > >
> > > Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
> > > Cc: jingjing.wu@intel.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  drivers/net/iavf/iavf_rxtx.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/net/iavf/iavf_rxtx.c
> > > b/drivers/net/iavf/iavf_rxtx.c index
> > > f7df4665d1..797cdda4b2 100644
> > > --- a/drivers/net/iavf/iavf_rxtx.c
> > > +++ b/drivers/net/iavf/iavf_rxtx.c
> > > @@ -755,6 +755,13 @@ iavf_dev_rx_queue_setup(struct rte_eth_dev *dev,
> > > uint16_t queue_idx,
> > >  	if (check_rx_vec_allow(rxq) == false)
> > >  		ad->rx_vec_allowed = false;
> > >
> > > +#if defined RTE_ARCH_X86 || defined RTE_ARCH_ARM
> > > +	/* check vector conflict */
> > > +	if (ad->rx_vec_allowed && iavf_rxq_vec_setup(rxq)) {
> > > +		PMD_DRV_LOG(ERR, "Failed vector rx setup.");
> > > +		return -EINVAL;
> > > +	}
> > > +#endif
> > 
> > Bruce:
> > 
> > 	May I know more details about how to reproduce this issue?
> > 	As the iavf PMD does not support
> > RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP (i40e does)
> 
> OK, not sure if the patch 4/4 answered my question 😊
> 
> should I squash patch 3, 4 into one? , for my understanding patch 3 doesn't appear to be a bug fix unless we announce RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP.
> 
You may have a point. I was experimenting with queue reconfiguration which
is where I hit this issue.
However, even without queue reconfig support, the device still needs to
support queue-stop followed by queue-start, I think, and there may still be
an issue there - I'll have to check.

/Bruce

RE: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure

[Zhang, Qi Z]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Monday, September 4, 2023 3:54 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on reconfigure
> 
> On Mon, Sep 04, 2023 at 02:30:32AM +0100, Zhang, Qi Z wrote:
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Monday, September 4, 2023 9:15 AM
> > > To: Bruce Richardson <bruce.richardson@intel.com>; dev@dpdk.org
> > > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; stable@dpdk.org
> > > Subject: RE: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on
> > > reconfigure
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > > Sent: Thursday, August 31, 2023 8:34 PM
> > > > To: dev@dpdk.org
> > > > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Wu, Jingjing
> > > > <jingjing.wu@intel.com>; stable@dpdk.org
> > > > Subject: [PATCH v2 3/4] net/iavf: fix restart of Rx queue on
> > > > reconfigure
> > > >
> > > > After reconfiguring an RX queue the mbuf_initializer value was not
> > > > being correctly set. Fix this by calling the appropriate function
> > > > if vector processing is enabled. This mirrors the behaviour by the i40e
> driver.
> > > >
> > > > Fixes: 319c421f3890 ("net/avf: enable SSE Rx Tx")
> > > > Cc: jingjing.wu@intel.com
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > ---
> > > >  drivers/net/iavf/iavf_rxtx.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/net/iavf/iavf_rxtx.c
> > > > b/drivers/net/iavf/iavf_rxtx.c index
> > > > f7df4665d1..797cdda4b2 100644
> > > > --- a/drivers/net/iavf/iavf_rxtx.c
> > > > +++ b/drivers/net/iavf/iavf_rxtx.c
> > > > @@ -755,6 +755,13 @@ iavf_dev_rx_queue_setup(struct rte_eth_dev
> > > > *dev, uint16_t queue_idx,
> > > >  	if (check_rx_vec_allow(rxq) == false)
> > > >  		ad->rx_vec_allowed = false;
> > > >
> > > > +#if defined RTE_ARCH_X86 || defined RTE_ARCH_ARM
> > > > +	/* check vector conflict */
> > > > +	if (ad->rx_vec_allowed && iavf_rxq_vec_setup(rxq)) {
> > > > +		PMD_DRV_LOG(ERR, "Failed vector rx setup.");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +#endif
> > >
> > > Bruce:
> > >
> > > 	May I know more details about how to reproduce this issue?
> > > 	As the iavf PMD does not support
> > > RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP (i40e does)
> >
> > OK, not sure if the patch 4/4 answered my question 😊
> >
> > should I squash patch 3, 4 into one? , for my understanding patch 3 doesn't
> appear to be a bug fix unless we announce
> RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP.
> >
> You may have a point. I was experimenting with queue reconfiguration which
> is where I hit this issue.
> However, even without queue reconfig support, the device still needs to
> support queue-stop followed by queue-start, I think, and there may still be an
> issue there - I'll have to check.

Yes, queue start / stop should be supported.
Btw, I didn't see mbuf_initializer has been reset during queue stop / start, it might be a different issue.




> /Bruce

[PATCH v2 4/4] net/iavf: add support for runtime queue reconfiguration

[Bruce Richardson]

Unlike the i40e driver, the iavf driver does not advertise support for
runtime reconfiguration, meaning that application using the same
hardware may get different behaviour when using a PF vs a VF. On testing
with a 40G NIC, the only blocker to reconfiguring an RX queue on the fly
is the fact that this support is not advertised by the driver.

Add support for runtime reconfig by reporting it in the device info
flags.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/nics/features/iavf.ini | 2 ++
 drivers/net/iavf/iavf_ethdev.c    | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/doc/guides/nics/features/iavf.ini b/doc/guides/nics/features/iavf.ini
index b72cd98484..db4f92ce71 100644
--- a/doc/guides/nics/features/iavf.ini
+++ b/doc/guides/nics/features/iavf.ini
@@ -11,6 +11,8 @@ Speed capabilities   = Y
 Link status          = Y
 Rx interrupt         = Y
 Queue start/stop     = Y
+Runtime Rx queue setup = Y
+Runtime Tx queue setup = Y
 Power mgmt address monitor = Y
 MTU update           = Y
 Scattered Rx         = Y
diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index f2fc5a5621..22fbd7d6b2 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -1127,7 +1127,9 @@ iavf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->reta_size = vf->vf_res->rss_lut_size;
 	dev_info->flow_type_rss_offloads = IAVF_RSS_OFFLOAD_ALL;
 	dev_info->max_mac_addrs = IAVF_NUM_MACADDR_MAX;
-	dev_info->dev_capa &= ~RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP;
+	dev_info->dev_capa =
+		RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP |
+		RTE_ETH_DEV_CAPA_RUNTIME_TX_QUEUE_SETUP;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_VLAN_STRIP |
 		RTE_ETH_RX_OFFLOAD_QINQ_STRIP |
-- 
2.39.2

RE: [PATCH v2 4/4] net/iavf: add support for runtime queue reconfiguration

[Zhang, Qi Z]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, August 31, 2023 8:34 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [PATCH v2 4/4] net/iavf: add support for runtime queue
> reconfiguration
> 
> Unlike the i40e driver, the iavf driver does not advertise support for runtime
> reconfiguration, meaning that application using the same hardware may get
> different behaviour when using a PF vs a VF. On testing with a 40G NIC, the
> only blocker to reconfiguring an RX queue on the fly is the fact that this
> support is not advertised by the driver.
> 
> Add support for runtime reconfig by reporting it in the device info flags.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  doc/guides/nics/features/iavf.ini | 2 ++
>  drivers/net/iavf/iavf_ethdev.c    | 4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/nics/features/iavf.ini
> b/doc/guides/nics/features/iavf.ini
> index b72cd98484..db4f92ce71 100644
> --- a/doc/guides/nics/features/iavf.ini
> +++ b/doc/guides/nics/features/iavf.ini
> @@ -11,6 +11,8 @@ Speed capabilities   = Y
>  Link status          = Y
>  Rx interrupt         = Y
>  Queue start/stop     = Y
> +Runtime Rx queue setup = Y
> +Runtime Tx queue setup = Y
>  Power mgmt address monitor = Y
>  MTU update           = Y
>  Scattered Rx         = Y
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index f2fc5a5621..22fbd7d6b2 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -1127,7 +1127,9 @@ iavf_dev_info_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
>  	dev_info->reta_size = vf->vf_res->rss_lut_size;
>  	dev_info->flow_type_rss_offloads = IAVF_RSS_OFFLOAD_ALL;
>  	dev_info->max_mac_addrs = IAVF_NUM_MACADDR_MAX;
> -	dev_info->dev_capa &= ~RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP;
> +	dev_info->dev_capa =
> +		RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP |
> +		RTE_ETH_DEV_CAPA_RUNTIME_TX_QUEUE_SETUP;
>  	dev_info->rx_offload_capa =
>  		RTE_ETH_RX_OFFLOAD_VLAN_STRIP |
>  		RTE_ETH_RX_OFFLOAD_QINQ_STRIP |
> --
> 2.39.2

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Squashed with Patch 3/4, applied to dpdk-next-net-intel.

Thanks
Qi