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

[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 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

[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

[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 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

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 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

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