[PATCH v14 3/6] memarea: support alloc and free API
Burakov, Anatoly
anatoly.burakov at intel.com
Thu Jun 22 17:29:23 CEST 2023
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 at huawei.com>
> Reviewed-by: Dongdong Liu <liudongdong3 at huawei.com>
> Acked-by: Morten Brørup <mb at 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
More information about the dev
mailing list