> From: Feifei Wang [mailto:feifei.wa...@arm.com]
> Sent: Thursday, 30 March 2023 08.30
>
[...]
> +/**
> + * @internal
> + * Rx routine for rte_eth_dev_buf_recycle().
> + * Refill Rx descriptors in buffer recycle mode.
> + *
> + * @note
> + * This API can only be called by rte_eth_dev_buf_recycle().
> + * Before calling this API, rte_eth_tx_buf_stash() should be
> + * called to stash Tx used buffers into Rx buffer ring.
> + *
> + * When this functionality is not implemented in the driver, the return
> + * buffer number is 0.
> + *
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + * @param queue_id
> + * The index of the receive queue.
> + * The value must be in the range [0, nb_rx_queue - 1] previously supplied
> + * to rte_eth_dev_configure().
> + *@param nb
> + * The number of Rx descriptors to be refilled.
> + * @return
> + * The number Rx descriptors correct to be refilled.
> + * - ENODEV: bad port or queue (only if compiled with debug).
If you want errors reported by the return value, the function return type
cannot be uint16_t.
> + */
> +static inline uint16_t rte_eth_rx_descriptors_refill(uint16_t port_id,
> + uint16_t queue_id, uint16_t nb)
> +{
> + 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);
> + rte_errno = ENODEV;
> + return 0;
If p->rx_descriptors_refill() is likely to return 0, this function should not
use 0 as return value to indicate errors.
> + }
> +#endif
> +
> + p = &rte_eth_fp_ops[port_id];
> + qd = p->rxq.data[queue_id];
> +
> +#ifdef RTE_ETHDEV_DEBUG_RX
> + if (!rte_eth_dev_is_valid_port(port_id)) {
> + RTE_ETHDEV_LOG(ERR, "Invalid Rx port_id=%u\n", port_id);
> + rte_errno = ENODEV;
> + return 0;
> +
> + if (qd == NULL) {
> + RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u for port_id=%u\n",
> + queue_id, port_id);
> + rte_errno = ENODEV;
> + return 0;
> + }
> +#endif
> +
> + if (p->rx_descriptors_refill == NULL)
> + return 0;
> +
> + return p->rx_descriptors_refill(qd, nb);
> +}
> +
> /**@{@name Rx hardware descriptor states
> * @see rte_eth_rx_descriptor_status
> */
> @@ -6483,6 +6597,122 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id,
> return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
> }
>
> +/**
> + * @internal
> + * Tx routine for rte_eth_dev_buf_recycle().
> + * Stash Tx used buffers into Rx buffer ring in buffer recycle mode.
> + *
> + * @note
> + * This API can only be called by rte_eth_dev_buf_recycle().
> + * After calling this API, rte_eth_rx_descriptors_refill() should be
> + * called to refill Rx ring descriptors.
> + *
> + * When this functionality is not implemented in the driver, the return
> + * buffer number is 0.
> + *
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + * @param queue_id
> + * The index of the transmit queue.
> + * The value must be in the range [0, nb_tx_queue - 1] previously supplied
> + * to rte_eth_dev_configure().
> + * @param rxq_buf_recycle_info
> + * A pointer to a structure of Rx queue buffer ring information in buffer
> + * recycle mode.
> + *
> + * @return
> + * The number buffers correct to be filled in the Rx buffer ring.
> + * - ENODEV: bad port or queue (only if compiled with debug).
If you want errors reported by the return value, the function return type
cannot be uint16_t.
> + */
> +static inline uint16_t rte_eth_tx_buf_stash(uint16_t port_id, uint16_t
> queue_id,
> + struct rte_eth_rxq_buf_recycle_info *rxq_buf_recycle_info)
> +{
> + struct rte_eth_fp_ops *p;
> + void *qd;
> +
> +#ifdef RTE_ETHDEV_DEBUG_TX
> + 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);
> + rte_errno = ENODEV;
> + return 0;
If p->tx_buf_stash() is likely to return 0, this function should not use 0 as
return value to indicate errors.
> + }
> +#endif
> +
> + p = &rte_eth_fp_ops[port_id];
> + qd = p->txq.data[queue_id];
> +
> +#ifdef RTE_ETHDEV_DEBUG_TX
> + if (!rte_eth_dev_is_valid_port(port_id)) {
> + RTE_ETHDEV_LOG(ERR, "Invalid Tx port_id=%u\n", port_id);
> + rte_errno = ENODEV;
> + return 0;
> +
> + if (qd == NULL) {
> + RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u for port_id=%u\n",
> + queue_id, port_id);
> + rte_erno = ENODEV;
> + return 0;
> + }
> +#endif
> +
> + if (p->tx_buf_stash == NULL)
> + return 0;
> +
> + return p->tx_buf_stash(qd, rxq_buf_recycle_info);
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Buffer recycle mode can let Tx queue directly put used buffers into Rx
> buffer
> + * ring. This avoids freeing buffers into mempool and allocating buffers from
> + * mempool.
This function description generally describes the buffer recycle mode.
Please update it to describe what this specific function does.
> + *
> + * @param rx_port_id
> + * Port identifying the receive side.
> + * @param rx_queue_id
> + * The index of the receive 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 tx_port_id
> + * Port identifying the transmit side.
> + * @param tx_queue_id
> + * The index of the transmit queue identifying the transmit side.
> + * The value must be in the range [0, nb_tx_queue - 1] previously supplied
> + * to rte_eth_dev_configure().
> + * @param rxq_recycle_info
> + * A pointer to a structure of type *rte_eth_txq_rearm_data* to be filled.
> + * @return
> + * - (0) on success or no recycling buffer.
Why not return the return value of rte_eth_rx_descriptors_refill() instead of 0
on success? (This is a question, not a suggestion.)
Or, if rxq_buf_recycle_info must be valid, the function return type could be
void instead of int.
> + * - (-EINVAL) rxq_recycle_info is NULL.
> + */
> +__rte_experimental
> +static inline int
> +rte_eth_dev_buf_recycle(uint16_t rx_port_id, uint16_t rx_queue_id,
> + uint16_t tx_port_id, uint16_t tx_queue_id,
> + struct rte_eth_rxq_buf_recycle_info *rxq_buf_recycle_info)
> +{
> + /* The number of recycling buffers. */
> + uint16_t nb_buf;
> +
> + if (!rxq_buf_recycle_info)
> + return -EINVAL;
This is a fast path function. In which situation is this function called with
rxq_buf_recycle_info == NULL?
If this function can genuinely be called with rxq_buf_recycle_info == NULL, you
should test for (rxq_buf_recycle_info == NULL), not (! rxq_buf_recycle_info).
Otherwise, I think RTE_ASSERT(rxq_buf_recycle_info != NULL) is more appropriate.
> +
> + /* Stash Tx used buffers into Rx buffer ring */
> + nb_buf = rte_eth_tx_buf_stash(tx_port_id, tx_queue_id,
> + rxq_buf_recycle_info);
> + /* If there are recycling buffers, refill Rx queue descriptors. */
> + if (nb_buf)
> + rte_eth_rx_descriptors_refill(rx_port_id, rx_queue_id,
> + nb_buf);
> +
> + return 0;
> +}
> +
> /**
> * @warning
> * @b EXPERIMENTAL: this API may change without prior notice
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> index dcf8adab92..a138fd4dbc 100644
> --- 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);
Please add proper function descriptions for the two callbacks above.
> +
> /**
> * @internal
> * Structure used to hold opaque pointers to internal ethdev Rx/Tx
> @@ -90,9 +97,11 @@ 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;
> + /** Refill Rx descriptors in buffer recycle mode */
> + eth_rx_descriptors_refill_t rx_descriptors_refill;
> /** Rx queues data. */
> struct rte_ethdev_qdata rxq;
> - uintptr_t reserved1[3];
> + uintptr_t reserved1[4];
You added a function pointer above, so to keep the structure alignment, you
must remove one here, not add one:
- uintptr_t reserved1[3];
+ uintptr_t reserved1[2];
> /**@}*/
>
> /**@{*/
> @@ -106,9 +115,11 @@ struct rte_eth_fp_ops {
> eth_tx_prep_t tx_pkt_prepare;
> /** Check the status of a Tx descriptor. */
> eth_tx_descriptor_status_t tx_descriptor_status;
> + /** Stash Tx used buffers into RX ring in buffer recycle mode */
> + eth_tx_buf_stash_t tx_buf_stash;
> /** Tx queues data. */
> struct rte_ethdev_qdata txq;
> - uintptr_t reserved2[3];
> + uintptr_t reserved2[4];
You added a function pointer above, so to keep the structure alignment, you
must remove one here, not add one:
- uintptr_t reserved1[3];
+ uintptr_t reserved1[2];
> /**@}*/
>
> } __rte_cache_aligned;
