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

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

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.

-- 
Dmtiry Kozlyuk