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

[Ananyev, Konstantin]

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 :)

Sounds reaosanable to me :)
Konstantin