net/i40e: fix Rx instability with vector mode

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

Checks

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

Commit Message

Xing, Beilei Oct. 25, 2018, 7:40 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>
---
 doc/guides/nics/i40e.rst     |  7 +++++++
 drivers/net/i40e/i40e_rxtx.c | 18 +++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)
  

Comments

Qi Zhang Oct. 25, 2018, 4:47 p.m. UTC | #1
> -----Original Message-----
> From: Xing, Beilei
> Sent: Thursday, October 25, 2018 2:41 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH] net/i40e: fix Rx instability with vector mode
> 
> 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>
> ---
>  doc/guides/nics/i40e.rst     |  7 +++++++
>  drivers/net/i40e/i40e_rxtx.c | 18 +++++++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> 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..aa46d83 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1735,10 +1735,17 @@ 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.
> +		 */
> +		if ((rxq->nb_rx_desc & (rxq->nb_rx_desc - 1)))
> +			ad->rx_vec_allowed = false;
> +		else
> +			ad->rx_vec_allowed = true;
>  		i40e_set_rx_function(dev);
>  		return 0;
>  	}
> @@ -1811,6 +1818,15 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev
> *dev,
>  		return -EINVAL;
>  	}
> 
> +	/**
> +	 * Vector mode is allowed only when number of Rx queue
> +	 * descriptor is a power of 2.
> +	 */
> +	if ((nb_desc & (nb_desc - 1)))
> +		ad->rx_vec_allowed = false;
> +	else
> +		ad->rx_vec_allowed = true;

rx_vec_allowed will be overwritten unexpectedly for multi-queues case

We may only to overwrite with below condition check.

If (first_queue || ad->rx_vec_allowed)
	ad->rx_vec_allowed = ! (nb_desc & (nb_desc - 1));

> +
>  	/* Free memory if needed */
>  	if (dev->data->rx_queues[queue_idx]) {
>  		i40e_dev_rx_queue_release(dev->data->rx_queues[queue_idx]);
> --
> 2.5.5
  

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..aa46d83 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1735,10 +1735,17 @@  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.
+		 */
+		if ((rxq->nb_rx_desc & (rxq->nb_rx_desc - 1)))
+			ad->rx_vec_allowed = false;
+		else
+			ad->rx_vec_allowed = true;
 		i40e_set_rx_function(dev);
 		return 0;
 	}
@@ -1811,6 +1818,15 @@  i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
+	/**
+	 * Vector mode is allowed only when number of Rx queue
+	 * descriptor is a power of 2.
+	 */
+	if ((nb_desc & (nb_desc - 1)))
+		ad->rx_vec_allowed = false;
+	else
+		ad->rx_vec_allowed = true;
+
 	/* Free memory if needed */
 	if (dev->data->rx_queues[queue_idx]) {
 		i40e_dev_rx_queue_release(dev->data->rx_queues[queue_idx]);