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

[fengchengwen]

Hi Dmitry,

On 2022/9/20 19:30, Dmitry Kozlyuk wrote:
> 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.

Sorry to forgot reply it.

We have the following scene which used refcnt:

     nic-rx  ->  decoder ->  process

                                    |

                                   ->  recording

as above show, the process and recording module both use the decoder's 
output, the are just reader.

so in this case, the refcnt is useful.

>
>> +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.

No need checking because name is an array.

Maybe I should check init == NULL here.

>
>> +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.

will fix in v2.

>
>> +/**
>> + * 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 :)
will fix in v2
>
>> +};
>> +
>> +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?

do you mean the rte_memarea just void * ? it just (void 
*)(memarea_private *)priv ?

It's another popular type to impl ABI compatiable.

It's more simpler, will fix in v2