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.nagaraha...@arm.com>
> Suggested-by: Ruifeng Wang <ruifeng.w...@arm.com>
> Signed-off-by: Feifei Wang <feifei.wa...@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.w...@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@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?
