[dpdk-dev] [PATCH v2 02/20] mem: allow memseg lists to be marked as external

[Burakov, Anatoly]

On 20-Sep-18 10:30 AM, Andrew Rybchenko wrote:
> On 9/19/18 4:56 PM, Anatoly Burakov wrote:
>> When we allocate and use DPDK memory, we need to be able to
>> differentiate between DPDK hugepage segments and segments that
>> were made part of DPDK but are externally allocated. Add such
>> a property to memseg lists.
>>
>> This breaks the ABI, so bump the EAL library ABI version and
>> document the change in release notes.
>>
>> All current calls for memseg walk functions were adjusted to
>> ignore external segments where it made sense.
>>
>> Mempools is a special case, because we may be asked to allocate
>> a mempool on a specific socket, and we need to ignore all page
>> sizes on other heaps or other sockets. Previously, this
>> assumption of knowing all page sizes was not a problem, but it
>> will be now, so we have to match socket ID with page size when
>> calculating minimum page size for a mempool.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
> 
> A couple of minor questions/suggestions below, but it is OK to
> go as is even if rejected.
> 
> Acked-by: Andrew Rybchenko <arybchenko at solarflare.com>
> 
> <...>
> 
>> diff --git a/lib/librte_mempool/rte_mempool.c 
>> b/lib/librte_mempool/rte_mempool.c
>> index 03e6b5f73..d61c77da3 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -99,25 +99,40 @@ static unsigned optimize_object_size(unsigned 
>> obj_size)
>>       return new_obj_size * RTE_MEMPOOL_ALIGN;
>>   }
>> +struct pagesz_walk_arg {
>> +    int socket_id;
>> +    size_t min;
>> +};
>> +
>>   static int
>>   find_min_pagesz(const struct rte_memseg_list *msl, void *arg)
>>   {
>> -    size_t *min = arg;
>> +    struct pagesz_walk_arg *wa = arg;
>> +    bool valid;
>> -    if (msl->page_sz < *min)
>> -        *min = msl->page_sz;
>> +    valid = msl->socket_id == wa->socket_id;
> 
> Is it intended that we accept externally allocated segment
> if it is on requested socket? If so, it would be good to add
> comment to explain why.

Accepting externally allocated segments is precisely the point here - we 
want to find page size of underlying memory, regardless of whether it's 
internal or external. We use socket ID to identify valid page sizes for 
a particular heap (since socket ID is technically a heap identifier, as 
far as external code is concerned), but within that heap there can be 
multiple segment lists corresponding to that socket ID, each with its 
own page size.

> 
>> +    valid |= wa->socket_id == SOCKET_ID_ANY && msl->external == 0;
>> +
>> +    if (!valid)
>> +        return 0;
>> +
>> +    if (msl->page_sz < wa->min)
>> +        wa->min = msl->page_sz;
> 
> I'd suggest to keep single return (it is just a bit shorter)
> if (valid && msl->page_sz < wa->min)
>           wa->min = msl->page_sz;

Sure. If there will be other comments that warrant a v3 respin, i'll 
incorporate this feedback :)

Thanks for the review!

> 
> <...>
> 


-- 
Thanks,
Anatoly