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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.