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

Ananyev, Konstantin konstantin.ananyev at intel.com
Mon Jun 13 12:47:35 CEST 2016


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].lock)
> > > > > 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



More information about the dev mailing list