[dpdk-dev] [PATCH] net/i40e: fix Rx instability with vector mode

[dpdk-dev] [PATCH] net/i40e: fix Rx instability with vector mode

[Beilei Xing]

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;
+
 	/* 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

Re: [dpdk-dev] [PATCH] net/i40e: fix Rx instability with vector mode

[Zhang, Qi Z]

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

[dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector mode

[Beilei Xing]

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

v2 changes:
 - rx_vec_allowed is global configuration, avoid overwrite.

 doc/guides/nics/i40e.rst     |  7 +++++++
 drivers/net/i40e/i40e_rxtx.c | 13 ++++++++++++-
 2 files changed, 19 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..f6dcca0 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1735,10 +1735,14 @@ 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 = !(rxq->nb_rx_desc & (rxq->nb_rx_desc - 1));
 		i40e_set_rx_function(dev);
 		return 0;
 	}
@@ -1811,6 +1815,13 @@ 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 (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

Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector mode

[Ananyev, Konstantin]

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>
> ---
> 
> v2 changes:
>  - rx_vec_allowed is global configuration, avoid overwrite.
> 
>  doc/guides/nics/i40e.rst     |  7 +++++++
>  drivers/net/i40e/i40e_rxtx.c | 13 ++++++++++++-
>  2 files changed, 19 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..f6dcca0 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1735,10 +1735,14 @@ 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 = !(rxq->nb_rx_desc & (rxq->nb_rx_desc - 1));
>  		i40e_set_rx_function(dev);
>  		return 0;
>  	}

I think this is not complete - if it is not  the first function, we have to do:
If (if (ad->rx_vec_allowed && (rxq->nb_rx_desc & (rxq->nb_rx_desc - 1) != 0) {
      PMD_DRV_LOG(ERR, ...);
       return -EINVAL;
}

BTW, here and below why not to use rte_is_power_of_2()? 
Konstantin

> @@ -1811,6 +1815,13 @@ 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 (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]);

Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector mode

[Zhang, Qi Z]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, October 26, 2018 3:47 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 v2] 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>
> > ---
> >
> > v2 changes:
> >  - rx_vec_allowed is global configuration, avoid overwrite.
> >
> >  doc/guides/nics/i40e.rst     |  7 +++++++
> >  drivers/net/i40e/i40e_rxtx.c | 13 ++++++++++++-
> >  2 files changed, 19 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..f6dcca0 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1735,10 +1735,14 @@ 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 = !(rxq->nb_rx_desc & (rxq->nb_rx_desc - 1));
> >  		i40e_set_rx_function(dev);
> >  		return 0;
> >  	}
> 
> I think this is not complete - if it is not  the first function, we have to do:
> If (if (ad->rx_vec_allowed && (rxq->nb_rx_desc & (rxq->nb_rx_desc - 1) != 0)
> {
>       PMD_DRV_LOG(ERR, ...);
>        return -EINVAL;
> }

Yes, we should prevent downgrade if the device is already running.

> 
> BTW, here and below why not to use rte_is_power_of_2()?
> Konstantin
> 
> > @@ -1811,6 +1815,13 @@ 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 (ad->rx_vec_allowed)
> > +		ad->rx_vec_allowed = !(nb_desc & (nb_desc - 1));

Looks like we are not able to get back to vec mode once some queue is set to no power of 2, even if all queues has been re-setup to power of 2 again.
Seems we still need to check all other queue's configure here.

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

Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector mode

[Xing, Beilei]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Friday, October 26, 2018 11:42 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector
> mode
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Friday, October 26, 2018 3:47 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 v2] 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>
> > > ---
> > >
> > > v2 changes:
> > >  - rx_vec_allowed is global configuration, avoid overwrite.
> > >
> > >  doc/guides/nics/i40e.rst     |  7 +++++++
> > >  drivers/net/i40e/i40e_rxtx.c | 13 ++++++++++++-
> > >  2 files changed, 19 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..f6dcca0 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -1735,10 +1735,14 @@ 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 = !(rxq->nb_rx_desc & (rxq->nb_rx_desc -
> 1));
> > >  		i40e_set_rx_function(dev);
> > >  		return 0;
> > >  	}
> >
> > I think this is not complete - if it is not  the first function, we have to do:
> > If (if (ad->rx_vec_allowed && (rxq->nb_rx_desc & (rxq->nb_rx_desc - 1)
> > != 0) {
> >       PMD_DRV_LOG(ERR, ...);
> >        return -EINVAL;
> > }
> 
> Yes, we should prevent downgrade if the device is already running.

OK, will update in next version.

> 
> >
> > BTW, here and below why not to use rte_is_power_of_2()?

Thanks for the reminder, will use the inline function in next version.


> > Konstantin
> >
> > > @@ -1811,6 +1815,13 @@ 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 (ad->rx_vec_allowed)
> > > +		ad->rx_vec_allowed = !(nb_desc & (nb_desc - 1));
> 
> Looks like we are not able to get back to vec mode once some queue is set
> to no power of 2, even if all queues has been re-setup to power of 2 again.
> Seems we still need to check all other queue's configure here.

We can allow vector again when runtime configuring the first queue whose descriptor number is power of 2, right?

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

Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector mode

[Bruce Richardson]

On Fri, Oct 26, 2018 at 02:33:27PM +0800, Beilei Xing wrote:
> 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>
> ---
> 
Do we not limit our descriptor ring sizes to powers of two anyway? Is there
a real need for non-power-of-2 ring sizes?

Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector mode

[Zhang, Qi Z]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Friday, October 26, 2018 6:12 AM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx instability with vector
> mode
> 
> On Fri, Oct 26, 2018 at 02:33:27PM +0800, Beilei Xing wrote:
> > 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>
> > ---
> >
> Do we not limit our descriptor ring sizes to powers of two anyway? 
Is there a
> real need for non-power-of-2 ring sizes?

This is tracked by some Bugzilla, seems someone is using this kind of configure, 
and I guess a more flexible queue size configure may help on the platform that have limited cache?

[dpdk-dev] [PATCH v3] net/i40e: fix Rx instability with vector mode

[Beilei Xing]

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);
 		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;
-- 
2.5.5

Re: [dpdk-dev] [PATCH v3] net/i40e: fix Rx instability with vector mode

[Ananyev, Konstantin]

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

Re: [dpdk-dev] [PATCH v3] net/i40e: fix Rx instability with vector mode

[Zhang, Qi Z]

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

[dpdk-dev] [PATCH v4] net/i40e: fix Rx instability with vector mode

[Beilei Xing]

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>
---
v4 changes:
 - Configure rx_vec_allowed in i40e_rx_vec_dev_conf_condition_check function.
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            |  5 +++++
 drivers/net/i40e/i40e_rxtx_vec_common.h | 38 +++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+)

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..5bb1a3c 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1741,6 +1741,11 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
 			ad->rx_bulk_alloc_allowed = false;
 		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 */
diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index f00f6d6..0e6ffa0 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -192,8 +192,13 @@ static inline int
 i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
 {
 #ifndef RTE_LIBRTE_IEEE1588
+	struct i40e_adapter *ad =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
+	struct i40e_rx_queue *rxq;
+	uint16_t desc, i;
+	bool first_queue;
 
 	/* no fdir support */
 	if (fconf->mode != RTE_FDIR_MODE_NONE)
@@ -207,6 +212,39 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
 	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
 		return -1;
 
+	/**
+	 * Vector mode is allowed only when number of Rx queue
+	 * descriptor is power of 2.
+	 */
+	if (!dev->data->dev_started) {
+		first_queue = true;
+		for (i = 0; i < dev->data->nb_rx_queues; i++) {
+			rxq = dev->data->rx_queues[i];
+			if (!rxq)
+				continue;
+			desc = rxq->nb_rx_desc;
+			if (first_queue)
+				ad->rx_vec_allowed =
+					rte_is_power_of_2(desc);
+			else
+				ad->rx_vec_allowed =
+					ad->rx_vec_allowed ?
+					rte_is_power_of_2(desc) :
+					ad->rx_vec_allowed;
+			first_queue = false;
+		}
+	} else {
+		/* Only check the first queue's descriptor number */
+		for (i = 0; i < dev->data->nb_rx_queues; i++) {
+			rxq = dev->data->rx_queues[i];
+			if (!rxq)
+				continue;
+			desc = rxq->nb_rx_desc;
+			ad->rx_vec_allowed = rte_is_power_of_2(desc);
+			break;
+		}
+	}
+
 	return 0;
 #else
 	RTE_SET_USED(dev);
-- 
2.5.5

Re: [dpdk-dev] [PATCH v4] net/i40e: fix Rx instability with vector mode

[Ananyev, Konstantin]

> 
> 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>
> ---
> v4 changes:
>  - Configure rx_vec_allowed in i40e_rx_vec_dev_conf_condition_check function.
> 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            |  5 +++++
>  drivers/net/i40e/i40e_rxtx_vec_common.h | 38 +++++++++++++++++++++++++++++++++
>  3 files changed, 50 insertions(+)
> 
> 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..5bb1a3c 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1741,6 +1741,11 @@ i40e_dev_rx_queue_setup_runtime(struct rte_eth_dev *dev,
>  			ad->rx_bulk_alloc_allowed = false;
>  		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 */
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index f00f6d6..0e6ffa0 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -192,8 +192,13 @@ static inline int
>  i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>  {
>  #ifndef RTE_LIBRTE_IEEE1588
> +	struct i40e_adapter *ad =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>  	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>  	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
> +	struct i40e_rx_queue *rxq;
> +	uint16_t desc, i;
> +	bool first_queue;
> 
>  	/* no fdir support */
>  	if (fconf->mode != RTE_FDIR_MODE_NONE)
> @@ -207,6 +212,39 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
>  		return -1;
> 
> +	/**
> +	 * Vector mode is allowed only when number of Rx queue
> +	 * descriptor is power of 2.
> +	 */
> +	if (!dev->data->dev_started) {
> +		first_queue = true;
> +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +			rxq = dev->data->rx_queues[i];
> +			if (!rxq)
> +				continue;
> +			desc = rxq->nb_rx_desc;
> +			if (first_queue)
> +				ad->rx_vec_allowed =
> +					rte_is_power_of_2(desc);
> +			else
> +				ad->rx_vec_allowed =
> +					ad->rx_vec_allowed ?
> +					rte_is_power_of_2(desc) :
> +					ad->rx_vec_allowed;
> +			first_queue = false;
> +		}
> +	} else {
> +		/* Only check the first queue's descriptor number */
> +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +			rxq = dev->data->rx_queues[i];
> +			if (!rxq)
> +				continue;
> +			desc = rxq->nb_rx_desc;
> +			ad->rx_vec_allowed = rte_is_power_of_2(desc);
> +			break;
> +		}
> +	}
> +
>  	return 0;
>  #else
>  	RTE_SET_USED(dev);
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.5.5

Re: [dpdk-dev] [PATCH v4] net/i40e: fix Rx instability with vector mode

[Zhang, Qi Z]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, November 5, 2018 4:05 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: [PATCH v4] 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>
> > ---
> > v4 changes:
> >  - Configure rx_vec_allowed in i40e_rx_vec_dev_conf_condition_check
> function.
> > 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            |  5 +++++
> >  drivers/net/i40e/i40e_rxtx_vec_common.h | 38
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 50 insertions(+)
> >
> > 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..5bb1a3c 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1741,6 +1741,11 @@ i40e_dev_rx_queue_setup_runtime(struct
> rte_eth_dev *dev,
> >  			ad->rx_bulk_alloc_allowed = false;
> >  		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 */
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > index f00f6d6..0e6ffa0 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > @@ -192,8 +192,13 @@ static inline int
> > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> > {  #ifndef RTE_LIBRTE_IEEE1588
> > +	struct i40e_adapter *ad =
> > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> >  	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> >  	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
> > +	struct i40e_rx_queue *rxq;
> > +	uint16_t desc, i;
> > +	bool first_queue;
> >
> >  	/* no fdir support */
> >  	if (fconf->mode != RTE_FDIR_MODE_NONE) @@ -207,6 +212,39 @@
> > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> >  		return -1;
> >
> > +	/**
> > +	 * Vector mode is allowed only when number of Rx queue
> > +	 * descriptor is power of 2.
> > +	 */
> > +	if (!dev->data->dev_started) {
> > +		first_queue = true;
> > +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +			rxq = dev->data->rx_queues[i];
> > +			if (!rxq)
> > +				continue;
> > +			desc = rxq->nb_rx_desc;
> > +			if (first_queue)
> > +				ad->rx_vec_allowed =
> > +					rte_is_power_of_2(desc);
> > +			else
> > +				ad->rx_vec_allowed =
> > +					ad->rx_vec_allowed ?
> > +					rte_is_power_of_2(desc) :
> > +					ad->rx_vec_allowed;
> > +			first_queue = false;
> > +		}
> > +	} else {
> > +		/* Only check the first queue's descriptor number */
> > +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +			rxq = dev->data->rx_queues[i];
> > +			if (!rxq)
> > +				continue;
> > +			desc = rxq->nb_rx_desc;
> > +			ad->rx_vec_allowed = rte_is_power_of_2(desc);
> > +			break;
> > +		}
> > +	}
> > +
> >  	return 0;
> >  #else
> >  	RTE_SET_USED(dev);
> > --
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

> 
> > 2.5.5