[PATCH v14 1/6] memarea: introduce memarea library

[Burakov, Anatoly]

On 2/9/2023 6:36 AM, Chengwen Feng wrote:
> The memarea library is an allocator of variable-size object which based
> on a memory region.
> 
> This patch provides rte_memarea_create() and rte_memarea_destroy() API.
> 
> Signed-off-by: Chengwen Feng <fengchengwen at huawei.com>
> Reviewed-by: Dongdong Liu <liudongdong3 at huawei.com>
> Acked-by: Morten Brørup <mb at smartsharesystems.com>
> ---


> +#include "memarea_private.h"
> +
> +RTE_LOG_REGISTER_DEFAULT(rte_memarea_logtype, INFO);
> +#define RTE_MEMAREA_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, rte_memarea_logtype, \
> +		"MEMAREA: %s(): " fmt "\n", __func__, ## args)
> +
> +static int
> +memarea_check_param(const struct rte_memarea_param *init)
> +{
> +#define MEMAREA_MIN_SIZE	1024

I don't see this limitation being documented anywhere? This probably 
should either be moved to `rte_memarea.h`, or at least called out in the 
API documentation.

> +	size_t len;
> +
> +	if (init == NULL) {
> +		RTE_MEMAREA_LOG(ERR, "init param is NULL!");
> +		return -EINVAL;
> +	}
> +
> +	len = strnlen(init->name, RTE_MEMAREA_NAMESIZE);
> +	if (len == 0 || len >= RTE_MEMAREA_NAMESIZE) {
> +		RTE_MEMAREA_LOG(ERR, "name size: %zu invalid!", len);
> +		return -EINVAL;
> +	}
> +
> +	if (init->source != RTE_MEMAREA_SOURCE_HEAP &&
> +	    init->source != RTE_MEMAREA_SOURCE_LIBC &&
> +	    init->source != RTE_MEMAREA_SOURCE_MEMAREA) {
> +		RTE_MEMAREA_LOG(ERR, "%s source: %d not supported!",
> +			init->name, init->source);
> +		return -EINVAL;
> +	}
> +
> +	if (init->total_sz < MEMAREA_MIN_SIZE) {
> +		RTE_MEMAREA_LOG(ERR, "%s total-size: %zu too small!",
> +			init->name, init->total_sz);
> +		return -EINVAL;
> +	}
> +
> +	if (init->source == RTE_MEMAREA_SOURCE_MEMAREA && init->src_ma == NULL) {
> +		RTE_MEMAREA_LOG(ERR, "%s source memarea is NULL!", init->name);
> +		return -EINVAL;
> +	}
> +
> +	if (init->alg != RTE_MEMAREA_ALGORITHM_NEXTFIT) {
> +		RTE_MEMAREA_LOG(ERR, "%s algorithm: %d not supported!",
> +			init->name, init->alg);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void *
> +memarea_alloc_from_libc(size_t size)
> +{
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +	return _aligned_malloc(size, RTE_CACHE_LINE_SIZE);
> +#else
> +	void *ptr = NULL;
> +	int ret;
> +	ret = posix_memalign(&ptr, RTE_CACHE_LINE_SIZE, size);
> +	if (ret)
> +		return NULL;

Would `ptr` not be NULL if ret wasn't 0?


> +struct rte_memarea *
> +rte_memarea_create(const struct rte_memarea_param *init)
> +{
> +	struct memarea_objhdr *hdr, *guard_hdr;
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	struct memarea_objtlr *tlr;
> +#endif
> +	struct rte_memarea *ma;
> +	size_t align_sz;
> +	void *ptr;
> +	int ret;
> +
> +	ret = memarea_check_param(init);
> +	if (ret)
> +		return NULL;
> +
> +	ptr = memarea_alloc_area(init);
> +	if (ptr == NULL) {
> +		RTE_MEMAREA_LOG(ERR, "%s alloc memory area fail!", init->name);
> +		return NULL;
> +	}
> +
> +	ma = rte_zmalloc(NULL, sizeof(struct rte_memarea), RTE_CACHE_LINE_SIZE);
> +	if (ma == NULL) {
> +		memarea_free_area(init, ptr);
> +		RTE_MEMAREA_LOG(ERR, "%s alloc management object fail!", init->name);
> +		return NULL;
> +	}
> +
> +	hdr = ptr;
> +	align_sz = RTE_ALIGN_FLOOR(init->total_sz, MEMAREA_OBJECT_SIZE_ALIGN);
> +	guard_hdr = RTE_PTR_ADD(ptr, align_sz - sizeof(struct memarea_objhdr));
> +
> +	ma->init = *init;
> +	rte_spinlock_init(&ma->lock);
> +	ma->area_base = ptr;
> +	ma->guard_hdr = guard_hdr;
> +	TAILQ_INIT(&ma->obj_list);
> +	TAILQ_INIT(&ma->avail_list);
> +
> +	TAILQ_INSERT_TAIL(&ma->obj_list, hdr, obj_next);
> +	TAILQ_INSERT_TAIL(&ma->avail_list, hdr, avail_next);
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	hdr->cookie = MEMAREA_OBJECT_HEADER_AVAILABLE_COOKIE;
> +	tlr = RTE_PTR_SUB(guard_hdr, sizeof(struct memarea_objtlr));
> +	tlr->cookie = MEMAREA_OBJECT_TRAILER_COOKIE;
> +#endif
> +
> +	memset(guard_hdr, 0, sizeof(struct memarea_objhdr));
> +	TAILQ_INSERT_AFTER(&ma->obj_list, hdr, guard_hdr, obj_next);
> +	MEMAREA_OBJECT_MARK_ALLOCATED(guard_hdr);
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	guard_hdr->cookie = MEMAREA_OBJECT_HEADER_ALLOCATED_COOKIE;
> +	/* The guard object have no trailer cookie. */
> +#endif

Nitpicking, but can we move the #ifdef-ery into functions? E.g. have 
something like

set_header_cookie(hdr);
...
set_trailer_cookie(guard_hdr);

and have them just be noops if RTE_LIBRTE_MEMAREA_DEBUG is not defined?

> +
> +	return ma;
> +}
> +
> +void
> +rte_memarea_destroy(struct rte_memarea *ma)
> +{
> +	if (ma == NULL)
> +		return;
> +	memarea_free_area(&ma->init, ma->area_base);
> +	rte_free(ma);
> +}
> diff --git a/lib/memarea/rte_memarea.h b/lib/memarea/rte_memarea.h
> new file mode 100644
> index 0000000000..435dca293f
> --- /dev/null
> +++ b/lib/memarea/rte_memarea.h
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 HiSilicon Limited
> + */
> +
> +#ifndef RTE_MEMAREA_H
> +#define RTE_MEMAREA_H
> +
> +/**
> + * @file
> + * RTE Memarea.
> + *
> + * The memarea is an allocator of variable-size object which based on a memory
> + * region. It has the following features:
> + *
> + * - The memory region can be initialized from the following memory sources:
> + *   1. HEAP: e.g. invoke rte_malloc_socket.
> + *   2. LIBC: e.g. invoke posix_memalign.
> + *   3. Another memarea: it can be allocated from another memarea.
> + *
> + * - It supports MT-safe as long as it's specified at creation time. If not
> + *   specified, all the functions of the memarea API are lock-free, and assume
> + *   to not be invoked in parallel on different logical cores to work on the
> + *   same memarea.

I think it would be nice to explicitly mention three things here:

1) that the alignment is always on cache line boundary
2) that the memory is not intended for DMA purposes
3) that secondary processes are not supported

Also, *technically*, "another memarea" is only supported starting at 
commit 3, so I would suggest either adding stubs to support it in this 
commit, or not add it to the enums.

> + */
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +
> +#include <rte_compat.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define RTE_MEMAREA_NAMESIZE	64
> +
> +/**
> + * Memarea memory source.
> + */
> +enum rte_memarea_source {
> +	/** Memory source comes from rte memory. */

DPDK memory? rte_malloc memory? I don't think we use the term "rte" to 
refer to DPDK anywhere outside of legacy usages

> +	RTE_MEMAREA_SOURCE_HEAP,
> +	/** Memory source comes from libc. */
> +	RTE_MEMAREA_SOURCE_LIBC,
> +	/** Memory source comes from another memarea. */
> +	RTE_MEMAREA_SOURCE_MEMAREA,

See above note about this not being supported in this commit.

> +};
> +
> +/**
> + * Memarea memory management algorithm.
> + */
> +enum rte_memarea_algorithm {
> +	/** The default management algorithm is a variant of the next fit
> +	 * algorithm. It uses a free-list to apply for memory and uses an
> +	 * object-list in ascending order of address to support merging
> +	 * upon free.
> +	 */
> +	RTE_MEMAREA_ALGORITHM_NEXTFIT,
> +};
> +
> +struct rte_memarea;
> +
> +/**
> + * Memarea creation parameters.
> + */
> +struct rte_memarea_param {
> +	char name[RTE_MEMAREA_NAMESIZE]; /**< Name of memarea. */
> +	enum rte_memarea_source source;  /**< Memory source of memarea. */
> +	enum rte_memarea_algorithm alg;  /**< Memory management algorithm. */
> +	size_t total_sz;                 /**< Total size (bytes) of memarea. */
> +	/** Indicates whether the memarea API should be MT-safe. */
> +	uint32_t mt_safe : 1;
> +	union {
> +		/** Numa socket from which to apply for memarea's memory, this
> +		 * field is valid only when the source is set to be
> +		 * RTE_MEMAREA_SOURCE_HEAP.
> +		 */
> +		int numa_socket;
> +		/** Source memarea, this field is valid only when the source is
> +		 * set to be RTE_MEMAREA_SOURCE_MEMAREA.
> +		 */
> +		struct rte_memarea *src_ma;

Wouldn't it be better to have these fields inside structs indicating 
relevant modes? E.g. param->heap.numa_socket as opposed to 
param->numa_socket - I think this will be more self-explanatory in code. 
Also, I think the convention around DPDK is to refer to NUMA sockets as 
`socket_id` rather than `numa_socket` so for consistency I think it 
would be nice if this was the case here as well.

-- 
Thanks,
Anatoly