diff mbox

[dpdk-dev,v2,4/4] net/i40e: enable deferred queue setup

Message ID	20180302041306.90324-5-qi.z.zhang@intel.com (mailing list archive)
State	Superseded, archived
Delegated to:	Ferruh Yigit
Headers	From: Qi Zhang <qi.z.zhang@intel.com> To: thomas@monjalon.net Cc: dev@dpdk.org, beilei.xing@intel.com, jingjing.wu@intel.com, wenzhuo.lu@intel.com, Qi Zhang <qi.z.zhang@intel.com> Date: Fri, 2 Mar 2018 12:13:06 +0800 Message-Id: <20180302041306.90324-5-qi.z.zhang@intel.com> In-Reply-To: <20180302041306.90324-1-qi.z.zhang@intel.com> References: <20180212045314.171616-1-qi.z.zhang@intel.com> <20180302041306.90324-1-qi.z.zhang@intel.com> Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue setup Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>

Checks

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

Commit Message

Qi Zhang March 2, 2018, 4:13 a.m. UTC

  Expose the deferred queue configuration capability and enhance
i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation when
device already started.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c |  6 ++++
 drivers/net/i40e/i40e_rxtx.c   | 62 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 66 insertions(+), 2 deletions(-)

Comments

Ananyev, Konstantin March 14, 2018, 12:35 p.m. UTC | #1

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> Sent: Friday, March 2, 2018 4:13 AM
> To: thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue setup
> 
> Expose the deferred queue configuration capability and enhance
> i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation when
> device already started.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c |  6 ++++
>  drivers/net/i40e/i40e_rxtx.c   | 62 ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 06b0f03a1..843a0c42a 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
>  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
>  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> +	dev_info->deferred_queue_config_capa =
> +		DEV_DEFERRED_RX_QUEUE_SETUP |
> +		DEV_DEFERRED_TX_QUEUE_SETUP |
> +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> +
>  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX + 1) *
>  						sizeof(uint32_t);
>  	dev_info->reta_size = pf->hash_lut_size;
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 1217e5a61..e5f532cf7 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
>  	uint16_t len, i;
>  	uint16_t reg_idx, base, bsf, tc_mapping;
>  	int q_offset, use_def_burst_func = 1;
> +	int ret = 0;
> 
>  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type == I40E_MAC_X722_VF) {
>  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
>  			rxq->dcb_tc = i;
>  	}
> 
> +	if (dev->data->dev_started) {
> +		ret = i40e_rx_queue_init(rxq);
> +		if (ret != I40E_SUCCESS) {
> +			PMD_DRV_LOG(ERR,
> +				    "Failed to do RX queue initialization");
> +			return ret;
> +		}
> +		if (ad->rx_vec_allowed)

Better to check what rx function is installed right now.

> +			i40e_rxq_vec_setup(rxq);
> +		if (!rxq->rx_deferred_start) {
> +			ret = i40e_dev_rx_queue_start(dev, queue_idx);

I don't think it is a good idea to start/stop queue inside queue_setup/queue_release.
There is special API (queue_start/queue_stop) to do this.
Konstantin

> +			if (ret != I40E_SUCCESS) {
> +				PMD_DRV_LOG(ERR,
> +					    "Failed to start RX queue");
> +				return ret;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
> 
> @@ -1848,13 +1868,21 @@ void
>  i40e_dev_rx_queue_release(void *rxq)
>  {
>  	struct i40e_rx_queue *q = (struct i40e_rx_queue *)rxq;
> +	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
> 
>  	if (!q) {
>  		PMD_DRV_LOG(DEBUG, "Pointer to rxq is NULL");
>  		return;
>  	}
> 
> -	i40e_rx_queue_release_mbufs(q);
> +	if (dev->data->dev_started) {
> +		if (dev->data->rx_queue_state[q->queue_id] ==
> +			RTE_ETH_QUEUE_STATE_STARTED)
> +			i40e_dev_rx_queue_stop(dev, q->queue_id);
> +	} else {
> +		i40e_rx_queue_release_mbufs(q);
> +	}
> +
>  	rte_free(q->sw_ring);
>  	rte_free(q);
>  }
> @@ -1980,6 +2008,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  			const struct rte_eth_txconf *tx_conf)
>  {
>  	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_adapter *ad =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>  	struct i40e_vsi *vsi;
>  	struct i40e_pf *pf = NULL;
>  	struct i40e_vf *vf = NULL;
> @@ -1989,6 +2019,7 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  	uint16_t tx_rs_thresh, tx_free_thresh;
>  	uint16_t reg_idx, i, base, bsf, tc_mapping;
>  	int q_offset;
> +	int ret = 0;
> 
>  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type == I40E_MAC_X722_VF) {
>  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> @@ -2162,6 +2193,25 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  			txq->dcb_tc = i;
>  	}
> 
> +	if (dev->data->dev_started) {
> +		ret = i40e_tx_queue_init(txq);
> +		if (ret != I40E_SUCCESS) {
> +			PMD_DRV_LOG(ERR,
> +				    "Failed to do TX queue initialization");
> +			return ret;
> +		}
> +		if (ad->tx_vec_allowed)
> +			i40e_txq_vec_setup(txq);
> +		if (!txq->tx_deferred_start) {
> +			ret = i40e_dev_tx_queue_start(dev, queue_idx);
> +			if (ret != I40E_SUCCESS) {
> +				PMD_DRV_LOG(ERR,
> +					    "Failed to start TX queue");
> +				return ret;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
> 
> @@ -2169,13 +2219,21 @@ void
>  i40e_dev_tx_queue_release(void *txq)
>  {
>  	struct i40e_tx_queue *q = (struct i40e_tx_queue *)txq;
> +	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
> 
>  	if (!q) {
>  		PMD_DRV_LOG(DEBUG, "Pointer to TX queue is NULL");
>  		return;
>  	}
> 
> -	i40e_tx_queue_release_mbufs(q);
> +	if (dev->data->dev_started) {
> +		if (dev->data->tx_queue_state[q->queue_id] ==
> +			RTE_ETH_QUEUE_STATE_STARTED)
> +			i40e_dev_tx_queue_stop(dev, q->queue_id);
> +	} else {
> +		i40e_tx_queue_release_mbufs(q);
> +	}
> +
>  	rte_free(q->sw_ring);
>  	rte_free(q);
>  }
> --
> 2.13.6

Qi Zhang March 15, 2018, 3:22 a.m. UTC | #2

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, March 14, 2018 8:36 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > Sent: Friday, March 2, 2018 4:13 AM
> > To: thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> Qi
> > Z <qi.z.zhang@intel.com>
> > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> > Expose the deferred queue configuration capability and enhance
> > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation when
> > device already started.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> >  drivers/net/i40e/i40e_rxtx.c   | 62
> ++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 66 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev *dev,
> struct rte_eth_dev_info *dev_info)
> >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > +	dev_info->deferred_queue_config_capa =
> > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > +
> >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX + 1) *
> >  						sizeof(uint32_t);
> >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index
> > 1217e5a61..e5f532cf7 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev
> *dev,
> >  	uint16_t len, i;
> >  	uint16_t reg_idx, base, bsf, tc_mapping;
> >  	int q_offset, use_def_burst_func = 1;
> > +	int ret = 0;
> >
> >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> I40E_MAC_X722_VF) {
> >  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev
> *dev,
> >  			rxq->dcb_tc = i;
> >  	}
> >
> > +	if (dev->data->dev_started) {
> > +		ret = i40e_rx_queue_init(rxq);
> > +		if (ret != I40E_SUCCESS) {
> > +			PMD_DRV_LOG(ERR,
> > +				    "Failed to do RX queue initialization");
> > +			return ret;
> > +		}
> > +		if (ad->rx_vec_allowed)
> 
> Better to check what rx function is installed right now.
Yes, it should be fixed, need to return fail if any conflict
> 
> > +			i40e_rxq_vec_setup(rxq);
> > +		if (!rxq->rx_deferred_start) {
> > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> 
> I don't think it is a good idea to start/stop queue inside
> queue_setup/queue_release.
> There is special API (queue_start/queue_stop) to do this.

The idea is if dev already started, the queue is supposed to be started automatically after queue_setup.
The defered_start flag can be used if application don't want this.
But maybe it's better to call dev_ops->rx_queue_stop in etherdev layer. (same thing for queue_stop in previous patch)

Thanks
Qi

> Konstantin
> 
> > +			if (ret != I40E_SUCCESS) {
> > +				PMD_DRV_LOG(ERR,
> > +					    "Failed to start RX queue");
> > +				return ret;
> > +			}
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > @@ -1848,13 +1868,21 @@ void
> >  i40e_dev_rx_queue_release(void *rxq)
> >  {
> >  	struct i40e_rx_queue *q = (struct i40e_rx_queue *)rxq;
> > +	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
> >
> >  	if (!q) {
> >  		PMD_DRV_LOG(DEBUG, "Pointer to rxq is NULL");
> >  		return;
> >  	}
> >
> > -	i40e_rx_queue_release_mbufs(q);
> > +	if (dev->data->dev_started) {
> > +		if (dev->data->rx_queue_state[q->queue_id] ==
> > +			RTE_ETH_QUEUE_STATE_STARTED)
> > +			i40e_dev_rx_queue_stop(dev, q->queue_id);
> > +	} else {
> > +		i40e_rx_queue_release_mbufs(q);
> > +	}
> > +
> >  	rte_free(q->sw_ring);
> >  	rte_free(q);
> >  }
> > @@ -1980,6 +2008,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> *dev,
> >  			const struct rte_eth_txconf *tx_conf)  {
> >  	struct i40e_hw *hw =
> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +	struct i40e_adapter *ad =
> > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> >  	struct i40e_vsi *vsi;
> >  	struct i40e_pf *pf = NULL;
> >  	struct i40e_vf *vf = NULL;
> > @@ -1989,6 +2019,7 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> *dev,
> >  	uint16_t tx_rs_thresh, tx_free_thresh;
> >  	uint16_t reg_idx, i, base, bsf, tc_mapping;
> >  	int q_offset;
> > +	int ret = 0;
> >
> >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> I40E_MAC_X722_VF) {
> >  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > @@ -2162,6 +2193,25 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> *dev,
> >  			txq->dcb_tc = i;
> >  	}
> >
> > +	if (dev->data->dev_started) {
> > +		ret = i40e_tx_queue_init(txq);
> > +		if (ret != I40E_SUCCESS) {
> > +			PMD_DRV_LOG(ERR,
> > +				    "Failed to do TX queue initialization");
> > +			return ret;
> > +		}
> > +		if (ad->tx_vec_allowed)
> > +			i40e_txq_vec_setup(txq);
> > +		if (!txq->tx_deferred_start) {
> > +			ret = i40e_dev_tx_queue_start(dev, queue_idx);
> > +			if (ret != I40E_SUCCESS) {
> > +				PMD_DRV_LOG(ERR,
> > +					    "Failed to start TX queue");
> > +				return ret;
> > +			}
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > @@ -2169,13 +2219,21 @@ void
> >  i40e_dev_tx_queue_release(void *txq)
> >  {
> >  	struct i40e_tx_queue *q = (struct i40e_tx_queue *)txq;
> > +	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
> >
> >  	if (!q) {
> >  		PMD_DRV_LOG(DEBUG, "Pointer to TX queue is NULL");
> >  		return;
> >  	}
> >
> > -	i40e_tx_queue_release_mbufs(q);
> > +	if (dev->data->dev_started) {
> > +		if (dev->data->tx_queue_state[q->queue_id] ==
> > +			RTE_ETH_QUEUE_STATE_STARTED)
> > +			i40e_dev_tx_queue_stop(dev, q->queue_id);
> > +	} else {
> > +		i40e_tx_queue_release_mbufs(q);
> > +	}
> > +
> >  	rte_free(q->sw_ring);
> >  	rte_free(q);
> >  }
> > --
> > 2.13.6

Qi Zhang March 15, 2018, 3:50 a.m. UTC | #3

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z
> Sent: Thursday, March 15, 2018 11:22 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Wednesday, March 14, 2018 8:36 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> Qi
> > Z <qi.z.zhang@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > > Sent: Friday, March 2, 2018 4:13 AM
> > > To: thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> > Qi
> > > Z <qi.z.zhang@intel.com>
> > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > setup
> > >
> > > Expose the deferred queue configuration capability and enhance
> > > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation when
> > > device already started.
> > >
> > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > ++++++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev *dev,
> > struct rte_eth_dev_info *dev_info)
> > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > +	dev_info->deferred_queue_config_capa =
> > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > +
> > >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX + 1) *
> > >  						sizeof(uint32_t);
> > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index
> > > 1217e5a61..e5f532cf7 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev
> > *dev,
> > >  	uint16_t len, i;
> > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > >  	int q_offset, use_def_burst_func = 1;
> > > +	int ret = 0;
> > >
> > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > I40E_MAC_X722_VF) {
> > >  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> rte_eth_dev
> > *dev,
> > >  			rxq->dcb_tc = i;
> > >  	}
> > >
> > > +	if (dev->data->dev_started) {
> > > +		ret = i40e_rx_queue_init(rxq);
> > > +		if (ret != I40E_SUCCESS) {
> > > +			PMD_DRV_LOG(ERR,
> > > +				    "Failed to do RX queue initialization");
> > > +			return ret;
> > > +		}
> > > +		if (ad->rx_vec_allowed)
> >
> > Better to check what rx function is installed right now.
> Yes, it should be fixed, need to return fail if any conflict
Sorry, For i40e, I think rx function set is not impacted by queue_setup parameters but only dev_configure's, so it's not necessary to check installed function, and there will be no conflict here.
> >
> > > +			i40e_rxq_vec_setup(rxq);
> > > +		if (!rxq->rx_deferred_start) {
> > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> >
> > I don't think it is a good idea to start/stop queue inside
> > queue_setup/queue_release.
> > There is special API (queue_start/queue_stop) to do this.
> 
> The idea is if dev already started, the queue is supposed to be started
> automatically after queue_setup.
> The defered_start flag can be used if application don't want this.
> But maybe it's better to call dev_ops->rx_queue_stop in etherdev layer.
> (same thing for queue_stop in previous patch)
> 
> Thanks
> Qi
> 
> > Konstantin
> >
> > > +			if (ret != I40E_SUCCESS) {
> > > +				PMD_DRV_LOG(ERR,
> > > +					    "Failed to start RX queue");
> > > +				return ret;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -1848,13 +1868,21 @@ void
> > >  i40e_dev_rx_queue_release(void *rxq)  {
> > >  	struct i40e_rx_queue *q = (struct i40e_rx_queue *)rxq;
> > > +	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
> > >
> > >  	if (!q) {
> > >  		PMD_DRV_LOG(DEBUG, "Pointer to rxq is NULL");
> > >  		return;
> > >  	}
> > >
> > > -	i40e_rx_queue_release_mbufs(q);
> > > +	if (dev->data->dev_started) {
> > > +		if (dev->data->rx_queue_state[q->queue_id] ==
> > > +			RTE_ETH_QUEUE_STATE_STARTED)
> > > +			i40e_dev_rx_queue_stop(dev, q->queue_id);
> > > +	} else {
> > > +		i40e_rx_queue_release_mbufs(q);
> > > +	}
> > > +
> > >  	rte_free(q->sw_ring);
> > >  	rte_free(q);
> > >  }
> > > @@ -1980,6 +2008,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> > *dev,
> > >  			const struct rte_eth_txconf *tx_conf)  {
> > >  	struct i40e_hw *hw =
> > I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > > +	struct i40e_adapter *ad =
> > > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > >  	struct i40e_vsi *vsi;
> > >  	struct i40e_pf *pf = NULL;
> > >  	struct i40e_vf *vf = NULL;
> > > @@ -1989,6 +2019,7 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> > *dev,
> > >  	uint16_t tx_rs_thresh, tx_free_thresh;
> > >  	uint16_t reg_idx, i, base, bsf, tc_mapping;
> > >  	int q_offset;
> > > +	int ret = 0;
> > >
> > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > I40E_MAC_X722_VF) {
> > >  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > @@ -2162,6 +2193,25 @@ i40e_dev_tx_queue_setup(struct
> rte_eth_dev
> > *dev,
> > >  			txq->dcb_tc = i;
> > >  	}
> > >
> > > +	if (dev->data->dev_started) {
> > > +		ret = i40e_tx_queue_init(txq);
> > > +		if (ret != I40E_SUCCESS) {
> > > +			PMD_DRV_LOG(ERR,
> > > +				    "Failed to do TX queue initialization");
> > > +			return ret;
> > > +		}
> > > +		if (ad->tx_vec_allowed)
> > > +			i40e_txq_vec_setup(txq);
> > > +		if (!txq->tx_deferred_start) {
> > > +			ret = i40e_dev_tx_queue_start(dev, queue_idx);
> > > +			if (ret != I40E_SUCCESS) {
> > > +				PMD_DRV_LOG(ERR,
> > > +					    "Failed to start TX queue");
> > > +				return ret;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -2169,13 +2219,21 @@ void
> > >  i40e_dev_tx_queue_release(void *txq)
> > >  {
> > >  	struct i40e_tx_queue *q = (struct i40e_tx_queue *)txq;
> > > +	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
> > >
> > >  	if (!q) {
> > >  		PMD_DRV_LOG(DEBUG, "Pointer to TX queue is NULL");
> > >  		return;
> > >  	}
> > >
> > > -	i40e_tx_queue_release_mbufs(q);
> > > +	if (dev->data->dev_started) {
> > > +		if (dev->data->tx_queue_state[q->queue_id] ==
> > > +			RTE_ETH_QUEUE_STATE_STARTED)
> > > +			i40e_dev_tx_queue_stop(dev, q->queue_id);
> > > +	} else {
> > > +		i40e_tx_queue_release_mbufs(q);
> > > +	}
> > > +
> > >  	rte_free(q->sw_ring);
> > >  	rte_free(q);
> > >  }
> > > --
> > > 2.13.6

Ananyev, Konstantin March 15, 2018, 1:22 p.m. UTC | #4

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Thursday, March 15, 2018 3:22 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue setup
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Wednesday, March 14, 2018 8:36 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > > Sent: Friday, March 2, 2018 4:13 AM
> > > To: thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> > Qi
> > > Z <qi.z.zhang@intel.com>
> > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > setup
> > >
> > > Expose the deferred queue configuration capability and enhance
> > > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation when
> > > device already started.
> > >
> > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > ++++++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev *dev,
> > struct rte_eth_dev_info *dev_info)
> > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > +	dev_info->deferred_queue_config_capa =
> > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > +
> > >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX + 1) *
> > >  						sizeof(uint32_t);
> > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index
> > > 1217e5a61..e5f532cf7 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev
> > *dev,
> > >  	uint16_t len, i;
> > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > >  	int q_offset, use_def_burst_func = 1;
> > > +	int ret = 0;
> > >
> > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > I40E_MAC_X722_VF) {
> > >  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev
> > *dev,
> > >  			rxq->dcb_tc = i;
> > >  	}
> > >
> > > +	if (dev->data->dev_started) {
> > > +		ret = i40e_rx_queue_init(rxq);
> > > +		if (ret != I40E_SUCCESS) {
> > > +			PMD_DRV_LOG(ERR,
> > > +				    "Failed to do RX queue initialization");
> > > +			return ret;
> > > +		}
> > > +		if (ad->rx_vec_allowed)
> >
> > Better to check what rx function is installed right now.
> Yes, it should be fixed, need to return fail if any conflict
> >
> > > +			i40e_rxq_vec_setup(rxq);
> > > +		if (!rxq->rx_deferred_start) {
> > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> >
> > I don't think it is a good idea to start/stop queue inside
> > queue_setup/queue_release.
> > There is special API (queue_start/queue_stop) to do this.
> 
> The idea is if dev already started, the queue is supposed to be started automatically after queue_setup.

Why is that?
Might be user doesn't want to start queue, might be he only wants to start it.
Might be he would need to call queue_setup() once again later before starting it - based on some logic?
If the user wants to setup and start the queue immediately he can always do:

rc = queue_setup(...);
if (rc == 0)
   queue_start(...);

We have a pretty well defined API here let's keep it like that.
Konstantin

Qi Zhang March 15, 2018, 2:30 p.m. UTC | #5

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, March 15, 2018 9:23 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Thursday, March 15, 2018 3:22 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Wednesday, March 14, 2018 8:36 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> > > Qi Z <qi.z.zhang@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > queue setup
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > To: thomas@monjalon.net
> > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > <wenzhuo.lu@intel.com>; Zhang,
> > > Qi
> > > > Z <qi.z.zhang@intel.com>
> > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > > setup
> > > >
> > > > Expose the deferred queue configuration capability and enhance
> > > > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation
> > > > when device already started.
> > > >
> > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > ---
> > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > ++++++++++++++++++++++++++++++++++++++++--
> > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a 100644
> > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev
> *dev,
> > > struct rte_eth_dev_info *dev_info)
> > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > +	dev_info->deferred_queue_config_capa =
> > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > +
> > > >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX + 1) *
> > > >  						sizeof(uint32_t);
> > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > > index
> > > > 1217e5a61..e5f532cf7 100644
> > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> rte_eth_dev
> > > *dev,
> > > >  	uint16_t len, i;
> > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > >  	int q_offset, use_def_burst_func = 1;
> > > > +	int ret = 0;
> > > >
> > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > I40E_MAC_X722_VF) {
> > > >  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> rte_eth_dev
> > > *dev,
> > > >  			rxq->dcb_tc = i;
> > > >  	}
> > > >
> > > > +	if (dev->data->dev_started) {
> > > > +		ret = i40e_rx_queue_init(rxq);
> > > > +		if (ret != I40E_SUCCESS) {
> > > > +			PMD_DRV_LOG(ERR,
> > > > +				    "Failed to do RX queue initialization");
> > > > +			return ret;
> > > > +		}
> > > > +		if (ad->rx_vec_allowed)
> > >
> > > Better to check what rx function is installed right now.
> > Yes, it should be fixed, need to return fail if any conflict
> > >
> > > > +			i40e_rxq_vec_setup(rxq);
> > > > +		if (!rxq->rx_deferred_start) {
> > > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> > >
> > > I don't think it is a good idea to start/stop queue inside
> > > queue_setup/queue_release.
> > > There is special API (queue_start/queue_stop) to do this.
> >
> > The idea is if dev already started, the queue is supposed to be started
> automatically after queue_setup.
> 
> Why is that?
Because device is already started, its like a running conveyor belt, anything you put or replace on it just moves automatically.

> Might be user doesn't want to start queue, might be he only wants to start
> it.
Use deferred_start_flag,  
> Might be he would need to call queue_setup() once again later before
> starting it - based on some logic?
Dev_ops->queue_stop will be called first before dev_ops->queue_setup in rte_eth_rx|tx_queue_setup, if a queue is running.



> If the user wants to setup and start the queue immediately he can always do:
> 
> rc = queue_setup(...);
> if (rc == 0)
>    queue_start(...);

application no need to call queue_start explicitly in this case.

> 
> We have a pretty well defined API here let's keep it like that.
> Konstantin

Ananyev, Konstantin March 15, 2018, 3:22 p.m. UTC | #6

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Thursday, March 15, 2018 2:30 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue setup
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, March 15, 2018 9:23 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Thursday, March 15, 2018 3:22 AM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > setup
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Wednesday, March 14, 2018 8:36 PM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> > > > Qi Z <qi.z.zhang@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > queue setup
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > To: thomas@monjalon.net
> > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > Qi
> > > > > Z <qi.z.zhang@intel.com>
> > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > > > setup
> > > > >
> > > > > Expose the deferred queue configuration capability and enhance
> > > > > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation
> > > > > when device already started.
> > > > >
> > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > ---
> > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a 100644
> > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev
> > *dev,
> > > > struct rte_eth_dev_info *dev_info)
> > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > +	dev_info->deferred_queue_config_capa =
> > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > +
> > > > >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX + 1) *
> > > > >  						sizeof(uint32_t);
> > > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > > > index
> > > > > 1217e5a61..e5f532cf7 100644
> > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> > rte_eth_dev
> > > > *dev,
> > > > >  	uint16_t len, i;
> > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > +	int ret = 0;
> > > > >
> > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > > I40E_MAC_X722_VF) {
> > > > >  		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> > rte_eth_dev
> > > > *dev,
> > > > >  			rxq->dcb_tc = i;
> > > > >  	}
> > > > >
> > > > > +	if (dev->data->dev_started) {
> > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > +		if (ret != I40E_SUCCESS) {
> > > > > +			PMD_DRV_LOG(ERR,
> > > > > +				    "Failed to do RX queue initialization");
> > > > > +			return ret;
> > > > > +		}
> > > > > +		if (ad->rx_vec_allowed)
> > > >
> > > > Better to check what rx function is installed right now.
> > > Yes, it should be fixed, need to return fail if any conflict
> > > >
> > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > +		if (!rxq->rx_deferred_start) {
> > > > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> > > >
> > > > I don't think it is a good idea to start/stop queue inside
> > > > queue_setup/queue_release.
> > > > There is special API (queue_start/queue_stop) to do this.
> > >
> > > The idea is if dev already started, the queue is supposed to be started
> > automatically after queue_setup.
> >
> > Why is that?
> Because device is already started, its like a running conveyor belt, anything you put or replace on it just moves automatically.

Why is that? :)
You do break existing behavior.
Right now it possible to do:
queue_setup(); queue_setup();
for the same queue.
With you patch is not any more.
And I don't see an good reason to break existing behavior.
What is the advantage of implicit call queue_start() implicitly from the queue_setup()/?
Konstantin

> 
> > Might be user doesn't want to start queue, might be he only wants to start
> > it.
> Use deferred_start_flag,
> > Might be he would need to call queue_setup() once again later before
> > starting it - based on some logic?
> Dev_ops->queue_stop will be called first before dev_ops->queue_setup in rte_eth_rx|tx_queue_setup, if a queue is running.
> 
> 
> 
> > If the user wants to setup and start the queue immediately he can always do:
> >
> > rc = queue_setup(...);
> > if (rc == 0)
> >    queue_start(...);
> 
> application no need to call queue_start explicitly in this case.
> 
> >
> > We have a pretty well defined API here let's keep it like that.
> > Konstantin

Qi Zhang March 16, 2018, 12:52 a.m. UTC | #7

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, March 15, 2018 11:22 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Thursday, March 15, 2018 2:30 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Thursday, March 15, 2018 9:23 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > queue setup
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z
> > > > Sent: Thursday, March 15, 2018 3:22 AM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > thomas@monjalon.net
> > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > <wenzhuo.lu@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > queue setup
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ananyev, Konstantin
> > > > > Sent: Wednesday, March 14, 2018 8:36 PM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > <wenzhuo.lu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > queue setup
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > > To: thomas@monjalon.net
> > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > > Qi
> > > > > > Z <qi.z.zhang@intel.com>
> > > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > > queue setup
> > > > > >
> > > > > > Expose the deferred queue configuration capability and enhance
> > > > > > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation
> > > > > > when device already started.
> > > > > >
> > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > ---
> > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a
> > > > > > 100644
> > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev
> > > *dev,
> > > > > struct rte_eth_dev_info *dev_info)
> > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > +
> > > > > >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX +
> 1) *
> > > > > >  						sizeof(uint32_t);
> > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > > > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > > > > index
> > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> > > rte_eth_dev
> > > > > *dev,
> > > > > >  	uint16_t len, i;
> > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > +	int ret = 0;
> > > > > >
> > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > > > I40E_MAC_X722_VF) {
> > > > > >  		vf =
> I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> > > rte_eth_dev
> > > > > *dev,
> > > > > >  			rxq->dcb_tc = i;
> > > > > >  	}
> > > > > >
> > > > > > +	if (dev->data->dev_started) {
> > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > +				    "Failed to do RX queue initialization");
> > > > > > +			return ret;
> > > > > > +		}
> > > > > > +		if (ad->rx_vec_allowed)
> > > > >
> > > > > Better to check what rx function is installed right now.
> > > > Yes, it should be fixed, need to return fail if any conflict
> > > > >
> > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> > > > >
> > > > > I don't think it is a good idea to start/stop queue inside
> > > > > queue_setup/queue_release.
> > > > > There is special API (queue_start/queue_stop) to do this.
> > > >
> > > > The idea is if dev already started, the queue is supposed to be
> > > > started
> > > automatically after queue_setup.
> > >
> > > Why is that?
> > Because device is already started, its like a running conveyor belt, anything
> you put or replace on it just moves automatically.
> 
> Why is that? :)
> You do break existing behavior.
> Right now it possible to do:
> queue_setup(); queue_setup();
> for the same queue.
> With you patch is not any more
Why not?
I think with my patch,
It assumes we can run below scenario on the same queue. 
(note, I assume queue_stop/start has been moved from i40e to ethedev layer already.)
queue_setup + queue_setup + dev_start + queue_setup + queue_setup, 
queue_stop/start are handled inside queue_setup automatically after dev_started?
.
> And I don't see an good reason to break existing behavior.
> What is the advantage of implicit call queue_start() implicitly from the
> queue_setup()/?
> Konstantin
> 
> >
> > > Might be user doesn't want to start queue, might be he only wants to
> > > start it.
> > Use deferred_start_flag,
> > > Might be he would need to call queue_setup() once again later before
> > > starting it - based on some logic?
> > Dev_ops->queue_stop will be called first before dev_ops->queue_setup in
> rte_eth_rx|tx_queue_setup, if a queue is running.
> >
> >
> >
> > > If the user wants to setup and start the queue immediately he can always
> do:
> > >
> > > rc = queue_setup(...);
> > > if (rc == 0)
> > >    queue_start(...);
> >
> > application no need to call queue_start explicitly in this case.
> >
> > >
> > > We have a pretty well defined API here let's keep it like that.
> > > Konstantin

Ananyev, Konstantin March 16, 2018, 9:54 a.m. UTC | #8

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Friday, March 16, 2018 12:52 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue setup
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, March 15, 2018 11:22 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Thursday, March 15, 2018 2:30 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > setup
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Thursday, March 15, 2018 9:23 PM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > queue setup
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Qi Z
> > > > > Sent: Thursday, March 15, 2018 3:22 AM
> > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > thomas@monjalon.net
> > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > <wenzhuo.lu@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > queue setup
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ananyev, Konstantin
> > > > > > Sent: Wednesday, March 14, 2018 8:36 PM
> > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > <wenzhuo.lu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > > queue setup
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > > > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > > > To: thomas@monjalon.net
> > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > > > Qi
> > > > > > > Z <qi.z.zhang@intel.com>
> > > > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > > > queue setup
> > > > > > >
> > > > > > > Expose the deferred queue configuration capability and enhance
> > > > > > > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation
> > > > > > > when device already started.
> > > > > > >
> > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > ---
> > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a
> > > > > > > 100644
> > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev
> > > > *dev,
> > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > +
> > > > > > >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX +
> > 1) *
> > > > > > >  						sizeof(uint32_t);
> > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > > > > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > index
> > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> > > > rte_eth_dev
> > > > > > *dev,
> > > > > > >  	uint16_t len, i;
> > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > +	int ret = 0;
> > > > > > >
> > > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > > > > I40E_MAC_X722_VF) {
> > > > > > >  		vf =
> > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> > > > rte_eth_dev
> > > > > > *dev,
> > > > > > >  			rxq->dcb_tc = i;
> > > > > > >  	}
> > > > > > >
> > > > > > > +	if (dev->data->dev_started) {
> > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > +				    "Failed to do RX queue initialization");
> > > > > > > +			return ret;
> > > > > > > +		}
> > > > > > > +		if (ad->rx_vec_allowed)
> > > > > >
> > > > > > Better to check what rx function is installed right now.
> > > > > Yes, it should be fixed, need to return fail if any conflict
> > > > > >
> > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> > > > > >
> > > > > > I don't think it is a good idea to start/stop queue inside
> > > > > > queue_setup/queue_release.
> > > > > > There is special API (queue_start/queue_stop) to do this.
> > > > >
> > > > > The idea is if dev already started, the queue is supposed to be
> > > > > started
> > > > automatically after queue_setup.
> > > >
> > > > Why is that?
> > > Because device is already started, its like a running conveyor belt, anything
> > you put or replace on it just moves automatically.
> >
> > Why is that? :)
> > You do break existing behavior.
> > Right now it possible to do:
> > queue_setup(); queue_setup();
> > for the same queue.
> > With you patch is not any more
> Why not?
> I think with my patch,
> It assumes we can run below scenario on the same queue.
> (note, I assume queue_stop/start has been moved from i40e to ethedev layer already.)
> queue_setup + queue_setup + dev_start + queue_setup + queue_setup,

Because you can't do queue_setup() on already started queue.
So if you do start() inside setup() second setup() should fail.

> queue_stop/start are handled inside queue_setup automatically after dev_started?

Again - I don't see any advantages to change existing API behavior and introduce implicit
start/stop inside setup.
It only introduce extra confusion for the users.
So I still think we better keep existing behavior.
Konstantin

> .
> > And I don't see an good reason to break existing behavior.
> > What is the advantage of implicit call queue_start() implicitly from the
> > queue_setup()/?
> > Konstantin
> >
> > >
> > > > Might be user doesn't want to start queue, might be he only wants to
> > > > start it.
> > > Use deferred_start_flag,
> > > > Might be he would need to call queue_setup() once again later before
> > > > starting it - based on some logic?
> > > Dev_ops->queue_stop will be called first before dev_ops->queue_setup in
> > rte_eth_rx|tx_queue_setup, if a queue is running.
> > >
> > >
> > >
> > > > If the user wants to setup and start the queue immediately he can always
> > do:
> > > >
> > > > rc = queue_setup(...);
> > > > if (rc == 0)
> > > >    queue_start(...);
> > >
> > > application no need to call queue_start explicitly in this case.
> > >
> > > >
> > > > We have a pretty well defined API here let's keep it like that.
> > > > Konstantin

Bruce Richardson March 16, 2018, 11 a.m. UTC | #9

On Fri, Mar 16, 2018 at 09:54:03AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Friday, March 16, 2018 12:52 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue setup
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Thursday, March 15, 2018 11:22 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > setup
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z
> > > > Sent: Thursday, March 15, 2018 2:30 PM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > thomas@monjalon.net
> > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > > > setup
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ananyev, Konstantin
> > > > > Sent: Thursday, March 15, 2018 9:23 PM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > queue setup
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zhang, Qi Z
> > > > > > Sent: Thursday, March 15, 2018 3:22 AM
> > > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > > thomas@monjalon.net
> > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > <wenzhuo.lu@intel.com>
> > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > > queue setup
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ananyev, Konstantin
> > > > > > > Sent: Wednesday, March 14, 2018 8:36 PM
> > > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > <wenzhuo.lu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > > > queue setup
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi Zhang
> > > > > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > > > > To: thomas@monjalon.net
> > > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > > > > Qi
> > > > > > > > Z <qi.z.zhang@intel.com>
> > > > > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > > > > queue setup
> > > > > > > >
> > > > > > > > Expose the deferred queue configuration capability and enhance
> > > > > > > > i40e_dev_[rx|tx]_queue_[setup|release] to handle the situation
> > > > > > > > when device already started.
> > > > > > > >
> > > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > b/drivers/net/i40e/i40e_ethdev.c index 06b0f03a1..843a0c42a
> > > > > > > > 100644
> > > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct rte_eth_dev
> > > > > *dev,
> > > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > > +
> > > > > > > >  	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX +
> > > 1) *
> > > > > > > >  						sizeof(uint32_t);
> > > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > > > > > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > index
> > > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> > > > > rte_eth_dev
> > > > > > > *dev,
> > > > > > > >  	uint16_t len, i;
> > > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > > +	int ret = 0;
> > > > > > > >
> > > > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > > > > > I40E_MAC_X722_VF) {
> > > > > > > >  		vf =
> > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> > > > > rte_eth_dev
> > > > > > > *dev,
> > > > > > > >  			rxq->dcb_tc = i;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > +	if (dev->data->dev_started) {
> > > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > > +				    "Failed to do RX queue initialization");
> > > > > > > > +			return ret;
> > > > > > > > +		}
> > > > > > > > +		if (ad->rx_vec_allowed)
> > > > > > >
> > > > > > > Better to check what rx function is installed right now.
> > > > > > Yes, it should be fixed, need to return fail if any conflict
> > > > > > >
> > > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> > > > > > >
> > > > > > > I don't think it is a good idea to start/stop queue inside
> > > > > > > queue_setup/queue_release.
> > > > > > > There is special API (queue_start/queue_stop) to do this.
> > > > > >
> > > > > > The idea is if dev already started, the queue is supposed to be
> > > > > > started
> > > > > automatically after queue_setup.
> > > > >
> > > > > Why is that?
> > > > Because device is already started, its like a running conveyor belt, anything
> > > you put or replace on it just moves automatically.
> > >
> > > Why is that? :)
> > > You do break existing behavior.
> > > Right now it possible to do:
> > > queue_setup(); queue_setup();
> > > for the same queue.
> > > With you patch is not any more
> > Why not?
> > I think with my patch,
> > It assumes we can run below scenario on the same queue.
> > (note, I assume queue_stop/start has been moved from i40e to ethedev layer already.)
> > queue_setup + queue_setup + dev_start + queue_setup + queue_setup,
> 
> Because you can't do queue_setup() on already started queue.
> So if you do start() inside setup() second setup() should fail.
> 
> > queue_stop/start are handled inside queue_setup automatically after dev_started?
> 
> Again - I don't see any advantages to change existing API behavior and introduce implicit
> start/stop inside setup.
> It only introduce extra confusion for the users.
> So I still think we better keep existing behavior.
> Konstantin
> 
+1 for keeping existing behaviour unless there is a compelling reason to
change.

Qi Zhang March 16, 2018, 1:18 p.m. UTC | #10

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, March 16, 2018 5:54 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Friday, March 16, 2018 12:52 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Thursday, March 15, 2018 11:22 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > queue setup
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z
> > > > Sent: Thursday, March 15, 2018 2:30 PM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > thomas@monjalon.net
> > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > <wenzhuo.lu@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > queue setup
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ananyev, Konstantin
> > > > > Sent: Thursday, March 15, 2018 9:23 PM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > <wenzhuo.lu@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > queue setup
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zhang, Qi Z
> > > > > > Sent: Thursday, March 15, 2018 3:22 AM
> > > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > > thomas@monjalon.net
> > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > <wenzhuo.lu@intel.com>
> > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > deferred queue setup
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ananyev, Konstantin
> > > > > > > Sent: Wednesday, March 14, 2018 8:36 PM
> > > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > <wenzhuo.lu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > > deferred queue setup
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi
> > > > > > > > Zhang
> > > > > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > > > > To: thomas@monjalon.net
> > > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>;
> > > > > > > > Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > > > > Qi
> > > > > > > > Z <qi.z.zhang@intel.com>
> > > > > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > > > deferred queue setup
> > > > > > > >
> > > > > > > > Expose the deferred queue configuration capability and
> > > > > > > > enhance i40e_dev_[rx|tx]_queue_[setup|release] to handle
> > > > > > > > the situation when device already started.
> > > > > > > >
> > > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > b/drivers/net/i40e/i40e_ethdev.c index
> > > > > > > > 06b0f03a1..843a0c42a
> > > > > > > > 100644
> > > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct
> > > > > > > > rte_eth_dev
> > > > > *dev,
> > > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > > +
> > > > > > > >  	dev_info->hash_key_size =
> (I40E_PFQF_HKEY_MAX_INDEX +
> > > 1) *
> > > > > > > >  						sizeof(uint32_t);
> > > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > > > > > a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > b/drivers/net/i40e/i40e_rxtx.c index
> > > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> > > > > rte_eth_dev
> > > > > > > *dev,
> > > > > > > >  	uint16_t len, i;
> > > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > > +	int ret = 0;
> > > > > > > >
> > > > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > > > > > I40E_MAC_X722_VF) {
> > > > > > > >  		vf =
> > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> > > > > rte_eth_dev
> > > > > > > *dev,
> > > > > > > >  			rxq->dcb_tc = i;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > +	if (dev->data->dev_started) {
> > > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > > +				    "Failed to do RX queue initialization");
> > > > > > > > +			return ret;
> > > > > > > > +		}
> > > > > > > > +		if (ad->rx_vec_allowed)
> > > > > > >
> > > > > > > Better to check what rx function is installed right now.
> > > > > > Yes, it should be fixed, need to return fail if any conflict
> > > > > > >
> > > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > > +			ret = i40e_dev_rx_queue_start(dev, queue_idx);
> > > > > > >
> > > > > > > I don't think it is a good idea to start/stop queue inside
> > > > > > > queue_setup/queue_release.
> > > > > > > There is special API (queue_start/queue_stop) to do this.
> > > > > >
> > > > > > The idea is if dev already started, the queue is supposed to
> > > > > > be started
> > > > > automatically after queue_setup.
> > > > >
> > > > > Why is that?
> > > > Because device is already started, its like a running conveyor
> > > > belt, anything
> > > you put or replace on it just moves automatically.
> > >
> > > Why is that? :)
> > > You do break existing behavior.
> > > Right now it possible to do:
> > > queue_setup(); queue_setup();
> > > for the same queue.
> > > With you patch is not any more
> > Why not?
> > I think with my patch,
> > It assumes we can run below scenario on the same queue.
> > (note, I assume queue_stop/start has been moved from i40e to ethedev
> > layer already.) queue_setup + queue_setup + dev_start + queue_setup +
> > queue_setup,
> 
> Because you can't do queue_setup() on already started queue.
> So if you do start() inside setup() second setup() should fail.
NO, because in queue_release, it will call queue_stop
And as I said before, it's better to move to queue_stop in ether layer, it's not an issue.
> 
> > queue_stop/start are handled inside queue_setup automatically after
> dev_started?
> 
> Again - I don't see any advantages to change existing API behavior and
> introduce implicit start/stop inside setup.
> It only introduce extra confusion for the users.
> So I still think we better keep existing behavior.
> Konstantin

OK, let me try again :)
I think the patch try to keep deferred setup independent of deferred start
Deferred setup does not necessary to imply a deferred start.
Which means
Queue_setup + dev_start  = dev_start + queue_setup
Queue_setup(deferred) + dev_start + queue_start = dev_start + queue_setup(deferred) + queue_start.
Queue_setup + dev_start + queue_setup(same queue) = dev_start + queue_setup + queue_setup(same queue)

But not
Queue_setup + dev_start = dev_start+ queue_setup + queue_start
Queue_setup(deffered) + dev_start +qeueu_start = dev_start+ queue_setup (ignore deferred)+ queue_start
Queue_setup + dev_start + queue_setup(same queue) = dev_start + queue_setup + queue_stop + queue_setup + queue_start.

I think option 1 have the pattern and easy to understand and option2 just add unnecessary queue_start/queue_stop and make deferred_start redundant at some situation.
> 
> > .
> > > And I don't see an good reason to break existing behavior.
I don't think it break any exist behavior, again deferred setup does not imply deferred start, because dev_start imply queue_start, and we follow this logic.

> > > What is the advantage of implicit call queue_start() implicitly from
> > > the queue_setup()/?
> > > Konstantin
> > >
> > > >
> > > > > Might be user doesn't want to start queue, might be he only
> > > > > wants to start it.
> > > > Use deferred_start_flag,
> > > > > Might be he would need to call queue_setup() once again later
> > > > > before starting it - based on some logic?
> > > > Dev_ops->queue_stop will be called first before
> > > > dev_ops->queue_setup in
> > > rte_eth_rx|tx_queue_setup, if a queue is running.
> > > >
> > > >
> > > >
> > > > > If the user wants to setup and start the queue immediately he
> > > > > can always
> > > do:
> > > > >
> > > > > rc = queue_setup(...);
> > > > > if (rc == 0)
> > > > >    queue_start(...);
> > > >
> > > > application no need to call queue_start explicitly in this case.
> > > >
> > > > >
> > > > > We have a pretty well defined API here let's keep it like that.
> > > > > Konstantin

Qi Zhang March 16, 2018, 2:15 p.m. UTC | #11

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Friday, March 16, 2018 9:18 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Friday, March 16, 2018 5:54 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Friday, March 16, 2018 12:52 AM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > thomas@monjalon.net
> > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > queue setup
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Thursday, March 15, 2018 11:22 PM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > <wenzhuo.lu@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > queue setup
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Qi Z
> > > > > Sent: Thursday, March 15, 2018 2:30 PM
> > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > thomas@monjalon.net
> > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > <wenzhuo.lu@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred
> > > > > queue setup
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ananyev, Konstantin
> > > > > > Sent: Thursday, March 15, 2018 9:23 PM
> > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > <wenzhuo.lu@intel.com>
> > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > deferred queue setup
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Zhang, Qi Z
> > > > > > > Sent: Thursday, March 15, 2018 3:22 AM
> > > > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > > > thomas@monjalon.net
> > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > > > > > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > <wenzhuo.lu@intel.com>
> > > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > > deferred queue setup
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ananyev, Konstantin
> > > > > > > > Sent: Wednesday, March 14, 2018 8:36 PM
> > > > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>;
> > > > > > > > thomas@monjalon.net
> > > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>;
> > > > > > > > Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > > <wenzhuo.lu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > > > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > > > deferred queue setup
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi
> > > > > > > > > Zhang
> > > > > > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > > > > > To: thomas@monjalon.net
> > > > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>;
> > > > > > > > > Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > > > > > Qi
> > > > > > > > > Z <qi.z.zhang@intel.com>
> > > > > > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > > > > deferred queue setup
> > > > > > > > >
> > > > > > > > > Expose the deferred queue configuration capability and
> > > > > > > > > enhance i40e_dev_[rx|tx]_queue_[setup|release] to handle
> > > > > > > > > the situation when device already started.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > b/drivers/net/i40e/i40e_ethdev.c index
> > > > > > > > > 06b0f03a1..843a0c42a
> > > > > > > > > 100644
> > > > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct
> > > > > > > > > rte_eth_dev
> > > > > > *dev,
> > > > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > > > +
> > > > > > > > >  	dev_info->hash_key_size =
> > (I40E_PFQF_HKEY_MAX_INDEX +
> > > > 1) *
> > > > > > > > >  						sizeof(uint32_t);
> > > > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > > > > > > a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > b/drivers/net/i40e/i40e_rxtx.c index
> > > > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> > > > > > rte_eth_dev
> > > > > > > > *dev,
> > > > > > > > >  	uint16_t len, i;
> > > > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > > > +	int ret = 0;
> > > > > > > > >
> > > > > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > > > > > > I40E_MAC_X722_VF) {
> > > > > > > > >  		vf =
> > > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> > > > > > rte_eth_dev
> > > > > > > > *dev,
> > > > > > > > >  			rxq->dcb_tc = i;
> > > > > > > > >  	}
> > > > > > > > >
> > > > > > > > > +	if (dev->data->dev_started) {
> > > > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > > > +				    "Failed to do RX queue initialization");
> > > > > > > > > +			return ret;
> > > > > > > > > +		}
> > > > > > > > > +		if (ad->rx_vec_allowed)
> > > > > > > >
> > > > > > > > Better to check what rx function is installed right now.
> > > > > > > Yes, it should be fixed, need to return fail if any conflict
> > > > > > > >
> > > > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > > > +			ret = i40e_dev_rx_queue_start(dev,
> queue_idx);
> > > > > > > >
> > > > > > > > I don't think it is a good idea to start/stop queue inside
> > > > > > > > queue_setup/queue_release.
> > > > > > > > There is special API (queue_start/queue_stop) to do this.
> > > > > > >
> > > > > > > The idea is if dev already started, the queue is supposed to
> > > > > > > be started
> > > > > > automatically after queue_setup.
> > > > > >
> > > > > > Why is that?
> > > > > Because device is already started, its like a running conveyor
> > > > > belt, anything
> > > > you put or replace on it just moves automatically.
> > > >
> > > > Why is that? :)
> > > > You do break existing behavior.
> > > > Right now it possible to do:
> > > > queue_setup(); queue_setup();
> > > > for the same queue.
> > > > With you patch is not any more
> > > Why not?
> > > I think with my patch,
> > > It assumes we can run below scenario on the same queue.
> > > (note, I assume queue_stop/start has been moved from i40e to ethedev
> > > layer already.) queue_setup + queue_setup + dev_start + queue_setup
> > > + queue_setup,
> >
> > Because you can't do queue_setup() on already started queue.
> > So if you do start() inside setup() second setup() should fail.
> NO, because in queue_release, it will call queue_stop And as I said before, it's
> better to move to queue_stop in ether layer, it's not an issue.
> >
> > > queue_stop/start are handled inside queue_setup automatically after
> > dev_started?
> >
> > Again - I don't see any advantages to change existing API behavior and
> > introduce implicit start/stop inside setup.
> > It only introduce extra confusion for the users.
> > So I still think we better keep existing behavior.
> > Konstantin
> 
> OK, let me try again :)
> I think the patch try to keep deferred setup independent of deferred start
> Deferred setup does not necessary to imply a deferred start.
> Which means
> Queue_setup + dev_start  = dev_start + queue_setup
> Queue_setup(deferred) + dev_start + queue_start = dev_start +
> queue_setup(deferred) + queue_start.
> Queue_setup + dev_start + queue_setup(same queue) = dev_start +
> queue_setup + queue_setup(same queue)
> 

One mistake for the third item, It should be 
Queue_setup + Queue_setup(same queue) + dev_start = queue_setup + dev_start + queue_setup(same queue)

> But not
> Queue_setup + dev_start = dev_start+ queue_setup + queue_start
> Queue_setup(deffered) + dev_start +qeueu_start = dev_start+ queue_setup
> (ignore deferred)+ queue_start Queue_setup + dev_start +
> queue_setup(same queue) = dev_start + queue_setup + queue_stop +
> queue_setup + queue_start.

Third item should be
Queue_setup + Queue_setup(same queue) + dev_start = queue_setup + dev_start + queue_stop + queue_setup(same queue) + queue_start
> 
> I think option 1 have the pattern and easy to understand and option2 just
> add unnecessary queue_start/queue_stop and make deferred_start
> redundant at some situation.
> >
> > > .
> > > > And I don't see an good reason to break existing behavior.
> I don't think it break any exist behavior, again deferred setup does not imply
> deferred start, because dev_start imply queue_start, and we follow this logic.
> 
> > > > What is the advantage of implicit call queue_start() implicitly
> > > > from the queue_setup()/?
> > > > Konstantin
> > > >
> > > > >
> > > > > > Might be user doesn't want to start queue, might be he only
> > > > > > wants to start it.
> > > > > Use deferred_start_flag,
> > > > > > Might be he would need to call queue_setup() once again later
> > > > > > before starting it - based on some logic?
> > > > > Dev_ops->queue_stop will be called first before
> > > > > dev_ops->queue_setup in
> > > > rte_eth_rx|tx_queue_setup, if a queue is running.
> > > > >
> > > > >
> > > > >
> > > > > > If the user wants to setup and start the queue immediately he
> > > > > > can always
> > > > do:
> > > > > >
> > > > > > rc = queue_setup(...);
> > > > > > if (rc == 0)
> > > > > >    queue_start(...);
> > > > >
> > > > > application no need to call queue_start explicitly in this case.
> > > > >
> > > > > >
> > > > > > We have a pretty well defined API here let's keep it like that.
> > > > > > Konstantin

Ananyev, Konstantin March 16, 2018, 6:47 p.m. UTC | #12

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Friday, March 16, 2018 2:15 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue setup
> 
> 
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qi
> > > > > > > > > > Zhang
> > > > > > > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > > > > > > To: thomas@monjalon.net
> > > > > > > > > > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>;
> > > > > > > > > > Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > > > > > > Qi
> > > > > > > > > > Z <qi.z.zhang@intel.com>
> > > > > > > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > > > > > deferred queue setup
> > > > > > > > > >
> > > > > > > > > > Expose the deferred queue configuration capability and
> > > > > > > > > > enhance i40e_dev_[rx|tx]_queue_[setup|release] to handle
> > > > > > > > > > the situation when device already started.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > b/drivers/net/i40e/i40e_ethdev.c index
> > > > > > > > > > 06b0f03a1..843a0c42a
> > > > > > > > > > 100644
> > > > > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct
> > > > > > > > > > rte_eth_dev
> > > > > > > *dev,
> > > > > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > > > > +
> > > > > > > > > >  	dev_info->hash_key_size =
> > > (I40E_PFQF_HKEY_MAX_INDEX +
> > > > > 1) *
> > > > > > > > > >  						sizeof(uint32_t);
> > > > > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff --git
> > > > > > > > > > a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > b/drivers/net/i40e/i40e_rxtx.c index
> > > > > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > @@ -1712,6 +1712,7 @@ i40e_dev_rx_queue_setup(struct
> > > > > > > rte_eth_dev
> > > > > > > > > *dev,
> > > > > > > > > >  	uint16_t len, i;
> > > > > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > > > > +	int ret = 0;
> > > > > > > > > >
> > > > > > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type ==
> > > > > > > > > I40E_MAC_X722_VF) {
> > > > > > > > > >  		vf =
> > > > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > > > > @@ -1841,6 +1842,25 @@ i40e_dev_rx_queue_setup(struct
> > > > > > > rte_eth_dev
> > > > > > > > > *dev,
> > > > > > > > > >  			rxq->dcb_tc = i;
> > > > > > > > > >  	}
> > > > > > > > > >
> > > > > > > > > > +	if (dev->data->dev_started) {
> > > > > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > > > > +				    "Failed to do RX queue initialization");
> > > > > > > > > > +			return ret;
> > > > > > > > > > +		}
> > > > > > > > > > +		if (ad->rx_vec_allowed)
> > > > > > > > >
> > > > > > > > > Better to check what rx function is installed right now.
> > > > > > > > Yes, it should be fixed, need to return fail if any conflict
> > > > > > > > >
> > > > > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > > > > +			ret = i40e_dev_rx_queue_start(dev,
> > queue_idx);
> > > > > > > > >
> > > > > > > > > I don't think it is a good idea to start/stop queue inside
> > > > > > > > > queue_setup/queue_release.
> > > > > > > > > There is special API (queue_start/queue_stop) to do this.
> > > > > > > >
> > > > > > > > The idea is if dev already started, the queue is supposed to
> > > > > > > > be started
> > > > > > > automatically after queue_setup.
> > > > > > >
> > > > > > > Why is that?
> > > > > > Because device is already started, its like a running conveyor
> > > > > > belt, anything
> > > > > you put or replace on it just moves automatically.
> > > > >
> > > > > Why is that? :)
> > > > > You do break existing behavior.
> > > > > Right now it possible to do:
> > > > > queue_setup(); queue_setup();
> > > > > for the same queue.
> > > > > With you patch is not any more
> > > > Why not?
> > > > I think with my patch,
> > > > It assumes we can run below scenario on the same queue.
> > > > (note, I assume queue_stop/start has been moved from i40e to ethedev
> > > > layer already.) queue_setup + queue_setup + dev_start + queue_setup
> > > > + queue_setup,
> > >
> > > Because you can't do queue_setup() on already started queue.
> > > So if you do start() inside setup() second setup() should fail.
> > NO, because in queue_release, it will call queue_stop And as I said before, it's
> > better to move to queue_stop in ether layer, it's not an issue.
> > >
> > > > queue_stop/start are handled inside queue_setup automatically after
> > > dev_started?
> > >
> > > Again - I don't see any advantages to change existing API behavior and
> > > introduce implicit start/stop inside setup.
> > > It only introduce extra confusion for the users.
> > > So I still think we better keep existing behavior.
> > > Konstantin
> >
> > OK, let me try again :)
> > I think the patch try to keep deferred setup independent of deferred start
> > Deferred setup does not necessary to imply a deferred start.

I don't understand what means 'deferred setup'.
We do have deferred_start for queue config, but it only used by dev_start().
Please, stop imply anything.
We have an API which is quite straightforward and does exactly what it states.

- queue_setup() - if queue is not started, then setup the queue.
- queue_start() - if queue is not started, then start the queue.
- queue_stop() - if queue is started, then stop the queue.
- dev_start() - in terms of queue behavior
    for all configured queues; do
        if queue->deferred_start != 0; then queue_start(queue);
     done

Let's keep it like that - nice and simple.
No need to introduce such no-sense as 'deferred setup' or implicit stop in start.
That just would add more mess and confusion.

> > Which means
> > Queue_setup + dev_start  = dev_start + queue_setup
> > Queue_setup(deferred) + dev_start + queue_start = dev_start +
> > queue_setup(deferred) + queue_start.
> > Queue_setup + dev_start + queue_setup(same queue) = dev_start +
> > queue_setup + queue_setup(same queue)
> >
> 
> One mistake for the third item, It should be
> Queue_setup + Queue_setup(same queue) + dev_start = queue_setup + dev_start + queue_setup(same queue)
> 
> > But not
> > Queue_setup + dev_start = dev_start+ queue_setup + queue_start
> > Queue_setup(deffered) + dev_start +qeueu_start = dev_start+ queue_setup
> > (ignore deferred)+ queue_start Queue_setup + dev_start +
> > queue_setup(same queue) = dev_start + queue_setup + queue_stop +
> > queue_setup + queue_start.
> 
> Third item should be
> Queue_setup + Queue_setup(same queue) + dev_start = queue_setup + dev_start + queue_stop + queue_setup(same queue) + queue_start
> >
> > I think option 1 have the pattern and easy to understand

I don't think so.
From my perspective it just introduce more confusion to the user. 

> and option2 just add unnecessary queue_start/queue_stop

Why unnecessary - if the user wants to start the queue - he/she calls queue_start(),
It is obvious, isn't it?

> and make deferred_start  redundant at some situation.

Deferred start is used only by dev_start, that's what it was intended for.
Let it stay that way.
BTW, we can get rid of it and add to dev_start() as a parameter
a list of queues to start (not to start)  - would be great.
But that's the matter of different discussion, I think.

Konstantin

> > >
> > > > .
> > > > > And I don't see an good reason to break existing behavior.
> > I don't think it break any exist behavior, again deferred setup does not imply
> > deferred start, because dev_start imply queue_start, and we follow this logic.
> >
> > > > > What is the advantage of implicit call queue_start() implicitly
> > > > > from the queue_setup()/?
> > > > > Konstantin
> > > > >
> > > > > >
> > > > > > > Might be user doesn't want to start queue, might be he only
> > > > > > > wants to start it.
> > > > > > Use deferred_start_flag,
> > > > > > > Might be he would need to call queue_setup() once again later
> > > > > > > before starting it - based on some logic?
> > > > > > Dev_ops->queue_stop will be called first before
> > > > > > dev_ops->queue_setup in
> > > > > rte_eth_rx|tx_queue_setup, if a queue is running.
> > > > > >
> > > > > >
> > > > > >
> > > > > > > If the user wants to setup and start the queue immediately he
> > > > > > > can always
> > > > > do:
> > > > > > >
> > > > > > > rc = queue_setup(...);
> > > > > > > if (rc == 0)
> > > > > > >    queue_start(...);
> > > > > >
> > > > > > application no need to call queue_start explicitly in this case.
> > > > > >
> > > > > > >
> > > > > > > We have a pretty well defined API here let's keep it like that.
> > > > > > > Konstantin

Qi Zhang March 18, 2018, 7:55 a.m. UTC | #13

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Saturday, March 17, 2018 2:48 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Friday, March 16, 2018 2:15 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > thomas@monjalon.net
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> > setup
> >
> >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> > > > > > > > > > > Qi Zhang
> > > > > > > > > > > Sent: Friday, March 2, 2018 4:13 AM
> > > > > > > > > > > To: thomas@monjalon.net
> > > > > > > > > > > Cc: dev@dpdk.org; Xing, Beilei
> > > > > > > > > > > <beilei.xing@intel.com>; Wu, Jingjing
> > > > > > > > > > > <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > > > > > > > > > <wenzhuo.lu@intel.com>; Zhang,
> > > > > > > > > > Qi
> > > > > > > > > > > Z <qi.z.zhang@intel.com>
> > > > > > > > > > > Subject: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable
> > > > > > > > > > > deferred queue setup
> > > > > > > > > > >
> > > > > > > > > > > Expose the deferred queue configuration capability
> > > > > > > > > > > and enhance i40e_dev_[rx|tx]_queue_[setup|release]
> > > > > > > > > > > to handle the situation when device already started.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > b/drivers/net/i40e/i40e_ethdev.c index
> > > > > > > > > > > 06b0f03a1..843a0c42a
> > > > > > > > > > > 100644
> > > > > > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct
> > > > > > > > > > > rte_eth_dev
> > > > > > > > *dev,
> > > > > > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > > > > > +
> > > > > > > > > > >  	dev_info->hash_key_size =
> > > > (I40E_PFQF_HKEY_MAX_INDEX +
> > > > > > 1) *
> > > > > > > > > > >  						sizeof(uint32_t);
> > > > > > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff
> > > > > > > > > > > --git a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > b/drivers/net/i40e/i40e_rxtx.c index
> > > > > > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > @@ -1712,6 +1712,7 @@
> i40e_dev_rx_queue_setup(struct
> > > > > > > > rte_eth_dev
> > > > > > > > > > *dev,
> > > > > > > > > > >  	uint16_t len, i;
> > > > > > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > > > > > +	int ret = 0;
> > > > > > > > > > >
> > > > > > > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type
> ==
> > > > > > > > > > I40E_MAC_X722_VF) {
> > > > > > > > > > >  		vf =
> > > > > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > > > > > @@ -1841,6 +1842,25 @@
> > > > > > > > > > > i40e_dev_rx_queue_setup(struct
> > > > > > > > rte_eth_dev
> > > > > > > > > > *dev,
> > > > > > > > > > >  			rxq->dcb_tc = i;
> > > > > > > > > > >  	}
> > > > > > > > > > >
> > > > > > > > > > > +	if (dev->data->dev_started) {
> > > > > > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > > > > > +				    "Failed to do RX queue
> initialization");
> > > > > > > > > > > +			return ret;
> > > > > > > > > > > +		}
> > > > > > > > > > > +		if (ad->rx_vec_allowed)
> > > > > > > > > >
> > > > > > > > > > Better to check what rx function is installed right now.
> > > > > > > > > Yes, it should be fixed, need to return fail if any
> > > > > > > > > conflict
> > > > > > > > > >
> > > > > > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > > > > > +			ret = i40e_dev_rx_queue_start(dev,
> > > queue_idx);
> > > > > > > > > >
> > > > > > > > > > I don't think it is a good idea to start/stop queue
> > > > > > > > > > inside queue_setup/queue_release.
> > > > > > > > > > There is special API (queue_start/queue_stop) to do this.
> > > > > > > > >
> > > > > > > > > The idea is if dev already started, the queue is
> > > > > > > > > supposed to be started
> > > > > > > > automatically after queue_setup.
> > > > > > > >
> > > > > > > > Why is that?
> > > > > > > Because device is already started, its like a running
> > > > > > > conveyor belt, anything
> > > > > > you put or replace on it just moves automatically.
> > > > > >
> > > > > > Why is that? :)
> > > > > > You do break existing behavior.
> > > > > > Right now it possible to do:
> > > > > > queue_setup(); queue_setup();
> > > > > > for the same queue.
> > > > > > With you patch is not any more
> > > > > Why not?
> > > > > I think with my patch,
> > > > > It assumes we can run below scenario on the same queue.
> > > > > (note, I assume queue_stop/start has been moved from i40e to
> > > > > ethedev layer already.) queue_setup + queue_setup + dev_start +
> > > > > queue_setup
> > > > > + queue_setup,
> > > >
> > > > Because you can't do queue_setup() on already started queue.
> > > > So if you do start() inside setup() second setup() should fail.
> > > NO, because in queue_release, it will call queue_stop And as I said
> > > before, it's better to move to queue_stop in ether layer, it's not an issue.
> > > >
> > > > > queue_stop/start are handled inside queue_setup automatically
> > > > > after
> > > > dev_started?
> > > >
> > > > Again - I don't see any advantages to change existing API behavior
> > > > and introduce implicit start/stop inside setup.
> > > > It only introduce extra confusion for the users.
> > > > So I still think we better keep existing behavior.
> > > > Konstantin
> > >
> > > OK, let me try again :)
> > > I think the patch try to keep deferred setup independent of deferred
> > > start Deferred setup does not necessary to imply a deferred start.
> 
> I don't understand what means 'deferred setup'.
> We do have deferred_start for queue config, but it only used by dev_start().

> Please, stop imply anything.
> We have an API which is quite straightforward and does exactly what it
> states.
> 
> - queue_setup() - if queue is not started, then setup the queue.
> - queue_start() - if queue is not started, then start the queue.
> - queue_stop() - if queue is started, then stop the queue.
> - dev_start() - in terms of queue behavior
>     for all configured queues; do
>         if queue->deferred_start != 0; then queue_start(queue);
>      done
> 
> Let's keep it like that - nice and simple.
Yes, let's keep it nice and simple at dev_ops layer,.
But etherdev layer should be more friendly to application, we need imply something.

For example, why we don't expose queue_release to ether layer, 
Why queue_setup imply a queue_release on a queue already be setup?
Shouldn't it return fail to warn user, that a queue can't be reconfigure without release if first?

I thinks it's the same pattern for why we have queue_stop / queue_start here.
if application want to setup a queue on a running device, of cause it want queue be started immediately (if not? It can use deferred_start)
if application want to re_setup a queue on a running device, of cause it want queue can be stopped first.
Why we set unnecessary barriers here?

> No need to introduce such no-sense as 'deferred setup' or implicit stop in
> start.
> That just would add more mess and confusion.
> 
> > > Which means
> > > Queue_setup + dev_start  = dev_start + queue_setup
> > > Queue_setup(deferred) + dev_start + queue_start = dev_start +
> > > queue_setup(deferred) + queue_start.
> > > Queue_setup + dev_start + queue_setup(same queue) = dev_start +
> > > queue_setup + queue_setup(same queue)
> > >
> >
> > One mistake for the third item, It should be Queue_setup +
> > Queue_setup(same queue) + dev_start = queue_setup + dev_start +
> > queue_setup(same queue)
> >
> > > But not
> > > Queue_setup + dev_start = dev_start+ queue_setup + queue_start
> > > Queue_setup(deffered) + dev_start +qeueu_start = dev_start+
> > > queue_setup (ignore deferred)+ queue_start Queue_setup + dev_start +
> > > queue_setup(same queue) = dev_start + queue_setup + queue_stop +
> > > queue_setup + queue_start.
> >
> > Third item should be
> > Queue_setup + Queue_setup(same queue) + dev_start = queue_setup +
> > dev_start + queue_stop + queue_setup(same queue) + queue_start
> > >
> > > I think option 1 have the pattern and easy to understand
> 
> I don't think so.
> From my perspective it just introduce more confusion to the user.

I can't agree this, actually it's quite simple to use the APIs.
User just need to remember, now, it's free to re-order queue_setup and dev_start, both call sequence reach the same destination. 
And if user does want to control queue start at specific time, just use deferred_start_flag and call queue_start explicitly as unusually, nothing changes
Actually I agree with what Bruce said:
"keeping existing behavior unless there is a compelling reason to change"
The patch does try to keep consistent behavior from user's view.

Regards
Qi
> 
> > and option2 just add unnecessary queue_start/queue_stop
> 
> Why unnecessary - if the user wants to start the queue - he/she calls
> queue_start(), It is obvious, isn't it?
> 
> > and make deferred_start  redundant at some situation.
> 
> Deferred start is used only by dev_start, that's what it was intended for.
> Let it stay that way.
> BTW, we can get rid of it and add to dev_start() as a parameter a list of
> queues to start (not to start)  - would be great.
> But that's the matter of different discussion, I think.
> 
> Konstantin
> 
> > > >
> > > > > .
> > > > > > And I don't see an good reason to break existing behavior.
> > > I don't think it break any exist behavior, again deferred setup does
> > > not imply deferred start, because dev_start imply queue_start, and we
> follow this logic.
> > >
> > > > > > What is the advantage of implicit call queue_start()
> > > > > > implicitly from the queue_setup()/?
> > > > > > Konstantin
> > > > > >
> > > > > > >
> > > > > > > > Might be user doesn't want to start queue, might be he
> > > > > > > > only wants to start it.
> > > > > > > Use deferred_start_flag,
> > > > > > > > Might be he would need to call queue_setup() once again
> > > > > > > > later before starting it - based on some logic?
> > > > > > > Dev_ops->queue_stop will be called first before
> > > > > > > dev_ops->queue_setup in
> > > > > > rte_eth_rx|tx_queue_setup, if a queue is running.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > If the user wants to setup and start the queue immediately
> > > > > > > > he can always
> > > > > > do:
> > > > > > > >
> > > > > > > > rc = queue_setup(...);
> > > > > > > > if (rc == 0)
> > > > > > > >    queue_start(...);
> > > > > > >
> > > > > > > application no need to call queue_start explicitly in this case.
> > > > > > >
> > > > > > > >
> > > > > > > > We have a pretty well defined API here let's keep it like that.
> > > > > > > > Konstantin

Ananyev, Konstantin March 20, 2018, 1:18 p.m. UTC | #14

> > > > > > > > > > > >
> > > > > > > > > > > > Expose the deferred queue configuration capability
> > > > > > > > > > > > and enhance i40e_dev_[rx|tx]_queue_[setup|release]
> > > > > > > > > > > > to handle the situation when device already started.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > > > > > > >  2 files changed, 66 insertions(+), 2 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > > b/drivers/net/i40e/i40e_ethdev.c index
> > > > > > > > > > > > 06b0f03a1..843a0c42a
> > > > > > > > > > > > 100644
> > > > > > > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct
> > > > > > > > > > > > rte_eth_dev
> > > > > > > > > *dev,
> > > > > > > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > > > > > > +
> > > > > > > > > > > >  	dev_info->hash_key_size =
> > > > > (I40E_PFQF_HKEY_MAX_INDEX +
> > > > > > > 1) *
> > > > > > > > > > > >  						sizeof(uint32_t);
> > > > > > > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff
> > > > > > > > > > > > --git a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > > b/drivers/net/i40e/i40e_rxtx.c index
> > > > > > > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > > @@ -1712,6 +1712,7 @@
> > i40e_dev_rx_queue_setup(struct
> > > > > > > > > rte_eth_dev
> > > > > > > > > > > *dev,
> > > > > > > > > > > >  	uint16_t len, i;
> > > > > > > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > > > > > > +	int ret = 0;
> > > > > > > > > > > >
> > > > > > > > > > > >  	if (hw->mac.type == I40E_MAC_VF || hw->mac.type
> > ==
> > > > > > > > > > > I40E_MAC_X722_VF) {
> > > > > > > > > > > >  		vf =
> > > > > > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > > > > > > @@ -1841,6 +1842,25 @@
> > > > > > > > > > > > i40e_dev_rx_queue_setup(struct
> > > > > > > > > rte_eth_dev
> > > > > > > > > > > *dev,
> > > > > > > > > > > >  			rxq->dcb_tc = i;
> > > > > > > > > > > >  	}
> > > > > > > > > > > >
> > > > > > > > > > > > +	if (dev->data->dev_started) {
> > > > > > > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > > > > > > +				    "Failed to do RX queue
> > initialization");
> > > > > > > > > > > > +			return ret;
> > > > > > > > > > > > +		}
> > > > > > > > > > > > +		if (ad->rx_vec_allowed)
> > > > > > > > > > >
> > > > > > > > > > > Better to check what rx function is installed right now.
> > > > > > > > > > Yes, it should be fixed, need to return fail if any
> > > > > > > > > > conflict
> > > > > > > > > > >
> > > > > > > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > > > > > > +			ret = i40e_dev_rx_queue_start(dev,
> > > > queue_idx);
> > > > > > > > > > >
> > > > > > > > > > > I don't think it is a good idea to start/stop queue
> > > > > > > > > > > inside queue_setup/queue_release.
> > > > > > > > > > > There is special API (queue_start/queue_stop) to do this.
> > > > > > > > > >
> > > > > > > > > > The idea is if dev already started, the queue is
> > > > > > > > > > supposed to be started
> > > > > > > > > automatically after queue_setup.
> > > > > > > > >
> > > > > > > > > Why is that?
> > > > > > > > Because device is already started, its like a running
> > > > > > > > conveyor belt, anything
> > > > > > > you put or replace on it just moves automatically.
> > > > > > >
> > > > > > > Why is that? :)
> > > > > > > You do break existing behavior.
> > > > > > > Right now it possible to do:
> > > > > > > queue_setup(); queue_setup();
> > > > > > > for the same queue.
> > > > > > > With you patch is not any more
> > > > > > Why not?
> > > > > > I think with my patch,
> > > > > > It assumes we can run below scenario on the same queue.
> > > > > > (note, I assume queue_stop/start has been moved from i40e to
> > > > > > ethedev layer already.) queue_setup + queue_setup + dev_start +
> > > > > > queue_setup
> > > > > > + queue_setup,
> > > > >
> > > > > Because you can't do queue_setup() on already started queue.
> > > > > So if you do start() inside setup() second setup() should fail.
> > > > NO, because in queue_release, it will call queue_stop And as I said
> > > > before, it's better to move to queue_stop in ether layer, it's not an issue.
> > > > >
> > > > > > queue_stop/start are handled inside queue_setup automatically
> > > > > > after
> > > > > dev_started?
> > > > >
> > > > > Again - I don't see any advantages to change existing API behavior
> > > > > and introduce implicit start/stop inside setup.
> > > > > It only introduce extra confusion for the users.
> > > > > So I still think we better keep existing behavior.
> > > > > Konstantin
> > > >
> > > > OK, let me try again :)
> > > > I think the patch try to keep deferred setup independent of deferred
> > > > start Deferred setup does not necessary to imply a deferred start.
> >
> > I don't understand what means 'deferred setup'.
> > We do have deferred_start for queue config, but it only used by dev_start().
> 
> > Please, stop imply anything.
> > We have an API which is quite straightforward and does exactly what it
> > states.
> >
> > - queue_setup() - if queue is not started, then setup the queue.
> > - queue_start() - if queue is not started, then start the queue.
> > - queue_stop() - if queue is started, then stop the queue.
> > - dev_start() - in terms of queue behavior
> >     for all configured queues; do
> >         if queue->deferred_start != 0; then queue_start(queue);
> >      done
> >
> > Let's keep it like that - nice and simple.
> Yes, let's keep it nice and simple at dev_ops layer,.
> But etherdev layer should be more friendly to application, we need imply something.
> 
> For example, why we don't expose queue_release to ether layer,
> Why queue_setup imply a queue_release on a queue already be setup?
> Shouldn't it return fail to warn user, that a queue can't be reconfigure without release if first?

If you think queue_release() should be a public API - submit and RFC for that, then we can discuss it.

> 
> I thinks it's the same pattern for why we have queue_stop / queue_start here.

Not really from my perspective.
setup/release - to setup/teardown internal queue structures.
start/stop - to start/stop RX/TX on that queues.

> if application want to setup a queue on a running device, of cause it want queue be started immediately

Some apps might, some might not.
Those who want to start the queue will call queue_start() - simple and straightforward.

> (if not? It can use deferred_start)

rte_eth_rxconf.deferred_start right now is used by one particular purpose:
uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */

Now you are trying to overload it with extra meaning:
Do not start queue with rte_eth_dev_start()
if device is already started don't start the queue from the queue_setup().

Looks very confusing to me, plus what is probably worse there is now no consistent behavior
between queue_setup() invoked before dev_start() and queue_setup() invoked after dev_start.
I would expect queue_setup() in both cases to preserve current behavior or at least be as close as
possible to it.

Current queue_setup behaves like that: 

queue_setup(queue)
{
   if (device is started)
       return with error; 
    if (queue is already setup)
         queue_release(queue);

   do_queue_setup(queue);
}

Preserving current behavior and introducing ability to setup queue for
already started device:

queue_setup(queue)
{
   if (queue is not stopped)
       return with error; 
    if (queue is already setup)
         queue_release(queue);

   do_queue_setup(queue);
}

What is proposed in your patch:

queue_setup(queue)
{
     if (queue is already setup) {
         /* via release */
         if (if device is started AND queue is not stopped)
            queue_stop(queue);

         queue_release(queue);
     }

   do_queue_setup(queue);

   if (device is started AND deferred_start for the queue is off)
     queue_start(queue); 
}

That looks quite different from current queue_setup() behavior plus
you introduce extra meaning for  rte_eth_rxconf.deferred_start.
All of that in not obvious to the user way.

I still don't see any good reason to change existing queue_setup()
behavior in a such significant way.
So my vote for the proposed new behavior is NACK.

If you really strongly feel that current queue_setup() functionality has to be overloaded
(what you propose is really queue_stop_setup_start) - then I think it should be first stated clearly
within RFC and discussed with the community. 
Same for overloading deferred_setup field.   

> if application want to re_setup a queue on a running device, of cause it want queue can be stopped first.
> Why we set unnecessary barriers here?
> 
> > No need to introduce such no-sense as 'deferred setup' or implicit stop in
> > start.
> > That just would add more mess and confusion.
> >
> > > > Which means
> > > > Queue_setup + dev_start  = dev_start + queue_setup
> > > > Queue_setup(deferred) + dev_start + queue_start = dev_start +
> > > > queue_setup(deferred) + queue_start.
> > > > Queue_setup + dev_start + queue_setup(same queue) = dev_start +
> > > > queue_setup + queue_setup(same queue)
> > > >
> > >
> > > One mistake for the third item, It should be Queue_setup +
> > > Queue_setup(same queue) + dev_start = queue_setup + dev_start +
> > > queue_setup(same queue)
> > >
> > > > But not
> > > > Queue_setup + dev_start = dev_start+ queue_setup + queue_start
> > > > Queue_setup(deffered) + dev_start +qeueu_start = dev_start+
> > > > queue_setup (ignore deferred)+ queue_start Queue_setup + dev_start +
> > > > queue_setup(same queue) = dev_start + queue_setup + queue_stop +
> > > > queue_setup + queue_start.
> > >
> > > Third item should be
> > > Queue_setup + Queue_setup(same queue) + dev_start = queue_setup +
> > > dev_start + queue_stop + queue_setup(same queue) + queue_start
> > > >
> > > > I think option 1 have the pattern and easy to understand
> >
> > I don't think so.
> > From my perspective it just introduce more confusion to the user.
> 
> I can't agree this, actually it's quite simple to use the APIs.
> User just need to remember, now, it's free to re-order queue_setup and dev_start, both call sequence reach the same destination.
> And if user does want to control queue start at specific time, just use deferred_start_flag and call queue_start explicitly as unusually,
> nothing changes
> Actually I agree with what Bruce said:
> "keeping existing behavior unless there is a compelling reason to change"
> The patch does try to keep consistent behavior from user's view.

It doesn't - that's the problem.
Konstantin

> 
> Regards
> Qi
> >
> > > and option2 just add unnecessary queue_start/queue_stop
> >
> > Why unnecessary - if the user wants to start the queue - he/she calls
> > queue_start(), It is obvious, isn't it?
> >
> > > and make deferred_start  redundant at some situation.
> >
> > Deferred start is used only by dev_start, that's what it was intended for.
> > Let it stay that way.
> > BTW, we can get rid of it and add to dev_start() as a parameter a list of
> > queues to start (not to start)  - would be great.
> > But that's the matter of different discussion, I think.
> >
> > Konstantin
> >
> > > > >
> > > > > > .
> > > > > > > And I don't see an good reason to break existing behavior.
> > > > I don't think it break any exist behavior, again deferred setup does
> > > > not imply deferred start, because dev_start imply queue_start, and we
> > follow this logic.
> > > >
> > > > > > > What is the advantage of implicit call queue_start()
> > > > > > > implicitly from the queue_setup()/?
> > > > > > > Konstantin
> > > > > > >
> > > > > > > >
> > > > > > > > > Might be user doesn't want to start queue, might be he
> > > > > > > > > only wants to start it.
> > > > > > > > Use deferred_start_flag,
> > > > > > > > > Might be he would need to call queue_setup() once again
> > > > > > > > > later before starting it - based on some logic?
> > > > > > > > Dev_ops->queue_stop will be called first before
> > > > > > > > dev_ops->queue_setup in
> > > > > > > rte_eth_rx|tx_queue_setup, if a queue is running.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > If the user wants to setup and start the queue immediately
> > > > > > > > > he can always
> > > > > > > do:
> > > > > > > > >
> > > > > > > > > rc = queue_setup(...);
> > > > > > > > > if (rc == 0)
> > > > > > > > >    queue_start(...);
> > > > > > > >
> > > > > > > > application no need to call queue_start explicitly in this case.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > We have a pretty well defined API here let's keep it like that.
> > > > > > > > > Konstantin

Qi Zhang March 21, 2018, 1:53 a.m. UTC | #15

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, March 20, 2018 9:19 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 4/4] net/i40e: enable deferred queue
> setup
> 
> 
> 
> > > > > > > > > > > > >
> > > > > > > > > > > > > Expose the deferred queue configuration
> > > > > > > > > > > > > capability and enhance
> > > > > > > > > > > > > i40e_dev_[rx|tx]_queue_[setup|release]
> > > > > > > > > > > > > to handle the situation when device already started.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  drivers/net/i40e/i40e_ethdev.c |  6 ++++
> > > > > > > > > > > > >  drivers/net/i40e/i40e_rxtx.c   | 62
> > > > > > > > > > > > ++++++++++++++++++++++++++++++++++++++++--
> > > > > > > > > > > > >  2 files changed, 66 insertions(+), 2
> > > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > > > b/drivers/net/i40e/i40e_ethdev.c index
> > > > > > > > > > > > > 06b0f03a1..843a0c42a
> > > > > > > > > > > > > 100644
> > > > > > > > > > > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > > > > > > > > > > @@ -3195,6 +3195,12 @@ i40e_dev_info_get(struct
> > > > > > > > > > > > > rte_eth_dev
> > > > > > > > > > *dev,
> > > > > > > > > > > > struct rte_eth_dev_info *dev_info)
> > > > > > > > > > > > >  		DEV_TX_OFFLOAD_GRE_TNL_TSO |
> > > > > > > > > > > > >  		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
> > > > > > > > > > > > >  		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
> > > > > > > > > > > > > +	dev_info->deferred_queue_config_capa =
> > > > > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_SETUP |
> > > > > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_SETUP |
> > > > > > > > > > > > > +		DEV_DEFERRED_RX_QUEUE_RELEASE |
> > > > > > > > > > > > > +		DEV_DEFERRED_TX_QUEUE_RELEASE;
> > > > > > > > > > > > > +
> > > > > > > > > > > > >  	dev_info->hash_key_size =
> > > > > > (I40E_PFQF_HKEY_MAX_INDEX +
> > > > > > > > 1) *
> > > > > > > > > > > > >  						sizeof(uint32_t);
> > > > > > > > > > > > >  	dev_info->reta_size = pf->hash_lut_size; diff
> > > > > > > > > > > > > --git a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > > > b/drivers/net/i40e/i40e_rxtx.c index
> > > > > > > > > > > > > 1217e5a61..e5f532cf7 100644
> > > > > > > > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > > > > > > > @@ -1712,6 +1712,7 @@
> > > i40e_dev_rx_queue_setup(struct
> > > > > > > > > > rte_eth_dev
> > > > > > > > > > > > *dev,
> > > > > > > > > > > > >  	uint16_t len, i;
> > > > > > > > > > > > >  	uint16_t reg_idx, base, bsf, tc_mapping;
> > > > > > > > > > > > >  	int q_offset, use_def_burst_func = 1;
> > > > > > > > > > > > > +	int ret = 0;
> > > > > > > > > > > > >
> > > > > > > > > > > > >  	if (hw->mac.type == I40E_MAC_VF ||
> > > > > > > > > > > > > hw->mac.type
> > > ==
> > > > > > > > > > > > I40E_MAC_X722_VF) {
> > > > > > > > > > > > >  		vf =
> > > > > > > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > > > > > > > > > > > > @@ -1841,6 +1842,25 @@
> > > > > > > > > > > > > i40e_dev_rx_queue_setup(struct
> > > > > > > > > > rte_eth_dev
> > > > > > > > > > > > *dev,
> > > > > > > > > > > > >  			rxq->dcb_tc = i;
> > > > > > > > > > > > >  	}
> > > > > > > > > > > > >
> > > > > > > > > > > > > +	if (dev->data->dev_started) {
> > > > > > > > > > > > > +		ret = i40e_rx_queue_init(rxq);
> > > > > > > > > > > > > +		if (ret != I40E_SUCCESS) {
> > > > > > > > > > > > > +			PMD_DRV_LOG(ERR,
> > > > > > > > > > > > > +				    "Failed to do RX queue
> > > initialization");
> > > > > > > > > > > > > +			return ret;
> > > > > > > > > > > > > +		}
> > > > > > > > > > > > > +		if (ad->rx_vec_allowed)
> > > > > > > > > > > >
> > > > > > > > > > > > Better to check what rx function is installed right now.
> > > > > > > > > > > Yes, it should be fixed, need to return fail if any
> > > > > > > > > > > conflict
> > > > > > > > > > > >
> > > > > > > > > > > > > +			i40e_rxq_vec_setup(rxq);
> > > > > > > > > > > > > +		if (!rxq->rx_deferred_start) {
> > > > > > > > > > > > > +			ret = i40e_dev_rx_queue_start(dev,
> > > > > queue_idx);
> > > > > > > > > > > >
> > > > > > > > > > > > I don't think it is a good idea to start/stop
> > > > > > > > > > > > queue inside queue_setup/queue_release.
> > > > > > > > > > > > There is special API (queue_start/queue_stop) to do
> this.
> > > > > > > > > > >
> > > > > > > > > > > The idea is if dev already started, the queue is
> > > > > > > > > > > supposed to be started
> > > > > > > > > > automatically after queue_setup.
> > > > > > > > > >
> > > > > > > > > > Why is that?
> > > > > > > > > Because device is already started, its like a running
> > > > > > > > > conveyor belt, anything
> > > > > > > > you put or replace on it just moves automatically.
> > > > > > > >
> > > > > > > > Why is that? :)
> > > > > > > > You do break existing behavior.
> > > > > > > > Right now it possible to do:
> > > > > > > > queue_setup(); queue_setup(); for the same queue.
> > > > > > > > With you patch is not any more
> > > > > > > Why not?
> > > > > > > I think with my patch,
> > > > > > > It assumes we can run below scenario on the same queue.
> > > > > > > (note, I assume queue_stop/start has been moved from i40e to
> > > > > > > ethedev layer already.) queue_setup + queue_setup +
> > > > > > > dev_start + queue_setup
> > > > > > > + queue_setup,
> > > > > >
> > > > > > Because you can't do queue_setup() on already started queue.
> > > > > > So if you do start() inside setup() second setup() should fail.
> > > > > NO, because in queue_release, it will call queue_stop And as I
> > > > > said before, it's better to move to queue_stop in ether layer, it's not
> an issue.
> > > > > >
> > > > > > > queue_stop/start are handled inside queue_setup
> > > > > > > automatically after
> > > > > > dev_started?
> > > > > >
> > > > > > Again - I don't see any advantages to change existing API
> > > > > > behavior and introduce implicit start/stop inside setup.
> > > > > > It only introduce extra confusion for the users.
> > > > > > So I still think we better keep existing behavior.
> > > > > > Konstantin
> > > > >
> > > > > OK, let me try again :)
> > > > > I think the patch try to keep deferred setup independent of
> > > > > deferred start Deferred setup does not necessary to imply a deferred
> start.
> > >
> > > I don't understand what means 'deferred setup'.
> > > We do have deferred_start for queue config, but it only used by
> dev_start().
> >
> > > Please, stop imply anything.
> > > We have an API which is quite straightforward and does exactly what
> > > it states.
> > >
> > > - queue_setup() - if queue is not started, then setup the queue.
> > > - queue_start() - if queue is not started, then start the queue.
> > > - queue_stop() - if queue is started, then stop the queue.
> > > - dev_start() - in terms of queue behavior
> > >     for all configured queues; do
> > >         if queue->deferred_start != 0; then queue_start(queue);
> > >      done
> > >
> > > Let's keep it like that - nice and simple.
> > Yes, let's keep it nice and simple at dev_ops layer,.
> > But etherdev layer should be more friendly to application, we need imply
> something.
> >
> > For example, why we don't expose queue_release to ether layer, Why
> > queue_setup imply a queue_release on a queue already be setup?
> > Shouldn't it return fail to warn user, that a queue can't be reconfigure
> without release if first?
> 
> If you think queue_release() should be a public API - submit and RFC for that,
> then we can discuss it.
> 
> >
> > I thinks it's the same pattern for why we have queue_stop / queue_start
> here.
> 
> Not really from my perspective.
> setup/release - to setup/teardown internal queue structures.
> start/stop - to start/stop RX/TX on that queues.
> 
> > if application want to setup a queue on a running device, of cause it
> > want queue be started immediately
> 
> Some apps might, some might not.
> Those who want to start the queue will call queue_start() - simple and
> straightforward.
> 
> > (if not? It can use deferred_start)
> 
> rte_eth_rxconf.deferred_start right now is used by one particular purpose:
> uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start().
> */

> 
> Now you are trying to overload it with extra meaning:
Yes, based on exist comment, deferred_start is overloaded.
> Do not start queue with rte_eth_dev_start() if device is already started don't
> start the queue from the queue_setup().
This is correct but also could be explained in a simple way.
deferred_start=0: queue will be started automatically when device is started.
deferred_start=1: queue can only be started by queue_start manually. 
maybe "no_auto_start" could be a better name.
> 
> Looks very confusing to me, plus what is probably worse there is now no
> consistent behavior between queue_setup() invoked before dev_start() and
> queue_setup() invoked after dev_start.
> I would expect queue_setup() in both cases to preserve current behavior or
> at least be as close as possible to it.
> 
> Current queue_setup behaves like that:
> 
> queue_setup(queue)
> {
>    if (device is started)
>        return with error;
>     if (queue is already setup)
>          queue_release(queue);
> 
>    do_queue_setup(queue);
> }
> 
> Preserving current behavior and introducing ability to setup queue for
> already started device:
> 
> queue_setup(queue)
> {
>    if (queue is not stopped)
>        return with error;
>     if (queue is already setup)
>          queue_release(queue);
> 
>    do_queue_setup(queue);
> }
> 
> What is proposed in your patch:
> 
> queue_setup(queue)
> {
>      if (queue is already setup) {
>          /* via release */
>          if (if device is started AND queue is not stopped)
>             queue_stop(queue);
> 
>          queue_release(queue);
>      }
> 
>    do_queue_setup(queue);
> 
>    if (device is started AND deferred_start for the queue is off)
>      queue_start(queue);
> }
> 
> That looks quite different from current queue_setup() behavior plus you
> introduce extra meaning for  rte_eth_rxconf.deferred_start.
> All of that in not obvious to the user way.
> 
> I still don't see any good reason to change existing queue_setup() behavior in
> a such significant way.
> So my vote for the proposed new behavior is NACK.
> 
> If you really strongly feel that current queue_setup() functionality has to be
> overloaded (what you propose is really queue_stop_setup_start) - then I
> think it should be first stated clearly within RFC and discussed with the
> community.
> Same for overloading deferred_setup field.
OK, I will consider this on a separate RFC patch, I don't think involve auto start/stop in the queue_setup context bring any trouble,  
To me it simplify application's code, just like we don't need an additional queue_start call after queue_setup / dev_start,
since queue could be configure auto started at queue_setup.

Regards
Qi
> 
> > if application want to re_setup a queue on a running device, of cause it
> want queue can be stopped first.
> > Why we set unnecessary barriers here?
> >
> > > No need to introduce such no-sense as 'deferred setup' or implicit
> > > stop in start.
> > > That just would add more mess and confusion.
> > >
> > > > > Which means
> > > > > Queue_setup + dev_start  = dev_start + queue_setup
> > > > > Queue_setup(deferred) + dev_start + queue_start = dev_start +
> > > > > queue_setup(deferred) + queue_start.
> > > > > Queue_setup + dev_start + queue_setup(same queue) = dev_start +
> > > > > queue_setup + queue_setup(same queue)
> > > > >
> > > >
> > > > One mistake for the third item, It should be Queue_setup +
> > > > Queue_setup(same queue) + dev_start = queue_setup + dev_start +
> > > > queue_setup(same queue)
> > > >
> > > > > But not
> > > > > Queue_setup + dev_start = dev_start+ queue_setup + queue_start
> > > > > Queue_setup(deffered) + dev_start +qeueu_start = dev_start+
> > > > > queue_setup (ignore deferred)+ queue_start Queue_setup +
> > > > > dev_start + queue_setup(same queue) = dev_start + queue_setup +
> > > > > queue_stop + queue_setup + queue_start.
> > > >
> > > > Third item should be
> > > > Queue_setup + Queue_setup(same queue) + dev_start = queue_setup
> +
> > > > dev_start + queue_stop + queue_setup(same queue) + queue_start
> > > > >
> > > > > I think option 1 have the pattern and easy to understand
> > >
> > > I don't think so.
> > > From my perspective it just introduce more confusion to the user.
> >
> > I can't agree this, actually it's quite simple to use the APIs.
> > User just need to remember, now, it's free to re-order queue_setup and
> dev_start, both call sequence reach the same destination.
> > And if user does want to control queue start at specific time, just
> > use deferred_start_flag and call queue_start explicitly as unusually,
> > nothing changes Actually I agree with what Bruce said:
> > "keeping existing behavior unless there is a compelling reason to change"
> > The patch does try to keep consistent behavior from user's view.
> 
> It doesn't - that's the problem.
> Konstantin
> 
> >
> > Regards
> > Qi
> > >
> > > > and option2 just add unnecessary queue_start/queue_stop
> > >
> > > Why unnecessary - if the user wants to start the queue - he/she
> > > calls queue_start(), It is obvious, isn't it?
> > >
> > > > and make deferred_start  redundant at some situation.
> > >
> > > Deferred start is used only by dev_start, that's what it was intended for.
> > > Let it stay that way.
> > > BTW, we can get rid of it and add to dev_start() as a parameter a
> > > list of queues to start (not to start)  - would be great.
> > > But that's the matter of different discussion, I think.
> > >
> > > Konstantin
> > >
> > > > > >
> > > > > > > .
> > > > > > > > And I don't see an good reason to break existing behavior.
> > > > > I don't think it break any exist behavior, again deferred setup
> > > > > does not imply deferred start, because dev_start imply
> > > > > queue_start, and we
> > > follow this logic.
> > > > >
> > > > > > > > What is the advantage of implicit call queue_start()
> > > > > > > > implicitly from the queue_setup()/?
> > > > > > > > Konstantin
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > Might be user doesn't want to start queue, might be he
> > > > > > > > > > only wants to start it.
> > > > > > > > > Use deferred_start_flag,
> > > > > > > > > > Might be he would need to call queue_setup() once
> > > > > > > > > > again later before starting it - based on some logic?
> > > > > > > > > Dev_ops->queue_stop will be called first before
> > > > > > > > > dev_ops->queue_setup in
> > > > > > > > rte_eth_rx|tx_queue_setup, if a queue is running.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > If the user wants to setup and start the queue
> > > > > > > > > > immediately he can always
> > > > > > > > do:
> > > > > > > > > >
> > > > > > > > > > rc = queue_setup(...); if (rc == 0)
> > > > > > > > > >    queue_start(...);
> > > > > > > > >
> > > > > > > > > application no need to call queue_start explicitly in this case.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > We have a pretty well defined API here let's keep it like that.
> > > > > > > > > > Konstantin

diff mbox

Patch

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 06b0f03a1..843a0c42a 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3195,6 +3195,12 @@  i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		DEV_TX_OFFLOAD_GRE_TNL_TSO |
 		DEV_TX_OFFLOAD_IPIP_TNL_TSO |
 		DEV_TX_OFFLOAD_GENEVE_TNL_TSO;
+	dev_info->deferred_queue_config_capa =
+		DEV_DEFERRED_RX_QUEUE_SETUP |
+		DEV_DEFERRED_TX_QUEUE_SETUP |
+		DEV_DEFERRED_RX_QUEUE_RELEASE |
+		DEV_DEFERRED_TX_QUEUE_RELEASE;
+
 	dev_info->hash_key_size = (I40E_PFQF_HKEY_MAX_INDEX + 1) *
 						sizeof(uint32_t);
 	dev_info->reta_size = pf->hash_lut_size;
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 1217e5a61..e5f532cf7 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1712,6 +1712,7 @@  i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	uint16_t len, i;
 	uint16_t reg_idx, base, bsf, tc_mapping;
 	int q_offset, use_def_burst_func = 1;
+	int ret = 0;
 
 	if (hw->mac.type == I40E_MAC_VF || hw->mac.type == I40E_MAC_X722_VF) {
 		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
@@ -1841,6 +1842,25 @@  i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
 			rxq->dcb_tc = i;
 	}
 
+	if (dev->data->dev_started) {
+		ret = i40e_rx_queue_init(rxq);
+		if (ret != I40E_SUCCESS) {
+			PMD_DRV_LOG(ERR,
+				    "Failed to do RX queue initialization");
+			return ret;
+		}
+		if (ad->rx_vec_allowed)
+			i40e_rxq_vec_setup(rxq);
+		if (!rxq->rx_deferred_start) {
+			ret = i40e_dev_rx_queue_start(dev, queue_idx);
+			if (ret != I40E_SUCCESS) {
+				PMD_DRV_LOG(ERR,
+					    "Failed to start RX queue");
+				return ret;
+			}
+		}
+	}
+
 	return 0;
 }
 
@@ -1848,13 +1868,21 @@  void
 i40e_dev_rx_queue_release(void *rxq)
 {
 	struct i40e_rx_queue *q = (struct i40e_rx_queue *)rxq;
+	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
 
 	if (!q) {
 		PMD_DRV_LOG(DEBUG, "Pointer to rxq is NULL");
 		return;
 	}
 
-	i40e_rx_queue_release_mbufs(q);
+	if (dev->data->dev_started) {
+		if (dev->data->rx_queue_state[q->queue_id] ==
+			RTE_ETH_QUEUE_STATE_STARTED)
+			i40e_dev_rx_queue_stop(dev, q->queue_id);
+	} else {
+		i40e_rx_queue_release_mbufs(q);
+	}
+
 	rte_free(q->sw_ring);
 	rte_free(q);
 }
@@ -1980,6 +2008,8 @@  i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
 			const struct rte_eth_txconf *tx_conf)
 {
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_adapter *ad =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	struct i40e_vsi *vsi;
 	struct i40e_pf *pf = NULL;
 	struct i40e_vf *vf = NULL;
@@ -1989,6 +2019,7 @@  i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
 	uint16_t tx_rs_thresh, tx_free_thresh;
 	uint16_t reg_idx, i, base, bsf, tc_mapping;
 	int q_offset;
+	int ret = 0;
 
 	if (hw->mac.type == I40E_MAC_VF || hw->mac.type == I40E_MAC_X722_VF) {
 		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
@@ -2162,6 +2193,25 @@  i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
 			txq->dcb_tc = i;
 	}
 
+	if (dev->data->dev_started) {
+		ret = i40e_tx_queue_init(txq);
+		if (ret != I40E_SUCCESS) {
+			PMD_DRV_LOG(ERR,
+				    "Failed to do TX queue initialization");
+			return ret;
+		}
+		if (ad->tx_vec_allowed)
+			i40e_txq_vec_setup(txq);
+		if (!txq->tx_deferred_start) {
+			ret = i40e_dev_tx_queue_start(dev, queue_idx);
+			if (ret != I40E_SUCCESS) {
+				PMD_DRV_LOG(ERR,
+					    "Failed to start TX queue");
+				return ret;
+			}
+		}
+	}
+
 	return 0;
 }
 
@@ -2169,13 +2219,21 @@  void
 i40e_dev_tx_queue_release(void *txq)
 {
 	struct i40e_tx_queue *q = (struct i40e_tx_queue *)txq;
+	struct rte_eth_dev *dev = &rte_eth_devices[q->port_id];
 
 	if (!q) {
 		PMD_DRV_LOG(DEBUG, "Pointer to TX queue is NULL");
 		return;
 	}
 
-	i40e_tx_queue_release_mbufs(q);
+	if (dev->data->dev_started) {
+		if (dev->data->tx_queue_state[q->queue_id] ==
+			RTE_ETH_QUEUE_STATE_STARTED)
+			i40e_dev_tx_queue_stop(dev, q->queue_id);
+	} else {
+		i40e_tx_queue_release_mbufs(q);
+	}
+
 	rte_free(q->sw_ring);
 	rte_free(q);
 }