From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8861C42871; Thu, 30 Mar 2023 17:58:32 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6058140E25; Thu, 30 Mar 2023 17:58:32 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 8851140DF8 for ; Thu, 30 Mar 2023 17:58:31 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v5 1/3] ethdev: add API for buffer recycle mode Date: Thu, 30 Mar 2023 17:58:30 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8782F@smartserver.smartshare.dk> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 1/3] ethdev: add API for buffer recycle mode Thread-Index: AQHZYtEPO3yD/9FA0E+nLV0H3CV5M68S6kqAgAAXoRCAAGCxwIAAFbcw References: <20211224164613.32569-1-feifei.wang2@arm.com> <20230330062939.1206267-1-feifei.wang2@arm.com> <20230330062939.1206267-2-feifei.wang2@arm.com> <98CBD80474FA8B44BF855DF32C47DC35D8781F@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Feifei Wang" , , "Ferruh Yigit" , "Andrew Rybchenko" Cc: , , "nd" , "Honnappa Nagarahalli" , "Ruifeng Wang" , "nd" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Morten Br=F8rup > Sent: Thursday, 30 March 2023 17.15 >=20 > > From: Feifei Wang [mailto:Feifei.Wang2@arm.com] > > Sent: Thursday, 30 March 2023 11.31 > > > > > From: Morten Br=F8rup > > > 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 >=3D RTE_MAX_ETHPORTS || > > > > + queue_id >=3D RTE_MAX_QUEUES_PER_PORT) { > > > > + RTE_ETHDEV_LOG(ERR, > > > > + "Invalid port_id=3D%u or queue_id=3D%u\n", > > > > + port_id, queue_id); > > > > + rte_errno =3D 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. >=20 > OK. Thanks for the references. >=20 > 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. >=20 > 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. >=20 > > > > > > > > > + } > > > > +#endif > > > > + > > > > + p =3D &rte_eth_fp_ops[port_id]; > > > > + qd =3D 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=3D%u\n", port_id); > > > > + rte_errno =3D ENODEV; > > > > + return 0; > > > > + > > > > + if (qd =3D=3D NULL) { > > > > + RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=3D%u for > > > port_id=3D%u\n", > > > > + queue_id, port_id); > > > > + rte_errno =3D ENODEV; > > > > + return 0; > > > > + } > > > > +#endif > > > > + > > > > + if (p->rx_descriptors_refill =3D=3D NULL) > > > > + return 0; > > > > + > > > > + return p->rx_descriptors_refill(qd, nb); } >=20 > When does p->rx_descriptors_refill() return anything else than 'nb'? >=20 > 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. >=20 > > > > + > > > > /**@{@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. >=20 > I now see that you follow the convention of rte_eth_tx_prepare() here = too. >=20 > 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. >=20 > > > > > > > + */ > > > > +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 >=3D RTE_MAX_ETHPORTS || > > > > + queue_id >=3D RTE_MAX_QUEUES_PER_PORT) { > > > > + RTE_ETHDEV_LOG(ERR, > > > > + "Invalid port_id=3D%u or queue_id=3D%u\n", > > > > + port_id, queue_id); > > > > + rte_errno =3D 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. >=20 > 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. >=20 > > > > > > > + } > > > > +#endif > > > > + > > > > + p =3D &rte_eth_fp_ops[port_id]; > > > > + qd =3D 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=3D%u\n", port_id); > > > > + rte_errno =3D ENODEV; > > > > + return 0; > > > > + > > > > + if (qd =3D=3D NULL) { > > > > + RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=3D%u for > > > port_id=3D%u\n", > > > > + queue_id, port_id); > > > > + rte_erno =3D ENODEV; > > > > + return 0; > > > > + } > > > > +#endif > > > > + > > > > + if (p->tx_buf_stash =3D=3D 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. >=20 > If the user forgets to allocate the rxq_buf_recycle_info, it is a = serious bug > in the user's application. >=20 > We don't need to handle such bugs at runtime. >=20 > > > > Furthermore, the return value of this API should return success or = not. >=20 > If rxq_buf_recycle_info is not NULL, this function will always = succeed. So > there is no need for a return value. >=20 > If you want this function to return something, it could return nb_buf, = so the > application can it use for telemetry purposes or similar. >=20 > > > > > > + * - (-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 =3D=3D NULL? > > > > > > If this function can genuinely be called with rxq_buf_recycle_info = =3D=3D > NULL, > > > you should test for (rxq_buf_recycle_info =3D=3D NULL), not (! > > > rxq_buf_recycle_info). Otherwise, I think > > > RTE_ASSERT(rxq_buf_recycle_info !=3D NULL) is more appropriate. > > Agree. We should use ' RTE_ASSERT(rxq_buf_recycle_info !=3D NULL)'. > > > > > > > + > > > > + /* Stash Tx used buffers into Rx buffer ring */ > > > > + nb_buf =3D 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;