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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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.

> +	/** 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.

Error handling is missing. Not all drivers support this feature, and the parameters should be validated.

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.

> +
> +	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?

>  /**
>   * @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
>