From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 0A8663784 for ; Wed, 16 Sep 2015 18:27:24 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 16 Sep 2015 09:27:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,540,1437462000"; d="scan'208";a="770351760" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga001.jf.intel.com with ESMTP; 16 Sep 2015 09:27:23 -0700 Received: from irsmsx155.ger.corp.intel.com (163.33.192.3) by IRSMSX101.ger.corp.intel.com (163.33.3.153) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 16 Sep 2015 17:27:20 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by irsmsx155.ger.corp.intel.com ([169.254.14.184]) with mapi id 14.03.0224.002; Wed, 16 Sep 2015 17:27:20 +0100 From: "Ananyev, Konstantin" To: "De Lara Guarch, Pablo" Thread-Topic: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data Thread-Index: AQHQ8HHbpmzDOdqad0Gz31pWMlRi554/VWRg Date: Wed, 16 Sep 2015 16:27:20 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836A870DF@irsmsx105.ger.corp.intel.com> References: <1442402465-20158-1-git-send-email-pablo.de.lara.guarch@intel.com> In-Reply-To: <1442402465-20158-1-git-send-email-pablo.de.lara.guarch@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] ethdev: add new RX/TX queue state arrays in rte_eth_dev_data X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Sep 2015 16:27:26 -0000 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 r= te_eth_dev_data >=20 > 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. >=20 > This is important to avoid trying to start/stop twice a queue, > which will result in undefined behaviour > (which may cause RX/TX disruption). >=20 > 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. >=20 > Signed-off-by: Pablo de Lara > --- > 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(-) >=20 > diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_e= thdev.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 =3D (struct sge_eth_txq *) > (eth_dev->data->tx_queues[tx_queue_id]); >=20 > dev_debug(NULL, "%s: tx_queue_id =3D %d\n", __func__, tx_queue_id); >=20 > - return t4_sge_eth_txq_start(txq); > + ret =3D t4_sge_eth_txq_start(txq); > + if (ret =3D=3D 0) > + eth_dev->data->tx_queue_state[tx_queue_id] =3D 1; > + > + return ret; > } >=20 > 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 =3D (struct sge_eth_txq *) > (eth_dev->data->tx_queues[tx_queue_id]); >=20 > dev_debug(NULL, "%s: tx_queue_id =3D %d\n", __func__, tx_queue_id); >=20 > - return t4_sge_eth_txq_stop(txq); > + ret =3D t4_sge_eth_txq_stop(txq); > + if (ret =3D=3D 0) > + eth_dev->data->tx_queue_state[tx_queue_id] =3D 0; > + > + return ret; > } >=20 > 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 =3D (struct port_info *)(eth_dev->data->dev_privat= e); > struct adapter *adap =3D 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); >=20 > q =3D eth_dev->data->rx_queues[rx_queue_id]; > - return t4_sge_eth_rxq_start(adap, q); > + > + ret =3D t4_sge_eth_rxq_start(adap, q); > + if (ret =3D=3D 0) > + eth_dev->data->rx_queue_state[rx_queue_id] =3D 1; > + > + return ret; > } >=20 > static int cxgbe_dev_rx_queue_stop(struct rte_eth_dev *eth_dev, > uint16_t rx_queue_id) > { > + int ret; > struct port_info *pi =3D (struct port_info *)(eth_dev->data->dev_privat= e); > struct adapter *adap =3D pi->adapter; > struct sge_rspq *q; > @@ -482,7 +499,11 @@ static int cxgbe_dev_rx_queue_stop(struct rte_eth_de= v *eth_dev, > __func__, pi->port_id, rx_queue_id); >=20 > q =3D eth_dev->data->rx_queues[rx_queue_id]; > - return t4_sge_eth_rxq_stop(adap, q); > + ret =3D t4_sge_eth_rxq_stop(adap, q); > + if (ret =3D=3D 0) > + eth_dev->data->rx_queue_state[rx_queue_id] =3D 0; > + > + return ret; > } >=20 > 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_ethde= v.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(); >=20 > enic_start_wq(enic, queue_idx); > + eth_dev->data->tx_queue_state[queue_idx] =3D 1; >=20 > return 0; > } > @@ -209,6 +210,8 @@ static int enicpmd_dev_tx_queue_stop(struct rte_eth_d= ev *eth_dev, > ret =3D 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] =3D 0; >=20 > return ret; > } > @@ -221,6 +224,7 @@ static int enicpmd_dev_rx_queue_start(struct rte_eth_= dev *eth_dev, > ENICPMD_FUNC_TRACE(); >=20 > enic_start_rq(enic, queue_idx); > + eth_dev->data->rx_queue_state[queue_idx] =3D 1; >=20 > return 0; > } > @@ -236,6 +240,8 @@ static int enicpmd_dev_rx_queue_stop(struct rte_eth_d= ev *eth_dev, > ret =3D 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] =3D 0; >=20 > return ret; > } > diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_e= thdev.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, uin= t16_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] =3D 1; > } >=20 > return err; > @@ -542,6 +543,7 @@ fm10k_dev_rx_queue_stop(struct rte_eth_dev *dev, uint= 16_t rx_queue_id) >=20 > /* Free mbuf and clean HW ring */ > rx_queue_clean(dev->data->rx_queues[rx_queue_id]); > + dev->data->rx_queue_state[rx_queue_id] =3D 0; > } >=20 > return 0; > @@ -569,6 +571,7 @@ fm10k_dev_tx_queue_start(struct rte_eth_dev *dev, uin= t16_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] =3D 1; > } else > err =3D -1; >=20 > @@ -585,6 +588,7 @@ fm10k_dev_tx_queue_stop(struct rte_eth_dev *dev, uint= 16_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] =3D 0; > } >=20 > return 0; > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_et= hdev_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] =3D 1; > } >=20 > return err; > @@ -1397,6 +1399,7 @@ i40evf_dev_rx_queue_stop(struct rte_eth_dev *dev, u= int16_t rx_queue_id) >=20 > i40e_rx_queue_release_mbufs(rxq); > i40e_reset_rx_queue(rxq); > + dev->data->rx_queue_state[rx_queue_id] =3D 0; > } >=20 > 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] =3D 1; > } >=20 > return err; > @@ -1441,6 +1446,7 @@ i40evf_dev_tx_queue_stop(struct rte_eth_dev *dev, u= int16_t tx_queue_id) >=20 > i40e_tx_queue_release_mbufs(txq); > i40e_reset_tx_queue(txq); > + dev->data->tx_queue_state[tx_queue_id] =3D 0; > } >=20 > 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, ui= nt16_t rx_queue_id) >=20 > i40e_rx_queue_release_mbufs(rxq); > i40e_reset_rx_queue(rxq); > - } > + } else > + dev->data->rx_queue_state[rx_queue_id] =3D 1; > } >=20 > return err; > @@ -2038,6 +2039,7 @@ i40e_dev_rx_queue_stop(struct rte_eth_dev *dev, uin= t16_t rx_queue_id) > } > i40e_rx_queue_release_mbufs(rxq); > i40e_reset_rx_queue(rxq); > + dev->data->rx_queue_state[rx_queue_id] =3D 0; > } >=20 > return 0; > @@ -2063,6 +2065,8 @@ i40e_dev_tx_queue_start(struct rte_eth_dev *dev, ui= nt16_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] =3D 1; > } >=20 > return err; > @@ -2092,6 +2096,7 @@ i40e_dev_tx_queue_stop(struct rte_eth_dev *dev, uin= t16_t tx_queue_id) >=20 > i40e_tx_queue_release_mbufs(txq); > i40e_reset_tx_queue(txq); > + dev->data->tx_queue_state[tx_queue_id] =3D 0; > } >=20 > return 0; > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxt= x.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, u= int16_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] =3D 1; > } else > return -1; >=20 > @@ -4541,6 +4542,7 @@ ixgbe_dev_rx_queue_stop(struct rte_eth_dev *dev, ui= nt16_t rx_queue_id) >=20 > ixgbe_rx_queue_release_mbufs(rxq); > ixgbe_reset_rx_queue(adapter, rxq); > + dev->data->rx_queue_state[rx_queue_id] =3D 0; > } else > return -1; >=20 > @@ -4583,6 +4585,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, u= int16_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] =3D 1; > } else > return -1; >=20 > @@ -4643,6 +4646,7 @@ ixgbe_dev_tx_queue_stop(struct rte_eth_dev *dev, ui= nt16_t tx_queue_id) > txq->ops->release_mbufs(txq); > txq->ops->reset(txq); > } > + dev->data->tx_queue_state[tx_queue_id] =3D 0; > } else > return -1; >=20 > 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) >=20 > FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP); >=20 > + if (dev->data->rx_queue_state[rx_queue_id] =3D=3D 1) { > + PMD_DEBUG_TRACE("Queue %" PRIu16" of device with port_id=3D%" PRIu8 > + " already started\n", > + rx_queue_id, port_id); > + return 0; > + } > + > return dev->dev_ops->rx_queue_start(dev, rx_queue_id); >=20 > } > @@ -790,6 +797,13 @@ rte_eth_dev_rx_queue_stop(uint8_t port_id, uint16_t = rx_queue_id) >=20 > FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP); >=20 > + if (dev->data->rx_queue_state[rx_queue_id] =3D=3D 0) { > + PMD_DEBUG_TRACE("Queue %" PRIu16" of device with port_id=3D%" PRIu8 > + " already stopped\n", > + rx_queue_id, port_id); > + return 0; > + } > + > return dev->dev_ops->rx_queue_stop(dev, rx_queue_id); >=20 > } > @@ -813,6 +827,13 @@ rte_eth_dev_tx_queue_start(uint8_t port_id, uint16_t= tx_queue_id) >=20 > FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP); >=20 > + if (dev->data->tx_queue_state[tx_queue_id] =3D=3D 1) { Here and in other places probably better dev->data->tx_queue_state[tx_queue_id] !=3D 0 In case we'll have some future extra states (not only start & stop) in futu= re. Probably even better create a 2 macros: =20 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=3D%" 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 =3D dev->dev_ops->tx_queue_start(dev, tx_queue_id); if (ret =3D=3D 0) dev->data->tx_queue_state[tx_queue_id] =3D RTE_ETH_QUEUE_STATE_START; Same for dev_stop and RX. Then you hopefully wouldn't need to update each and every PMD,=20 only rteh_ethdev* would be affected. >=20 > } > @@ -836,6 +857,13 @@ rte_eth_dev_tx_queue_stop(uint8_t port_id, uint16_t = tx_queue_id) >=20 > FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP); >=20 > + if (dev->data->tx_queue_state[tx_queue_id] =3D=3D 0) { > + PMD_DEBUG_TRACE("Queue %" PRIu16" of device with port_id=3D%" PRIu8 > + " already stopped\n", > + tx_queue_id, port_id); > + return 0; > + } > + > return dev->dev_ops->tx_queue_stop(dev, tx_queue_id); >=20 > } > 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) */ > }; >=20 > /** > -- > 2.4.2