Re: [dpdk-dev] [PATCH] mempool: fix search of maximum contiguous pages

[Olivier Matz <olivier.matz@6wind.com>]

Hi Thomas,

On 10/25/2016 04:37 PM, Thomas Monjalon wrote:
> 2016-10-13 17:05, Olivier MATZ:
>> Hi Wei,
>>
>> On 10/13/2016 02:31 PM, Ananyev, Konstantin wrote:
>>>
>>>>
>>>>>>> diff --git a/lib/librte_mempool/rte_mempool.c
>>>>>>> b/lib/librte_mempool/rte_mempool.c
>>>>>>> index 71017e1..e3e254a 100644
>>>>>>> --- a/lib/librte_mempool/rte_mempool.c
>>>>>>> +++ b/lib/librte_mempool/rte_mempool.c
>>>>>>> @@ -426,9 +426,12 @@ rte_mempool_populate_phys_tab(struct
>>>>>>> rte_mempool *mp, char *vaddr,
>>>>>>>
>>>>>>>   	for (i = 0; i < pg_num && mp->populated_size < mp->size; i += n) {
>>>>>>>
>>>>>>> +		phys_addr_t paddr_next;
>>>>>>> +		paddr_next = paddr[i] + pg_sz;
>>>>>>> +
>>>>>>>   		/* populate with the largest group of contiguous pages */
>>>>>>>   		for (n = 1; (i + n) < pg_num &&
>>>>>>> -			     paddr[i] + pg_sz == paddr[i+n]; n++)
>>>>>>> +			     paddr_next == paddr[i+n]; n++, paddr_next += pg_sz)
>>>>>>>   			;
>>>>>>
>>>>>> Good catch.
>>>>>> Why not just paddr[i + n - 1] != paddr[i + n]?
>>>>>
>>>>> Sorry, I meant 'paddr[i + n - 1] + pg_sz == paddr[i+n]' off course.
>>>>>
>>>>>> Then you don't need extra variable (paddr_next) here.
>>>>>> Konstantin
>>>>
>>>> Thank you, Konstantin
>>>> 'paddr[i + n - 1] + pg_sz = paddr[i + n]' also can fix it and have straight meaning.
>>>> But I assume that my revision with paddr_next += pg_sz may have a bit better performance.
>>>
>>> I don't think there would be any real difference, again it is not performance critical code-path.
>>>
>>>> By the way, paddr[i] + n * pg_sz = paddr[i + n] can also resolve it.
>>>
>>> Yes, that's one seems even better for me - make things more clear.
>>
>> Thank you for fixing this.
>>
>> My vote would go for "addr[i + n - 1] + pg_sz == paddr[i + n]"
>>
>> If you feel "paddr[i] + n * pg_sz = paddr[i + n]" is clearer, I have no 
>> problem with it either.
> 
> No answer from Wei Dai.
> Please Olivier advise what to do with this patch.
> Thanks
> 

I think it's good to have this fix in 16.11.
I'm sending a v2 based on Wei's patch.

Olivier