From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 773E82142 for ; Sun, 12 Jun 2016 03:03:16 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP; 11 Jun 2016 18:03:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,459,1459839600"; d="scan'208";a="717654476" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 11 Jun 2016 18:03:15 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 11 Jun 2016 18:03:15 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 11 Jun 2016 18:03:14 -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; Sun, 12 Jun 2016 09:03:13 +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///PGAIAACy6AgAZCwJA= Date: Sun, 12 Jun 2016 01:03:12 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC09090348492D@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> <2601191342CEEE43887BDE71AB97725836B6CD0F@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836B6CD0F@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 01:03:17 -0000 Hi Konstantin, > -----Original Message----- > From: Ananyev, Konstantin > Sent: Wednesday, June 8, 2016 5:22 PM > To: Ananyev, Konstantin; 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: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, > > Konstantin > > Sent: Wednesday, June 08, 2016 9:42 AM > > To: Lu, Wenzhuo; Tao, Zhe; dev@dpdk.org > > Cc: Richardson, Bruce; Chen, Jing D; Liang, Cunming; Wu, Jingjing; > > Zhang, Helin > > Subject: Re: [dpdk-dev] [PATCH v4 4/8] ixgbe: implement device reset > > on VF > > > > > > > > > -----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 th= e locks. > > > As you see, we want to lock every queue. If we don't use fake functio= ns 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 oth= er queues > will run into the fake functions. So we need not wait for them and get th= e locks > directly. > > > > Well, data-path invokes only try_lock(), so it shouldn't be affected si= gnificantly, > 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/datapath API anyway, we don't need any additional tricks here w= ith > rx/tx function replacement, correct? > > So let's keep it clean and simple, after all it is a control path and n= ot need to be > lightning fast. > > Konstantin > > > > > > > > > > > > > > + > > > > > + 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= _ethdev. > > > 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 = this > failure, so the reset function can try again. >=20 > Still not sure I understand what do you mean here... > If you think function should fail here, then why only for lcok enabled, w= hy not to > make that change generic? O, I should make it generic. I'll change it. >=20 > > > > > > > Thanks > > > > Konstantin