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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Morten Brørup
> Sent: Thursday, 30 March 2023 17.15
> 
> > From: Feifei Wang [mailto:Feifei.Wang2@arm.com]
> > Sent: Thursday, 30 March 2023 11.31
> >
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Thursday, March 30, 2023 3:19 PM
> > >
> > > > From: Feifei Wang [mailto:feifei.wang2@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.
> > Agree. Actually, in the code path, if errors happen, the function will
> return
> > 0.
> > For this description line, I refer to 'rte_eth_tx_prepare' notes. Maybe we
> > should delete
> > this line.
> >
> > >
> > > > + */
> > > > +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.
> > However, refer to dpdk code style in ethdev, most of API write like this.
> > For example, 'rte_eth_rx/tx_burst', 'rte_eth_tx_prep'.
> >
> > I'm also confused what's return type for this due to I want
> > to indicate errors and show the processed buffer number.
> 
> OK. Thanks for the references.
> 
> Looking at rte_eth_rx/tx_burst(), you could follow the same conventions here,
> i.e.:
> - Use uint16_t as return type.
> - Return 0 on error.
> - Do not set rte_errno.
> - Remove the "ENODEV" line from the @return description.
> - Use RTE_ETHDEV_LOG(ERR,...) as the only method to indicate errors.
> 
> I now see that you follow the convention of rte_eth_tx_prepare(). This is also
> perfectly fine; then you just need to update the description of @return to
> mention that the error value is set in rte_errno if a value less than 'nb' is
> returned.

After further consideration, I have changed my mind:

The primary purpose of rte_eth_tx_prepare() is to test if a packet burst is valid, so the ability to return an error value is a natural requirement.

This is not the purpose your functions. The purpose of your functions resemble rte_eth_rx/tx_burst(), where there is no requirement to return an error value. So you should follow the convention of rte_eth_rx/tx_burst(), as I just suggested.

> 
> >
> > >
> > > > +	}
> > > > +#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); }
> 
> When does p->rx_descriptors_refill() return anything else than 'nb'?
> 
> If p->rx_descriptors_refill() always succeeds (and thus always returns 'nb'),
> you could make its return type void. And thus, you could also make the return
> type of rte_eth_rx_descriptors_refill() void.
> 
> > > > +
> > > >  /**@{@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.
> 
> I now see that you follow the convention of rte_eth_tx_prepare() here too.
> 
> This is perfectly fine; then you just need to update the description of
> @return to mention that the error value is set in rte_errno if a value less
> than 'nb' is returned.

Same comment about conventions as above: This function is more like rte_eth_rx/tx_burst() than rte_eth_tx_prepare(), so follow the convention of rte_eth_rx/tx_burst() instead.

> 
> > >
> > > > + */
> > > > +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.
> 
> I now see that you follow the convention of rte_eth_tx_prepare() here too.
> Then please ignore my comment about using 0 as return value on errors for this
> function.

Same comment about conventions as above: This function is more like rte_eth_rx/tx_burst() than rte_eth_tx_prepare(), so follow the convention of rte_eth_rx/tx_burst() instead.

> 
> > >
> > > > +	}
> > > > +#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.
> > Ack.
> > >
> > > > + *
> > > > + * @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.
> > >
> > In some time, users may forget to allocate the room for '
> rxq_buf_recycle_info
> > '
> > and call 'rte_rxq_buf_recycle_info_get' API. Thus, I think we need to check
> > with this.
> 
> If the user forgets to allocate the rxq_buf_recycle_info, it is a serious bug
> in the user's application.
> 
> We don't need to handle such bugs at runtime.
> 
> >
> > Furthermore, the return value of this API should return success or not.
> 
> If rxq_buf_recycle_info is not NULL, this function will always succeed. So
> there is no need for a return value.
> 
> If you want this function to return something, it could return nb_buf, so the
> application can it use for telemetry purposes or similar.
> 
> >
> > > > + *   - (-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.
> > Agree. We should use ' RTE_ASSERT(rxq_buf_recycle_info != NULL)'.
> > >
> > > > +
> > > > +	/* 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.
> > Ack.
> > >
> > > > +
> > > >  /**
> > > >   * @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];
> > >
> > Ack.
> > > >  	/**@}*/
> > > >
> > > >  	/**@{*/
> > > > @@ -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];
> > >
> > Ack.
> > > >  	/**@}*/
> > > >
> > > >  } __rte_cache_aligned;