> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Wednesday, April 19, 2023 10:46 PM
> To: Feifei Wang <feifei.wa...@arm.com>; tho...@monjalon.net; Andrew
> Rybchenko <andrew.rybche...@oktetlabs.ru>
> Cc: dev@dpdk.org; konstantin.v.anan...@yandex.ru;
> m...@smartsharesystems.com; nd <n...@arm.com>; Honnappa Nagarahalli
> <honnappa.nagaraha...@arm.com>; Ruifeng Wang
> <ruifeng.w...@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.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().
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.
