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 9AB91A04A3; Mon, 3 Jan 2022 15:31:28 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E5FD9410E0; Mon, 3 Jan 2022 15:31:24 +0100 (CET) Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by mails.dpdk.org (Postfix) with ESMTP id 4987240DDA for ; Thu, 23 Dec 2021 12:11:40 +0100 (CET) Received: by mail-wr1-f44.google.com with SMTP id v11so10721047wrw.10 for ; Thu, 23 Dec 2021 03:11:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=NmwCSwMyyLLEifvSlzsBi4yFk9ogBR0zSG3NmBwD4ZM=; b=lGGq0wcvK/Eq+cwBH57bxISlgyft1NeGzHy0cakZjPyuaGtM5qdBNhDToMbGQEKdfS HMzffTUI4J0rqEfwJRm0D0LPZgF07ZGWxM0keq+p/E12lR0Wzwse2B26BOpRkBF2smoU aFIu+k8RDTwUbknDfy4RhS/+ECVdso/r7o36PIHWqdu2xLsrXrFgszXc/isW/v0NiFkj yvSHjYR7IDruZe3GN/KBHpCVOj75HBpoR2whVE5mnV5eW3ophSIrPCOkMPFvbbpxMX/K hqbGna3JJH6otP2dJe7bCpR6cK/6sfWtQ1qdu6qMcnU2gPD7hMynLLgrxSw0QyPtYXN6 8hjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=NmwCSwMyyLLEifvSlzsBi4yFk9ogBR0zSG3NmBwD4ZM=; b=cGPxtwH5lJeMOS7FgRaW/Nh20L7FrWDvoIHzXg5ZLH3bcJLiPv7DbBHYtJNVJl+Zym DI0syjRBXwdH/F3n8xMGw+Icr8Q3q4PaHZxvlXEhXyPohm6T/+OarctvKDY/I6XoX31V W6BKHBgJ6gioStf89FaQ3TeLGxVvYnEEdUknMUhejc9jddH2dFaxFXXk5l2NJ/aIl+76 Tv8RMPA+7ABPjp07ps3LGhy753N+WSBSQtGk17GYYlrzhpelAhvuozRep0Sh+jSQGNGi gx4/Yn5hINaD+NSBBQDV290jNPHhG1xiwS3L1t6D/IvwJkPVBc52eGdTZwyRaVxP3JCa YFcQ== X-Gm-Message-State: AOAM5332649UFNJMLvuDQX8gXE5t3fyEMTNnJjJ+FM+WRDkqQL2qQKap ZQC9aqfyfp+JvgdINREsacA= X-Google-Smtp-Source: ABdhPJzeZk3H1PDjNlE8QHRHMFyGZU4dkeN5m+0LruqtMj/EmTb9znIekmGypWdds/dTlY+8dljflQ== X-Received: by 2002:a05:6000:1d1:: with SMTP id t17mr1359961wrx.569.1640257899952; Thu, 23 Dec 2021 03:11:39 -0800 (PST) Received: from smtpclient.apple ([85.203.46.188]) by smtp.gmail.com with ESMTPSA id p22sm10786059wms.2.2021.12.23.03.11.37 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 23 Dec 2021 03:11:39 -0800 (PST) From: zhiheng chen Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_7DEBA9CB-3279-4BA5-AA83-3B20E76899DA" Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.40.0.1.81\)) Subject: Re: [PATCH v2] mempool: fix the description of some function return values Date: Thu, 23 Dec 2021 19:11:30 +0800 In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D86D9D@smartserver.smartshare.dk> Cc: Olivier Matz , Andrew Rybchenko , dev@dpdk.org To: =?utf-8?Q?Morten_Br=C3=B8rup?= References: <20211222082550.436-1-chenzhiheng0227@gmail.com> <98CBD80474FA8B44BF855DF32C47DC35D86D9D@smartserver.smartshare.dk> X-Mailer: Apple Mail (2.3693.40.0.1.81) X-Mailman-Approved-At: Mon, 03 Jan 2022 15:31:22 +0100 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 --Apple-Mail=_7DEBA9CB-3279-4BA5-AA83-3B20E76899DA Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Thank you for pointing out my problem, I will fix it in the next version > 2021=E5=B9=B412=E6=9C=8822=E6=97=A5 =E4=B8=8B=E5=8D=887:45=EF=BC=8CMorte= n Br=C3=B8rup =E5=86=99=E9=81=93=EF=BC=9A >=20 >> From: Zhiheng Chen [mailto:chenzhiheng0227@gmail.com = ] >> Sent: Wednesday, 22 December 2021 09.26 >>=20 >> Compared to patch version 1, this version updates the descriptions >> of underlying functions. >=20 > Some comments inline below, regarding the return value of success. >=20 > 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. >=20 > @Olivier, @Andrew: Do we want to impose this restriction on the API? >=20 > 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. >=20 >>=20 >> 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. >>=20 >> Fixes: ea5dd2744b90 ("mempool: cache optimisations") >>=20 >> Signed-off-by: Zhiheng Chen >> --- >> lib/mempool/rte_mempool.h | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >>=20 >> 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. >> + * - >=3D0: Success; number of objects supplied. >> + * - -ENOBUFS: Not enough entries in the mempool; no object is >> retrieved. >=20 > NAK regarding success: Return value 0 is correct, >=3D0 cannot happen. >=20 >> */ >> 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 >> * - >=3D0: Success; number of objects supplied. >=20 > This is also wrong. It should be: > -* - >=3D0: Success; number of objects supplied. > +* - 0: Success; got n objects. >=20 >> - * - <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. >> + * - >=3D0: Success; number of objects supplied. >> + * - -ENOBUFS: Not enough entries in the mempool; no object is >> retrieved. >=20 > NAK regarding success: Return value 0 is correct, >=3D0 cannot happen. >=20 >> */ >> 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. >> + * - >=3D0: Success; number of objects supplied. >> + * - -ENOBUFS: Not enough entries in the mempool; no object is >> retrieved. >=20 > NAK regarding success: Return value 0 is correct, >=3D0 cannot happen. >=20 >> */ >> 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 --Apple-Mail=_7DEBA9CB-3279-4BA5-AA83-3B20E76899DA Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Thank= you for pointing out my problem, I will fix it in the next version

2021=E5=B9=B412=E6=9C=8822=E6=97=A5 =E4=B8=8B=E5=8D=887:45=EF=BC= =8CMorten Br=C3=B8rup <mb@smartsharesystems.com> =E5=86=99=E9=81=93=EF=BC=9A
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.
+ *   - >=3D0: Success; = number of objects supplied.
+ *   - -ENOBUFS: = Not enough entries in the mempool; no object is
retrieved.

NAK regarding success: Return value 0 is correct, >=3D0 = 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
 *   - >=3D0: Success; number of objects = supplied.

This is also wrong. It should be:
-*   - >=3D0: = 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.
+ *   - = >=3D0: Success; number of objects supplied.
+ * =   - -ENOBUFS: Not enough entries in the mempool; no object = is
retrieved.

NAK regarding success: Return = value 0 is correct, >=3D0 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.
+ *   - >=3D0: Success; number of objects = supplied.
+ *   - -ENOBUFS: Not enough entries = in the mempool; no object is
retrieved.

NAK regarding success: Return value 0 is correct, >=3D0 = 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

= --Apple-Mail=_7DEBA9CB-3279-4BA5-AA83-3B20E76899DA--