Re: [dpdk-stable] [PATCH] net/i40e: fix queue related exception handling

["Yang, Qiming" <qiming.yang@intel.com>]

Hi, Xiaolong

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Thursday, May 14, 2020 15:44
> To: Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; stable@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.yang@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
> >