[dpdk-dev] [PATCH v4 4/8] ixgbe: implement device reset on VF

Ananyev, Konstantin konstantin.ananyev at intel.com
Wed Jun 8 11:22:25 CEST 2016



> -----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?

> >
> > > Thanks
> > > Konstantin



More information about the dev mailing list