Re: [PATCH v14 3/6] memarea: support alloc and free API

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 2/9/2023 6:36 AM, Chengwen Feng wrote:
> This patch supports rte_memarea_alloc() and rte_memarea_free() API.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---

General note: this patchset could benefit from a bit more comments. I 
don't suggest commenting every line, but at least more comments denoting 
various logical steps (like you have in `rte_memarea_free`) would be 
nice to have.


>   #include <rte_common.h>
>   #include <rte_log.h>
> @@ -88,6 +90,8 @@ memarea_alloc_area(const struct rte_memarea_param *init)
>   					init->numa_socket);
>   	else if (init->source == RTE_MEMAREA_SOURCE_LIBC)
>   		ptr = memarea_alloc_from_libc(init->total_sz);
> +	else if (init->source == RTE_MEMAREA_SOURCE_MEMAREA)
> +		ptr = rte_memarea_alloc(init->src_ma, init->total_sz);

Why `if` not `switch`?

>   
>   	return ptr;
>   }
> @@ -99,6 +103,8 @@ memarea_free_area(const struct rte_memarea_param *init, void *ptr)
>   		rte_free(ptr);
>   	else if (init->source == RTE_MEMAREA_SOURCE_LIBC)
>   		free(ptr);
> +	else if (init->source == RTE_MEMAREA_SOURCE_MEMAREA)
> +		rte_memarea_free(init->src_ma, ptr);

Similarly here: why `if` not `switch`?

> +static inline void
> +memarea_add_node(struct rte_memarea *ma, struct memarea_objhdr *hdr, size_t alloc_sz)
> +{
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	struct memarea_objtlr *cur_tlr;
> +#endif
> +	struct memarea_objhdr *new_hdr;
> +
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	cur_tlr = RTE_PTR_ADD(hdr, sizeof(struct memarea_objhdr) + alloc_sz);
> +	cur_tlr->cookie = MEMAREA_OBJECT_TRAILER_COOKIE;
> +	new_hdr = RTE_PTR_ADD(cur_tlr, sizeof(struct memarea_objtlr));
> +	new_hdr->cookie = MEMAREA_OBJECT_HEADER_AVAILABLE_COOKIE;
> +#else
> +	new_hdr = RTE_PTR_ADD(hdr, sizeof(struct memarea_objhdr) + alloc_sz);
> +#endif
> +	TAILQ_INSERT_AFTER(&ma->obj_list, hdr, new_hdr, obj_next);
> +	TAILQ_INSERT_AFTER(&ma->avail_list, hdr, new_hdr, avail_next);
> +}

It seems to me that this function isn't "adding" node but rather is 
splitting the `hdr` into two nodes. This is nitpicking, but I feel like 
this part could be clearer semantically (splitting the function, adding 
comments, some other way...).

> +
> +void *
> +rte_memarea_alloc(struct rte_memarea *ma, size_t size)
> +{
> +	size_t align_sz = RTE_ALIGN(size, MEMAREA_OBJECT_SIZE_ALIGN);
> +	struct memarea_objhdr *hdr;
> +	size_t avail_sz;
> +	void *ptr = NULL;
> +
> +	if (unlikely(ma == NULL || size == 0 || align_sz < size))
> +		return ptr;

It would be nice if API also set rte_errno to indicate what kind of 
error has happened.

> +
> +	memarea_lock(ma);
> +	TAILQ_FOREACH(hdr, &ma->avail_list, avail_next) {
> +		memarea_check_cookie(ma, hdr, 0);
> +		avail_sz = MEMAREA_OBJECT_GET_SIZE(hdr);
> +		if (avail_sz < align_sz)
> +			continue;
> +		if (memarea_whether_add_node(avail_sz, align_sz))
> +			memarea_add_node(ma, hdr, align_sz);

I didn't get this at first, which means it needs comments :) 
Specifically, it seems to me that we're only "adding" a node when we can 
comfortably split it. So, in addition to comments documenting the above, 
perhaps the above functions should also be called differently? Like 
`memarea_can_split()` and `memarea_split`? IMO it'd communicate the 
intent better (unless I misunderstood the intent, that is!).

> +		TAILQ_REMOVE(&ma->avail_list, hdr, avail_next);
> +		MEMAREA_OBJECT_MARK_ALLOCATED(hdr);
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +		hdr->cookie = MEMAREA_OBJECT_HEADER_ALLOCATED_COOKIE;
> +#endif
> +		ptr = RTE_PTR_ADD(hdr, sizeof(struct memarea_objhdr));
> +		break;
> +	}
> +	memarea_unlock(ma);
> +
> +	return ptr;

It seems that it's possible to reach the end of the loop and return NULL 
as `ptr`, without any error. I would suggest setting rte_errno to 
`ENOMEM` initially, and clearing it when we find a suitable element.

> +}
> +
> +static inline void
> +memarea_merge_node(struct rte_memarea *ma, struct memarea_objhdr *curr,
> +		   struct memarea_objhdr *next)
> +{
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	struct memarea_objtlr *tlr;
> +#endif
> +	RTE_SET_USED(curr);
> +	TAILQ_REMOVE(&ma->obj_list, next, obj_next);
> +	TAILQ_REMOVE(&ma->avail_list, next, avail_next);
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	next->cookie = 0;
> +	tlr = RTE_PTR_SUB(next, sizeof(struct memarea_objtlr));
> +	tlr->cookie = 0;
> +#endif
> +}
> +
> +void
> +rte_memarea_free(struct rte_memarea *ma, void *ptr)
> +{
> +	struct memarea_objhdr *hdr, *prev, *next;
> +
> +	if (unlikely(ma == NULL || ptr == NULL))
> +		return;
> +
> +	hdr = RTE_PTR_SUB(ptr, sizeof(struct memarea_objhdr));
> +	if (unlikely(!MEMAREA_OBJECT_IS_ALLOCATED(hdr))) {

Here and above - I question the value of using `unlikely` here. Are 
there any numbers to prove these are useful?

> +		RTE_MEMAREA_LOG(ERR, "detect invalid object in %s!", ma->init.name);
> +		return;
> +	}
> +	memarea_check_cookie(ma, hdr, 1);
> +
> +	memarea_lock(ma);
> +
> +	/** 1st: add to avail list. */
> +#ifdef RTE_LIBRTE_MEMAREA_DEBUG
> +	hdr->cookie = MEMAREA_OBJECT_HEADER_AVAILABLE_COOKIE;
> +#endif
> +	TAILQ_INSERT_HEAD(&ma->avail_list, hdr, avail_next);
> +
> +	/** 2nd: merge if previous object is avail. */
> +	prev = TAILQ_PREV(hdr, memarea_objhdr_list, obj_next);
> +	if (prev != NULL && !MEMAREA_OBJECT_IS_ALLOCATED(prev)) {
> +		memarea_check_cookie(ma, prev, 0);
> +		memarea_merge_node(ma, prev, hdr);
> +		hdr = prev;
> +	}
> +
> +	/** 3rd: merge if next object is avail. */
> +	next = TAILQ_NEXT(hdr, obj_next);
> +	if (next != NULL && !MEMAREA_OBJECT_IS_ALLOCATED(next)) {
> +		memarea_check_cookie(ma, next, 0);
> +		memarea_merge_node(ma, hdr, next);
> +	}
> +
> +	memarea_unlock(ma);
> +}

This function is an example of how I would like to see other functions: 
good comments to denote logical blocks, clear and concise.

-- 
Thanks,
Anatoly