[dpdk-dev] [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode

[Lu, Wenzhuo]

Hi Konstantin,


> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, June 13, 2016 6:48 PM
> To: Lu, Wenzhuo; Tao, Zhe; dev at dpdk.org
> Cc: Richardson, Bruce; Chen, Jing D; Liang, Cunming; Wu, Jingjing; Zhang,
> Helin
> Subject: RE: [PATCH v4 2/8] lib/librte_ether: defind RX/TX lock mode
> 
> Hi Wenzhuo,
> 
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > 3.  I thought the plan was to introduce a locking in all
> > > > > > > appropriate control path functions (dev_start/dev_stop etc.)
> > > > > > > Without that locking version of RX/TX seems a bit useless.
> > > > > > > Yes, I understand that you do use locking inside dev_reset,
> > > > > > > but I suppose the plan was to have a generic solution, no?
> > > > > > > Again, interrupt fire when user invokes dev_start/stop or
> > > > > > > so, so we still need some synchronisation between them.
> > > > > > >
> > > > > > > To be more specific, I thought about something like that:
> > > > > > >
> > > > > > > static inline uint16_t
> > > > > > > rte_eth_rx_burst_lock(uint8_t port_id, uint16_t queue_id,
> > > > > > >                  struct rte_mbuf **rx_pkts, const uint16_t nb_pkts) {
> > > > > > >         struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > > > > >
> > > > > > > #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > > > > >         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > > > > > >         RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
> > > > > > >
> > > > > > >         if (queue_id >= dev->data->nb_rx_queues) {
> > > > > > >                 RTE_PMD_DEBUG_TRACE("Invalid RX
> > > > > > > queue_id=%d\n",
> > > queue_id);
> > > > > > >                 return 0;
> > > > > > >         }
> > > > > > > #endif
> > > > > > >
> > > > > > > + if
> > > > > > > + (rte_spinlock_trylock(&dev->data-
> >rx_queue_state[rx_queue_id].
> > > > > > > + lock)
> > > > > == 0)
> > > > > > > +	return 0;
> > > > > > > +  else if (dev->data->rx_queue_state[rx_queue_id] ==
> > > > > > > RTE_ETH_QUEUE_STATE_STOPPED)) {
> > > > > > > +	rte_spinlock_unlock(&dev->data-
> > > >rx_queue_state[rx_queue_id].unlock);
> > > > > > > +	return 0;
> > > > > > > +
> > > > > > >
> > > > > > >  nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> > > > > > >                         rx_pkts, nb_pkts);
> > > > > > >
> > > > > > > + rte_spinlock_unlock(&dev->data-
> > > >rx_queue_state[rx_queue_id].un
> > > > > > > + lock
> > > > > > > + );
> > > > > > >
> > > > > > > ....
> > > > > > >
> > > > > > > return nb_rx;
> > > > > > > }
> > > > > > >
> > > > > > > And inside queue_start:
> > > > > > >
> > > > > > > int
> > > > > > > rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t
> > > > > > > rx_queue_id)
> > > {
> > > > > > >         struct rte_eth_dev *dev;
> > > > > > >
> > > > > > >         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > > > > > >
> > > > > > >         dev = &rte_eth_devices[port_id];
> > > > > > >         if (rx_queue_id >= dev->data->nb_rx_queues) {
> > > > > > >                 RTE_PMD_DEBUG_TRACE("Invalid RX
> > > > > > > queue_id=%d\n",
> > > > > rx_queue_id);
> > > > > > >                 return -EINVAL;
> > > > > > >         }
> > > > > > >
> > > > > > >
> > > > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start,
> > > > > > > -ENOTSUP);
> > > > > > >
> > > > > > >
> > > > > > > rte_spinlock_lock(&dev->data->rx_queue_state[rx_queue_id].lo
> > > > > > > ck)
> > > > > > I think you add the lock here to stop the rx/tx.
> > > > > > But to my opinion, we should lock the rx/tx much earlier
> > > > > > before starting the queue. For example, when stop the port,
> > > > > > the resource of the
> > > > > queues may be released.
> > > > >
> > > > > I didn't get you here...
> > > > > Before releasing the queue resources, queue_stop() has to be
> > > > > executed,
> > > right?
> > > > Sorry, I saw your example with rte_eth_dev_rx_queue_start, I
> > > > didn't know you also want to change rte_eth_dev_rx_queue_stop too.
> > > > Agree this should work it we call queue_start/stop when reset the
> > > > port. But we will not call them. I find the queue_stop/start are
> > > > per- queue
> > > functions and not supported by all NICs.
> > >
> > > But right now you do reset only for ixgbe/i40e.
> > Not only for ixgbe/i40e. You forget igb, which doesn't support
> > queue_start/stop :)
> >
> > > For these devices we defiantly do support queue start/stop.
> > > And again, it is not only about reset op.
> > > If we want to add rx locked (synced), I think it should be in sync
> > > with all control API that changes queue state.
> > > As I said before it is a lot of work and a lot of hassle...
> > > So probably the easiest (and might be safiest) way just leave things
> > > as there are right now:
> > > we allow user to setup a callback on VF reset, and it is user
> > > responsibility to make sure no RX/TX is active while reset operation is
> performed.
> > > Pretty much what Olivier and Stephen suggested, as I understand.
> > Agree. It's not a good way to add lock for just one feature. It could
> > be tricky if we want to extend the lock to other features. A whole picture
> is needed.
> > We've sent another patch set to let the user setup a callback on VF
> > reset. Depend on that, user can use existing rte APIs to reset the VF port.
> But how about your opinion if we add a specific rte_reset API? It may be
> easier for the user.
> 
> You mean add rte_eth_dev_reset() without any locking inside?
> So it when VF reset happens, it would be user responsibility to make sure
> all IO over that device is stopped, and then he can call rte_eth_dev_reset(),
> correct?
> Konstantin
Yes, that's exactly what I plan to do :)