> -----Original Message-----
> From: Qiu, Michael
> Sent: Wednesday, September 23, 2015 7:57 AM
> To: Stephen Hemminger; De Lara Guarch, Pablo
> Cc: dev@dpdk.org; thomas.monjalon@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: add new RX/TX queue state
> arrays in rte_eth_dev_data
>
> On 2015/9/22 6:40, Stephen Hemminger wrote:
> > On Wed, 16 Sep 2015 22:51:24 +0100
> > Pablo de Lara <pablo.de.lara.guarch@intel.com> wrote:
> >
> >> 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>
> > I agree that the DPDK API should check for buggy manipulation
> > in the control path. But this should be done in generic code.
> > Anything where you have to change any driver is making more work
> > than necessary.
>
I see little way to set the queue state without touching the PMDs.
Basically, because queues can be started either calling directly
the function from the PMDs or calling the generic function from ethdev,
therefore some code must be placed in the PMDs.
I guess there are two possible ways to overcome this:
- Use in all PMDs rte_eth_rx_queue_start/stop, instead of directly
the internal functions: first, it is necessary the port id, which I am not sure if it is always available
(plus, when it is, it is available in the specific queue of that PMD, so it does not have to be there).
Anyway, this option requires changing the PMDs.
- Apparently, the only function that calls these queue_start/stop functions is dev_start/stop.
In some PMDs, it is possible to defer the start of some queues, so it would not initialize
these queues, but in other PMDs this is not implemented.
We will also be depending at how it is going on within the PMDs,
so the code will not be generic either.
Any ideas to make this truly generic?
> I agree with you, but I have a question, why we need expose the queue
> start and stop function to app?
>
> In my opinion, user app will hardly to start a device but stop the
> device queue. what's the purpose of it?
This API was added in 1.7 if I am not wrong. The purpose was that user could decide
not to start some of the queues in a device at start up, and then a new API to start
these queues was necessary, but there were no checks of their current state,
leading to undefined behaviour.
Thanks both Stephen and Michael,
Pablo
>
> Thanks,
> Michael
@@ -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] = RTE_ETH_QUEUE_STATE_STARTED;
+
+ 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] = RTE_ETH_QUEUE_STATE_STOPPED;
+
+ 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] = RTE_ETH_QUEUE_STATE_STARTED;
+
+ 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] = RTE_ETH_QUEUE_STATE_STOPPED;
+
+ return ret;
}
static int cxgbe_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
@@ -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] = RTE_ETH_QUEUE_STATE_STARTED;
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] = RTE_ETH_QUEUE_STATE_STOPPED;
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] = RTE_ETH_QUEUE_STATE_STARTED;
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] = RTE_ETH_QUEUE_STATE_STOPPED;
return ret;
}
@@ -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] = RTE_ETH_QUEUE_STATE_STARTED;
}
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] = RTE_ETH_QUEUE_STATE_STOPPED;
}
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] = RTE_ETH_QUEUE_STATE_STARTED;
} 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] = RTE_ETH_QUEUE_STATE_STOPPED;
}
return 0;
@@ -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] = RTE_ETH_QUEUE_STATE_STARTED;
}
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] = RTE_ETH_QUEUE_STATE_STOPPED;
}
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] = RTE_ETH_QUEUE_STATE_STARTED;
}
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] = RTE_ETH_QUEUE_STATE_STOPPED;
}
return 0;
@@ -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] = RTE_ETH_QUEUE_STATE_STARTED;
}
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] = RTE_ETH_QUEUE_STATE_STOPPED;
}
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] = RTE_ETH_QUEUE_STATE_STARTED;
}
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] = RTE_ETH_QUEUE_STATE_STOPPED;
}
return 0;
@@ -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] = RTE_ETH_QUEUE_STATE_STARTED;
} 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] = RTE_ETH_QUEUE_STATE_STOPPED;
} 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] = RTE_ETH_QUEUE_STATE_STARTED;
} 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] = RTE_ETH_QUEUE_STATE_STOPPED;
} else
return -1;
@@ -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] != RTE_ETH_QUEUE_STATE_STOPPED) {
+ 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] == RTE_ETH_QUEUE_STATE_STOPPED) {
+ 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] != RTE_ETH_QUEUE_STATE_STOPPED) {
+ 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] == RTE_ETH_QUEUE_STATE_STOPPED) {
+ 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);
}
@@ -962,6 +962,12 @@ struct rte_eth_xstats {
uint64_t value;
};
+/**
+ * RX/TX queue states
+ */
+#define RTE_ETH_QUEUE_STATE_STOPPED 0
+#define RTE_ETH_QUEUE_STATE_STARTED 1
+
struct rte_eth_dev;
struct rte_eth_dev_callback;
@@ -1635,6 +1641,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) */
};
/**