DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Yang, Qiming" <qiming.yang@intel.com>
To: "Ye, Xiaolong" <xiaolong.ye@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"Xing, Beilei" <beilei.xing@intel.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] net/i40e: fix queue related exception handling
Date: Thu, 14 May 2020 08:40:04 +0000	[thread overview]
Message-ID: <MN2PR11MB3582D9FCFAABB2EEFE43AF6CE5BC0@MN2PR11MB3582.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200514074404.GC102809@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
> >

  reply	other threads:[~2020-05-14  8:40 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13  7:56 [dpdk-dev] " Qiming Yang
2020-05-14  7:44 ` [dpdk-dev] [dpdk-stable] " Ye Xiaolong
2020-05-14  8:40   ` Yang, Qiming [this message]
2020-05-18  5:45 ` [dpdk-dev] [PATCH v2] " Qiming Yang
2020-05-19  1:56   ` Ye Xiaolong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MN2PR11MB3582D9FCFAABB2EEFE43AF6CE5BC0@MN2PR11MB3582.namprd11.prod.outlook.com \
    --to=qiming.yang@intel.com \
    --cc=beilei.xing@intel.com \
    --cc=dev@dpdk.org \
    --cc=stable@dpdk.org \
    --cc=xiaolong.ye@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).