Re: [PATCH v5 1/3] ethdev: add API for buffer recycle mode

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 3/30/2023 7:29 AM, Feifei Wang wrote:
> There are 4 upper APIs for buffer recycle mode:
> 1. 'rte_eth_rx_queue_buf_recycle_info_get'
> This is to retrieve buffer ring information about given ports's Rx
> queue in buffer recycle mode. And due to this, buffer recycle can
> be no longer limited to the same driver in Rx and Tx.
> 
> 2. 'rte_eth_dev_buf_recycle'
> Users can call this API to enable buffer recycle mode in data path.
> There are 2 internal APIs in it, which is separately for Rx and TX.
> 

Overall, can we have a namespace in the functions related to the buffer
recycle, to clarify API usage, something like (just putting as sample to
clarify my point):

rte_eth_recycle_buf
rte_eth_recycle_tx_buf_stash
rte_eth_recycle_rx_descriptors_refill
rte_eth_recycle_rx_queue_info_get


> 3. 'rte_eth_tx_buf_stash'
> Internal API for buffer recycle mode. This is to stash Tx used
> buffers into Rx buffer ring.
> 

This API is to move/recycle descriptors from Tx queue to Rx queue, but
name on its own, 'rte_eth_tx_buf_stash', reads like we are stashing
something to Tx queue. What do you think, can naming be improved?

> 4. 'rte_eth_rx_descriptors_refill'
> Internal API for buffer recycle mode. This is to refill Rx
> descriptors.
> 
> Above all APIs are just implemented at the upper level.
> For different APIs, we need to define specific functions separately.
> 
> 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>

<...>

>  
> +int
> +rte_eth_rx_queue_buf_recycle_info_get(uint16_t port_id, uint16_t queue_id,
> +		struct rte_eth_rxq_buf_recycle_info *rxq_buf_recycle_info)
> +{
> +	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;
> +	}
> +
> +	RTE_ASSERT(rxq_buf_recycle_info != NULL);
> +

This is slow path API, I think better to validate parameter and return
an error instead of assert().

<...>

> --- 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 Stash Tx used buffers into RX ring in buffer recycle mode */
> +typedef uint16_t (*eth_tx_buf_stash_t)(void *txq,
> +		struct rte_eth_rxq_buf_recycle_info *rxq_buf_recycle_info);
> +
> +/** @internal Refill Rx descriptors in buffer recycle mode */
> +typedef uint16_t (*eth_rx_descriptors_refill_t)(void *rxq, uint16_t nb);
> +

Since there is only single API exposed to the application, is it really
required to have two dev_ops, why not have a single one?