From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Ananyev, Konstantin" Subject: Re: [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode Date: Tue, 14 Jun 2016 08:42:47 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B70788@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> <2601191342CEEE43887BDE71AB97725836B6FCC1@irsmsx105.ger.corp.intel.com> <6A0DE07E22DDAD4C9103DF62FEBC0909034859A8@shsmsx102.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Richardson, Bruce" , "Chen, Jing D" , "Liang, Cunming" , "Wu, Jingjing" , "Zhang, Helin" To: "Lu, Wenzhuo" , "Tao, Zhe" , "dev@dpdk.org" Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 77F806CA2 for ; Tue, 14 Jun 2016 10:42:51 +0200 (CEST) In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC0909034859A8@shsmsx102.ccr.corp.intel.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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, 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_i= d]; > > > > > > > > > > > > > > > > #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].l= o > > > > > > > > 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 thing= s > > > > 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 operatio= n 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 pi= cture > > 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? It may b= e > > 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 su= re > > all IO over that device is stopped, and then he can call rte_eth_dev_re= set(), > > correct? > > Konstantin > Yes, that's exactly what I plan to do :) Sounds reaosanable to me :) Konstantin =20