From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Zhiheng Chen" <chenzhiheng0227@gmail.com>,
"Olivier Matz" <olivier.matz@6wind.com>,
"Andrew Rybchenko" <andrew.rybchenko@oktetlabs.ru>
Cc: <dev@dpdk.org>
Subject: RE: [PATCH v2] mempool: fix the description of some function return values
Date: Wed, 22 Dec 2021 12:45:30 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86D9D@smartserver.smartshare.dk> (raw)
In-Reply-To: <20211222082550.436-1-chenzhiheng0227@gmail.com>
> From: Zhiheng Chen [mailto:chenzhiheng0227@gmail.com]
> Sent: Wednesday, 22 December 2021 09.26
>
> Compared to patch version 1, this version updates the descriptions
> of underlying functions.
Some comments inline below, regarding the return value of success.
You should also update the description of the dequeue function prototype on line 451, so no one implements an alternative dequeue operation that returns anything else than -ENOBUFS as error value.
@Olivier, @Andrew: Do we want to impose this restriction on the API?
Otherwise, the patch should take the opposite direction, and update the descriptions of high level functions - i.e. rte_mempool_generic_get(), rte_mempool_get_bulk(), rte_mempool_get(), etc. - to reflect that any error value <0 can be returned, originating from the underlying function.
>
> In rte_mempool_ring.c, the committer uses the symbol ENOBUFS to
> describe the return value of function common_ring_sc_dequeue,
> but in rte_mempool.h, the symbol ENOENT is used to describe
> the return value of function rte_mempool_get. If the user of
> dpdk uses the symbol ENOENT as the judgment condition of
> the return value, it may cause some abnormal phenomena
> in their own programs, such as when the mempool space is exhausted.
>
> Fixes: ea5dd2744b90 ("mempool: cache optimisations")
>
> Signed-off-by: Zhiheng Chen <chenzhiheng0227@gmail.com>
> ---
> lib/mempool/rte_mempool.h | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 1e7a3c1527..3b52bd6737 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -737,8 +737,8 @@ rte_mempool_ops_alloc(struct rte_mempool *mp);
> * @param n
> * Number of objects to get.
> * @return
> - * - 0: Success; got n objects.
> - * - <0: Error; code of dequeue function.
> + * - >=0: Success; number of objects supplied.
> + * - -ENOBUFS: Not enough entries in the mempool; no object is
> retrieved.
NAK regarding success: Return value 0 is correct, >=0 cannot happen.
> */
> static inline int
> rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
> @@ -1453,7 +1453,7 @@ rte_mempool_put(struct rte_mempool *mp, void
> *obj)
> * A pointer to a mempool cache structure. May be NULL if not
> needed.
> * @return
> * - >=0: Success; number of objects supplied.
This is also wrong. It should be:
-* - >=0: Success; number of objects supplied.
+* - 0: Success; got n objects.
> - * - <0: Error; code of ring dequeue function.
> + * - -ENOBUFS: Not enough entries in the mempool; no object is
> retrieved.
> */
> static __rte_always_inline int
> rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table,
> @@ -1521,7 +1521,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.
> *
> @@ -1534,8 +1534,8 @@ rte_mempool_do_generic_get(struct rte_mempool
> *mp, void **obj_table,
> * @param cache
> * 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.
> + * - >=0: Success; number of objects supplied.
> + * - -ENOBUFS: Not enough entries in the mempool; no object is
> retrieved.
NAK regarding success: Return value 0 is correct, >=0 cannot happen.
> */
> static __rte_always_inline int
> rte_mempool_generic_get(struct rte_mempool *mp, void **obj_table,
> @@ -1557,7 +1557,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.
> *
> @@ -1568,8 +1568,8 @@ rte_mempool_generic_get(struct rte_mempool *mp,
> void **obj_table,
> * @param n
> * 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.
> + * - >=0: Success; number of objects supplied.
> + * - -ENOBUFS: Not enough entries in the mempool; no object is
> retrieved.
NAK regarding success: Return value 0 is correct, >=0 cannot happen.
> */
> static __rte_always_inline int
> rte_mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
> unsigned int n)
> @@ -1588,7 +1588,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.
> *
> @@ -1598,7 +1598,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.32.0
>
next prev parent reply other threads:[~2021-12-22 11:45 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-22 8:25 Zhiheng Chen
2021-12-22 11:45 ` Morten Brørup [this message]
2021-12-23 11:11 ` zhiheng chen
2021-12-23 10:07 ` [PATCH v3] " Zhiheng Chen
2022-01-24 17:04 ` Olivier Matz
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=98CBD80474FA8B44BF855DF32C47DC35D86D9D@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=chenzhiheng0227@gmail.com \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.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).