<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof"> > <o:p class="ContentPasted1"> </o:p> > So, how do we want to fix this:<o:p class="ContentPasted1"> </o:p> > <o:p class="ContentPasted1"> </o:p> > 1. Specify the allowed return values in the documentation for the mempool<o:p class="ContentPasted1"> </o:p> > library callback types, and require the mempool drivers to follow the updated<o:p class="ContentPasted1"> </o:p> > specification?<o:p class="ContentPasted1"> </o:p> > 2. Update mempool API documentation to list the many currently possible error<o:p class="ContentPasted1"> </o:p> > return values (-ENOBUFS, -ENOENT, -ENOMEM, and -EPERM (=-1))?<o:p class="ContentPasted1"> </o:p> > 3. Update mempool API documentation to say something along the lines of<o:p class="ContentPasted1"> </o:p> > "negative return value indicates error; rte_errno is not set if -1"?<o:p class="ContentPasted1"> </o:p> > 4. Switch to a general convention of returning -1 and setting rte_errno in all<o:p class="ContentPasted1"> </o:p> > DPDK APIs, starting with the mempool library? However; this would still require a<o:p class="ContentPasted1"> </o:p> > documented list of possible rte_errno values set by the functions - essentially<o:p class="ContentPasted1"> </o:p> > the problem would remain the same: "possible return values" vs. "possible<o:p class="ContentPasted1"> </o:p> > rte_errno values".<o:p class="ContentPasted1"> </o:p> > <o:p class="ContentPasted1"> </o:p> > Personally, I am in strong favor of any option that tightens the API specification<o:p class="ContentPasted1"> </o:p> > and treats non-conformance as bugs.<o:p class="ContentPasted1"> </o:p> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof ContentPasted2 ContentPasted3 ContentPasted4"> Thanks for your detailed reply! <div> </div> Couldn't agree more with your third option, which is much more generalized. </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof"> </div> <div class="elementToProof"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div id="Signature"> <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Best wishes, Rma </div> </div> </div> </div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Morten Brørup <mb@smartsharesystems.com> Sent: Wednesday, August 2, 2023 16:58 To: Rma Ma <rma.ma@jaguarmicro.com>; dpdk-dev <dev@dpdk.org>; Olivier Matz <olivier.matz@6wind.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> Cc: Ashwin Sekhar T K <asekhar@marvell.com>; Pavan Nikhilesh <pbhagavatula@marvell.com>; Harman Kalra <hkalra@marvell.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@oss.nxp.com> Subject: RE: [PATCH v1] mempool: fix some errors in html api <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">+CC various mempool driver maintainers > From: Rma Ma [<a href="mailto:rma.ma@jaguarmicro.com">mailto:rma.ma@jaguarmicro.com</a>] > Sent: Monday, 3 July 2023 08.18 > > This patch fix some error descriptions of > return value in mempool api which affect in html api. > > Signed-off-by: Rma Ma <rma.ma@jaguarmicro.com> > --- > lib/mempool/rte_mempool.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h > index 160975a7e7..d4d707533a 100644 > --- a/lib/mempool/rte_mempool.h > +++ b/lib/mempool/rte_mempool.h > @@ -1610,7 +1610,7 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void > **obj_table, > * Get several objects from the mempool. > * > * If cache is enabled, objects will be retrieved first from cache, > - * subsequently from the common pool. Note that it can return -ENOENT when > + * subsequently from the common pool. Note that it can return -ENOBUFS when > * the local cache and common pool are empty, even if cache from other > * lcores are full. > * > @@ -1624,7 +1624,7 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void > **obj_table, > * A pointer to a mempool cache structure. May be NULL if not needed. > * @return > * - 0: Success; objects taken. > - * - -ENOENT: Not enough entries in the mempool; no object is retrieved. > + * - -ENOBUFS: Not enough entries in the mempool; no object is retrieved. > */ > static __rte_always_inline int > rte_mempool_generic_get(struct rte_mempool *mp, void **obj_table, > @@ -1646,7 +1646,7 @@ rte_mempool_generic_get(struct rte_mempool *mp, void > **obj_table, > * mempool creation time (see flags). > * > * If cache is enabled, objects will be retrieved first from cache, > - * subsequently from the common pool. Note that it can return -ENOENT when > + * subsequently from the common pool. Note that it can return -ENOBUFS when > * the local cache and common pool are empty, even if cache from other > * lcores are full. > * > @@ -1658,7 +1658,7 @@ rte_mempool_generic_get(struct rte_mempool *mp, void > **obj_table, > * The number of objects to get from the mempool to obj_table. > * @return > * - 0: Success; objects taken > - * - -ENOENT: Not enough entries in the mempool; no object is retrieved. > + * - -ENOBUFS: Not enough entries in the mempool; no object is retrieved. > */ > static __rte_always_inline int > rte_mempool_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned int > n) > @@ -1677,7 +1677,7 @@ rte_mempool_get_bulk(struct rte_mempool *mp, void > **obj_table, unsigned int n) > * mempool creation (see flags). > * > * If cache is enabled, objects will be retrieved first from cache, > - * subsequently from the common pool. Note that it can return -ENOENT when > + * subsequently from the common pool. Note that it can return -ENOBUFS when > * the local cache and common pool are empty, even if cache from other > * lcores are full. > * > @@ -1687,7 +1687,7 @@ rte_mempool_get_bulk(struct rte_mempool *mp, void > **obj_table, unsigned int n) > * A pointer to a void * pointer (object) that will be filled. > * @return > * - 0: Success; objects taken. > - * - -ENOENT: Not enough entries in the mempool; no object is retrieved. > + * - -ENOBUFS: Not enough entries in the mempool; no object is retrieved. > */ > static __rte_always_inline int > rte_mempool_get(struct rte_mempool *mp, void **obj_p) > -- > 2.17.1 Unfortunately, it is not that simple... Here is the list of where mempool drivers are registered: <a href="https://elixir.bootlin.com/dpdk/v23.07/C/ident/RTE_MEMPOOL_REGISTER_OPS">https://elixir.bootlin.com/dpdk/v23.07/C/ident/RTE_MEMPOOL_REGISTER_OPS</a> Some of the mempool drivers return -ENOBUFS, e.g. the ring driver and the stack driver: <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/ring/rte_mempool_ring.c#L145">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/ring/rte_mempool_ring.c#L145</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/ring/rte_mempool_ring.c#L48">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/ring/rte_mempool_ring.c#L48</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/stack/rte_mempool_stack.c#L78">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/stack/rte_mempool_stack.c#L78</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/stack/rte_mempool_stack.c#L59">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/stack/rte_mempool_stack.c#L59</a> However, I found one mempool driver (Marvell cnxk) that returns -ENOENT: <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/cnxk/cn10k_hwpool_ops.c#L261">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/cnxk/cn10k_hwpool_ops.c#L261</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/cnxk/cn10k_hwpool_ops.c#L69">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/cnxk/cn10k_hwpool_ops.c#L69</a> And one mempool driver (Cavium OCTEON TX) returns -ENOMEM: <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/octeontx/rte_mempool_octeontx.c#L194">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/octeontx/rte_mempool_octeontx.c#L194</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/octeontx/rte_mempool_octeontx.c#L111">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/octeontx/rte_mempool_octeontx.c#L111</a> One mempool driver (NXP dpaa) returns -ENOBUFS, or (if requesting too many objects) -1: <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa/dpaa_mempool.c#L351">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa/dpaa_mempool.c#L351</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa/dpaa_mempool.c#L225">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa/dpaa_mempool.c#L225</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa/dpaa_mempool.c#L257">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa/dpaa_mempool.c#L257</a> And one mempool driver (NXP dpaa2) returns -ENOBUFS, or in rare cases -ENOENT or -1: <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L471">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L471</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L373">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L373</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L336">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L336</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L342">https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa2/dpaa2_hw_mempool.c#L342</a> The root cause for this misery is the general lack of documentation of callbacks in DPDK (not just in the mempool library!), which leaves the callback implementers unaware of what to return. E.g. the mempool driver enqueue and dequeue callback types have no descriptions of their return values: <a href="https://elixir.bootlin.com/dpdk/v23.07/source/lib/mempool/rte_mempool.h#L467">https://elixir.bootlin.com/dpdk/v23.07/source/lib/mempool/rte_mempool.h#L467</a> <a href="https://elixir.bootlin.com/dpdk/v23.07/source/lib/mempool/rte_mempool.h#L473">https://elixir.bootlin.com/dpdk/v23.07/source/lib/mempool/rte_mempool.h#L473</a> So in theory, the mempool drivers are free to return any value... Initially, I didn't think any mempool drivers would return -1 and set rte_errno, but apparently they do (except the driver doesn't set rte_errno when returning -1)! On a techboard meeting not long ago, Tyler mentioned the lack of consistency in return value conventions as an general annoyance. Now having looked at these mempool drivers, I realize that the situation is much worse than I would imagine in my worst nightmare! So, how do we want to fix this: 1. Specify the allowed return values in the documentation for the mempool library callback types, and require the mempool drivers to follow the updated specification? 2. Update mempool API documentation to list the many currently possible error return values (-ENOBUFS, -ENOENT, -ENOMEM, and -EPERM (=-1))? 3. Update mempool API documentation to say something along the lines of "negative return value indicates error; rte_errno is not set if -1"? 4. Switch to a general convention of returning -1 and setting rte_errno in all DPDK APIs, starting with the mempool library? However; this would still require a documented list of possible rte_errno values set by the functions - essentially the problem would remain the same: "possible return values" vs. "possible rte_errno values". Personally, I am in strong favor of any option that tightens the API specification and treats non-conformance as bugs. </div> </div> </body> </html>