Hi, Xiaolong
> -----Original Message-----
> From: Ye, Xiaolong <xiaolong...@intel.com>
> Sent: Thursday, May 14, 2020 15:44
> To: Yang, Qiming <qiming.y...@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.x...@intel.com>; sta...@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] net/i40e: fix queue related exception
> handling
>
> On 05/13, Qiming Yang wrote:
> >There should have different behavior in queue start fail and stop fail case.
> >When queue start fail, all the next actions should be terminated and
> >then started queues should be cleared. But for queue stop stage, one
> >queue stop fail should not end other queues stop. This patch fixed that
> >issue in PF and VF.
> >
> >Fixes: b6583ee40265 ("i40e: full VMDQ pools support")
> >Fixes: 3f6a696f1054 ("i40evf: queue start and stop")
> >
> >Signed-off-by: Qiming Yang <qiming.y...@intel.com>
> >---
> > drivers/net/i40e/i40e_ethdev.c | 116
> > ++++++++------------------------------
> > drivers/net/i40e/i40e_ethdev_vf.c | 2 -
> > drivers/net/i40e/i40e_rxtx.c | 28 +++++++++
> > 3 files changed, 53 insertions(+), 93 deletions(-)
> >
Snip ...
> > return I40E_SUCCESS;
> >
> >-err_up:
> >- i40e_dev_switch_queues(pf, FALSE);
> >- i40e_dev_clear_queues(dev);
> >+tx_err:
> >+ for (i = 0; i < nb_txq; i++)
> >+ i40e_dev_tx_queue_stop(dev, i);
> >+rx_err:
> >+ for (i = 0; i < nb_rxq; i++)
> >+ i40e_dev_rx_queue_stop(dev, i);
>
> I think we still need to clear queues in the error handling.
I delete clear function doesn't means clear queue action be deleted.
In i40e_dev_clear_queues, it calls these four functions
i40e_tx_queue_release_mbufs(dev->data->tx_queues[i]);
i40e_reset_tx_queue(dev->data->tx_queues[i]);
i40e_rx_queue_release_mbufs(dev->data->rx_queues[i]);
i40e_reset_rx_queue(dev->data->rx_queues[i]);
it covered by queue_stop function.
>
> >
> > return ret;
> > }
> >@@ -2442,7 +2452,11 @@ i40e_dev_stop(struct rte_eth_dev *dev)
> > }
> >
> > /* Disable all queues */
> >- i40e_dev_switch_queues(pf, FALSE);
> >+ for (i = 0; i < dev->data->nb_tx_queues; i++)
> >+ i40e_dev_tx_queue_stop(dev, i);
> >+
> >+ for (i = 0; i < dev->data->nb_rx_queues; i++)
> >+ i40e_dev_rx_queue_stop(dev, i);
> >
> > /* un-map queues with interrupt registers */
> > i40e_vsi_disable_queues_intr(main_vsi);
>
> [snip]
>
> >diff --git a/drivers/net/i40e/i40e_rxtx.c
> >b/drivers/net/i40e/i40e_rxtx.c index f6d23c9..d0bada9 100644
> >--- a/drivers/net/i40e/i40e_rxtx.c
> >+++ b/drivers/net/i40e/i40e_rxtx.c
> >@@ -1570,6 +1570,15 @@ i40e_dev_rx_queue_start(struct rte_eth_dev
> *dev, uint16_t rx_queue_id)
> > PMD_INIT_FUNC_TRACE();
> >
> > rxq = dev->data->rx_queues[rx_queue_id];
> >+ if (!rxq || !rxq->q_set) {
> >+ PMD_DRV_LOG(ERR, "RX queue %u not available or setup",
> >+ rx_queue_id);
> >+ return -EINVAL;
> >+ }
> >+
> >+ if (rxq->rx_deferred_start)
> >+ PMD_DRV_LOG(ERR, "RX queue %u is deferrd start",
> >+ rx_queue_id);
>
> Do we need to take any action if rx_deferred_start is set?
> Just print an ERR log doesn't make sense to me.
If defer set, this queue start will be skipped. But this should not block other
queues' start.
So I add a log to let user know but no return error.
Maybe return WARNING will be more softer, what do you think about?
>
> >
> > err = i40e_alloc_rx_queue_mbufs(rxq);
> > if (err) {
> >@@ -1602,6 +1611,11 @@ i40e_dev_rx_queue_stop(struct rte_eth_dev
> *dev, uint16_t rx_queue_id)
> > struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> >
> > rxq = dev->data->rx_queues[rx_queue_id];
> >+ if (!rxq || !rxq->q_set) {
> >+ PMD_DRV_LOG(ERR, "RX queue %u not available or setup",
> >+ rx_queue_id);
> >+ return -EINVAL;
> >+ }
> >
> > /*
> > * rx_queue_id is queue id application refers to, while @@ -1630,6
> >+1644,15 @@ i40e_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t
> tx_queue_id)
> > PMD_INIT_FUNC_TRACE();
> >
> > txq = dev->data->tx_queues[tx_queue_id];
> >+ if (!txq || !txq->q_set) {
> >+ PMD_DRV_LOG(ERR, "TX queue %u is not available or setup",
> >+ tx_queue_id);
> >+ return -EINVAL;
> >+ }
> >+
> >+ if (txq->tx_deferred_start)
> >+ PMD_DRV_LOG(ERR, "TX queue %u is deferrd start",
> >+ tx_queue_id);
>
> Ditto.
>
> Thanks,
> Xiaolong
>
> >
> > /*
> > * tx_queue_id is queue id application refers to, while @@ -1654,6
> >+1677,11 @@ i40e_dev_tx_queue_stop(struct rte_eth_dev *dev, uint16_t
> tx_queue_id)
> > struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> >
> > txq = dev->data->tx_queues[tx_queue_id];
> >+ if (!txq || !txq->q_set) {
> >+ PMD_DRV_LOG(ERR, "TX queue %u is not available or setup",
> >+ tx_queue_id);
> >+ return -EINVAL;
> >+ }
> >
> > /*
> > * tx_queue_id is queue id application refers to, while
> >--
> >2.9.5
> >
