diff mbox series

mempool: fix memory allocation in memzones during retry.

Message ID 1594611634-7730-1-git-send-email-wangzhike@jd.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series mempool: fix memory allocation in memzones during retry. |

Checks

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

Commit Message

王志克 July 13, 2020, 3:40 a.m. UTC 
  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).

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.

It is wrong that max_alloc_size is divided by 2 in the
case of successful allocation as well, or invalid memory
can be allocated, and leads to population failure, then errno
other than ENOMEM may be returned.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Signed-off-by: Zhike Wang <wangzhike@jd.com>
---
 lib/librte_mempool/rte_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Rybchenko July 13, 2020, 10:32 a.m. UTC | #1 
On 7/13/20 6:40 AM, Zhike Wang wrote:
> 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).
> 
> 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.
> 
> It is wrong that max_alloc_size is divided by 2 in the
> case of successful allocation as well, or invalid memory
> can be allocated, and leads to population failure, then errno
> other than ENOMEM may be returned.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Signed-off-by: Zhike Wang <wangzhike@jd.com>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

> ---
>  lib/librte_mempool/rte_mempool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index a2bd249..b8f2629 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -635,7 +635,7 @@ struct pagesz_walk_arg {
>  				RTE_MIN((size_t)mem_size, max_alloc_size),
>  				mp->socket_id, mz_flags, align);
>  
> -			if (mz == NULL && rte_errno != ENOMEM)
> +			if ((mz != NULL) || (mz == NULL && rte_errno != ENOMEM))
>  				break;
>  
>  			max_alloc_size = RTE_MIN(max_alloc_size,
>
Burakov, Anatoly July 13, 2020, 11:17 a.m. UTC | #2 
On 13-Jul-20 4:40 AM, Zhike Wang wrote:
> 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).
> 
> 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.
> 
> It is wrong that max_alloc_size is divided by 2 in the
> case of successful allocation as well, or invalid memory
> can be allocated, and leads to population failure, then errno
> other than ENOMEM may be returned.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Signed-off-by: Zhike Wang <wangzhike@jd.com>
> ---
>   lib/librte_mempool/rte_mempool.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index a2bd249..b8f2629 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -635,7 +635,7 @@ struct pagesz_walk_arg {
>   				RTE_MIN((size_t)mem_size, max_alloc_size),
>   				mp->socket_id, mz_flags, align);
>   
> -			if (mz == NULL && rte_errno != ENOMEM)
> +			if ((mz != NULL) || (mz == NULL && rte_errno != ENOMEM))

I think checking mz == NULL for the second time is redundant, as if 
we're hitting the second branch, we've already failed the "mz != NULL" 
test and can therefore assume that mz == NULL.

That said, i'm struggling to think of circumstances where this would 
matter. Could you please provide an example?

>   				break;
>   
>   			max_alloc_size = RTE_MIN(max_alloc_size,
> 

This should have a Fixes: tag.
Andrew Rybchenko July 13, 2020, 11:29 a.m. UTC | #3 
On 7/13/20 2:17 PM, Burakov, Anatoly wrote:
> On 13-Jul-20 4:40 AM, Zhike Wang wrote:
>> 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).
>>
>> 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.
>>
>> It is wrong that max_alloc_size is divided by 2 in the
>> case of successful allocation as well, or invalid memory
>> can be allocated, and leads to population failure, then errno
>> other than ENOMEM may be returned.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> Signed-off-by: Zhike Wang <wangzhike@jd.com>
>> ---
>>   lib/librte_mempool/rte_mempool.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c
>> b/lib/librte_mempool/rte_mempool.c
>> index a2bd249..b8f2629 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -635,7 +635,7 @@ struct pagesz_walk_arg {
>>                   RTE_MIN((size_t)mem_size, max_alloc_size),
>>                   mp->socket_id, mz_flags, align);
>>   -            if (mz == NULL && rte_errno != ENOMEM)
>> +            if ((mz != NULL) || (mz == NULL && rte_errno != ENOMEM))
> 
> I think checking mz == NULL for the second time is redundant, as if
> we're hitting the second branch, we've already failed the "mz != NULL"
> test and can therefore assume that mz == NULL.

Yes, of course. (Also parenthesis will be not required.)

> 
> That said, i'm struggling to think of circumstances where this would
> matter. Could you please provide an example?

If the question about break in the case of mz != NULL,
it is important to avoid decreasing max_alloc_size to
try the same size once again if one more iteration is
needed to allocate remaining elements.

> 
>>                   break;
>>                 max_alloc_size = RTE_MIN(max_alloc_size,
>>
> 
> This should have a Fixes: tag.
> 

Yes, missed it.

Many thanks for the review.
Burakov, Anatoly July 13, 2020, 2:52 p.m. UTC | #4 
On 13-Jul-20 12:29 PM, Andrew Rybchenko wrote:
> On 7/13/20 2:17 PM, Burakov, Anatoly wrote:
>> On 13-Jul-20 4:40 AM, Zhike Wang wrote:
>>> 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).
>>>
>>> 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.
>>>
>>> It is wrong that max_alloc_size is divided by 2 in the
>>> case of successful allocation as well, or invalid memory
>>> can be allocated, and leads to population failure, then errno
>>> other than ENOMEM may be returned.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Signed-off-by: Zhike Wang <wangzhike@jd.com>
>>> ---
>>>    lib/librte_mempool/rte_mempool.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_mempool/rte_mempool.c
>>> b/lib/librte_mempool/rte_mempool.c
>>> index a2bd249..b8f2629 100644
>>> --- a/lib/librte_mempool/rte_mempool.c
>>> +++ b/lib/librte_mempool/rte_mempool.c
>>> @@ -635,7 +635,7 @@ struct pagesz_walk_arg {
>>>                    RTE_MIN((size_t)mem_size, max_alloc_size),
>>>                    mp->socket_id, mz_flags, align);
>>>    -            if (mz == NULL && rte_errno != ENOMEM)
>>> +            if ((mz != NULL) || (mz == NULL && rte_errno != ENOMEM))
>>
>> I think checking mz == NULL for the second time is redundant, as if
>> we're hitting the second branch, we've already failed the "mz != NULL"
>> test and can therefore assume that mz == NULL.
> 
> Yes, of course. (Also parenthesis will be not required.)
> 
>>
>> That said, i'm struggling to think of circumstances where this would
>> matter. Could you please provide an example?
> 
> If the question about break in the case of mz != NULL,
> it is important to avoid decreasing max_alloc_size to
> try the same size once again if one more iteration is
> needed to allocate remaining elements.

Right, no further questions :)

> 
>>
>>>                    break;
>>>                  max_alloc_size = RTE_MIN(max_alloc_size,
>>>
>>
>> This should have a Fixes: tag.
>>
> 
> Yes, missed it.
> 
> Many thanks for the review.
>
王志克 July 14, 2020, 7:36 a.m. UTC | #5 
Thanks for review.

Sent out v2.

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: Burakov, Anatoly [mailto:anatoly.burakov@intel.com] 
Sent: Monday, July 13, 2020 10:53 PM
To: Andrew Rybchenko; 王志克; dev@dpdk.org
Cc: olivier.matz@6wind.com
Subject: Re: [dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

On 13-Jul-20 12:29 PM, Andrew Rybchenko wrote:
> On 7/13/20 2:17 PM, Burakov, Anatoly wrote:
>> On 13-Jul-20 4:40 AM, Zhike Wang wrote:
>>> 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).
>>>
>>> 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.
>>>
>>> It is wrong that max_alloc_size is divided by 2 in the
>>> case of successful allocation as well, or invalid memory
>>> can be allocated, and leads to population failure, then errno
>>> other than ENOMEM may be returned.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Signed-off-by: Zhike Wang <wangzhike@jd.com>
>>> ---
>>>    lib/librte_mempool/rte_mempool.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_mempool/rte_mempool.c
>>> b/lib/librte_mempool/rte_mempool.c
>>> index a2bd249..b8f2629 100644
>>> --- a/lib/librte_mempool/rte_mempool.c
>>> +++ b/lib/librte_mempool/rte_mempool.c
>>> @@ -635,7 +635,7 @@ struct pagesz_walk_arg {
>>>                    RTE_MIN((size_t)mem_size, max_alloc_size),
>>>                    mp->socket_id, mz_flags, align);
>>>    -            if (mz == NULL && rte_errno != ENOMEM)
>>> +            if ((mz != NULL) || (mz == NULL && rte_errno != ENOMEM))
>>
>> I think checking mz == NULL for the second time is redundant, as if
>> we're hitting the second branch, we've already failed the "mz != NULL"
>> test and can therefore assume that mz == NULL.
> 
> Yes, of course. (Also parenthesis will be not required.)
> 
>>
>> That said, i'm struggling to think of circumstances where this would
>> matter. Could you please provide an example?
> 
> If the question about break in the case of mz != NULL,
> it is important to avoid decreasing max_alloc_size to
> try the same size once again if one more iteration is
> needed to allocate remaining elements.

Right, no further questions :)

> 
>>
>>>                    break;
>>>                  max_alloc_size = RTE_MIN(max_alloc_size,
>>>
>>
>> This should have a Fixes: tag.
>>
> 
> Yes, missed it.
> 
> Many thanks for the review.
>
diff mbox series

Patch

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index a2bd249..b8f2629 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -635,7 +635,7 @@  struct pagesz_walk_arg {
 				RTE_MIN((size_t)mem_size, max_alloc_size),
 				mp->socket_id, mz_flags, align);
 
-			if (mz == NULL && rte_errno != ENOMEM)
+			if ((mz != NULL) || (mz == NULL && rte_errno != ENOMEM))
 				break;
 
 			max_alloc_size = RTE_MIN(max_alloc_size,