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 <fengcheng...@huawei.com>
Reviewed-by: Dongdong Liu <liudongdo...@huawei.com>
Acked-by: Morten Brørup <m...@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
