Re: [dpdk-dev] [PATCH v3 06/10] eal: introduce memory management wrappers

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

Answering the questions. All snipped comments will be fixed in v4.

> [...]
> > +/**
> > + * Memory reservation flags.
> > + */
> > +enum eal_mem_reserve_flags {
> > +	/**< Reserve hugepages (support may be limited or missing). */
> > +	EAL_RESERVE_HUGEPAGES = 1 << 0,
> > +	/**< Fail if requested address is not available. */
> > +	EAL_RESERVE_EXACT_ADDRESS = 1 << 1
> > +};  
> 
> Maybe more context is needed to understand the meaning of these flags.

Will extend the comment in v4. It's basically MAP_HUGE and MAP_FIXED.

> [...]
> > -eal_get_virtual_area(void *requested_addr, size_t *size,
> > -		size_t page_sz, int flags, int mmap_flags);
> > +eal_get_virtual_area(void *requested_addr, size_t *size, size_t page_sz,
> > +	int flags, int mmap_flags);  
> 
> Is there any change here?

No, will fix this artifact.

> [...]
> > + * If @code virt @endcode and @code size @endcode describe a part of the  
> 
> I am not sure about using @code.
> It makes reading from source harder.
> Is there a real benefit?

It should be either @p or no markup (as in the rest of the comments), @code is
indeed inappropriate.

> > +
> > +/**
> > + * Memory mapping additional flags.
> > + *
> > + * In Linux and FreeBSD, each flag is semantically equivalent
> > + * to OS-specific mmap(3) flag with the same or similar name.
> > + * In Windows, POSIX and MAP_ANONYMOUS semantics are followed.
> > + */  
> 
> I don't understand this comment.
> The flags and meanings are the same no matter the OS, right?

Correct. MAP_ANONYMOUS is not POSIX so I mentioned it explicitly. I'll try to
come up with better wording.

> > +static void *
> > +mem_map(void *requested_addr, size_t size, int prot, int flags,
> > +	int fd, size_t offset)
> > +{
> > +	void *virt = mmap(requested_addr, size, prot, flags, fd, offset);
> > +	if (virt == MAP_FAILED) {
> > +		RTE_LOG(ERR, EAL,  
> 
> Not sure it should be a log level so high.
> We could imagine checking a memory map.
> What about INFO level?
> The real error log will be made by the caller.
> 
> > +			"Cannot mmap(%p, 0x%zx, 0x%x, 0x%x, %d, 0x%zx): %s\n",
> > +			requested_addr, size, prot, flags, fd, offset,
> > +			strerror(errno));
> > +		rte_errno = errno;
> > +		return NULL;  

The same level is used now in places from which this code is extracted:
lib/librte_eal/common/{eal_common_fbarray.c:97,eal_common_memory:131}, see
also lib/librte_pci/rte_pci.c:144. To my understanding, DEBUG is used to log
implementation-specific details like these OS API calls, so I'll change
level to that.

> [...]
> > +int
> > +rte_get_page_size(void)
> > +{
> > +	return getpagesize();
> > +}
> > +
> > +int
> > +rte_mem_lock(const void *virt, size_t size)
> > +{
> > +	return mlock(virt, size);
> > +}  
> 
> Why don't you replace existing code with these new functions?

In this patchset I tried to touch existing code as little as possible, at
least I'd like to limit the scope to EAL. Libraries and drivers using Unix
functions directly will fail to compile when enabled on Windows, but patches
will be trivial. I propose replacing calls in EAL in v4.

-- 
Dmitry Kozlyuk