diff mbox series

[v3] net/i40e: fix Rx instability with vector mode

Message ID 1541051597-28949-1-git-send-email-beilei.xing@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series [v3] net/i40e: fix Rx instability with vector mode |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Xing, Beilei Nov. 1, 2018, 5:53 a.m. UTC 
  Previously, there is instability during vector Rx if descriptor
number is not power of 2, e.g. process hang and some Rx packets
are unexpectedly empty. That's because vector Rx mode assumes Rx
descriptor number is power of 2 when doing bit mask.
This patch allows vector mode only when the number of Rx descriptor
is power of 2.

Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
Fixes: a3c83a2527e1 ("net/i40e: enable runtime queue setup")
Cc: stable@dpdk.org

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
v3 changes:
 - Add branch for non-first queue during runtime queue setup.
 - Use function rte_is_power_of_2().
 - Configure rx_vec_allowed during setting Rx function.
v2 changes:
 - rx_vec_allowed is global configuration, avoid overwrite.
 
 doc/guides/nics/i40e.rst     |  7 +++++++
 drivers/net/i40e/i40e_rxtx.c | 30 +++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 3 deletions(-)

Comments

Ananyev, Konstantin Nov. 1, 2018, 11:48 a.m. UTC | #1 
Hi

> 
> Previously, there is instability during vector Rx if descriptor
> number is not power of 2, e.g. process hang and some Rx packets
> are unexpectedly empty. That's because vector Rx mode assumes Rx
> descriptor number is power of 2 when doing bit mask.
> This patch allows vector mode only when the number of Rx descriptor
> is power of 2.
> 
> Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
> Fixes: a3c83a2527e1 ("net/i40e: enable runtime queue setup")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v3 changes:
>  - Add branch for non-first queue during runtime queue setup.
>  - Use function rte_is_power_of_2().
>  - Configure rx_vec_allowed during setting Rx function.
> v2 changes:
>  - rx_vec_allowed is global configuration, avoid overwrite.
> 
>  doc/guides/nics/i40e.rst     |  7 +++++++
>  drivers/net/i40e/i40e_rxtx.c | 30 +++++++++++++++++++++++++++---
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> index ab3928a..bfacbd1 100644
> --- a/doc/guides/nics/i40e.rst
> +++ b/doc/guides/nics/i40e.rst
> @@ -172,6 +172,13 @@ Runtime Config Options
> 
>    -w 84:00.0,use-latest-supported-vec=1
> 
> +Vector RX Pre-conditions
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +For Vector RX it is assumed that the number of descriptor rings will be a power
> +of 2. With this pre-condition, the ring pointer can easily scroll back to the
> +head after hitting the tail without a conditional check. In addition Vector RX
> +can use this assumption to do a bit mask using ``ring_size - 1``.
> +
>  Driver compilation and testing
>  ------------------------------
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index a827456..771193a 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1735,12 +1735,21 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
>  		 * i40e_set_rx_function.
>  		 */
>  		ad->rx_bulk_alloc_allowed = true;
> -		ad->rx_vec_allowed = true;
>  		dev->data->scattered_rx = use_scattered_rx;
>  		if (use_def_burst_func)
>  			ad->rx_bulk_alloc_allowed = false;
> +		/**
> +		 * Vector mode is allowed only when number of Rx queue
> +		 * descriptor is a power of 2.
> +		 */
> +		ad->rx_vec_allowed = rte_is_power_of_2(rxq->nb_rx_desc);

Actually do we need to do it here?
We call set_rx_function() anyway, it would do that check for us, wouldn't it?

>  		i40e_set_rx_function(dev);
>  		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"
> +			    " number %d of queue %d isn't power of 2",
> +			    rxq->nb_rx_desc, rxq->queue_id);
> +		return -EINVAL;
>  	}
> 
>  	/* check bulk alloc conflict */
> @@ -2948,11 +2957,26 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
>  {
>  	struct i40e_adapter *ad =
>  		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	struct i40e_rx_queue *rxq;
>  	uint16_t rx_using_sse, i;
> +	uint16_t desc;

Please add empty line between var definitions and start of code.
Helps readability.

>  	/* In order to allow Vector Rx there are a few configuration
>  	 * conditions to be met and Rx Bulk Allocation should be allowed.
>  	 */
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		if (!dev->data->dev_started) {
> +			for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +				rxq = dev->data->rx_queues[i];
> +				desc = rxq->nb_rx_desc;
> +				if (!i)
> +					ad->rx_vec_allowed =
> +						rte_is_power_of_2(desc);
> +				else if (ad->rx_vec_allowed &&
> +					 !rte_is_power_of_2(desc))
> +					ad->rx_vec_allowed = false;

Wouldn't be a bit cleaner:
ad->rx_vec_allowed = (ad->rx_vec_allowed == true) ? rte_is_power_of_2(desc)) : ad->rx_vec_allowed;

> +			}
> +		}
> +

Probably better to move that code into i40e_rx_vec_dev_conf_condition_check()
that will be called on next line?

>  		if (i40e_rx_vec_dev_conf_condition_check(dev) ||
>  		    !ad->rx_bulk_alloc_allowed) {
>  			PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet"
> @@ -2961,10 +2985,10 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
> 
>  			ad->rx_vec_allowed = false;
>  		}
> +
>  		if (ad->rx_vec_allowed) {
>  			for (i = 0; i < dev->data->nb_rx_queues; i++) {
> -				struct i40e_rx_queue *rxq =
> -					dev->data->rx_queues[i];
> +				rxq = dev->data->rx_queues[i];
> 
>  				if (rxq && i40e_rxq_vec_setup(rxq)) {
>  					ad->rx_vec_allowed = false;
> --
> 2.5.5
Qi Zhang Nov. 1, 2018, 1:13 p.m. UTC | #2 
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, November 1, 2018 6:48 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3] net/i40e: fix Rx instability with vector
> mode
> 
> 
> Hi
> 
> >
> > Previously, there is instability during vector Rx if descriptor number
> > is not power of 2, e.g. process hang and some Rx packets are
> > unexpectedly empty. That's because vector Rx mode assumes Rx
> > descriptor number is power of 2 when doing bit mask.
> > This patch allows vector mode only when the number of Rx descriptor is
> > power of 2.
> >
> > Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
> > Fixes: a3c83a2527e1 ("net/i40e: enable runtime queue setup")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> > v3 changes:
> >  - Add branch for non-first queue during runtime queue setup.
> >  - Use function rte_is_power_of_2().
> >  - Configure rx_vec_allowed during setting Rx function.
> > v2 changes:
> >  - rx_vec_allowed is global configuration, avoid overwrite.
> >
> >  doc/guides/nics/i40e.rst     |  7 +++++++
> >  drivers/net/i40e/i40e_rxtx.c | 30 +++++++++++++++++++++++++++---
> >  2 files changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst index
> > ab3928a..bfacbd1 100644
> > --- a/doc/guides/nics/i40e.rst
> > +++ b/doc/guides/nics/i40e.rst
> > @@ -172,6 +172,13 @@ Runtime Config Options
> >
> >    -w 84:00.0,use-latest-supported-vec=1
> >
> > +Vector RX Pre-conditions
> > +~~~~~~~~~~~~~~~~~~~~~~~~
> > +For Vector RX it is assumed that the number of descriptor rings will
> > +be a power of 2. With this pre-condition, the ring pointer can easily
> > +scroll back to the head after hitting the tail without a conditional
> > +check. In addition Vector RX can use this assumption to do a bit mask
> using ``ring_size - 1``.
> > +
> >  Driver compilation and testing
> >  ------------------------------
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > b/drivers/net/i40e/i40e_rxtx.c index a827456..771193a 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1735,12 +1735,21 @@ i40e_dev_rx_queue_setup_runtime(struct
> rte_eth_dev *dev,
> >  		 * i40e_set_rx_function.
> >  		 */
> >  		ad->rx_bulk_alloc_allowed = true;
> > -		ad->rx_vec_allowed = true;
> >  		dev->data->scattered_rx = use_scattered_rx;
> >  		if (use_def_burst_func)
> >  			ad->rx_bulk_alloc_allowed = false;
> > +		/**
> > +		 * Vector mode is allowed only when number of Rx queue
> > +		 * descriptor is a power of 2.
> > +		 */
> > +		ad->rx_vec_allowed = rte_is_power_of_2(rxq->nb_rx_desc);
> 
> Actually do we need to do it here?
> We call set_rx_function() anyway, it would do that check for us, wouldn't it?
> 
> >  		i40e_set_rx_function(dev);
> >  		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"
> > +			    " number %d of queue %d isn't power of 2",
> > +			    rxq->nb_rx_desc, rxq->queue_id);
> > +		return -EINVAL;
> >  	}
> >
> >  	/* check bulk alloc conflict */
> > @@ -2948,11 +2957,26 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
> > {
> >  	struct i40e_adapter *ad =
> >  		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > +	struct i40e_rx_queue *rxq;
> >  	uint16_t rx_using_sse, i;
> > +	uint16_t desc;
> 
> Please add empty line between var definitions and start of code.
> Helps readability.
> 
> >  	/* In order to allow Vector Rx there are a few configuration
> >  	 * conditions to be met and Rx Bulk Allocation should be allowed.
> >  	 */
> >  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +		if (!dev->data->dev_started) {
> > +			for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +				rxq = dev->data->rx_queues[i];
> > +				desc = rxq->nb_rx_desc;
> > +				if (!i)
queue_setup is not necessary start from queue 0, we can setup queue at any order, so we can't assume i=0 is the first queue to setup.
And also we need to check if (dev->data->rx_queues[i] == NULL) to avoid segment fault, since some queue may never be setup yet.

> > +					ad->rx_vec_allowed =
> > +						rte_is_power_of_2(desc);
> > +				else if (ad->rx_vec_allowed &&
> > +					 !rte_is_power_of_2(desc))
> > +					ad->rx_vec_allowed = false;
> 
> Wouldn't be a bit cleaner:
> ad->rx_vec_allowed = (ad->rx_vec_allowed == true) ?
> ad->rte_is_power_of_2(desc)) : ad->rx_vec_allowed;


> 
> > +			}
> > +		}
> > +
> 
> Probably better to move that code into
> i40e_rx_vec_dev_conf_condition_check()
> that will be called on next line?
> 
> >  		if (i40e_rx_vec_dev_conf_condition_check(dev) ||
> >  		    !ad->rx_bulk_alloc_allowed) {
> >  			PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet"
> > @@ -2961,10 +2985,10 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
> >
> >  			ad->rx_vec_allowed = false;
> >  		}
> > +
> >  		if (ad->rx_vec_allowed) {
> >  			for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > -				struct i40e_rx_queue *rxq =
> > -					dev->data->rx_queues[i];
> > +				rxq = dev->data->rx_queues[i];
> >
> >  				if (rxq && i40e_rxq_vec_setup(rxq)) {
> >  					ad->rx_vec_allowed = false;
> > --
> > 2.5.5
diff mbox series

Patch

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index ab3928a..bfacbd1 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -172,6 +172,13 @@  Runtime Config Options
 
   -w 84:00.0,use-latest-supported-vec=1
 
+Vector RX Pre-conditions
+~~~~~~~~~~~~~~~~~~~~~~~~
+For Vector RX it is assumed that the number of descriptor rings will be a power
+of 2. With this pre-condition, the ring pointer can easily scroll back to the
+head after hitting the tail without a conditional check. In addition Vector RX
+can use this assumption to do a bit mask using ``ring_size - 1``.
+
 Driver compilation and testing
 ------------------------------
 
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index a827456..771193a 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1735,12 +1735,21 @@  i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
 		 * i40e_set_rx_function.
 		 */
 		ad->rx_bulk_alloc_allowed = true;
-		ad->rx_vec_allowed = true;
 		dev->data->scattered_rx = use_scattered_rx;
 		if (use_def_burst_func)
 			ad->rx_bulk_alloc_allowed = false;
+		/**
+		 * Vector mode is allowed only when number of Rx queue
+		 * descriptor is a power of 2.
+		 */
+		ad->rx_vec_allowed = rte_is_power_of_2(rxq->nb_rx_desc);
 		i40e_set_rx_function(dev);
 		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"
+			    " number %d of queue %d isn't power of 2",
+			    rxq->nb_rx_desc, rxq->queue_id);
+		return -EINVAL;
 	}
 
 	/* check bulk alloc conflict */
@@ -2948,11 +2957,26 @@  i40e_set_rx_function(struct rte_eth_dev *dev)
 {
 	struct i40e_adapter *ad =
 		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct i40e_rx_queue *rxq;
 	uint16_t rx_using_sse, i;
+	uint16_t desc;
 	/* In order to allow Vector Rx there are a few configuration
 	 * conditions to be met and Rx Bulk Allocation should be allowed.
 	 */
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		if (!dev->data->dev_started) {
+			for (i = 0; i < dev->data->nb_rx_queues; i++) {
+				rxq = dev->data->rx_queues[i];
+				desc = rxq->nb_rx_desc;
+				if (!i)
+					ad->rx_vec_allowed =
+						rte_is_power_of_2(desc);
+				else if (ad->rx_vec_allowed &&
+					 !rte_is_power_of_2(desc))
+					ad->rx_vec_allowed = false;
+			}
+		}
+
 		if (i40e_rx_vec_dev_conf_condition_check(dev) ||
 		    !ad->rx_bulk_alloc_allowed) {
 			PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet"
@@ -2961,10 +2985,10 @@  i40e_set_rx_function(struct rte_eth_dev *dev)
 
 			ad->rx_vec_allowed = false;
 		}
+
 		if (ad->rx_vec_allowed) {
 			for (i = 0; i < dev->data->nb_rx_queues; i++) {
-				struct i40e_rx_queue *rxq =
-					dev->data->rx_queues[i];
+				rxq = dev->data->rx_queues[i];
 
 				if (rxq && i40e_rxq_vec_setup(rxq)) {
 					ad->rx_vec_allowed = false;