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 085F7A2F for ; Sun, 12 Jun 2016 02:58:08 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP; 11 Jun 2016 17:58:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,459,1459839600"; d="scan'208";a="1000115271" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga002.fm.intel.com with ESMTP; 11 Jun 2016 17:58:07 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 11 Jun 2016 17:58:07 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.147]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.116]) with mapi id 14.03.0248.002; Sun, 12 Jun 2016 08:58:05 +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 4/8] ixgbe: implement device reset on VF Thread-Index: AQHRwIlSvCHYW1W/D0WZNVy55cLE2J/dQGEAgAGIBWD///PGAIAGTTfA Date: Sun, 12 Jun 2016 00:58:05 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC090903484910@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-5-git-send-email-zhe.tao@intel.com> <2601191342CEEE43887BDE71AB97725836B6C46D@irsmsx105.ger.corp.intel.com> <6A0DE07E22DDAD4C9103DF62FEBC090903483A3D@shsmsx102.ccr.corp.intel.com> <2601191342CEEE43887BDE71AB97725836B6CC99@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836B6CC99@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 4/8] ixgbe: implement device reset on VF 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: Sun, 12 Jun 2016 00:58:09 -0000 Hi Konstantin, > -----Original Message----- > From: Ananyev, Konstantin > Sent: Wednesday, June 8, 2016 4:42 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 4/8] ixgbe: implement device reset on VF >=20 >=20 >=20 > > -----Original Message----- > > From: Lu, Wenzhuo > > Sent: Wednesday, June 08, 2016 8:24 AM > > To: Ananyev, Konstantin; Tao, Zhe; dev@dpdk.org > > Cc: Richardson, Bruce; Chen, Jing D; Liang, Cunming; Wu, Jingjing; > > Zhang, Helin > > Subject: RE: [PATCH v4 4/8] ixgbe: implement device reset on VF > > > > Hi Konstantin, > > > > > -----Original Message----- > > > From: Ananyev, Konstantin > > > Sent: Tuesday, June 7, 2016 6:03 PM > > > To: Tao, Zhe; dev@dpdk.org > > > Cc: Lu, Wenzhuo; Richardson, Bruce; Chen, Jing D; Liang, Cunming; > > > Wu, Jingjing; Zhang, Helin > > > Subject: RE: [PATCH v4 4/8] ixgbe: implement device reset on VF > > > > > > > > > > > > > -----Original Message----- > > > > From: Tao, Zhe > > > > Sent: Tuesday, June 07, 2016 7:53 AM > > > > To: dev@dpdk.org > > > > Cc: Lu, Wenzhuo; Tao, Zhe; Ananyev, Konstantin; Richardson, Bruce; > > > > Chen, Jing D; Liang, Cunming; Wu, Jingjing; Zhang, Helin > > > > Subject: [PATCH v4 4/8] ixgbe: implement device reset on VF > > > > > > > > Implement the device reset function. > > > > 1, Add the fake RX/TX functions. > > > > 2, The reset function tries to stop RX/TX by replacing > > > > the RX/TX functions with the fake ones and getting the > > > > locks to make sure the regular RX/TX finished. > > > > 3, After the RX/TX stopped, reset the VF port, and then > > > > release the locks and restore the RX/TX functions. > > > > > > > > Signed-off-by: Wenzhuo Lu > > > > > > > > static int > > > > +ixgbevf_dev_reset(struct rte_eth_dev *dev) { > > > > + struct ixgbe_hw *hw =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data- > > > >dev_private); > > > > + struct ixgbe_adapter *adapter =3D > > > > + (struct ixgbe_adapter *)dev->data->dev_private; > > > > + int diag =3D 0; > > > > + uint32_t vteiam; > > > > + uint16_t i; > > > > + struct ixgbe_rx_queue *rxq; > > > > + struct ixgbe_tx_queue *txq; > > > > + > > > > + /* Nothing needs to be done if the device is not started. */ > > > > + if (!dev->data->dev_started) > > > > + return 0; > > > > + > > > > + PMD_DRV_LOG(DEBUG, "Link up/down event detected."); > > > > + > > > > + /** > > > > + * Stop RX/TX by fake functions and locks. > > > > + * Fake functions are used to make RX/TX lock easier. > > > > + */ > > > > + adapter->rx_backup =3D dev->rx_pkt_burst; > > > > + adapter->tx_backup =3D dev->tx_pkt_burst; > > > > + dev->rx_pkt_burst =3D ixgbevf_recv_pkts_fake; > > > > + dev->tx_pkt_burst =3D ixgbevf_xmit_pkts_fake; > > > > > > If you have locking over each queue underneath, why do you still > > > need fake functions? > > The fake functions are used to help saving the time of waiting for the = locks. > > As you see, we want to lock every queue. If we don't use fake functions= we > have to wait for every queue. > > But if the real functions are replaced by fake functions, ideally when > > we're waiting for the release of the first queue's lock, the other queu= es will run > into the fake functions. So we need not wait for them and get the locks d= irectly. >=20 > Well, data-path invokes only try_lock(), so it shouldn't be affected sign= ificantly, > right? > Control path still have to spin on lock and grab it before it can proceed= , if it'll > spin a bit longer I wouldn't see a big deal here. > What I am trying to say - if we'll go that way - introduce sync control/d= atapath > API anyway, we don't need any additional tricks here with rx/tx function > replacement, correct? > So let's keep it clean and simple, after all it is a control path and not= need to be > lightning fast. > Konstantin Agree, it's not necessary to add the fake functions. I'll remove them to ma= ke it simple. >=20 > > > > > > > > > + > > > > + if (dev->data->rx_queues) > > > > + for (i =3D 0; i < dev->data->nb_rx_queues; i++) { > > > > + rxq =3D dev->data->rx_queues[i]; > > > > + rte_spinlock_lock(&rxq->rx_lock); > > > > + } > > > > + > > > > + if (dev->data->tx_queues) > > > > + for (i =3D 0; i < dev->data->nb_tx_queues; i++) { > > > > + txq =3D dev->data->tx_queues[i]; > > > > + rte_spinlock_lock(&txq->tx_lock); > > > > + } > > > > > > Probably worth to create a separate function for the lines above: > > > lock_all_queues(), unlock_all_queues. > > > But as I sadi in previous mail - I think that code better be in rte_e= thdev. > > We're discussing it in the previous thread :) > > > > > > > > > > @@ -5235,11 +5243,21 @@ ixgbevf_dev_rxtx_start(struct rte_eth_dev > *dev) > > > > rxdctl =3D IXGBE_READ_REG(hw, IXGBE_VFRXDCTL(i)); > > > > } while (--poll_ms && !(rxdctl & IXGBE_RXDCTL_ENABLE)); > > > > if (!poll_ms) > > > > +#ifndef RTE_NEXT_ABI > > > > + PMD_INIT_LOG(ERR, "Could not enable Rx Queue %d", > > > i); #else > > > > + { > > > > PMD_INIT_LOG(ERR, "Could not enable Rx Queue %d", > > > i); > > > > + if (dev->data->dev_conf.rxmode.lock_mode) > > > > + return -1; > > > > + } > > > > +#endif > > > > > > > > > Why the code has to be different here? > > As you see this rxtx_start may have chance to fail. I want to expose th= is failure, > so the reset function can try again. > > > > > Thanks > > > Konstantin