[dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data

[dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data

[Pablo de Lara]

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@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) {
+		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);
 
 }
@@ -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

Re: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data

[Ananyev, Konstantin]

Hi Pablo,
Few comments from me, please see below.
Konstantin

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> Sent: Wednesday, September 16, 2015 12:21 PM
> To: dev@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@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

Re: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data

[De Lara Guarch, Pablo]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, September 16, 2015 5:27 PM
> To: De Lara Guarch, Pablo
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays
> in rte_eth_dev_data
> 
> Hi Pablo,
> Few comments from me, please see below.
> Konstantin
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> > Sent: Wednesday, September 16, 2015 12:21 PM
> > To: dev@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@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.

Looks good. Will change and send a v2.

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

Problem is that some PMDs call internally queue_start/stop, but not the generic rte_eth_rx_queue_start (stop and RX),
so in that case this would not update the state of the queue.

Thanks,
Pablo

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

Re: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data

[Thomas Monjalon]

2015-09-16 21:22, De Lara Guarch, Pablo:
> From: Ananyev, Konstantin
> > 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.
> 
> Problem is that some PMDs call internally queue_start/stop, but not the generic rte_eth_rx_queue_start (stop and RX),
> so in that case this would not update the state of the queue.

Why not changing PMD to call rte_eth_rx_queue_start? Do you think it will be
too much error prone for later updates?

Re: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, September 17, 2015 8:24 AM
> To: De Lara Guarch, Pablo
> Cc: dev@dpdk.org; Ananyev, Konstantin
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays
> in rte_eth_dev_data
> 
> 2015-09-16 21:22, De Lara Guarch, Pablo:
> > From: Ananyev, Konstantin
> > > 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.
> >
> > Problem is that some PMDs call internally queue_start/stop, but not the
> generic rte_eth_rx_queue_start (stop and RX),
> > so in that case this would not update the state of the queue.
> 
> Why not changing PMD to call rte_eth_rx_queue_start? Do you think it will
> be
> too much error prone for later updates?

I thought PMDs didn't call API from rte_ethdev. In fact, that function needs a port id,
which is not available in the PMD functions usually.
Well, port id can be available in this case since the queues have it as a field, but I see it as a worse solution,
as it would be inconsistent with the way PMDs work, IMO.

Thanks,
Pablo