[dpdk-dev] [PATCH] mempool: return ENOMEM if initial alloc size can not be satisfied

[dpdk-dev] [PATCH] mempool: return ENOMEM if initial alloc size can not be satisfied

[Zhike Wang]

Signed-off-by: Zhike Wang <wangzhike@jd.com>
---
 lib/librte_mempool/rte_mempool.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 0bde995..b24feb6 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -622,6 +622,12 @@ struct pagesz_walk_arg {
 			goto fail;
 		}
 
+		if (max_alloc_size < min_chunk_size) {
+			rte_errno = ENOMEM;
+			ret = -rte_errno;
+			goto fail;
+		}
+
 		/* if we're trying to reserve contiguous memory, add appropriate
 		 * memzone flag.
 		 */
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] mempool: return ENOMEM if initial alloc size can not be satisfied

[Andrew Rybchenko]

On 7/3/20 11:41 AM, Zhike Wang wrote:
> Signed-off-by: Zhike Wang <wangzhike@jd.com>
> ---
>  lib/librte_mempool/rte_mempool.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 0bde995..b24feb6 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -622,6 +622,12 @@ struct pagesz_walk_arg {
>  			goto fail;
>  		}
>  
> +		if (max_alloc_size < min_chunk_size) {
> +			rte_errno = ENOMEM;
> +			ret = -rte_errno;
> +			goto fail;
> +		}
> +
>  		/* if we're trying to reserve contiguous memory, add appropriate
>  		 * memzone flag.
>  		 */
> 

As far as I can see there is really a bug in nearby code, but
the fix suggested here is a wrong direction.

The check is already present below in do-while loop condition,
but it is wrong that max_alloc_size is divided by 2 in the
case of successful allocation as well.
If allocation is successful on the first attempt, typically
there is no problem since we allocated everything required and
we'll terminate the loop (if memory chunk is really sufficient
to populate required number of mempool elements).

However, if the first attempt fails, we try to allocate half
of mem_size and it succeed, we'll have one more iteration of
the for-loop to allocate memory for remaining elements and
should not try the next time with quarter of the mem_size.
mem_size will be recalculated, but max_alloc_size will limit
allocation attempt size.

So, I think it is required to add "mz != NULL ||" to the
if condition in do-while loop. It will guarantee that
max_alloc_size is reduced if and only if mz == NULL and
if it becomes smaller than min_chunk_size, if condition
after do-while loop will return error.

Re: [dpdk-dev] [PATCH] mempool: return ENOMEM if initial alloc size can not be satisfied

[王志克]

Thanks, you are right.

Will send new patch.

Br,
Zhike Wang 
JDCloud, Product Development, IaaS   
------------------------------------------------------------------------------------------------
Mobile／+86 13466719566
E- mail／wangzhike@jd.com
Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
Https://JDCloud.com
------------------------------------------------------------------------------------------------



-----Original Message-----
From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] 
Sent: Friday, July 03, 2020 5:23 PM
To: 王志克; dev@dpdk.org
Cc: olivier.matz@6wind.com
Subject: Re: [dpdk-dev] [PATCH] mempool: return ENOMEM if initial alloc size can not be satisfied

On 7/3/20 11:41 AM, Zhike Wang wrote:
> Signed-off-by: Zhike Wang <wangzhike@jd.com>
> ---
>  lib/librte_mempool/rte_mempool.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 0bde995..b24feb6 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -622,6 +622,12 @@ struct pagesz_walk_arg {
>  			goto fail;
>  		}
>  
> +		if (max_alloc_size < min_chunk_size) {
> +			rte_errno = ENOMEM;
> +			ret = -rte_errno;
> +			goto fail;
> +		}
> +
>  		/* if we're trying to reserve contiguous memory, add appropriate
>  		 * memzone flag.
>  		 */
> 

As far as I can see there is really a bug in nearby code, but
the fix suggested here is a wrong direction.

The check is already present below in do-while loop condition,
but it is wrong that max_alloc_size is divided by 2 in the
case of successful allocation as well.
If allocation is successful on the first attempt, typically
there is no problem since we allocated everything required and
we'll terminate the loop (if memory chunk is really sufficient
to populate required number of mempool elements).

However, if the first attempt fails, we try to allocate half
of mem_size and it succeed, we'll have one more iteration of
the for-loop to allocate memory for remaining elements and
should not try the next time with quarter of the mem_size.
mem_size will be recalculated, but max_alloc_size will limit
allocation attempt size.

So, I think it is required to add "mz != NULL ||" to the
if condition in do-while loop. It will guarantee that
max_alloc_size is reduced if and only if mz == NULL and
if it becomes smaller than min_chunk_size, if condition
after do-while loop will return error.