* [PATCH v2] mempool: fix the description of some function return values
@ 2021-12-22 8:25 Zhiheng Chen
2021-12-22 11:45 ` Morten Brørup
2021-12-23 10:07 ` [PATCH v3] " Zhiheng Chen
0 siblings, 2 replies; 5+ messages in thread
From: Zhiheng Chen @ 2021-12-22 8:25 UTC (permalink / raw)
To: Olivier Matz, Andrew Rybchenko; +Cc: dev, Zhiheng Chen
Compared to patch version 1, this version updates the descriptions
of underlying functions.
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.
*/
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.
- * - <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.
*/
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.
*/
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] mempool: fix the description of some function return values
2021-12-22 8:25 [PATCH v2] mempool: fix the description of some function return values Zhiheng Chen
@ 2021-12-22 11:45 ` Morten Brørup
2021-12-23 11:11 ` zhiheng chen
2021-12-23 10:07 ` [PATCH v3] " Zhiheng Chen
1 sibling, 1 reply; 5+ messages in thread
From: Morten Brørup @ 2021-12-22 11:45 UTC (permalink / raw)
To: Zhiheng Chen, Olivier Matz, Andrew Rybchenko; +Cc: dev
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3] mempool: fix the description of some function return values
2021-12-22 8:25 [PATCH v2] mempool: fix the description of some function return values Zhiheng Chen
2021-12-22 11:45 ` Morten Brørup
@ 2021-12-23 10:07 ` Zhiheng Chen
2022-01-24 17:04 ` Olivier Matz
1 sibling, 1 reply; 5+ messages in thread
From: Zhiheng Chen @ 2021-12-23 10:07 UTC (permalink / raw)
To: Olivier Matz, Andrew Rybchenko; +Cc: dev, Zhiheng Chen
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.
v2:
* Update the descriptions of underlying functions.
v3:
* Correct the description that the return value cannot be greater than 0
* Update the description of the dequeue function prototype
Signed-off-by: Zhiheng Chen <chenzhiheng0227@gmail.com>
---
lib/mempool/rte_mempool.h | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 1e7a3c1527..cae81d8a32 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -447,6 +447,16 @@ typedef int (*rte_mempool_enqueue_t)(struct rte_mempool *mp,
/**
* Dequeue an object from the external pool.
+ *
+ * @param mp
+ * Pointer to the memory pool.
+ * @param obj_table
+ * Pointer to a table of void * pointers (objects).
+ * @param n
+ * Number of objects to get.
+ * @return
+ * - 0: Success; got n objects.
+ * - -ENOBUFS: Not enough entries in the mempool; no object is retrieved.
*/
typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp,
void **obj_table, unsigned int n);
@@ -738,7 +748,7 @@ rte_mempool_ops_alloc(struct rte_mempool *mp);
* Number of objects to get.
* @return
* - 0: Success; got n objects.
- * - <0: Error; code of dequeue function.
+ * - -ENOBUFS: Not enough entries in the mempool; no object is retrieved.
*/
static inline int
rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
@@ -1452,8 +1462,8 @@ rte_mempool_put(struct rte_mempool *mp, void *obj)
* @param cache
* A pointer to a mempool cache structure. May be NULL if not needed.
* @return
- * - >=0: Success; number of objects supplied.
- * - <0: Error; code of ring dequeue function.
+ * - 0: Success; got n objects.
+ * - -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 +1531,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 +1544,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; got n objects.
+ * - -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,
@@ -1557,7 +1567,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 +1578,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; got n objects.
+ * - -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)
@@ -1588,7 +1598,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.
*
@@ -1597,8 +1607,8 @@ rte_mempool_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned int n)
* @param obj_p
* 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.
+ * - 0: Success; got n objects.
+ * - -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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mempool: fix the description of some function return values
2021-12-22 11:45 ` Morten Brørup
@ 2021-12-23 11:11 ` zhiheng chen
0 siblings, 0 replies; 5+ messages in thread
From: zhiheng chen @ 2021-12-23 11:11 UTC (permalink / raw)
To: Morten Brørup; +Cc: Olivier Matz, Andrew Rybchenko, dev
[-- Attachment #1: Type: text/plain, Size: 6451 bytes --]
Thank you for pointing out my problem, I will fix it in the next version
> 2021年12月22日 下午7:45，Morten Brørup <mb@smartsharesystems.com> 写道：
>
>> From: Zhiheng Chen [mailto:chenzhiheng0227@gmail.com <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
[-- Attachment #2: Type: text/html, Size: 22356 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] mempool: fix the description of some function return values
2021-12-23 10:07 ` [PATCH v3] " Zhiheng Chen
@ 2022-01-24 17:04 ` Olivier Matz
0 siblings, 0 replies; 5+ messages in thread
From: Olivier Matz @ 2022-01-24 17:04 UTC (permalink / raw)
To: Zhiheng Chen; +Cc: Andrew Rybchenko, dev
Hi Zhiheng,
Thank you for your patch proposal.
On Thu, Dec 23, 2021 at 10:07:41AM +0000, Zhiheng Chen wrote:
> 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.
The issue I see with this approach is that currently, there
is no standard error code in mempool drivers dequeue:
bucket: -ENOBUFS
cn10k: -ENOENT
cn9k: -ENOENT
dpaa: -1, -ENOBUFS
dpaa2: -1, -ENOENT, -ENOBUFS
octeontx: -ENOMEM
ring: -ENOBUFS
stack: -ENOBUFS
After your patch, the drivers do not match the documentation.
I agree it would be better to return the same code for the same error,
whatever the driver is used. But I think we should keep the possibility
for a driver to return another code. For instance, it could be an
hardware error in case of hardware mempool driver.
I see 2 possibilities:
1/ simplest one: relax documentation and do not talk about -ENOENT or
-ENOBUFS, just say negative value is an error
2/ fix driver and doc
Mempool drivers should be modified first, knowing that changing
them is an ABI modification (which I think is acceptable, because the
error code varies depending on driver). Then, this patch could be applied.
For reference, note that the documentation was probably right initially,
but the behavior changed in commit cfa7c9e6fc1f ("ring: make bulk and
burst return values consistent"), returning -ENOBUFS instead of -ENOENT
on dequeue error.
> v2:
> * Update the descriptions of underlying functions.
>
> v3:
> * Correct the description that the return value cannot be greater than 0
> * Update the description of the dequeue function prototype
>
> Signed-off-by: Zhiheng Chen <chenzhiheng0227@gmail.com>
> ---
> lib/mempool/rte_mempool.h | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 1e7a3c1527..cae81d8a32 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -447,6 +447,16 @@ typedef int (*rte_mempool_enqueue_t)(struct rte_mempool *mp,
>
> /**
> * Dequeue an object from the external pool.
> + *
> + * @param mp
> + * Pointer to the memory pool.
> + * @param obj_table
> + * Pointer to a table of void * pointers (objects).
> + * @param n
> + * Number of objects to get.
> + * @return
> + * - 0: Success; got n objects.
> + * - -ENOBUFS: Not enough entries in the mempool; no object is retrieved.
Also, we should have, in addition to -ENOBUFS:
- <0: Another driver-specific error code (-errno)
This comment applies for the other functions below.
> */
> typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp,
> void **obj_table, unsigned int n);
> @@ -738,7 +748,7 @@ rte_mempool_ops_alloc(struct rte_mempool *mp);
> * Number of objects to get.
> * @return
> * - 0: Success; got n objects.
> - * - <0: Error; code of dequeue function.
> + * - -ENOBUFS: Not enough entries in the mempool; no object is retrieved.
> */
> static inline int
> rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
> @@ -1452,8 +1462,8 @@ rte_mempool_put(struct rte_mempool *mp, void *obj)
> * @param cache
> * A pointer to a mempool cache structure. May be NULL if not needed.
> * @return
> - * - >=0: Success; number of objects supplied.
> - * - <0: Error; code of ring dequeue function.
> + * - 0: Success; got n objects.
> + * - -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 +1531,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 +1544,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; got n objects.
> + * - -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,
> @@ -1557,7 +1567,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 +1578,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; got n objects.
> + * - -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)
> @@ -1588,7 +1598,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.
> *
> @@ -1597,8 +1607,8 @@ rte_mempool_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned int n)
> * @param obj_p
> * 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.
> + * - 0: Success; got n objects.
> + * - -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
>
Thanks,
Olivier
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-24 17:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-22 8:25 [PATCH v2] mempool: fix the description of some function return values Zhiheng Chen
2021-12-22 11:45 ` Morten Brørup
2021-12-23 11:11 ` zhiheng chen
2021-12-23 10:07 ` [PATCH v3] " Zhiheng Chen
2022-01-24 17:04 ` Olivier Matz
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).