From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 6862E95DA for ; Wed, 8 Jun 2016 09:24:09 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP; 08 Jun 2016 00:24:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,438,1459839600"; d="scan'208";a="118097480" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga004.fm.intel.com with ESMTP; 08 Jun 2016 00:24:08 -0700 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 8 Jun 2016 00:24:08 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 8 Jun 2016 00:24:07 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.147]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.24]) with mapi id 14.03.0248.002; Wed, 8 Jun 2016 15:24:02 +0800 From: "Lu, Wenzhuo" To: "Ananyev, Konstantin" , "Tao, Zhe" , "dev@dpdk.org" CC: "Richardson, Bruce" , "Chen, Jing D" , "Liang, Cunming" , "Wu, Jingjing" , "Zhang, Helin" Thread-Topic: [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode Thread-Index: AQHRwIlQvj4+w3LRO06zb58e27oReZ/dPyEAgAGGJDA= Date: Wed, 8 Jun 2016 07:24:01 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC090903483A2C@shsmsx102.ccr.corp.intel.com> References: <1465278858-5131-1-git-send-email-zhe.tao@intel.com> <1465282390-6025-1-git-send-email-zhe.tao@intel.com> <1465282390-6025-3-git-send-email-zhe.tao@intel.com> <2601191342CEEE43887BDE71AB97725836B6C44E@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836B6C44E@irsmsx105.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode 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, 08 Jun 2016 07:24:10 -0000 Hi Konstantin, > -----Original Message----- > From: Ananyev, Konstantin > Sent: Tuesday, June 7, 2016 5:59 PM > To: Tao, Zhe; dev@dpdk.org > Cc: Lu, Wenzhuo; Richardson, Bruce; Chen, Jing D; Liang, Cunming; Wu, Jin= gjing; > Zhang, Helin > Subject: RE: [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode >=20 >=20 > Hi Zhe & Wenzhuo, >=20 > Please find my comments below. > BTW, for clarification - is that patch for 16.11? > I believe it's too late to introduce such significant change in 16.07. > Thanks > Konstantin Thanks for the comments. Honestly, our purpose is 16.07. Realizing the big impact, we use NEXT_ABI t= o comment our change. So, I think although we want to merge it in 16.07 thi= s change will become effective after we remove NEXT_ABI in 16.11. >=20 > > Define lock mode for RX/TX queue. Because when resetting the device we > > want the resetting thread to get the lock of the RX/TX queue to make > > sure the RX/TX is stopped. > > > > Using next ABI macro for this ABI change as it has too much impact. 7 > > APIs and 1 global variable are impacted. > > > > Signed-off-by: Wenzhuo Lu > > Signed-off-by: Zhe Tao > > --- > > lib/librte_ether/rte_ethdev.h | 62 > > +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > > > diff --git a/lib/librte_ether/rte_ethdev.h > > b/lib/librte_ether/rte_ethdev.h index 74e895f..4efb5e9 100644 > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > @@ -354,7 +354,12 @@ struct rte_eth_rxmode { > > jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */ > > hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */ > > enable_scatter : 1, /**< Enable scatter packets rx handler */ > > +#ifndef RTE_NEXT_ABI > > enable_lro : 1; /**< Enable LRO */ > > +#else > > + enable_lro : 1, /**< Enable LRO */ > > + lock_mode : 1; /**< Using lock path */ > > +#endif > > }; > > > > /** > > @@ -634,11 +639,68 @@ struct rte_eth_txmode { > > /**< If set, reject sending out tagged pkts */ > > hw_vlan_reject_untagged : 1, > > /**< If set, reject sending out untagged pkts */ > > +#ifndef RTE_NEXT_ABI > > hw_vlan_insert_pvid : 1; > > /**< If set, enable port based VLAN insertion */ > > +#else > > + hw_vlan_insert_pvid : 1, > > + /**< If set, enable port based VLAN insertion */ > > + lock_mode : 1; > > + /**< If set, using lock path */ > > +#endif > > }; > > > > /** > > + * The macros for the RX/TX lock mode functions */ #ifdef > > +RTE_NEXT_ABI #define RX_LOCK_FUNCTION(dev, func) \ > > + (dev->data->dev_conf.rxmode.lock_mode ? \ > > + func ## _lock : func) > > + > > +#define TX_LOCK_FUNCTION(dev, func) \ > > + (dev->data->dev_conf.txmode.lock_mode ? \ > > + func ## _lock : func) > > +#else > > +#define RX_LOCK_FUNCTION(dev, func) func > > + > > +#define TX_LOCK_FUNCTION(dev, func) func #endif > > + > > +/* Add the lock RX/TX function for VF reset */ #define > > +GENERATE_RX_LOCK(func, nic) \ uint16_t func ## _lock(void *rx_queue, > > +\ > > + struct rte_mbuf **rx_pkts, \ > > + uint16_t nb_pkts) \ > > +{ \ > > + struct nic ## _rx_queue *rxq =3D rx_queue; \ > > + uint16_t nb_rx =3D 0; \ > > + \ > > + if (rte_spinlock_trylock(&rxq->rx_lock)) { \ > > + nb_rx =3D func(rx_queue, rx_pkts, nb_pkts); \ > > + rte_spinlock_unlock(&rxq->rx_lock); \ > > + } \ > > + \ > > + return nb_rx; \ > > +} > > + > > +#define GENERATE_TX_LOCK(func, nic) \ uint16_t func ## _lock(void > > +*tx_queue, \ > > + struct rte_mbuf **tx_pkts, \ > > + uint16_t nb_pkts) \ > > +{ \ > > + struct nic ## _tx_queue *txq =3D tx_queue; \ > > + uint16_t nb_tx =3D 0; \ > > + \ > > + if (rte_spinlock_trylock(&txq->tx_lock)) { \ > > + nb_tx =3D func(tx_queue, tx_pkts, nb_pkts); \ > > + rte_spinlock_unlock(&txq->tx_lock); \ > > + } \ > > + \ > > + return nb_tx; \ > > +} >=20 > 1. As I said in off-line dicussiion, I think this locking could (and I th= ink better be) > impelented completely on rte_ethdev layer. > So actual PMD code will be unaffected. > Again that avoids us to introduce _lock version of every RX/Tx function i= n each > PMD. One purpose of implementing the lock in PMD layer is to avoid ABI change. B= ut we introduce the field lock_mode in struct rte_eth_rx/txmode. So seems i= t's not a good reason now :) The other purpose is we want to add a lock for every queue. But in rte laye= r the queue is void *, so we add the lock in the specific structures of the= NICs. But as you mentioned below, we can add the lock as dev->data->rx_que= ue_state it the struct rte_eth_dev_data. So, I prefer to add the lock in rte layer now. >=20 > 2. Again, as discussed offline, I think it is better to have an explicit > rte_eth_(rx|tx)_burst_lock(sync?) API, instead of add new fileds into RX/= TX > config strcutures. > Would help to avoid any confusion, I think. We want the users to choose the rx/tx path without lock if they're sensiti= ve to the performance and can handle the reset event in their APP. After in= troducing new fields of config struct, users can change the config to choos= e the different path. If we introduce new API, it may be harder for the use to use it. I mean whe= n users want to use lock mode, they may need to replace all the rte_eth_rx/= tx_burst by rte_eth_rx/tx_burst_lock. So if we add the lock in rte layer, I= still prefer adding lock_mode in the configuration, and the rte_eth_rx/tx_= burst is changed like this, rte_eth_rx/tx_burst { + if lock_mode + try_lock ...... + if lock_mode + release_lock } >=20 > 3. I thought the plan was to introduce a locking in all appropriate cont= rol path > functions (dev_start/dev_stop etc.) Without that locking version of RX/TX= seems > a bit useless. > Yes, I understand that you do use locking inside dev_reset, but I suppose= the > plan was to have a generic solution, no? > Again, interrupt fire when user invokes dev_start/stop or so, so we still= need > some synchronisation between them. >=20 > To be more specific, I thought about something like that: >=20 > static inline uint16_t > rte_eth_rx_burst_lock(uint8_t port_id, uint16_t queue_id, > struct rte_mbuf **rx_pkts, const uint16_t nb_pkts) { > struct rte_eth_dev *dev =3D &rte_eth_devices[port_id]; >=20 > #ifdef RTE_LIBRTE_ETHDEV_DEBUG > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0); > RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0); >=20 > if (queue_id >=3D dev->data->nb_rx_queues) { > RTE_PMD_DEBUG_TRACE("Invalid RX queue_id=3D%d\n", queue_i= d); > return 0; > } > #endif >=20 > + if (rte_spinlock_trylock(&dev->data->rx_queue_state[rx_queue_id].lock) = =3D=3D 0) > + return 0; > + else if (dev->data->rx_queue_state[rx_queue_id] =3D=3D > RTE_ETH_QUEUE_STATE_STOPPED)) { > + rte_spinlock_unlock(&dev->data->rx_queue_state[rx_queue_id].unlock); > + return 0; > + >=20 > nb_rx =3D (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], > rx_pkts, nb_pkts); >=20 > + rte_spinlock_unlock(&dev->data->rx_queue_state[rx_queue_id].unlock); >=20 > .... >=20 > return nb_rx; > } >=20 > And inside queue_start: >=20 > int > rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t rx_queue_id) { > struct rte_eth_dev *dev; >=20 > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); >=20 > dev =3D &rte_eth_devices[port_id]; > if (rx_queue_id >=3D dev->data->nb_rx_queues) { > RTE_PMD_DEBUG_TRACE("Invalid RX queue_id=3D%d\n", rx_queu= e_id); > return -EINVAL; > } >=20 > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP); >=20 > rte_spinlock_lock(&dev->data->rx_queue_state[rx_queue_id].lock) I think you add the lock here to stop the rx/tx. But to my opinion, we should lock the rx/tx much earlier before starting th= e queue. For example, when stop the port, the resource of the queues may be= released. The rx/tx cannot be executed. So I prefer to get the lock before= stopping the ports. Maybe better to keep the spinlock in the dev_reset. >=20 > if (dev->data->rx_queue_state[rx_queue_id] !=3D > RTE_ETH_QUEUE_STATE_STOPPED) { > RTE_PMD_DEBUG_TRACE("Queue %" PRIu16" of device with > port_id=3D%" PRIu8 > " already started\n", > rx_queue_id, port_id); > ret =3D -EINVAL 0; > } else > ret =3D dev->dev_ops->rx_queue_start(dev, rx_queue_id); >=20 > rte_spinlock_unlock(&dev->data->rx_queue_state[rx_queue_id].unlock); >=20 > return ret; > } >=20 > Then again, we don't need to do explicit locking inside dev_reset(). > Does it make sense to you guys? Please see the answer above. >=20 >=20 > > + > > +/** > > * A structure used to configure an RX ring of an Ethernet port. > > */ > > struct rte_eth_rxconf { > > -- > > 2.1.4