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.
Regards,
Olivier
