Re: [dpdk-dev] [PATCH v3 6/9] memalloc: add EAL-internal API to get and set segment fd's

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 14-Sep-18 8:54 AM, Maxime Coquelin wrote:
> 
> 
> On 09/04/2018 05:15 PM, Anatoly Burakov wrote:
>> Enable setting and retrieving segment fd's internally.
>>
>> For now, retrieving fd's will not be used anywhere until we
>> get an external API, but it will be useful for things like
>> virtio, where we wish to share segment fd's.
>>
>> Setting segment fd's will not be available as a public API
>> at this time, but internally it is needed for legacy mode,
>> because we're not allocating our hugepages in memalloc in
>> legacy mode case, and we still need to store the fd.
>>
>> Another user of get segment fd API is memseg info dump, to
>> show which pages use which fd's.
>>
>> Not supported on FreeBSD.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/bsdapp/eal/eal_memalloc.c   | 12 +++++
>>   lib/librte_eal/common/eal_common_memory.c  |  8 +--
>>   lib/librte_eal/common/eal_memalloc.h       |  6 +++
>>   lib/librte_eal/linuxapp/eal/eal_memalloc.c | 60 +++++++++++++++++-----
>>   lib/librte_eal/linuxapp/eal/eal_memory.c   | 44 +++++++++++++---
>>   5 files changed, 109 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/librte_eal/bsdapp/eal/eal_memalloc.c 
>> b/lib/librte_eal/bsdapp/eal/eal_memalloc.c
>> index f7f07abd6..a5fb09f71 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal_memalloc.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal_memalloc.c
>> @@ -47,6 +47,18 @@ eal_memalloc_sync_with_primary(void)
>>       return -1;
>>   }
>> +int
>> +eal_memalloc_get_seg_fd(int list_idx, int seg_idx)
>> +{
>> +    return -1;
> 
> Why not returning -ENOTSUPP directly here? (see patch 7).
> 
>> +}
>> +
>> +int
>> +eal_memalloc_set_seg_fd(int list_idx, int seg_idx, int fd)
>> +{
>> +    return -1;
> 
> Ditto.
> 
>> +}
>> +
> 
> Other than that, the patch looks good to me.
> 
> Maxime
> 

Mainly for consistency reasons. Returning errno values but not using 
them looks weird to me, but i can change this to return those at the outset.

-- 
Thanks,
Anatoly