From: "Morten Brørup" <mb@smartsharesystems.com>
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
Date: Wed, 2 Aug 2023 10:58:05 +0200 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87AB7@smartserver.smartshare.dk> (raw)
In-Reply-To: <20230703061743.10576-1-rma.ma@jaguarmicro.com>
+CC various mempool driver maintainers
> From: Rma Ma [mailto:rma.ma@jaguarmicro.com]
> 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:
https://elixir.bootlin.com/dpdk/v23.07/C/ident/RTE_MEMPOOL_REGISTER_OPS
Some of the mempool drivers return -ENOBUFS, e.g. the ring driver and the stack driver:
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#L48
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#L59
However, I found one mempool driver (Marvell cnxk) that returns -ENOENT:
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#L69
And one mempool driver (Cavium OCTEON TX) returns -ENOMEM:
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#L111
One mempool driver (NXP dpaa) returns -ENOBUFS, or (if requesting too many objects) -1:
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#L225
https://elixir.bootlin.com/dpdk/v23.07/source/drivers/mempool/dpaa/dpaa_mempool.c#L257
And one mempool driver (NXP dpaa2) returns -ENOBUFS, or in rare cases -ENOENT or -1:
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#L373
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#L342
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:
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#L473
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.
next prev parent reply other threads:[~2023-08-02 8:58 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-03 6:17 Rma Ma
2023-08-02 2:20 ` Rma Ma
2023-08-02 8:58 ` Morten Brørup [this message]
2023-08-02 9:46 ` Rma Ma
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98CBD80474FA8B44BF855DF32C47DC35D87AB7@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=asekhar@marvell.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=hkalra@marvell.com \
--cc=olivier.matz@6wind.com \
--cc=pbhagavatula@marvell.com \
--cc=rma.ma@jaguarmicro.com \
--cc=sachin.saxena@oss.nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).