Re: [PATCH v2 1/3] ethdev: add API for direct rearm mode

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

27/09/2022 03:47, Feifei Wang пишет:
> Add API for enabling direct rearm mode and for mapping RX and TX
> queues. Currently, the API supports 1:1(txq : rxq) mapping.
> 
> Furthermore, to avoid Rx load Tx data directly, add API called
> 'rte_eth_txq_data_get' to get Tx sw_ring and its information.
> 
> 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   |  9 ++++
>   lib/ethdev/ethdev_private.c  |  1 +
>   lib/ethdev/rte_ethdev.c      | 37 ++++++++++++++
>   lib/ethdev/rte_ethdev.h      | 95 ++++++++++++++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev_core.h |  5 ++
>   lib/ethdev/version.map       |  4 ++
>   6 files changed, 151 insertions(+)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 47a55a419e..14f52907c1 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -58,6 +58,8 @@ 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;
> +	/**  Use Tx mbufs for Rx to rearm */
> +	eth_rx_direct_rearm_t rx_direct_rearm;
>   
>   	/**
>   	 * Device data that is shared between primary and secondary processes
> @@ -486,6 +488,11 @@ 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 Get Tx information of a transmit queue of an Ethernet device. */
> +typedef void (*eth_txq_data_get_t)(struct rte_eth_dev *dev,
> +				      uint16_t tx_queue_id,
> +				      struct rte_eth_txq_data *txq_data);
> +
>   /** @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);
> @@ -1138,6 +1145,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 the address where Tx data is stored */
> +	eth_txq_data_get_t         txq_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..bfe16c7d77 100644
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -276,6 +276,7 @@ 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->rx_direct_rearm = dev->rx_direct_rearm;
>   
>   	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 0c2c1088c0..0dccec2e4b 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1648,6 +1648,43 @@ rte_eth_dev_is_removed(uint16_t port_id)
>   	return ret;
>   }
>   
> +int
> +rte_eth_tx_queue_data_get(uint16_t port_id, uint16_t queue_id,
> +			struct rte_eth_txq_data *txq_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_tx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u\n", queue_id);
> +		return -EINVAL;
> +	}
> +
> +	if (txq_data == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u Tx queue %u data to NULL\n",
> +			port_id, queue_id);
> +		return -EINVAL;
> +	}
> +
> +	if (dev->data->tx_queues == NULL ||
> +			dev->data->tx_queues[queue_id] == NULL) {
> +		RTE_ETHDEV_LOG(ERR,
> +			   "Tx queue %"PRIu16" of device with port_id=%"
> +			   PRIu16" has not been setup\n",
> +			   queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
> +	if (*dev->dev_ops->txq_data_get == NULL)
> +		return -ENOTSUP;
> +
> +	dev->dev_ops->txq_data_get(dev, queue_id, txq_data);
> +
> +	return 0;
> +}
> +
>   static int
>   rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg,
>   			     uint16_t n_seg, uint32_t *mbp_buf_size,
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 2e783536c1..daf7f05d62 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1949,6 +1949,23 @@ 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 Tx data.
> + * The main purpose is to load and store Tx queue data in direct rearm mode.
> + */
> +struct rte_eth_txq_data {
> +	uint64_t *offloads;
> +	void *tx_sw_ring;
> +	volatile void *tx_ring;
> +	uint16_t *tx_next_dd;
> +	uint16_t *nb_tx_free;
> +	uint16_t nb_tx_desc;
> +	uint16_t tx_rs_thresh;
> +	uint16_t tx_free_thresh;
> +} __rte_cache_min_aligned;
> +

first of all it is not clear why this struct has to be in public header,
why it can't be in on of ethdev 'private' headers.
Second it looks like a snippet from private txq fields for
some Intel (and alike) PMDs (i40e, ice, etc.).
How it supposed to to be universal and be applicable
for any PMD that decides to implement this new API?


>   /* Generic Burst mode flag definition, values can be ORed. */
>   
>   /**
> @@ -4718,6 +4735,27 @@ int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
>   int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
>   		const struct rte_eth_rxtx_callback *user_cb);
>   
> +/**
> + * Get the address which Tx data is stored.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The Tx queue on the Ethernet device for which information
> + *   will be retrieved.
> + * @param txq_data
> + *   A pointer to a structure of type *rte_eth_txq_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_tx_queue_data_get(uint16_t port_id, uint16_t queue_id,
> +		struct rte_eth_txq_data *txq_data);
> +
>   /**
>    * Retrieve information about given port's Rx queue.
>    *
> @@ -6209,6 +6247,63 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id,
>   	return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
>   }
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Put Tx buffers into Rx sw-ring and rearm descs.
> + *
> + * @param port_id
> + *   Port identifying the receive side.
> + * @param queue_id
> + *   The index of the transmit 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 txq_data
> + *   A pointer to a structure of type *rte_eth_txq_data* to be filled.
> + * @return
> + *   The number of direct-rearmed buffers.
> + */
> +__rte_experimental
> +static __rte_always_inline uint16_t
> +rte_eth_rx_direct_rearm(uint16_t port_id, uint16_t queue_id,
> +		struct rte_eth_txq_data *txq_data)
> +{
> +	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 0;
> +	}
> +#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 0;
> +	}
> +
> +	if (!p->rx_direct_rearm)

This check should be done always (unconditionally).
it is not a mandatory function for the driver (it can safely skip to 
implement it).

> +		return -ENOTSUP;

This function returns uint16_t, why signed integers here?


> +#endif
> +
> +	nb_rearm = p->rx_direct_rearm(qd, txq_data);

So rx_direct_rearm() function knows how to extract data from TX queue?
As I understand that is possible only in one case:
rx_direct_rearm() has full knowledge and acess of txq internals, etc.
That means that rxq and txq have to belong to the same driver and device 
type.

First of all, I still think it is not the best design choice.
If we going ahead with introducing this feature, it better be as generic 
as possible.
Plus it mixes TX and RX code-paths together,
while it would be much better to to keep them independent as they
are right now.

Another thing with such approach - even for the same PMD,
both TXQ and RXQ can have different internal data format and
behavior logic (depending on port/queue configuration).
So rx_direct_rearm() function selection have to be done
based on both RXQ and TXQ config.
So instead of rte_eth_tx_queue_data_get(), you'll probably need:
eth_rx_direct_rearm_t rte_eth_get_rx_direct_rearm_func(rx_port, 
rx_queue, tx_port, tx_queue);
Then, it will be user responsibility to store it somewhere
and call periodically:

control_path:
	...
	rearm_func = rte_eth_get_rx_direct_rearm_func(rxport, rxqueue,
		 txport, txqueue);
data-path:
	while(...) {
		rearm_func(rxport, txport, rxqueue, txqueue);
		rte_eth_rx_burst(rxport, rxqueue, ....);
		rte_eth_tx_burst(txport, txqueue, ....);
	}
	

In that case there seems absolutely no point to introduce
struct rte_eth_txq_data. rx_direct_rearm() accesses TXQ private data
directly anyway.

Another way - make rte_eth_txq_data totally opaque and allow PMD to
store there some data that will help it to distinguish expected TXQ
format. That will allow PMD to keep rx_direct_rearm() the same for
all supported TXQ formats (it will make decision internally based
on data stored in txq_data).
Though in that case you'll probably need one more dev-op to free 
txq_data.


> +
> +	return nb_rearm;
> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> index dcf8adab92..6a328e2481 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -56,6 +56,9 @@ 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 Put Tx buffers into Rx sw-ring and rearm descs */
> +typedef uint16_t (*eth_rx_direct_rearm_t)(void *rxq, struct rte_eth_txq_data *txq_data);
> +
>   /**
>    * @internal
>    * Structure used to hold opaque pointers to internal ethdev Rx/Tx
> @@ -90,6 +93,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;
> +	/** Put Tx buffers into Rx sw-ring and rearm descs */
> +	eth_rx_direct_rearm_t rx_direct_rearm;
>   	/** Rx queues data. */
>   	struct rte_ethdev_qdata rxq;
>   	uintptr_t reserved1[3];
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 03f52fee91..69e4c376d3 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -285,6 +285,10 @@ EXPERIMENTAL {
>   	rte_mtr_color_in_protocol_priority_get;
>   	rte_mtr_color_in_protocol_set;
>   	rte_mtr_meter_vlan_table_update;
> +
> +	# added in 22.11
> +	rte_eth_tx_queue_data_get;
> +	rte_eth_rx_direct_rearm;
>   };
>   
>   INTERNAL {