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

[Feifei Wang <Feifei.Wang2@arm.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, April 19, 2023 10:46 PM
> To: Feifei Wang <Feifei.Wang2@arm.com>; thomas@monjalon.net; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org; konstantin.v.ananyev@yandex.ru;
> mb@smartsharesystems.com; nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>
> Subject: Re: [PATCH v5 1/3] ethdev: add API for buffer recycle mode
> 
> 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
> 
Agree.

> 
> > 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?
> 
Agree with this. And new name is 'rte_eth_tx_mbuf_reuse'

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

Thanks for the remind. Here I'll delete this check due to I realize it is unnecessary to check if users
have allocated memory for 'rxq_buf_recycle_info', which is an input parameter.

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

Two API is due to we need to separate Rx/TX path.  Then buffer recycle can support
the case of different pmds. For example, Rx port is i40e, and Tx port is ixgbe.