patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/ixgbe: fix RxQ/TxQ release
@ 2021-09-27 17:18 Julien Meunier
  2021-09-28  3:06 ` Wang, Haiyue
  2021-09-28  8:12 ` [dpdk-stable] [PATCH v2] " Julien Meunier
  0 siblings, 2 replies; 7+ messages in thread
From: Julien Meunier @ 2021-09-27 17:18 UTC (permalink / raw)
  To: dev; +Cc: stable, bruce.richardson, Haiyue Wang

On the vector implementation, during the tear-down, the mbufs not
drained in the RxQ and TxQ are freed based on an algorithm which
supposed that the number of descriptors is a power of 2 (max_desc).
Based on this hypothesis, this algorithm uses a bitmask in order to
detect an index overflow during the iteration, and to restart the loop
from 0.

However, there is no such power of 2 requirement in the ixgbe for the
number of descriptors in the RxQ / TxQ. The only requirement is to have
a number correctly aligned.

If a user requested to configure a number of descriptors which is not a
power of 2, as a consequence, during the tear-down, it was possible to
be in an infinite loop, and to never reach the exit loop condition.

By removing the bitmask and changing the loop method, we can avoid this
issue, and allow the user to configure a RxQ / TxQ which is not a power
of 2.

Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Julien Meunier <julien.meunier@nokia.com>
---
 drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
index adba855ca3..8912558918 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
@@ -150,11 +150,14 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 		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) {
+	i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
+	while (i != txq->tx_tail) {
 		txe = &txq->sw_ring_v[i];
 		rte_pktmbuf_free_seg(txe->mbuf);
+
+		i = i + 1;
+		if (i > max_desc)
+			i = 0;
 	}
 	txq->nb_tx_free = max_desc;
 
@@ -168,7 +171,7 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 static inline void
 _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
 {
-	const unsigned int mask = rxq->nb_rx_desc - 1;
+	const unsigned int max_desc = rxq->nb_rx_desc - 1;
 	unsigned int i;
 
 	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
@@ -181,11 +184,14 @@ _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
 				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
 		}
 	} else {
-		for (i = rxq->rx_tail;
-		     i != rxq->rxrearm_start;
-		     i = (i + 1) & mask) {
+		i = rxq->rx_tail;
+		while (i != rxq->rxrearm_start) {
 			if (rxq->sw_ring[i].mbuf != NULL)
 				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
+
+			i = i + 1;
+			if (i > max_desc)
+				i = 0;
 		}
 	}
 
-- 
2.17.1


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

* Re: [dpdk-stable] [PATCH] net/ixgbe: fix RxQ/TxQ release
  2021-09-27 17:18 [dpdk-stable] [PATCH] net/ixgbe: fix RxQ/TxQ release Julien Meunier
@ 2021-09-28  3:06 ` Wang, Haiyue
  2021-09-28  3:21   ` Wang, Haiyue
  2021-09-28  8:12 ` [dpdk-stable] [PATCH v2] " Julien Meunier
  1 sibling, 1 reply; 7+ messages in thread
From: Wang, Haiyue @ 2021-09-28  3:06 UTC (permalink / raw)
  To: Meunier, Julien, dev; +Cc: stable, Richardson, Bruce

> -----Original Message-----
> From: Julien Meunier <julien.meunier@nokia.com>
> Sent: Tuesday, September 28, 2021 01:18
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: [PATCH] net/ixgbe: fix RxQ/TxQ release
> 
> On the vector implementation, during the tear-down, the mbufs not
> drained in the RxQ and TxQ are freed based on an algorithm which
> supposed that the number of descriptors is a power of 2 (max_desc).
> Based on this hypothesis, this algorithm uses a bitmask in order to
> detect an index overflow during the iteration, and to restart the loop
> from 0.
> 
> However, there is no such power of 2 requirement in the ixgbe for the
> number of descriptors in the RxQ / TxQ. The only requirement is to have
> a number correctly aligned.
> 
> If a user requested to configure a number of descriptors which is not a
> power of 2, as a consequence, during the tear-down, it was possible to
> be in an infinite loop, and to never reach the exit loop condition.
> 

Are you able to setup not a power of 2 successfully ?

int
rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
		       uint16_t nb_tx_desc, unsigned int socket_id,
		       const struct rte_eth_txconf *tx_conf)
{
	...

	if (nb_tx_desc > dev_info.tx_desc_lim.nb_max ||
	    nb_tx_desc < dev_info.tx_desc_lim.nb_min ||
	    nb_tx_desc % dev_info.tx_desc_lim.nb_align != 0) {
		RTE_ETHDEV_LOG(ERR,
			"Invalid value for nb_tx_desc(=%hu), should be: <= %hu, >= %hu, and a product of %hu\n",
			nb_tx_desc, dev_info.tx_desc_lim.nb_max,
			dev_info.tx_desc_lim.nb_min,
			dev_info.tx_desc_lim.nb_align);
		return -EINVAL;
	}

	...

}

> By removing the bitmask and changing the loop method, we can avoid this
> issue, and allow the user to configure a RxQ / TxQ which is not a power
> of 2.
> 
> Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")
> Cc: bruce.richardson@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Julien Meunier <julien.meunier@nokia.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> index adba855ca3..8912558918 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> @@ -150,11 +150,14 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
>  		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) {
> +	i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
> +	while (i != txq->tx_tail) {
>  		txe = &txq->sw_ring_v[i];
>  		rte_pktmbuf_free_seg(txe->mbuf);
> +
> +		i = i + 1;
> +		if (i > max_desc)
> +			i = 0;
>  	}
>  	txq->nb_tx_free = max_desc;
> 
> @@ -168,7 +171,7 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
>  static inline void
>  _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
>  {
> -	const unsigned int mask = rxq->nb_rx_desc - 1;
> +	const unsigned int max_desc = rxq->nb_rx_desc - 1;
>  	unsigned int i;
> 
>  	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
> @@ -181,11 +184,14 @@ _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
>  				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
>  		}
>  	} else {
> -		for (i = rxq->rx_tail;
> -		     i != rxq->rxrearm_start;
> -		     i = (i + 1) & mask) {
> +		i = rxq->rx_tail;
> +		while (i != rxq->rxrearm_start) {
>  			if (rxq->sw_ring[i].mbuf != NULL)
>  				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
> +
> +			i = i + 1;
> +			if (i > max_desc)
> +				i = 0;
>  		}
>  	}
> 
> --
> 2.17.1


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

* Re: [dpdk-stable] [PATCH] net/ixgbe: fix RxQ/TxQ release
  2021-09-28  3:06 ` Wang, Haiyue
@ 2021-09-28  3:21   ` Wang, Haiyue
  2021-09-28  7:46     ` Julien Meunier
  0 siblings, 1 reply; 7+ messages in thread
From: Wang, Haiyue @ 2021-09-28  3:21 UTC (permalink / raw)
  To: Meunier, Julien, dev; +Cc: stable, Richardson, Bruce

> -----Original Message-----
> From: Wang, Haiyue
> Sent: Tuesday, September 28, 2021 11:06
> To: 'Julien Meunier' <julien.meunier@nokia.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: RE: [PATCH] net/ixgbe: fix RxQ/TxQ release
> 
> > -----Original Message-----
> > From: Julien Meunier <julien.meunier@nokia.com>
> > Sent: Tuesday, September 28, 2021 01:18
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Wang, Haiyue
> > <haiyue.wang@intel.com>
> > Subject: [PATCH] net/ixgbe: fix RxQ/TxQ release
> >
> > On the vector implementation, during the tear-down, the mbufs not
> > drained in the RxQ and TxQ are freed based on an algorithm which
> > supposed that the number of descriptors is a power of 2 (max_desc).
> > Based on this hypothesis, this algorithm uses a bitmask in order to
> > detect an index overflow during the iteration, and to restart the loop
> > from 0.
> >
> > However, there is no such power of 2 requirement in the ixgbe for the
> > number of descriptors in the RxQ / TxQ. The only requirement is to have
> > a number correctly aligned.
> >
> > If a user requested to configure a number of descriptors which is not a
> > power of 2, as a consequence, during the tear-down, it was possible to
> > be in an infinite loop, and to never reach the exit loop condition.
> >
> 
> Are you able to setup not a power of 2 successfully ?
> 

My fault, yes, possible. ;-)

> int
> rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
> 		       uint16_t nb_tx_desc, unsigned int socket_id,
> 		       const struct rte_eth_txconf *tx_conf)
> {
> 	...
> 
> 	if (nb_tx_desc > dev_info.tx_desc_lim.nb_max ||
> 	    nb_tx_desc < dev_info.tx_desc_lim.nb_min ||
> 	    nb_tx_desc % dev_info.tx_desc_lim.nb_align != 0) {
> 		RTE_ETHDEV_LOG(ERR,
> 			"Invalid value for nb_tx_desc(=%hu), should be: <= %hu, >= %hu, and a product
> of %hu\n",
> 			nb_tx_desc, dev_info.tx_desc_lim.nb_max,
> 			dev_info.tx_desc_lim.nb_min,
> 			dev_info.tx_desc_lim.nb_align);
> 		return -EINVAL;
> 	}
> 
> 	...
> 
> }
> 
> > By removing the bitmask and changing the loop method, we can avoid this
> > issue, and allow the user to configure a RxQ / TxQ which is not a power
> > of 2.
> >
> > Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")
> > Cc: bruce.richardson@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Julien Meunier <julien.meunier@nokia.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> > index adba855ca3..8912558918 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> > @@ -150,11 +150,14 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
> >  		return;
> >

Just one line ?

i = (i + 1) % txq->nb_tx_desc

> >  	/* 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) {
> > +	i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
> > +	while (i != txq->tx_tail) {
> >  		txe = &txq->sw_ring_v[i];
> >  		rte_pktmbuf_free_seg(txe->mbuf);
> > +
> > +		i = i + 1;
> > +		if (i > max_desc)
> > +			i = 0;
> >  	}
> >  	txq->nb_tx_free = max_desc;
> >
> > @@ -168,7 +171,7 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
> >  static inline void
> >  _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
> >  {
> > -	const unsigned int mask = rxq->nb_rx_desc - 1;
> > +	const unsigned int max_desc = rxq->nb_rx_desc - 1;
> >  	unsigned int i;
> >
> >  	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
> > @@ -181,11 +184,14 @@ _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
> >  				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
> >  		}
> >  	} else {
> > -		for (i = rxq->rx_tail;
> > -		     i != rxq->rxrearm_start;
> > -		     i = (i + 1) & mask) {
> > +		i = rxq->rx_tail;
> > +		while (i != rxq->rxrearm_start) {
> >  			if (rxq->sw_ring[i].mbuf != NULL)
> >  				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
> > +
> > +			i = i + 1;
> > +			if (i > max_desc)
> > +				i = 0;
> >  		}
> >  	}
> >
> > --
> > 2.17.1


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

* Re: [dpdk-stable] [PATCH] net/ixgbe: fix RxQ/TxQ release
  2021-09-28  3:21   ` Wang, Haiyue
@ 2021-09-28  7:46     ` Julien Meunier
  0 siblings, 0 replies; 7+ messages in thread
From: Julien Meunier @ 2021-09-28  7:46 UTC (permalink / raw)
  To: Wang, Haiyue, dev; +Cc: stable, Richardson, Bruce

Hello,

On 28/09/2021 05:21, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Wang, Haiyue
>> Sent: Tuesday, September 28, 2021 11:06
>> To: 'Julien Meunier' <julien.meunier@nokia.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
>> Subject: RE: [PATCH] net/ixgbe: fix RxQ/TxQ release
>>
>>> -----Original Message-----
>>> From: Julien Meunier <julien.meunier@nokia.com>
>>> Sent: Tuesday, September 28, 2021 01:18
>>> To: dev@dpdk.org
>>> Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Wang, Haiyue
>>> <haiyue.wang@intel.com>
>>> Subject: [PATCH] net/ixgbe: fix RxQ/TxQ release
>>>
>>> On the vector implementation, during the tear-down, the mbufs not
>>> drained in the RxQ and TxQ are freed based on an algorithm which
>>> supposed that the number of descriptors is a power of 2 (max_desc).
>>> Based on this hypothesis, this algorithm uses a bitmask in order to
>>> detect an index overflow during the iteration, and to restart the loop
>>> from 0.
>>>
>>> However, there is no such power of 2 requirement in the ixgbe for the
>>> number of descriptors in the RxQ / TxQ. The only requirement is to have
>>> a number correctly aligned.
>>>
>>> If a user requested to configure a number of descriptors which is not a
>>> power of 2, as a consequence, during the tear-down, it was possible to
>>> be in an infinite loop, and to never reach the exit loop condition.
>>>
>>
>> Are you able to setup not a power of 2 successfully ?
>>
> 
> My fault, yes, possible. ;-)
> 

Yes, we have some usecases where the nb of descriptiors for the TxQ is 
set to 1536.
I modified the test_pmd_perf in order to validate this behavior, as my 
ixgbe X550 supports the loopback mode:
- nb_desc = 2048 => txq is drained and stopped correctly
- nb_desc = 1536 => freeze during the teardown

>> int
>> rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>> 		       uint16_t nb_tx_desc, unsigned int socket_id,
>> 		       const struct rte_eth_txconf *tx_conf)
>> {
>> 	...
>>
>> 	if (nb_tx_desc > dev_info.tx_desc_lim.nb_max ||
>> 	    nb_tx_desc < dev_info.tx_desc_lim.nb_min ||
>> 	    nb_tx_desc % dev_info.tx_desc_lim.nb_align != 0) {
>> 		RTE_ETHDEV_LOG(ERR,
>> 			"Invalid value for nb_tx_desc(=%hu), should be: <= %hu, >= %hu, and a product
>> of %hu\n",
>> 			nb_tx_desc, dev_info.tx_desc_lim.nb_max,
>> 			dev_info.tx_desc_lim.nb_min,
>> 			dev_info.tx_desc_lim.nb_align);
>> 		return -EINVAL;
>> 	}
>>
>> 	...
>>
>> }
>>
>>> By removing the bitmask and changing the loop method, we can avoid this
>>> issue, and allow the user to configure a RxQ / TxQ which is not a power
>>> of 2.
>>>
>>> Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")
>>> Cc: bruce.richardson@intel.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Julien Meunier <julien.meunier@nokia.com>
>>> ---
>>>   drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 20 +++++++++++++-------
>>>   1 file changed, 13 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
>>> index adba855ca3..8912558918 100644
>>> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
>>> @@ -150,11 +150,14 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
>>>   		return;
>>>
> 
> Just one line ?
> 
> i = (i + 1) % txq->nb_tx_desc
> 

Ah yes, I was too focused with this bitmask...
The shorter, the better.

I will send a V2 today.

Thanks for this feedback !

>>>   	/* 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) {
>>> +	i = txq->tx_next_dd - (txq->tx_rs_thresh - 1);
>>> +	while (i != txq->tx_tail) {
>>>   		txe = &txq->sw_ring_v[i];
>>>   		rte_pktmbuf_free_seg(txe->mbuf);
>>> +
>>> +		i = i + 1;
>>> +		if (i > max_desc)
>>> +			i = 0;
>>>   	}
>>>   	txq->nb_tx_free = max_desc;
>>>
>>> @@ -168,7 +171,7 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
>>>   static inline void
>>>   _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
>>>   {
>>> -	const unsigned int mask = rxq->nb_rx_desc - 1;
>>> +	const unsigned int max_desc = rxq->nb_rx_desc - 1;
>>>   	unsigned int i;
>>>
>>>   	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
>>> @@ -181,11 +184,14 @@ _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
>>>   				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
>>>   		}
>>>   	} else {
>>> -		for (i = rxq->rx_tail;
>>> -		     i != rxq->rxrearm_start;
>>> -		     i = (i + 1) & mask) {
>>> +		i = rxq->rx_tail;
>>> +		while (i != rxq->rxrearm_start) {
>>>   			if (rxq->sw_ring[i].mbuf != NULL)
>>>   				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
>>> +
>>> +			i = i + 1;
>>> +			if (i > max_desc)
>>> +				i = 0;
>>>   		}
>>>   	}
>>>
>>> --
>>> 2.17.1
> 

-- 
Julien Meunier

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

* [dpdk-stable] [PATCH v2] net/ixgbe: fix RxQ/TxQ release
  2021-09-27 17:18 [dpdk-stable] [PATCH] net/ixgbe: fix RxQ/TxQ release Julien Meunier
  2021-09-28  3:06 ` Wang, Haiyue
@ 2021-09-28  8:12 ` Julien Meunier
  2021-09-28 12:18   ` Wang, Haiyue
  1 sibling, 1 reply; 7+ messages in thread
From: Julien Meunier @ 2021-09-28  8:12 UTC (permalink / raw)
  To: dev; +Cc: stable, bruce.richardson, Haiyue Wang

On the vector implementation, during the tear-down, the mbufs not
drained in the RxQ and TxQ are freed based on an algorithm which
supposed that the number of descriptors is a power of 2 (max_desc).
Based on this hypothesis, this algorithm uses a bitmask in order to
detect an index overflow during the iteration, and to restart the loop
from 0.

However, there is no such power of 2 requirement in the ixgbe for the
number of descriptors in the RxQ / TxQ. The only requirement is to have
a number correctly aligned.

If a user requested to configure a number of descriptors which is not a
power of 2, as a consequence, during the tear-down, it was possible to
be in an infinite loop, and to never reach the exit loop condition.

By removing the bitmask and changing the loop method, we can avoid this
issue, and allow the user to configure a RxQ / TxQ which is not a power
of 2.

Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Julien Meunier <julien.meunier@nokia.com>
---
 drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
index adba855ca3..005e60668a 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
@@ -152,7 +152,7 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 	/* 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) {
+	     i = (i + 1) % txq->nb_tx_desc) {
 		txe = &txq->sw_ring_v[i];
 		rte_pktmbuf_free_seg(txe->mbuf);
 	}
@@ -168,7 +168,6 @@ _ixgbe_tx_queue_release_mbufs_vec(struct ixgbe_tx_queue *txq)
 static inline void
 _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
 {
-	const unsigned int mask = rxq->nb_rx_desc - 1;
 	unsigned int i;

 	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
@@ -183,7 +182,7 @@ _ixgbe_rx_queue_release_mbufs_vec(struct ixgbe_rx_queue *rxq)
 	} else {
 		for (i = rxq->rx_tail;
 		     i != rxq->rxrearm_start;
-		     i = (i + 1) & mask) {
+		     i = (i + 1) % rxq->nb_rx_desc) {
 			if (rxq->sw_ring[i].mbuf != NULL)
 				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
 		}
--
2.17.1


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

* Re: [dpdk-stable] [PATCH v2] net/ixgbe: fix RxQ/TxQ release
  2021-09-28  8:12 ` [dpdk-stable] [PATCH v2] " Julien Meunier
@ 2021-09-28 12:18   ` Wang, Haiyue
  2021-10-09  4:43     ` Zhang, Qi Z
  0 siblings, 1 reply; 7+ messages in thread
From: Wang, Haiyue @ 2021-09-28 12:18 UTC (permalink / raw)
  To: Meunier, Julien, dev; +Cc: stable, Richardson, Bruce

> -----Original Message-----
> From: Julien Meunier <julien.meunier@nokia.com>
> Sent: Tuesday, September 28, 2021 16:13
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: [PATCH v2] net/ixgbe: fix RxQ/TxQ release
> 
> On the vector implementation, during the tear-down, the mbufs not
> drained in the RxQ and TxQ are freed based on an algorithm which
> supposed that the number of descriptors is a power of 2 (max_desc).
> Based on this hypothesis, this algorithm uses a bitmask in order to
> detect an index overflow during the iteration, and to restart the loop
> from 0.
> 
> However, there is no such power of 2 requirement in the ixgbe for the
> number of descriptors in the RxQ / TxQ. The only requirement is to have
> a number correctly aligned.
> 
> If a user requested to configure a number of descriptors which is not a
> power of 2, as a consequence, during the tear-down, it was possible to
> be in an infinite loop, and to never reach the exit loop condition.
> 
> By removing the bitmask and changing the loop method, we can avoid this
> issue, and allow the user to configure a RxQ / TxQ which is not a power
> of 2.
> 
> Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")
> Cc: bruce.richardson@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Julien Meunier <julien.meunier@nokia.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 

Acked-by: Haiyue Wang <haiyue.wang@intel.com>

> --
> 2.17.1


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

* Re: [dpdk-stable] [PATCH v2] net/ixgbe: fix RxQ/TxQ release
  2021-09-28 12:18   ` Wang, Haiyue
@ 2021-10-09  4:43     ` Zhang, Qi Z
  0 siblings, 0 replies; 7+ messages in thread
From: Zhang, Qi Z @ 2021-10-09  4:43 UTC (permalink / raw)
  To: Wang, Haiyue, Meunier, Julien, dev; +Cc: stable, Richardson, Bruce



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wang, Haiyue
> Sent: Tuesday, September 28, 2021 8:19 PM
> To: Meunier, Julien <julien.meunier@nokia.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix RxQ/TxQ release
> 
> > -----Original Message-----
> > From: Julien Meunier <julien.meunier@nokia.com>
> > Sent: Tuesday, September 28, 2021 16:13
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> > Wang, Haiyue <haiyue.wang@intel.com>
> > Subject: [PATCH v2] net/ixgbe: fix RxQ/TxQ release
> >
> > On the vector implementation, during the tear-down, the mbufs not
> > drained in the RxQ and TxQ are freed based on an algorithm which
> > supposed that the number of descriptors is a power of 2 (max_desc).
> > Based on this hypothesis, this algorithm uses a bitmask in order to
> > detect an index overflow during the iteration, and to restart the loop
> > from 0.
> >
> > However, there is no such power of 2 requirement in the ixgbe for the
> > number of descriptors in the RxQ / TxQ. The only requirement is to
> > have a number correctly aligned.
> >
> > If a user requested to configure a number of descriptors which is not
> > a power of 2, as a consequence, during the tear-down, it was possible
> > to be in an infinite loop, and to never reach the exit loop condition.
> >
> > By removing the bitmask and changing the loop method, we can avoid
> > this issue, and allow the user to configure a RxQ / TxQ which is not a
> > power of 2.
> >
> > Fixes: c95584dc2b18 ("ixgbe: new vectorized functions for Rx/Tx")
> > Cc: bruce.richardson@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Julien Meunier <julien.meunier@nokia.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> 
> Acked-by: Haiyue Wang <haiyue.wang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
> 
> > --
> > 2.17.1


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

end of thread, other threads:[~2021-10-09  4:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-27 17:18 [dpdk-stable] [PATCH] net/ixgbe: fix RxQ/TxQ release Julien Meunier
2021-09-28  3:06 ` Wang, Haiyue
2021-09-28  3:21   ` Wang, Haiyue
2021-09-28  7:46     ` Julien Meunier
2021-09-28  8:12 ` [dpdk-stable] [PATCH v2] " Julien Meunier
2021-09-28 12:18   ` Wang, Haiyue
2021-10-09  4:43     ` Zhang, Qi Z

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

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

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

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


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