From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 183925A52 for ; Tue, 14 Jun 2016 02:42:15 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 13 Jun 2016 17:42:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,469,1459839600"; d="scan'208";a="996965693" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga002.jf.intel.com with ESMTP; 13 Jun 2016 17:42:14 -0700 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 13 Jun 2016 17:42:12 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 13 Jun 2016 17:42:12 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.147]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.107]) with mapi id 14.03.0248.002; Tue, 14 Jun 2016 08:42:10 +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/dPyEAgAGGJDCAAAFWgIAGTU9QgADl4ICAAJ3JYIAAIzuAgAFu52A= Date: Tue, 14 Jun 2016 00:42:09 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC0909034859A8@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> <6A0DE07E22DDAD4C9103DF62FEBC090903483A2C@shsmsx102.ccr.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B6CCE1@irsmsx105.ger.corp.intel.com> <6A0DE07E22DDAD4C9103DF62FEBC09090348499B@shsmsx102.ccr.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B6E969@irsmsx105.ger.corp.intel.com> <6A0DE07E22DDAD4C9103DF62FEBC09090348516A@shsmsx102.ccr.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B6FCC1@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836B6FCC1@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: Tue, 14 Jun 2016 00:42:17 -0000 Hi Konstantin, > -----Original Message----- > From: Ananyev, Konstantin > Sent: Monday, June 13, 2016 6:48 PM > To: Lu, Wenzhuo; Tao, Zhe; dev@dpdk.org > Cc: Richardson, Bruce; Chen, Jing D; Liang, Cunming; Wu, Jingjing; Zhang, > Helin > Subject: RE: [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode >=20 > Hi Wenzhuo, >=20 > > > > > > > > > > > > > > > > > > > > > > > > > > 3. I thought the plan was to introduce a locking in all > > > > > > > appropriate control 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. > > > > > > > > > > > > > > To be more specific, I thought about something like that: > > > > > > > > > > > > > > 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]= ; > > > > > > > > > > > > > > #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); > > > > > > > > > > > > > > if (queue_id >=3D dev->data->nb_rx_queues) { > > > > > > > RTE_PMD_DEBUG_TRACE("Invalid RX > > > > > > > queue_id=3D%d\n", > > > queue_id); > > > > > > > return 0; > > > > > > > } > > > > > > > #endif > > > > > > > > > > > > > > + 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; > > > > > > > + > > > > > > > > > > > > > > nb_rx =3D (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id= ], > > > > > > > rx_pkts, nb_pkts); > > > > > > > > > > > > > > + rte_spinlock_unlock(&dev->data- > > > >rx_queue_state[rx_queue_id].un > > > > > > > + lock > > > > > > > + ); > > > > > > > > > > > > > > .... > > > > > > > > > > > > > > return nb_rx; > > > > > > > } > > > > > > > > > > > > > > And inside queue_start: > > > > > > > > > > > > > > int > > > > > > > rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t > > > > > > > rx_queue_id) > > > { > > > > > > > struct rte_eth_dev *dev; > > > > > > > > > > > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > > > > > > > > > > > > > > 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_queue_id); > > > > > > > return -EINVAL; > > > > > > > } > > > > > > > > > > > > > > > > > > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, > > > > > > > -ENOTSUP); > > > > > > > > > > > > > > > > > > > > > rte_spinlock_lock(&dev->data->rx_queue_state[rx_queue_id].lo > > > > > > > ck) > > > > > > 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 the queue. For example, when stop the port, > > > > > > the resource of the > > > > > queues may be released. > > > > > > > > > > I didn't get you here... > > > > > Before releasing the queue resources, queue_stop() has to be > > > > > executed, > > > right? > > > > Sorry, I saw your example with rte_eth_dev_rx_queue_start, I > > > > didn't know you also want to change rte_eth_dev_rx_queue_stop too. > > > > Agree this should work it we call queue_start/stop when reset the > > > > port. But we will not call them. I find the queue_stop/start are > > > > per- queue > > > functions and not supported by all NICs. > > > > > > But right now you do reset only for ixgbe/i40e. > > Not only for ixgbe/i40e. You forget igb, which doesn't support > > queue_start/stop :) > > > > > For these devices we defiantly do support queue start/stop. > > > And again, it is not only about reset op. > > > If we want to add rx locked (synced), I think it should be in sync > > > with all control API that changes queue state. > > > As I said before it is a lot of work and a lot of hassle... > > > So probably the easiest (and might be safiest) way just leave things > > > as there are right now: > > > we allow user to setup a callback on VF reset, and it is user > > > responsibility to make sure no RX/TX is active while reset operation = is > performed. > > > Pretty much what Olivier and Stephen suggested, as I understand. > > Agree. It's not a good way to add lock for just one feature. It could > > be tricky if we want to extend the lock to other features. A whole pict= ure > is needed. > > We've sent another patch set to let the user setup a callback on VF > > reset. Depend on that, user can use existing rte APIs to reset the VF p= ort. > But how about your opinion if we add a specific rte_reset API? It may be > easier for the user. >=20 > You mean add rte_eth_dev_reset() without any locking inside? > So it when VF reset happens, it would be user responsibility to make sure > all IO over that device is stopped, and then he can call rte_eth_dev_rese= t(), > correct? > Konstantin Yes, that's exactly what I plan to do :)