[dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

[dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

[Zhike Wang]

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))
 				break;
 
 			max_alloc_size = RTE_MIN(max_alloc_size,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

[Andrew Rybchenko]

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,
> 

Re: [dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

[Burakov, Anatoly]

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.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

[Andrew Rybchenko]

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.

Re: [dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

[Burakov, Anatoly]

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


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry.

[王志克]

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


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2] mempool: fix memory allocation in memzones during retry.

[Zhike Wang]

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.

Fixes: 3a3d0c75b43e ("mempool: fix slow allocation of large pools")

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..7774f0c 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 || rte_errno != ENOMEM)
 				break;
 
 			max_alloc_size = RTE_MIN(max_alloc_size,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] mempool: fix memory allocation in memzones during retry.

[Burakov, Anatoly]

On 14-Jul-20 8:26 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.
> 
> Fixes: 3a3d0c75b43e ("mempool: fix slow allocation of large pools")
> 
> 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..7774f0c 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 || rte_errno != ENOMEM)
>   				break;
>   
>   			max_alloc_size = RTE_MIN(max_alloc_size,
> 

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] mempool: fix memory allocation in memzones during retry.

[Thomas Monjalon]

14/07/2020 11:32, Burakov, Anatoly:
> On 14-Jul-20 8:26 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.
> > 
> > Fixes: 3a3d0c75b43e ("mempool: fix slow allocation of large pools")

Cc: stable@dpdk.org

> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > Signed-off-by: Zhike Wang <wangzhike@jd.com>
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks