Hi Konstantin, > -----Original Message----- > From: Ananyev, Konstantin > Sent: Wednesday, June 8, 2016 5:22 PM > To: Ananyev, Konstantin; Lu, Wenzhuo; Tao, Zhe; dev at 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 > > > > > -----Original Message----- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, > > Konstantin > > Sent: Wednesday, June 08, 2016 9:42 AM > > To: Lu, Wenzhuo; Tao, Zhe; dev at 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 at 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 at 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 at 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 <wenzhuo.lu at intel.com> > > > > > > > > > > static int > > > > > +ixgbevf_dev_reset(struct rte_eth_dev *dev) { > > > > > + struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev- > >data- > > > > >dev_private); > > > > > + struct ixgbe_adapter *adapter = > > > > > + (struct ixgbe_adapter *)dev->data->dev_private; > > > > > + int diag = 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 = dev->rx_pkt_burst; > > > > > + adapter->tx_backup = dev->tx_pkt_burst; > > > > > + dev->rx_pkt_burst = ixgbevf_recv_pkts_fake; > > > > > + dev->tx_pkt_burst = 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 > > > queues > will run into the fake functions. So we need not wait for them and get the > locks > directly. > > > > Well, data-path invokes only try_lock(), so it shouldn't be affected > > significantly, > 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 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 > > > > > > > > > > > > > > + > > > > > + if (dev->data->rx_queues) > > > > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > > > > + rxq = dev->data->rx_queues[i]; > > > > > + rte_spinlock_lock(&rxq->rx_lock); > > > > > + } > > > > > + > > > > > + if (dev->data->tx_queues) > > > > > + for (i = 0; i < dev->data->nb_tx_queues; i++) { > > > > > + txq = 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 = 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. > > Still not sure I understand what do you mean here... > If you think function should fail here, then why only for lcok enabled, why > not to > make that change generic? O, I should make it generic. I'll change it.
> > > > > > > > Thanks > > > > Konstantin
