RE: [PATCH v1 3/5] ethdev: add API for direct rearm mode

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> >
> > On 4/20/22 11:16, Feifei Wang wrote:
> > > Add API for enabling direct rearm mode and for mapping RX and TX
> > > queues. Currently, the API supports 1:1(txq : rxq) mapping.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > ---
> > >   lib/ethdev/ethdev_driver.h | 15 +++++++++++++++
> > >   lib/ethdev/rte_ethdev.c    | 14 ++++++++++++++
> > >   lib/ethdev/rte_ethdev.h    | 31 +++++++++++++++++++++++++++++++
> > >   lib/ethdev/version.map     |  1 +
> > >   4 files changed, 61 insertions(+)
> > >
> > > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > > index 69d9dc21d8..22022f6da9 100644
> > > --- a/lib/ethdev/ethdev_driver.h
> > > +++ b/lib/ethdev/ethdev_driver.h
> > > @@ -485,6 +485,16 @@ typedef int (*eth_rx_enable_intr_t)(struct
> > rte_eth_dev *dev,
> > >   typedef int (*eth_rx_disable_intr_t)(struct rte_eth_dev *dev,
> > >   				    uint16_t rx_queue_id);
> > >
> > > +/** @internal Enable direct rearm of a receive queue of an Ethernet
> > > +device. */ typedef int (*eth_rx_direct_rearm_enable_t)(struct
> > rte_eth_dev *dev,
> > > +						uint16_t queue_id);
> > > +
> > > +/**< @internal map Rx/Tx queue of direct rearm mode */ typedef int
> > > +(*eth_rx_direct_rearm_map_t)(struct rte_eth_dev *dev,
> > > +					uint16_t rx_queue_id,
> > > +					uint16_t tx_port_id,
> > > +					uint16_t tx_queue_id);
> > > +
> > >   /** @internal Release memory resources allocated by given Rx/Tx queue.
> > */
> > >   typedef void (*eth_queue_release_t)(struct rte_eth_dev *dev,
> > >   				    uint16_t queue_id);
> > > @@ -1152,6 +1162,11 @@ struct eth_dev_ops {
> > >   	/** Disable Rx queue interrupt */
> > >   	eth_rx_disable_intr_t      rx_queue_intr_disable;
> > >
> > > +	/** Enable Rx queue direct rearm mode */
> > > +	eth_rx_direct_rearm_enable_t rx_queue_direct_rearm_enable;
> > > +	/** Map Rx/Tx queue for direct rearm mode */
> > > +	eth_rx_direct_rearm_map_t  rx_queue_direct_rearm_map;
> > > +
> > >   	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device Tx
> > queue */
> > >   	eth_queue_release_t        tx_queue_release; /**< Release Tx queue
> > */
> > >   	eth_tx_done_cleanup_t      tx_done_cleanup;/**< Free Tx ring mbufs
> > */
> > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
> > > 29a3d80466..8e6f0284f4 100644
> > > --- a/lib/ethdev/rte_ethdev.c
> > > +++ b/lib/ethdev/rte_ethdev.c
> > > @@ -2139,6 +2139,20 @@ rte_eth_tx_hairpin_queue_setup(uint16_t
> > port_id, uint16_t tx_queue_id,
> > >   	return eth_err(port_id, ret);
> > >   }
> > >
> > > +int
> > > +rte_eth_direct_rxrearm_map(uint16_t rx_port_id, uint16_t rx_queue_id,
> > > +		uint16_t tx_port_id, uint16_t tx_queue_id) {
> > > +	struct rte_eth_dev *dev;
> > > +
> > > +	dev = &rte_eth_devices[rx_port_id];
> >
> > I think it is rather control path. So:
> > We need standard checks that rx_port_id is valid.
> > tx_port_id must be checked as well.
> > rx_queue_id and tx_queue_id must be checked to be in the rate.
> [Feifei] You are right, I will add check for these.
> 
> >
> > > +	(*dev->dev_ops->rx_queue_direct_rearm_enable)(dev,
> > rx_queue_id);
> > > +	(*dev->dev_ops->rx_queue_direct_rearm_map)(dev, rx_queue_id,
> > > +			tx_port_id, tx_queue_id);
> >
> > We must check that function pointers are not NULL as usual.
> > Return values must be checked.
> [Feifei] I agree with this, The check for pointer and return value will be added
> 
> > Isn't is safe to setup map and than enable.
> > Otherwise we definitely need disable.
> [Feifei] I will change code that map first and then set 'rxq->offload' to enable
> direct-rearm mode.
> 
> > Also, what should happen on Tx port unplug? How to continue if we
> > still have Rx port up and running?
> [Feifei] For direct rearm mode, if Tx port unplug, it means there is no buffer
> from Tx.
> And then, Rx will put buffer from mempool as usual for rearm.
Andrew, when you say 'TX port unplug', do you mean the 'rte_eth_dev_tx_queue_stop' is called? Is calling 'rte_eth_dev_tx_queue_stop' allowed when the device is running?

> 
> >
> > > +
> > > +	return 0;
> > > +}
> > > +
<snip>