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 <arybche...@solarflare.com>
>> Signed-off-by: Zhike Wang <wangzh...@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.
