From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 6247C9604 for ; Wed, 8 Jun 2016 10:42:27 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 08 Jun 2016 01:42:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,438,1459839600"; d="scan'208";a="715754069" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by FMSMGA003.fm.intel.com with ESMTP; 08 Jun 2016 01:42:25 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by IRSMSX154.ger.corp.intel.com ([169.254.12.28]) with mapi id 14.03.0248.002; Wed, 8 Jun 2016 09:42:25 +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 4/8] ixgbe: implement device reset on VF Thread-Index: AQHRwIlTHTaFpEmCdk+LcfWQ56/Q1p/dxWqQgAFWTQCAACVVIA== Date: Wed, 8 Jun 2016 08:42:24 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B6CC99@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-5-git-send-email-zhe.tao@intel.com> <2601191342CEEE43887BDE71AB97725836B6C46D@irsmsx105.ger.corp.intel.com> <6A0DE07E22DDAD4C9103DF62FEBC090903483A3D@shsmsx102.ccr.corp.intel.com> In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC090903483A3D@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 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: Wed, 08 Jun 2016 08:42:27 -0000 > -----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 >=20 > Hi Konstantin, >=20 > > -----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, J= ingjing; > > 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 f= ake > > functions? > The fake functions are used to help saving the time of waiting for the lo= cks. > As you see, we want to lock every queue. If we don't use fake functions w= e 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 queues will run into the fake functions. So we need not wait fo= r them and get the locks directly. Well, data-path invokes only try_lock(), so it shouldn't be affected signif= icantly, 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/dat= apath API anyway, we don't need any additional tricks here with rx/tx function replacement, c= orrect? So let's keep it clean and simple, after all it is a control path and not n= eed to be lightning fast. Konstantin >=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_eth= dev. > We're discussing it in the previous thread :) >=20 > > > > > > @@ -5235,11 +5243,21 @@ ixgbevf_dev_rxtx_start(struct rte_eth_dev *de= v) > > > 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 > > Thanks > > Konstantin