Re: [dpdk-dev] [PATCH v4 3/8] eal: introduce memory management wrappers

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

On 01-May-20 8:00 PM, Dmitry Kozlyuk wrote:
> Thanks for pointing out the errors, see some comments inline.
> 
> On 2020-04-29 18:13 GMT+0100 Burakov, Anatoly wrote:
>> On 29-Apr-20 12:50 AM, Dmitry Kozlyuk wrote:
> <snip>
>>> + *  Reservation size. Must be a multiple of system page size.
>>> + * @param flags
>>> + *  Reservation options, a combination of eal_mem_reserve_flags.
>>> + * @returns
>>> + *  Starting address of the reserved area on success, NULL on failure.
>>> + *  Callers must not access this memory until remapping it.
>>> + */
>>> +void *eal_mem_reserve(void *requested_addr, size_t size, int flags);
>>
>> Should we also require requested_addr to be page-aligned?
> 
> Yes.
> 
>> Also, here and in other added API's, nitpick but our coding style guide
>> (and the code style in this file) suggests that return value should be
>> on a separate line, e.g.
>>
>> void *
>> eal_mem_reserve(...);
> 
> Will follow your advice in v5 to keep the style within this file consistent.
> However, DPDK Coding Style explicitly says:
> 
> 	Unlike function definitions, the function prototypes do not need to
> 	place the function return type on a separate line.
> 
> [snip]
>>> +
>>> +int
>>> +rte_mem_lock(const void *virt, size_t size)
>>> +{
>>> +	return mlock(virt, size);
>>
>> This call can fail. It should pass errno as rte_errno as well, just like
>> all other calls from this family.
>>
>> Also, if the implementation "may require" page alignment, how about
>> requiring it unconditionally?
> 
> IMO even better to document this function as locking all pages crossed by the
> address region. This would save address checking/alignment at call site and
> all implementations work this way. Locking memory implies paging system.
> 

I don't think any other external API we provide does automagic pointer 
alignment, so i'm not sure if it indeed would be better to have it align 
automatically. It's also better from the standpoint of not silently 
allowing seemingly invalid arguments. So, i would lean on the side of 
requiring alignment, but not doing it ourselves.

-- 
Thanks,
Anatoly