Hi Pablo,
Few comments from me, please see below.
Konstantin
> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Pablo de Lara
> Sent: Wednesday, September 16, 2015 12:21 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in
> rte_eth_dev_data
>
> Following the same approach taken with dev_started field
> in rte_eth_dev_data structure, this patch adds two new fields
> in it, rx_queue_state and tx_queue_state arrays, which track
> which queues have been started and which not.
>
> This is important to avoid trying to start/stop twice a queue,
> which will result in undefined behaviour
> (which may cause RX/TX disruption).
>
> Mind that only the PMDs which have queue_start/stop functions
> have been changed to update this field, as the functions will
> check the queue state before switching it.
>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch at intel.com>
> ---
> drivers/net/cxgbe/cxgbe_ethdev.c | 29 +++++++++++++++++++++++++----
> drivers/net/enic/enic_ethdev.c | 6 ++++++
> drivers/net/fm10k/fm10k_ethdev.c | 4 ++++
> drivers/net/i40e/i40e_ethdev_vf.c | 6 ++++++
> drivers/net/i40e/i40e_rxtx.c | 7 ++++++-
> drivers/net/ixgbe/ixgbe_rxtx.c | 4 ++++
> lib/librte_ether/rte_ethdev.c | 28 ++++++++++++++++++++++++++++
> lib/librte_ether/rte_ethdev.h | 4 ++++
> 8 files changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c
> b/drivers/net/cxgbe/cxgbe_ethdev.c
> index 478051a..6ee4973 100644
> --- a/drivers/net/cxgbe/cxgbe_ethdev.c
> +++ b/drivers/net/cxgbe/cxgbe_ethdev.c
> @@ -368,23 +368,33 @@ static int cxgbe_dev_configure(struct rte_eth_dev
> *eth_dev)
> static int cxgbe_dev_tx_queue_start(struct rte_eth_dev *eth_dev,
> uint16_t tx_queue_id)
> {
> + int ret;
> struct sge_eth_txq *txq = (struct sge_eth_txq *)
> (eth_dev->data->tx_queues[tx_queue_id]);
>
> dev_debug(NULL, "%s: tx_queue_id = %d\n", __func__, tx_queue_id);
>
> - return t4_sge_eth_txq_start(txq);
> + ret = t4_sge_eth_txq_start(txq);
> + if (ret == 0)
> + eth_dev->data->tx_queue_state[tx_queue_id] = 1;
> +
> + return ret;
> }
>
> static int cxgbe_dev_tx_queue_stop(struct rte_eth_dev *eth_dev,
> uint16_t tx_queue_id)
> {
> + int ret;
> struct sge_eth_txq *txq = (struct sge_eth_txq *)
> (eth_dev->data->tx_queues[tx_queue_id]);
>
> dev_debug(NULL, "%s: tx_queue_id = %d\n", __func__, tx_queue_id);
>
> - return t4_sge_eth_txq_stop(txq);
> + ret = t4_sge_eth_txq_stop(txq);
> + if (ret == 0)
> + eth_dev->data->tx_queue_state[tx_queue_id] = 0;
> +
> + return ret;
> }
>
> static int cxgbe_dev_tx_queue_setup(struct rte_eth_dev *eth_dev,
> @@ -460,6 +470,7 @@ static void cxgbe_dev_tx_queue_release(void *q)
> static int cxgbe_dev_rx_queue_start(struct rte_eth_dev *eth_dev,
> uint16_t rx_queue_id)
> {
> + int ret;
> struct port_info *pi = (struct port_info *)(eth_dev->data->dev_private);
> struct adapter *adap = pi->adapter;
> struct sge_rspq *q;
> @@ -468,12 +479,18 @@ static int cxgbe_dev_rx_queue_start(struct rte_eth_dev
> *eth_dev,
> __func__, pi->port_id, rx_queue_id);
>
> q = eth_dev->data->rx_queues[rx_queue_id];
> - return t4_sge_eth_rxq_start(adap, q);
> +
> + ret = t4_sge_eth_rxq_start(adap, q);
> + if (ret == 0)
> + eth_dev->data->rx_queue_state[rx_queue_id] = 1;
> +
> + return ret;
> }
>
> static int cxgbe_dev_rx_queue_stop(struct rte_eth_dev *eth_dev,
> uint16_t rx_queue_id)
> {
> + int ret;
> struct port_info *pi = (struct port_info *)(eth_dev->data->dev_private);
> struct adapter *adap = pi->adapter;
> struct sge_rspq *q;
> @@ -482,7 +499,11 @@ static int cxgbe_dev_rx_queue_stop(struct rte_eth_dev
> *eth_dev,
> __func__, pi->port_id, rx_queue_id);
>
> q = eth_dev->data->rx_queues[rx_queue_id];
> - return t4_sge_eth_rxq_stop(adap, q);
> + ret = t4_sge_eth_rxq_stop(adap, q);
> + if (ret == 0)
> + eth_dev->data->rx_queue_state[rx_queue_id] = 0;
> +
> + return ret;
> }
>
> static int cxgbe_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index 8280cea..d646a00 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -194,6 +194,7 @@ static int enicpmd_dev_tx_queue_start(struct rte_eth_dev
> *eth_dev,
> ENICPMD_FUNC_TRACE();
>
> enic_start_wq(enic, queue_idx);
> + eth_dev->data->tx_queue_state[queue_idx] = 1;
>
> return 0;
> }
> @@ -209,6 +210,8 @@ static int enicpmd_dev_tx_queue_stop(struct rte_eth_dev
> *eth_dev,
> ret = enic_stop_wq(enic, queue_idx);
> if (ret)
> dev_err(enic, "error in stopping wq %d\n", queue_idx);
> + else
> + eth_dev->data->tx_queue_state[queue_idx] = 0;
>
> return ret;
> }
> @@ -221,6 +224,7 @@ static int enicpmd_dev_rx_queue_start(struct rte_eth_dev
> *eth_dev,
> ENICPMD_FUNC_TRACE();
>
> enic_start_rq(enic, queue_idx);
> + eth_dev->data->rx_queue_state[queue_idx] = 1;
>
> return 0;
> }
> @@ -236,6 +240,8 @@ static int enicpmd_dev_rx_queue_stop(struct rte_eth_dev
> *eth_dev,
> ret = enic_stop_rq(enic, queue_idx);
> if (ret)
> dev_err(enic, "error in stopping rq %d\n", queue_idx);
> + else
> + eth_dev->data->rx_queue_state[queue_idx] = 0;
>
> return ret;
> }
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> b/drivers/net/fm10k/fm10k_ethdev.c
> index a69c990..1a38223 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -524,6 +524,7 @@ fm10k_dev_rx_queue_start(struct rte_eth_dev *dev,
> uint16_t rx_queue_id)
> */
> FM10K_WRITE_REG(hw, FM10K_RDH(rx_queue_id), 0);
> FM10K_WRITE_REG(hw, FM10K_RDT(rx_queue_id), rxq->nb_desc - 1);
> + dev->data->rx_queue_state[rx_queue_id] = 1;
> }
>
> return err;
> @@ -542,6 +543,7 @@ fm10k_dev_rx_queue_stop(struct rte_eth_dev *dev, uint16_t
> rx_queue_id)
>
> /* Free mbuf and clean HW ring */
> rx_queue_clean(dev->data->rx_queues[rx_queue_id]);
> + dev->data->rx_queue_state[rx_queue_id] = 0;
> }
>
> return 0;
> @@ -569,6 +571,7 @@ fm10k_dev_tx_queue_start(struct rte_eth_dev *dev,
> uint16_t tx_queue_id)
> FM10K_WRITE_REG(hw, FM10K_TXDCTL(tx_queue_id),
> FM10K_TXDCTL_ENABLE | txdctl);
> FM10K_WRITE_FLUSH(hw);
> + dev->data->tx_queue_state[tx_queue_id] = 1;
> } else
> err = -1;
>
> @@ -585,6 +588,7 @@ fm10k_dev_tx_queue_stop(struct rte_eth_dev *dev, uint16_t
> tx_queue_id)
> if (tx_queue_id < dev->data->nb_tx_queues) {
> tx_queue_disable(hw, tx_queue_id);
> tx_queue_clean(dev->data->tx_queues[tx_queue_id]);
> + dev->data->tx_queue_state[tx_queue_id] = 0;
> }
>
> return 0;
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> b/drivers/net/i40e/i40e_ethdev_vf.c
> index b694400..13e4e6e 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -1373,6 +1373,8 @@ i40evf_dev_rx_queue_start(struct rte_eth_dev *dev,
> uint16_t rx_queue_id)
> if (err)
> PMD_DRV_LOG(ERR, "Failed to switch RX queue %u on",
> rx_queue_id);
> + else
> + dev->data->rx_queue_state[rx_queue_id] = 1;
> }
>
> return err;
> @@ -1397,6 +1399,7 @@ i40evf_dev_rx_queue_stop(struct rte_eth_dev *dev,
> uint16_t rx_queue_id)
>
> i40e_rx_queue_release_mbufs(rxq);
> i40e_reset_rx_queue(rxq);
> + dev->data->rx_queue_state[rx_queue_id] = 0;
> }
>
> return 0;
> @@ -1417,6 +1420,8 @@ i40evf_dev_tx_queue_start(struct rte_eth_dev *dev,
> uint16_t tx_queue_id)
> if (err)
> PMD_DRV_LOG(ERR, "Failed to switch TX queue %u on",
> tx_queue_id);
> + else
> + dev->data->tx_queue_state[tx_queue_id] = 1;
> }
>
> return err;
> @@ -1441,6 +1446,7 @@ i40evf_dev_tx_queue_stop(struct rte_eth_dev *dev,
> uint16_t tx_queue_id)
>
> i40e_tx_queue_release_mbufs(txq);
> i40e_reset_tx_queue(txq);
> + dev->data->tx_queue_state[tx_queue_id] = 0;
> }
>
> return 0;
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index fd656d5..43c936c 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -2009,7 +2009,8 @@ i40e_dev_rx_queue_start(struct rte_eth_dev *dev,
> uint16_t rx_queue_id)
>
> i40e_rx_queue_release_mbufs(rxq);
> i40e_reset_rx_queue(rxq);
> - }
> + } else
> + dev->data->rx_queue_state[rx_queue_id] = 1;
> }
>
> return err;
> @@ -2038,6 +2039,7 @@ i40e_dev_rx_queue_stop(struct rte_eth_dev *dev,
> uint16_t rx_queue_id)
> }
> i40e_rx_queue_release_mbufs(rxq);
> i40e_reset_rx_queue(rxq);
> + dev->data->rx_queue_state[rx_queue_id] = 0;
> }
>
> return 0;
> @@ -2063,6 +2065,8 @@ i40e_dev_tx_queue_start(struct rte_eth_dev *dev,
> uint16_t tx_queue_id)
> if (err)
> PMD_DRV_LOG(ERR, "Failed to switch TX queue %u on",
> tx_queue_id);
> + else
> + dev->data->tx_queue_state[tx_queue_id] = 1;
> }
>
> return err;
> @@ -2092,6 +2096,7 @@ i40e_dev_tx_queue_stop(struct rte_eth_dev *dev,
> uint16_t tx_queue_id)
>
> i40e_tx_queue_release_mbufs(txq);
> i40e_reset_tx_queue(txq);
> + dev->data->tx_queue_state[tx_queue_id] = 0;
> }
>
> return 0;
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index a598a72..8fc869d 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -4498,6 +4498,7 @@ ixgbe_dev_rx_queue_start(struct rte_eth_dev *dev,
> uint16_t rx_queue_id)
> rte_wmb();
> IXGBE_WRITE_REG(hw, IXGBE_RDH(rxq->reg_idx), 0);
> IXGBE_WRITE_REG(hw, IXGBE_RDT(rxq->reg_idx), rxq->nb_rx_desc -
> 1);
> + dev->data->rx_queue_state[rx_queue_id] = 1;
> } else
> return -1;
>
> @@ -4541,6 +4542,7 @@ ixgbe_dev_rx_queue_stop(struct rte_eth_dev *dev,
> uint16_t rx_queue_id)
>
> ixgbe_rx_queue_release_mbufs(rxq);
> ixgbe_reset_rx_queue(adapter, rxq);
> + dev->data->rx_queue_state[rx_queue_id] = 0;
> } else
> return -1;
>
> @@ -4583,6 +4585,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev,
> uint16_t tx_queue_id)
> rte_wmb();
> IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
> IXGBE_WRITE_REG(hw, IXGBE_TDT(txq->reg_idx), 0);
> + dev->data->tx_queue_state[tx_queue_id] = 1;
> } else
> return -1;
>
> @@ -4643,6 +4646,7 @@ ixgbe_dev_tx_queue_stop(struct rte_eth_dev *dev,
> uint16_t tx_queue_id)
> txq->ops->release_mbufs(txq);
> txq->ops->reset(txq);
> }
> + dev->data->tx_queue_state[tx_queue_id] = 0;
> } else
> return -1;
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index b309309..ebcf2f2 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -767,6 +767,13 @@ rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t
> rx_queue_id)
>
> FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
>
> + if (dev->data->rx_queue_state[rx_queue_id] == 1) {
> + PMD_DEBUG_TRACE("Queue %" PRIu16" of device with port_id=%"
> PRIu8
> + " already started\n",
> + rx_queue_id, port_id);
> + return 0;
> + }
> +
> return dev->dev_ops->rx_queue_start(dev, rx_queue_id);
>
> }
> @@ -790,6 +797,13 @@ rte_eth_dev_rx_queue_stop(uint8_t port_id, uint16_t
> rx_queue_id)
>
> FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
>
> + if (dev->data->rx_queue_state[rx_queue_id] == 0) {
> + PMD_DEBUG_TRACE("Queue %" PRIu16" of device with port_id=%"
> PRIu8
> + " already stopped\n",
> + rx_queue_id, port_id);
> + return 0;
> + }
> +
> return dev->dev_ops->rx_queue_stop(dev, rx_queue_id);
>
> }
> @@ -813,6 +827,13 @@ rte_eth_dev_tx_queue_start(uint8_t port_id, uint16_t
> tx_queue_id)
>
> FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
>
> + if (dev->data->tx_queue_state[tx_queue_id] == 1) {
Here and in other places probably better
dev->data->tx_queue_state[tx_queue_id] != 0
In case we'll have some future extra states (not only start & stop) in future.
Probably even better create a 2 macros:
RTE_ETH_QUEUE_STATE_STOP 0
RTE_ETH_QUEUE_STATE_START 1
And use them around.
> + PMD_DEBUG_TRACE("Queue %" PRIu16" of device with port_id=%"
> PRIu8
> + " already started\n",
> + tx_queue_id, port_id);
> + return 0;
> + }
> +
> return dev->dev_ops->tx_queue_start(dev, tx_queue_id);
Why not something like:
ret = dev->dev_ops->tx_queue_start(dev, tx_queue_id);
if (ret == 0)
dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_START;
Same for dev_stop and RX.
Then you hopefully wouldn't need to update each and every PMD,
only rteh_ethdev* would be affected.
>
> }
> @@ -836,6 +857,13 @@ rte_eth_dev_tx_queue_stop(uint8_t port_id, uint16_t
> tx_queue_id)
>
> FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
>
> + if (dev->data->tx_queue_state[tx_queue_id] == 0) {
> + PMD_DEBUG_TRACE("Queue %" PRIu16" of device with port_id=%"
> PRIu8
> + " already stopped\n",
> + tx_queue_id, port_id);
> + return 0;
> + }
> +
> return dev->dev_ops->tx_queue_stop(dev, tx_queue_id);
>
> }
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index fa06554..3abd888 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1635,6 +1635,10 @@ struct rte_eth_dev_data {
> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0).
> */
> lro : 1; /**< RX LRO is ON(1) / OFF(0) */
> + uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> + /** Queues state: STARTED(1) / STOPPED(0) */
> + uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> + /** Queues state: STARTED(1) / STOPPED(0) */
> };
>
> /**
> --
> 2.4.2
