Re: [PATCH v3 1/3] ethdev: enable direct rearm with separate API

[Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>]

Hi Feifei,

> Add 'tx_fill_sw_ring' and 'rx_flush_descriptor' API into direct rearm
> mode for separate Rx and Tx Operation. And this can support different
> multiple sources in direct rearm mode. For examples, Rx driver is ixgbe,
> and Tx driver is i40e.


Thanks for your effort and thanks for taking comments provided into 
consideration.
That approach looks much better then previous ones.
Few nits below.
Konstantin

> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Suggested-by: Ruifeng Wang <ruifeng.wang@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   |  10 ++
>   lib/ethdev/ethdev_private.c  |   2 +
>   lib/ethdev/rte_ethdev.c      |  52 +++++++++++
>   lib/ethdev/rte_ethdev.h      | 174 +++++++++++++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev_core.h |  11 +++
>   lib/ethdev/version.map       |   6 ++
>   6 files changed, 255 insertions(+)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 6a550cfc83..bc539ec862 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -59,6 +59,10 @@ struct rte_eth_dev {
>   	eth_rx_descriptor_status_t rx_descriptor_status;
>   	/** Check the status of a Tx descriptor */
>   	eth_tx_descriptor_status_t tx_descriptor_status;
> +	/** Fill Rx sw-ring with Tx buffers in direct rearm mode */
> +	eth_tx_fill_sw_ring_t tx_fill_sw_ring;
> +	/** Flush Rx descriptor in direct rearm mode */
> +	eth_rx_flush_descriptor_t rx_flush_descriptor;
>   
>   	/**
>   	 * Device data that is shared between primary and secondary processes
> @@ -504,6 +508,10 @@ typedef void (*eth_rxq_info_get_t)(struct rte_eth_dev *dev,
>   typedef void (*eth_txq_info_get_t)(struct rte_eth_dev *dev,
>   	uint16_t tx_queue_id, struct rte_eth_txq_info *qinfo);
>   
> +/**< @internal Get rearm data for a receive queue of an Ethernet device. */
> +typedef void (*eth_rxq_rearm_data_get_t)(struct rte_eth_dev *dev,
> +	uint16_t tx_queue_id, struct rte_eth_rxq_rearm_data *rxq_rearm_data);
> +
>   typedef int (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
>   	uint16_t queue_id, struct rte_eth_burst_mode *mode);
>   
> @@ -1215,6 +1223,8 @@ struct eth_dev_ops {
>   	eth_rxq_info_get_t         rxq_info_get;
>   	/** Retrieve Tx queue information */
>   	eth_txq_info_get_t         txq_info_get;
> +	/** Get Rx queue rearm data */
> +	eth_rxq_rearm_data_get_t   rxq_rearm_data_get;
>   	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get Rx burst mode */
>   	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get Tx burst mode */
>   	eth_fw_version_get_t       fw_version_get; /**< Get firmware version */
> diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> index 48090c879a..c5dd5e30f6 100644
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -276,6 +276,8 @@ eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
>   	fpo->rx_queue_count = dev->rx_queue_count;
>   	fpo->rx_descriptor_status = dev->rx_descriptor_status;
>   	fpo->tx_descriptor_status = dev->tx_descriptor_status;
> +	fpo->tx_fill_sw_ring = dev->tx_fill_sw_ring;
> +	fpo->rx_flush_descriptor = dev->rx_flush_descriptor;
>   
>   	fpo->rxq.data = dev->data->rx_queues;
>   	fpo->rxq.clbk = (void **)(uintptr_t)dev->post_rx_burst_cbs;
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 5d5e18db1e..2af5cb42fe 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -3282,6 +3282,21 @@ rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id, uint16_t rx_queue_id,
>   						stat_idx, STAT_QMAP_RX));
>   }
>   
> +int
> +rte_eth_dev_direct_rearm(uint16_t rx_port_id, uint16_t rx_queue_id,
> +		uint16_t tx_port_id, uint16_t tx_rx_queue_id,
> +		struct rte_eth_rxq_rearm_data *rxq_rearm_data)
> +{
> +	int nb_rearm = 0;
> +
> +	nb_rearm = rte_eth_tx_fill_sw_ring(tx_port_id, tx_rx_queue_id, rxq_rearm_data);
> +
> +	if (nb_rearm > 0)
> +		return rte_eth_rx_flush_descriptor(rx_port_id, rx_queue_id, nb_rearm);
> +
> +	return 0;
> +}
> +
>   int
>   rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size)
>   {
> @@ -5323,6 +5338,43 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>   	return 0;
>   }
>   
> +int
> +rte_eth_rx_queue_rearm_data_get(uint16_t port_id, uint16_t queue_id,
> +		struct rte_eth_rxq_rearm_data *rxq_rearm_data)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (queue_id >= dev->data->nb_rx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u\n", queue_id);
> +		return -EINVAL;
> +	}
> +
> +	if (rxq_rearm_data == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u Rx queue %u rearm data to NULL\n",
> +			port_id, queue_id);
> +		return -EINVAL;
> +	}
> +
> +	if (dev->data->rx_queues == NULL ||
> +			dev->data->rx_queues[queue_id] == NULL) {
> +		RTE_ETHDEV_LOG(ERR,
> +			   "Rx queue %"PRIu16" of device with port_id=%"
> +			   PRIu16" has not been setup\n",
> +			   queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
> +	if (*dev->dev_ops->rxq_rearm_data_get == NULL)
> +		return -ENOTSUP;
> +
> +	dev->dev_ops->rxq_rearm_data_get(dev, queue_id, rxq_rearm_data);
> +
> +	return 0;
> +}
> +
>   int
>   rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
>   			  struct rte_eth_burst_mode *mode)
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index c129ca1eaf..381c3d535f 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1818,6 +1818,17 @@ struct rte_eth_txq_info {
>   	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
>   } __rte_cache_min_aligned;
>   
> +/**
> + * @internal
> + * Structure used to hold pointers to internal ethdev Rx rearm data.
> + * The main purpose is to load Rx queue rearm data in direct rearm mode.
> + */

I think this structure owes a lot more expalantion.
What each fields suppose to do and what are the constraints, etc.

In general, more doc will be needed to explain that feature.
> +struct rte_eth_rxq_rearm_data {
> +	void *rx_sw_ring;

That's misleading, we always suppose to store mbufs ptrs here,
so why not be direct:
struct rte_mbuf **rx_sw_ring;

> +	uint16_t *rearm_start;
> +	uint16_t *rearm_nb;

I know that for Intel NICs uint16_t is sufficient,
wonder would it always be for other vendors?
Another thing to consider the case when ring position wrapping?
Again I know that it is not required for Intel NICs, but would
it be sufficient for API that supposed to be general?


> +} __rte_cache_min_aligned;
> +
>   /* Generic Burst mode flag definition, values can be ORed. */
>   
>   /**
> @@ -3184,6 +3195,34 @@ int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
>   					   uint16_t rx_queue_id,
>   					   uint8_t stat_idx);
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Directly put Tx freed buffers into Rx sw-ring and flush desc.
> + *
> + * @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().
> + * @param rxq_rearm_data
> + *   A pointer to a structure of type *rte_eth_txq_rearm_data* to be filled.
> + * @return
> + *   - 0: Success
> + */
> +__rte_experimental
> +int rte_eth_dev_direct_rearm(uint16_t rx_port_id, uint16_t rx_queue_id,
> +		uint16_t tx_port_id, uint16_t tx_queue_id,
> +		struct rte_eth_rxq_rearm_data *rxq_rearm_data);


I think we need one more parameter for that function 'uint16_t offset' 
or so.
So _rearm_ will start to populate rx_sw_ring from *rearm_start + offset
position. That way we can support populating from different sources.
Or should 'offset' be part of truct rte_eth_rxq_rearm_data?

> +
>   /**
>    * Retrieve the Ethernet address of an Ethernet device.
>    *
> @@ -4782,6 +4821,27 @@ int rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>   int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>   	struct rte_eth_txq_info *qinfo);
>   
> +/**
> + * Get rearm data about given ports's Rx queue.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The Rx queue on the Ethernet device for which rearm data
> + *   will be got.
> + * @param rxq_rearm_data
> + *   A pointer to a structure of type *rte_eth_txq_rearm_data* to be filled.
> + *
> + * @return
> + *   - 0: Success
> + *   - -ENODEV:  If *port_id* is invalid.
> + *   - -ENOTSUP: routine is not supported by the device PMD.
> + *   - -EINVAL:  The queue_id is out of range.
> + */
> +__rte_experimental
> +int rte_eth_rx_queue_rearm_data_get(uint16_t port_id, uint16_t queue_id,
> +		struct rte_eth_rxq_rearm_data *rxq_rearm_data);
> +
>   /**
>    * Retrieve information about the Rx packet burst mode.
>    *
> @@ -6103,6 +6163,120 @@ static inline int rte_eth_tx_descriptor_status(uint16_t port_id,
>   	return (*p->tx_descriptor_status)(qd, offset);
>   }
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Fill Rx sw-ring with Tx buffers in direct rearm mode.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The index of the transmit queue.
> + *   The value must be in the range [0, nb_tx_queue - 1] previously supplied
> + *   to rte_eth_dev_configure().
> + * @param rxq_rearm_data
> + *   A pointer to a structure of type *rte_eth_rxq_rearm_data* to be filled with
> + *   the rearm data of a receive queue.
> + * @return
> + *   - The number buffers correct to be filled in the Rx sw-ring.
> + *   - (-EINVAL) bad port or queue.
> + *   - (-ENODEV) bad port.
> + *   - (-ENOTSUP) if the device does not support this function.
> + *
> + */
> +__rte_experimental
> +static inline int rte_eth_tx_fill_sw_ring(uint16_t port_id,
> +	uint16_t queue_id, struct rte_eth_rxq_rearm_data *rxq_rearm_data)
> +{
> +	struct rte_eth_fp_ops *p;
> +	void *qd;
> +
> +#ifdef RTE_ETHDEV_DEBUG_TX
> +	if (port_id >= RTE_MAX_ETHPORTS ||
> +			queue_id >= RTE_MAX_QUEUES_PER_PORT) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Invalid port_id=%u or queue_id=%u\n",
> +			port_id, queue_id);
> +		return -EINVAL;
> +	}
> +#endif
> +
> +	p = &rte_eth_fp_ops[port_id];
> +	qd = p->txq.data[queue_id];
> +
> +#ifdef RTE_ETHDEV_DEBUG_TX
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> +
> +	if (qd == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u for port_id=%u\n",
> +			queue_id, port_id);
> +		return -ENODEV;
> +	}
> +#endif
> +
> +	if (p->tx_fill_sw_ring == NULL)
> +		return -ENOTSUP;
> +
> +	return p->tx_fill_sw_ring(qd, rxq_rearm_data);
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + *  Flush Rx descriptor in direct rearm mode.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The index of the receive queue.
> + *   The value must be in the range [0, nb_rx_queue - 1] previously supplied
> + *   to rte_eth_dev_configure().
> + *@param nb_rearm
> + *   The number of Rx sw-ring buffers need to be flushed.
> + * @return
> + *   - (0) if successful.
> + *   - (-EINVAL) bad port or queue.
> + *   - (-ENODEV) bad port.
> + *   - (-ENOTSUP) if the device does not support this function.
> + */
> +__rte_experimental
> +static inline int rte_eth_rx_flush_descriptor(uint16_t port_id,
> +	uint16_t queue_id, uint16_t nb_rearm)
> +{
> +	struct rte_eth_fp_ops *p;
> +	void *qd;
> +
> +#ifdef RTE_ETHDEV_DEBUG_RX
> +	if (port_id >= RTE_MAX_ETHPORTS ||
> +			queue_id >= RTE_MAX_QUEUES_PER_PORT) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Invalid port_id=%u or queue_id=%u\n",
> +			port_id, queue_id);
> +		return -EINVAL;
> +	}
> +#endif
> +
> +	p = &rte_eth_fp_ops[port_id];
> +	qd = p->rxq.data[queue_id];
> +
> +#ifdef RTE_ETHDEV_DEBUG_RX
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> +
> +	if (qd == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u for port_id=%u\n",
> +			queue_id, port_id);
> +		return -ENODEV;
> +	}
> +#endif
> +
> +	if (p->rx_flush_descriptor == NULL)
> +		return -ENOTSUP;
> +
> +	return p->rx_flush_descriptor(qd, nb_rearm);
> +}
> +
>   /**
>    * @internal
>    * Helper routine for rte_eth_tx_burst().
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> index dcf8adab92..5ecb57f6f0 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -56,6 +56,13 @@ typedef int (*eth_rx_descriptor_status_t)(void *rxq, uint16_t offset);
>   /** @internal Check the status of a Tx descriptor */
>   typedef int (*eth_tx_descriptor_status_t)(void *txq, uint16_t offset);
>   
> +/** @internal Fill Rx sw-ring with Tx buffers in direct rearm mode */
> +typedef int (*eth_tx_fill_sw_ring_t)(void *txq,
> +		struct rte_eth_rxq_rearm_data *rxq_rearm_data);
> +
> +/** @internal Flush Rx descriptor in direct rearm mode */
> +typedef int (*eth_rx_flush_descriptor_t)(void *rxq, uint16_t nb_rearm);
> +
>   /**
>    * @internal
>    * Structure used to hold opaque pointers to internal ethdev Rx/Tx
> @@ -90,6 +97,8 @@ struct rte_eth_fp_ops {
>   	eth_rx_queue_count_t rx_queue_count;
>   	/** Check the status of a Rx descriptor. */
>   	eth_rx_descriptor_status_t rx_descriptor_status;
> +	/** Flush Rx descriptor in direct rearm mode */
> +	eth_rx_flush_descriptor_t rx_flush_descriptor;
>   	/** Rx queues data. */
>   	struct rte_ethdev_qdata rxq;
>   	uintptr_t reserved1[3];
> @@ -106,6 +115,8 @@ struct rte_eth_fp_ops {
>   	eth_tx_prep_t tx_pkt_prepare;
>   	/** Check the status of a Tx descriptor. */
>   	eth_tx_descriptor_status_t tx_descriptor_status;
> +	/** Fill Rx sw-ring with Tx buffers in direct rearm mode */
> +	eth_tx_fill_sw_ring_t tx_fill_sw_ring;
>   	/** Tx queues data. */
>   	struct rte_ethdev_qdata txq;
>   	uintptr_t reserved2[3];
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 17201fbe0f..f39f02a69b 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -298,6 +298,12 @@ EXPERIMENTAL {
>   	rte_flow_get_q_aged_flows;
>   	rte_mtr_meter_policy_get;
>   	rte_mtr_meter_profile_get;
> +
> +	# added in 23.03
> +	rte_eth_dev_direct_rearm;
> +	rte_eth_rx_flush_descriptor;
> +	rte_eth_rx_queue_rearm_data_get;
> +	rte_eth_tx_fill_sw_ring;
>   };
>   
>   INTERNAL {