Re: [PATCH 1/8] memarea: introduce memory area library

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

2022-09-20 03:46 (UTC+0000), Chengwen Feng:
> The memarea library is an allocator of variable-size object. It is a
> collection of allocated objects that can be efficiently alloc or free
> all at once, the main features are as follows:
> a) it facilitate alloc and free of memory with low overhead.

Yet, the overhead is 64B per element, just like rte_malloc.

> b) it provides refcnt feature which could be useful in some scenes.

Are you sure refcnt should be in this library?
I've expressed my concerns here:

https://inbox.dpdk.org/dev/CAEYuUWBpC-9dCqKJ0LZi6RkCUwyeYEghLRBMBUBtUx4Ljg+pAQ@mail.gmail.com

There are more unanswered questions in that mail,
it would be good to clarify them before reviewing these patches
in order to understand all the intentions.

> +static int
> +memarea_check_param(const struct rte_memarea_param *init)
> +{
> +	size_t len;
> +
> +	len = strnlen(init->name, RTE_MEMAREA_NAMESIZE);
> +	if (len == 0 || len >= RTE_MEMAREA_NAMESIZE) {
> +		RTE_LOG(ERR, MEMAREA, "memarea name invalid!\n");
> +		return -EINVAL;
> +	}

Please check init->name == NULL first.

> +struct rte_memarea *
> +rte_memarea_create(const struct rte_memarea_param *init)
> +{
[...]
> +		RTE_LOG(ERR, MEMAREA, "malloc memarea management obj fail!\n");

In all error messages, it would be useful to provide details:
the name of the area, what size was requested, etc.

> +/**
> + * Memarea memory source.
> + */
> +enum rte_memarea_source {
> +	/** Memory source comes from system API (e.g. malloc). */
> +	RTE_MEMAREA_SOURCE_SYSTEM_API,
> +	/** Memory source comes from user-provided address. */
> +	RTE_MEMAREA_SOURCE_USER_ADDR,
> +	/** Memory source comes from user-provided memarea. */
> +	RTE_MEMAREA_SOURCE_USER_MEMAREA,
> +
> +	RTE_MEMAREA_SOURCE_BUTT

DPDK enumerations must not include an item to hold the element count,
because it is harmful for ABI (e.g. developers create arrays of this size
and when a new item is added in a new DPDK version, the array overflows).

If it's supposed to mean "the end of item list",
the proper word would be "last" or "max" BTW :)

> +};
> +
> +struct rte_memarea {
> +	void *private_data; /**< private management data pointer. */
> +};

Jerin and Stephen suggested to make the structure opaque,
i.e. only declare the struct and define it privately.
It would reduce ABI and simplify allocation.
Any justification to expose it?