From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id AF85E5584 for ; Mon, 13 Jun 2016 12:47:39 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 13 Jun 2016 03:47:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,466,1459839600"; d="scan'208";a="718204468" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by FMSMGA003.fm.intel.com with ESMTP; 13 Jun 2016 03:47:37 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by IRSMSX107.ger.corp.intel.com ([169.254.10.96]) with mapi id 14.03.0248.002; Mon, 13 Jun 2016 11:47:37 +0100 From: "Ananyev, Konstantin" To: "Lu, Wenzhuo" , "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: AQHRwIlPe9ZormIF7U2HZ+7hR7jCn5/du9hggAFfxoCAACa6QIAFyEGAgAFzWMCAAA/4AIAAsf/g Date: Mon, 13 Jun 2016 10:47:35 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B6FCC1@irsmsx105.ger.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> In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09090348516A@shsmsx102.ccr.corp.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.180] 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: Mon, 13 Jun 2016 10:47:40 -0000 Hi Wenzhuo, > > > > > > > > > > > > > > > > > > > > > > 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, s= o > > > > > > 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_p= kts) { > > > > > > 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_i= d) > > { > > > > > > 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].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 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 execut= ed, > > 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_star= t/stop :) >=20 > > 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 responsib= ility 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 > picture 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 port. But how about your opinion if we add a specific rte_reset API? I= t may be easier for the user. 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 a= ll IO over that device is stopped, and then he can call rte_eth_dev_reset(), corr= ect? Konstantin