[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 at 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 at intel.com] 
Sent: Monday, July 13, 2020 10:53 PM
To: Andrew Rybchenko; 王志克; dev at dpdk.org
Cc: olivier.matz at 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 at solarflare.com>
>>> Signed-off-by: Zhike Wang <wangzhike at 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