diff mbox series

[3/3] net/i40e: fix inadvertent override of vector RX allowance

Message ID 1557980885-183777-4-git-send-email-mesut.a.ergin@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Qi Zhang
Headers
Series net/i40e: improve rte_flow offload with MARK + RSS |

Checks

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

Commit Message

Ergin, Mesut A May 16, 2019, 4:28 a.m. UTC 
  When i40e_rx_vec_dev_conf_condition_check_default() determines whether
vector receive functions would be allowed during initialization phase,
it should honor previously recorded disallowance during configuration
phase, and not override simply because it is for the first queue.

Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
---
 drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Maxime Coquelin May 16, 2019, 8:17 a.m. UTC | #1 
On 5/16/19 6:28 AM, Mesut Ali Ergin wrote:
> When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> vector receive functions would be allowed during initialization phase,
> it should honor previously recorded disallowance during configuration
> phase, and not override simply because it is for the first queue.
> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> ---
>   drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index 0e6ffa0..f30cab4 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -212,6 +212,10 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>   	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
>   		return -1;
>   
> +	/* Should not override if vector was already disallowed */
> +	if (!ad->rx_vec_allowed)
> +	return -1;

nit: wrong indentation.

> +
>   	/**
>   	 * Vector mode is allowed only when number of Rx queue
>   	 * descriptor is power of 2.
>
Ergin, Mesut A May 16, 2019, 8:57 p.m. UTC | #2 
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, May 16, 2019 1:18 AM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix inadvertent override of vector
> RX allowance
> 
> 
> 
> On 5/16/19 6:28 AM, Mesut Ali Ergin wrote:
> > When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> > vector receive functions would be allowed during initialization phase,
> > it should honor previously recorded disallowance during configuration
> > phase, and not override simply because it is for the first queue.
> >
> > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > ---
> >   drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > index 0e6ffa0..f30cab4 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > @@ -212,6 +212,10 @@
> i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> >   	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> >   		return -1;
> >
> > +	/* Should not override if vector was already disallowed */
> > +	if (!ad->rx_vec_allowed)
> > +	return -1;
> 
> nit: wrong indentation.

Thanks. Fixed, and queued for v2 -- my checkpatch misses this for some reason.
> 
> > +
> >   	/**
> >   	 * Vector mode is allowed only when number of Rx queue
> >   	 * descriptor is power of 2.
> >
Qi Zhang May 22, 2019, 12:42 p.m. UTC | #3 
Hi Mesut:

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Thursday, May 16, 2019 12:28 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> Subject: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance
> 
> When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> vector receive functions would be allowed during initialization phase, it should
> honor previously recorded disallowance during configuration phase, and not
> override simply because it is for the first queue.
> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index 0e6ffa0..f30cab4 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -212,6 +212,10 @@
> i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
>  		return -1;
> 
> +	/* Should not override if vector was already disallowed */

It is possible a device be reconfigured between dev_stop/dev_start, vector mode may fit for the new configure, so the old rx_vec_allowd should be ignored, 

Regards
Qi

> +	if (!ad->rx_vec_allowed)
> +	return -1;
> +
>  	/**
>  	 * Vector mode is allowed only when number of Rx queue
>  	 * descriptor is power of 2.
> --
> 2.7.4
Ergin, Mesut A May 23, 2019, 6:25 p.m. UTC | #4 
Hi Qi,

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Wednesday, May 22, 2019 5:42 AM
> To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> allowance
> 
> Hi Mesut:
> 
> > -----Original Message-----
> > From: Ergin, Mesut A
> > Sent: Thursday, May 16, 2019 12:28 PM
> > To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> > Subject: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX allowance
> >
> > When i40e_rx_vec_dev_conf_condition_check_default() determines whether
> > vector receive functions would be allowed during initialization phase, it should
> > honor previously recorded disallowance during configuration phase, and not
> > override simply because it is for the first queue.
> >
> > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > index 0e6ffa0..f30cab4 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > @@ -212,6 +212,10 @@
> > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> >  		return -1;
> >
> > +	/* Should not override if vector was already disallowed */
> 
> It is possible a device be reconfigured between dev_stop/dev_start, vector
> mode may fit for the new configure, so the old rx_vec_allowd should be
> ignored,
> 

i40e_dev_configure would reset rx_vec_allowed already. Am I missing 
another reconfiguration path?

Mesut

> Regards
> Qi
> 
> > +	if (!ad->rx_vec_allowed)
> > +	return -1;
> > +
> >  	/**
> >  	 * Vector mode is allowed only when number of Rx queue
> >  	 * descriptor is power of 2.
> > --
> > 2.7.4
Qi Zhang May 24, 2019, 2:39 a.m. UTC | #5 
Hi Meset:

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Friday, May 24, 2019 2:26 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> allowance
> 
> Hi Qi,
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Wednesday, May 22, 2019 5:42 AM
> > To: Ergin, Mesut A <mesut.a.ergin@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Cc: dev@dpdk.org
> > Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector
> > RX allowance
> >
> > Hi Mesut:
> >
> > > -----Original Message-----
> > > From: Ergin, Mesut A
> > > Sent: Thursday, May 16, 2019 12:28 PM
> > > To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> > > Subject: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> > > allowance
> > >
> > > When i40e_rx_vec_dev_conf_condition_check_default() determines
> > > whether vector receive functions would be allowed during
> > > initialization phase, it should honor previously recorded	
> > > disallowance during configuration phase, and not override simply because
> it is for the first queue.
> > >
> > > Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > index 0e6ffa0..f30cab4 100644
> > > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > @@ -212,6 +212,10 @@
> > > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> > >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > >  		return -1;
> > >
> > > +	/* Should not override if vector was already disallowed */
> >
> > It is possible a device be reconfigured between dev_stop/dev_start,
> > vector mode may fit for the new configure, so the old rx_vec_allowd
> > should be ignored,
> >
> 
> i40e_dev_configure would reset rx_vec_allowed already. Am I missing another
> reconfiguration path?

Look at below scenario,

1. dev_configure (rx_vec_allowed is reset to true)
2. queue_setup (the ring size is not power of 2) 
3. dev_start (vector will not be selected due to ring size, rx_vec_allowed set to false) 
4. dev_stop 
5. queue_setup  (this time, with power of 2 ring size) 
6. dev_start (assume vector path should be selected, and rx_vec_allowed should be overwrite to true, but your patch will prevent it)

Also, I may not get the point of the gap you observed, would you share more detail scenario?

Regards
Qi

> 
> Mesut
> 
> > Regards
> > Qi
> >
> > > +	if (!ad->rx_vec_allowed)
> > > +	return -1;
> > > +
> > >  	/**
> > >  	 * Vector mode is allowed only when number of Rx queue
> > >  	 * descriptor is power of 2.
> > > --
> > > 2.7.4
Ergin, Mesut A May 24, 2019, 6:08 p.m. UTC | #6 
Hi,

> > > > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> > > >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > > >  		return -1;
> > > >
> > > > +	/* Should not override if vector was already disallowed */
> > >
> > > It is possible a device be reconfigured between dev_stop/dev_start,
> > > vector mode may fit for the new configure, so the old rx_vec_allowd
> > > should be ignored,
> > >
> >
> > i40e_dev_configure would reset rx_vec_allowed already. Am I missing another
> > reconfiguration path?
> 
> Look at below scenario,
> 
> 1. dev_configure (rx_vec_allowed is reset to true)
> 2. queue_setup (the ring size is not power of 2)
> 3. dev_start (vector will not be selected due to ring size, rx_vec_allowed set to
> false)
> 4. dev_stop
> 5. queue_setup  (this time, with power of 2 ring size)
> 6. dev_start (assume vector path should be selected, and rx_vec_allowed should
> be overwrite to true, but your patch will prevent it)
> 
> Also, I may not get the point of the gap you observed, would you share more
> detail scenario?
> 
> Regards
> Qi

It seems like queue setup should reset vector_allowed flag, too. Because 
i40e_dev_rx_queue_setup_runtime is already doing it, though covering
only half of the state space (i.e. device already started).

Do you think this patch plus that would be a preferred approach?

> 
> >
> > Mesut
> >
> > > Regards
> > > Qi
> > >
> > > > +	if (!ad->rx_vec_allowed)
> > > > +	return -1;
> > > > +
> > > >  	/**
> > > >  	 * Vector mode is allowed only when number of Rx queue
> > > >  	 * descriptor is power of 2.
> > > > --
> > > > 2.7.4
Qi Zhang May 25, 2019, 11:29 a.m. UTC | #7 
Hi Muset:

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Saturday, May 25, 2019 2:09 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 3/3] net/i40e: fix inadvertent override of vector RX
> allowance
> 
> Hi,
> 
> > > > > i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev
> *dev)
> > > > >  	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > > > >  		return -1;
> > > > >
> > > > > +	/* Should not override if vector was already disallowed */
> > > >
> > > > It is possible a device be reconfigured between
> > > > dev_stop/dev_start, vector mode may fit for the new configure, so
> > > > the old rx_vec_allowd should be ignored,
> > > >
> > >
> > > i40e_dev_configure would reset rx_vec_allowed already. Am I missing
> > > another reconfiguration path?
> >
> > Look at below scenario,
> >
> > 1. dev_configure (rx_vec_allowed is reset to true) 2. queue_setup (the
> > ring size is not power of 2) 3. dev_start (vector will not be selected
> > due to ring size, rx_vec_allowed set to
> > false)
> > 4. dev_stop
> > 5. queue_setup  (this time, with power of 2 ring size) 6. dev_start
> > (assume vector path should be selected, and rx_vec_allowed should be
> > overwrite to true, but your patch will prevent it)
> >
> > Also, I may not get the point of the gap you observed, would you share
> > more detail scenario?
> >
> > Regards
> > Qi
> 
> It seems like queue setup should reset vector_allowed flag, too. Because
> i40e_dev_rx_queue_setup_runtime is already doing it, though covering only
> half of the state space (i.e. device already started).
> 
> Do you think this patch plus that would be a preferred approach?

First, I need to understand your gap, that might be obvious, but so far I just did not figure out it base your commit log yet, so please give me more heads up :)
Secondary, I think move below codes from dev_configure to dev_start might be a better solution

        ad->rx_bulk_alloc_allowed = true;
        ad->rx_vec_allowed = true;
        ad->tx_simple_allowed = true;
        ad->tx_vec_allowed = true;

Regards
Qi
> 
> >
> > >
> > > Mesut
> > >
> > > > Regards
> > > > Qi
> > > >
> > > > > +	if (!ad->rx_vec_allowed)
> > > > > +	return -1;
> > > > > +
> > > > >  	/**
> > > > >  	 * Vector mode is allowed only when number of Rx queue
> > > > >  	 * descriptor is power of 2.
> > > > > --
> > > > > 2.7.4
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index 0e6ffa0..f30cab4 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -212,6 +212,10 @@  i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
 	if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
 		return -1;
 
+	/* Should not override if vector was already disallowed */
+	if (!ad->rx_vec_allowed)
+	return -1;
+
 	/**
 	 * Vector mode is allowed only when number of Rx queue
 	 * descriptor is power of 2.