Re: [dpdk-dev] [PATCH v8 03/11] eal: introduce memory management wrappers

[Thomas Monjalon <thomas@monjalon.net>]

10/06/2020 16:27, Dmitry Kozlyuk:
> Introduce OS-independent wrappers for memory management operations used
> across DPDK and specifically in common code of EAL:
> 
> * rte_mem_map()
> * rte_mem_unmap()
> * rte_mem_page_size()
> * rte_mem_lock()
> 
> Windows uses different APIs for memory mapping and reservation, while
> Unices reserve memory by mapping it. Introduce EAL private functions to
> support memory reservation in common code:
> 
> * eal_mem_reserve()
> * eal_mem_free()
> * eal_mem_set_dump()
> 
> Wrappers follow POSIX semantics limited to DPDK tasks, but their
> signatures deliberately differ from POSIX ones to be more safe and
> expressive. New symbols are internal. Being thin wrappers, they require
> no special maintenance.

[...]
> +/**
> + * Reserve a region of virtual memory.
> + *
> + * Use eal_mem_free() to free reserved memory.
> + *
> + * @param requested_addr
> + *  A desired reservation addressm which must be page-aligned.

Typo: addressm

> + *  The system might not respect it.
> + *  NULL means the address will be chosen by the system.
> + * @param size
> + *  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);

[...]
> +/**
> + * Configure memory region inclusion into core dumps.

Not sure about the word "core" here.

> + *
> + * @param virt
> + *  Starting address of the region.
> + * @param size
> + *  Size of the region.
> + * @param dump
> + *  True to include memory into core dumps, false to exclude.
> + * @return
> + *  0 on success, (-1) on failure and rte_errno is set.
> + */
> +int
> +eal_mem_set_dump(void *virt, size_t size, bool dump);

[...]
> --- /dev/null
> +++ b/lib/librte_eal/include/rte_eal_memory.h
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Dmitry Kozlyuk
> + */
> +
> +#include <stdint.h>
> +
> +#include <rte_compat.h>
> +
> +/** @file Mamory management wrappers used across DPDK. */

typo on "Mamory"
+
@file must be on a separate line:

/** @file
 *
 * Memory management wrappers used across DPDK.
 */

> +
> +/** Memory protection flags. */
> +enum rte_mem_prot {
> +	RTE_PROT_READ = 1 << 0,   /**< Read access. */
> +	RTE_PROT_WRITE = 1 << 1,  /**< Write access. */
> +	RTE_PROT_EXECUTE = 1 << 2 /**< Code execution. */
> +};
> +
> +/** Additional flags for memory mapping. */

Typo on "Addtional"

> +enum rte_map_flags {
> +	/** Changes to the mapped memory are visible to other processes. */
> +	RTE_MAP_SHARED = 1 << 0,
> +	/** Mapping is not backed by a regular file. */
> +	RTE_MAP_ANONYMOUS = 1 << 1,
> +	/** Copy-on-write mapping, changes are invisible to other processes. */
> +	RTE_MAP_PRIVATE = 1 << 2,
> +	/**
> +	 * Force mapping to the requested address. This flag should be used
> +	 * with caution, because to fulfill the request implementation
> +	 * may remove all other mappings in the requested region. However,
> +	 * it is not required to do so, thus mapping with this flag may fail.
> +	 */
> +	RTE_MAP_FORCE_ADDRESS = 1 << 3
> +};

[...]
> +INTERNAL {
> +	global:
> +
> +	rte_mem_lock;
> +	rte_mem_map;
> +	rte_mem_page_size;
> +	rte_mem_unmap;
> +};

Not sure why these functions are internal.
They may be useful for DPDK applications.
We would need to add the file in doxygen index.

If we want to keep them internal, we should add a doxygen marker
@internal.

> +#include <rte_eal_memory.h>

I think we should find a better file name for these wrappers.
"EAL memory" means DPDK memory allocator in my mind.
We need a file name which is about OS-independent wrappers,
or libc wrappers.
What about rte_libc_mem.h? rte_mem_os.h? something else?