mempool: return ENOMEM if initial alloc size can not be satisfied
diff mbox series

Message ID 1593765692-13265-1-git-send-email-wangzhike@jd.com
State Changes Requested
Headers show
Series
  • mempool: return ENOMEM if initial alloc size can not be satisfied
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues
ci/iol-broadcom-Performance success Performance Testing PASS

Commit Message

王志克 July 3, 2020, 8:41 a.m. UTC
Signed-off-by: Zhike Wang <wangzhike@jd.com>
---
 lib/librte_mempool/rte_mempool.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andrew Rybchenko July 3, 2020, 9:23 a.m. UTC | #1
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.
王志克 July 10, 2020, 8:44 a.m. UTC | #2
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.

Patch
diff mbox series

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.
 		 */