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

[Feifei Wang <Feifei.Wang2@arm.com>]

> -----邮件原件-----
> 发件人: Morten Brørup <mb@smartsharesystems.com>
> 发送时间: Wednesday, April 20, 2022 5:59 PM
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; thomas@monjalon.net;
> Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>
> 主题: RE: [PATCH v1 3/5] ethdev: add API for direct rearm mode
> 
> > From: Feifei Wang [mailto:feifei.wang2@arm.com]
> > Sent: Wednesday, 20 April 2022 10.17
> >
> > 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;
> 
> A disable function seems to be missing.
[Feifei] I will try to use offload bits to enable direct-rearm mode, thus this enable function will be
removed and disable function will be unnecessary.

> 
> > +	/** 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];
> > +	(*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);
> 
> Here you enable the mapping before you configure it. It could cause the
> driver to use an uninitialized map, if it processes packets between these two
> function calls.
[Feifei] I agree with this and will change the code.

> 
> Error handling is missing. Not all drivers support this feature, and the
> parameters should be validated.
[Feifei] You are right, I think after we use 'rxq->offload' bits, we can use some 'offload  bits API'
to check if driver can support this.
For the parameters, I will add some check.

> 
> Regarding driver support, the driver should also expose a capability flag to the
> application, similar to the RTE_ETH_DEV_CAPA_RXQ_SHARE or
> RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE flags. The documentation for this
> flag could include the description of all the restrictions to using it.
[Feifei] I  will do like this by 'rxq->offload' bits, and add description to the documentation.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  int
> >  rte_eth_hairpin_bind(uint16_t tx_port, uint16_t rx_port)  { diff
> > --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> > 04cff8ee10..4a431fcbed 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -5190,6 +5190,37 @@ __rte_experimental  int
> > rte_eth_dev_hairpin_capability_get(uint16_t port_id,
> >  				       struct rte_eth_hairpin_cap *cap);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > notice
> > + *
> > + * Enable direct re-arm mode. In this mode the RX queue will be re-
> > armed using
> > + * buffers that have completed transmission on the transmit side.
> > + *
> > + * @note
> > + *   It is assumed that the buffers have completed transmission belong
> > to the
> > + *   mempool used at the receive side, and have refcnt = 1.
> > + *
> > + * @param rx_port_id
> > + *   Port identifying the receive side.
> > + * @param rx_queue_id
> > + *   The index of the receive queue identifying the receive side.
> > + *   The value must be in the range [0, nb_rx_queue - 1] previously
> > supplied
> > + *   to rte_eth_dev_configure().
> > + * @param tx_port_id
> > + *   Port identifying the transmit side.
> > + * @param tx_queue_id
> > + *   The index of the transmit queue identifying the transmit side.
> > + *   The value must be in the range [0, nb_tx_queue - 1] previously
> > supplied
> > + *   to rte_eth_dev_configure().
> > + *
> > + * @return
> > + *   - (0) if successful.
> > + */
> > +__rte_experimental
> > +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);
> > +
> 
> I agree with the parameters to your proposed API here. Since the relevant
> use case only needs 1:1 mapping, exposing an API function to take some sort
> of array with N:M mappings would be premature, and probably not ever come
> into play anyway.
> 
> How do you remove, disable and/or change a mapping?
[Feifei] It is not recommended that users change the map in the process of sending and receiving packets,
which may bring some error risks. If user want to change mapping, he needs to stop the device and call
' rte_eth_direct_rxrearm_map ' API to rewrite the mapping.
Furthermore, for 'rxq->offload', user needs to set it before dev starts. If user want to change it, dev needs to be restarted.
> 
> >  /**
> >   * @warning
> >   * @b EXPERIMENTAL: this structure may change without prior notice.
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index
> > 20391ab29e..68d664498c 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -279,6 +279,7 @@ EXPERIMENTAL {
> >  	rte_flow_async_action_handle_create;
> >  	rte_flow_async_action_handle_destroy;
> >  	rte_flow_async_action_handle_update;
> > +	rte_eth_direct_rxrearm_map;
> >  };
> >
> >  INTERNAL {
> > --
> > 2.25.1
> >