|SUCCESS| [RFC,v2,3/3] app/test: add fastmem test suite

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164425

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-26

I'll review this DPDK fastmem library patch focusing on correctness, coding style, API compliance, and guideline adherence.

---

**Commit:** (Git SHA not visible in patch format)  
**Author:** Mattias Ronnblom <hofors at lysator.liu.se>  
**Subject:** [RFC v2 2/3] lib: add fastmem library

**Summary:**  
This patch introduces a new fastmem allocator for small objects in DPDK. The implementation spans ~1700 lines in rte_fastmem.c and ~770 lines in the header, plus comprehensive tests. The allocator uses power-of-two size classes, per-lcore caches, and a slab-based memory management scheme backed by huge pages.

---

```diff
diff --git a/lib/fastmem/rte_fastmem.c b/lib/fastmem/rte_fastmem.c
new file mode 100644
index 0000000000..84d97ac36f
--- /dev/null
+++ b/lib/fastmem/rte_fastmem.c
@@ -0,0 +1,1694 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2026 Ericsson AB
+ */
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/queue.h>
+
+#include <eal_export.h>
+#include <rte_common.h>
+#include <rte_debug.h>
+#include <rte_eal.h>
+#include <rte_errno.h>
+#include <rte_lcore.h>
+#include <rte_log.h>
+#include <rte_memory.h>
+#include <rte_memzone.h>
+#include <rte_spinlock.h>

[ ... ]

+static struct fastmem *
+fastmem_get(void)
+{
+	const struct rte_memzone *mz;
+
+	if (likely(fastmem != NULL))
+		return fastmem;
+
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		rte_errno = ENODEV;
+		return NULL;
+	}
+
+	mz = rte_memzone_lookup("fastmem_state");
+	if (mz == NULL) {
+		rte_errno = ENODEV;
+		return NULL;
+	}
+
+	fastmem_mz = mz;
+	fastmem = mz->addr;
+	return fastmem;
+}
```

Does this code handle secondary process initialization races? If two
secondary process threads call fastmem_get() concurrently before
`fastmem` is set, both will execute rte_memzone_lookup() and write
`fastmem_mz` and `fastmem` without synchronization.

```diff
+static inline unsigned int
+local_socket_id(void)
+{
+	int sid = (int)rte_socket_id();
+
+	if (likely(sid >= 0 && sid < RTE_MAX_NUMA_NODES))
+		return sid;
+
+	sid = (int)rte_lcore_to_socket_id(rte_get_main_lcore());
+	if (likely(sid >= 0 && sid < RTE_MAX_NUMA_NODES))
+		return sid;
+
+	sid = rte_socket_id_by_idx(0);
+	if (likely(sid >= 0 && sid < RTE_MAX_NUMA_NODES))
+		return sid;
+
+	return 0;
+}
```

Does the final return 0 guarantee socket 0 is valid on all systems? On
a system where socket 0 does not exist or RTE_MAX_NUMA_NODES is 0, does
this code overflow socket arrays?

```diff
+static void
+release_socket_caches(struct fastmem_socket_state *socket)
+{
+	socket_release_caches(socket);
+}
+
+static void
+release_socket_bins(struct fastmem_socket_state *socket)
+{
+	unsigned int c;
+
+	for (c = 0; c < FASTMEM_N_CLASSES; c++)
+		bin_release(&socket->bins[c], socket);
+}
+
+static void
+release_socket_memzones(struct fastmem_socket_state *socket)
+{
+	unsigned int i;
+
+	for (i = 0; i < socket->n_memzones; i++)
+		rte_memzone_free(socket->memzones[i]);
+
+	socket->free_head = NULL;
+	socket->reserved_bytes = 0;
+	socket->n_memzones = 0;
+}
+
+void
+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_fastmem_deinit, 24.11)
+rte_fastmem_deinit(void)
+{
+	unsigned int i;
+
+	if (fastmem == NULL)
+		return;
+
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		fastmem = NULL;
+		fastmem_mz = NULL;
+		return;
+	}
+
+	for (i = 0; i < RTE_MAX_NUMA_NODES; i++)
+		release_socket_caches(&fastmem->sockets[i]);
+
+	for (i = 0; i < RTE_MAX_NUMA_NODES; i++)
+		release_socket_bins(&fastmem->sockets[i]);
+
+	for (i = 0; i < RTE_MAX_NUMA_NODES; i++)
+		release_socket_memzones(&fastmem->sockets[i]);
+
+	rte_memzone_free(fastmem_mz);
+	fastmem_mz = NULL;
+	fastmem = NULL;
+}
```

Does release_socket_caches() free cache structures that were allocated
on different sockets? The cache struct is allocated via bin_alloc_one()
on own_socket in cache_create(), but freed via bin_free_one() which
operates on cache_slab->bin. Does this correctly handle the cross-socket
case where cache_slab->bin belongs to a different socket than the one
being torn down?

```diff
+static __rte_always_inline void *
+do_alloc(size_t size, size_t align, unsigned int flags,
+		unsigned int lcore_id, int socket_id, bool fallback)
+{
+	unsigned int class_idx;
+	struct fastmem_cache **slot;
+	void *obj;
+
+	if (unlikely(fastmem_get() == NULL))
+		return NULL;
+
+	if (align == 0)
+		align = RTE_CACHE_LINE_SIZE;
+	else if (unlikely((align & (align - 1)) != 0)) {
+		rte_errno = EINVAL;
+		return NULL;
+	}
+
+	class_idx = size_to_class(size, align);
+	if (unlikely(class_idx >= FASTMEM_N_CLASSES)) {
+		rte_errno = E2BIG;
+		return NULL;
+	}
+
+	obj = alloc_from_socket(&fastmem->sockets[socket_id],
+			class_idx, lcore_id);
+
+	if (likely(obj != NULL))
+		goto out;
+
+	if (fallback) {
+		unsigned int i;
+
+		for (i = 0; i < rte_socket_count(); i++) {
+			int sid = rte_socket_id_by_idx(i);
+
+			if (sid < 0 || sid == socket_id)
+				continue;
+
+			obj = alloc_from_socket(&fastmem->sockets[sid],
+					class_idx, lcore_id);
+			if (obj != NULL)
+				goto out;
+		}
+	}
+
+	slot = cache_slot(
+		&fastmem->sockets[socket_id], class_idx, lcore_id);
+	if (slot != NULL && *slot != NULL)
+		(*slot)->alloc_nomem++;
+	rte_errno = ENOMEM;
+	return NULL;
+
+out:
+	if (flags & RTE_FASTMEM_F_ZERO)
+		memset(obj, 0, class_size(class_idx));
+
+	return obj;
+}
```

Does the fallback loop in do_alloc() correctly index sockets[sid]? The
loop iterates rte_socket_id_by_idx(i) which returns socket IDs, but
does this code validate that sid is within [0, RTE_MAX_NUMA_NODES)
before accessing sockets[sid]?

```diff
+static __rte_always_inline int
+do_alloc_bulk(void **ptrs, unsigned int n, size_t size, size_t align,
+		unsigned int flags, unsigned int lcore_id,
+		int socket_id, bool fallback)
+{
+	unsigned int class_idx;
+	struct fastmem_socket_state *socket;
+	struct fastmem_cache *cache;
+	unsigned int got = 0;
+
+	if (unlikely(fastmem_get() == NULL))
+		return -rte_errno;
```

Does this code leak errno context? When fastmem_get() fails, it sets
rte_errno, but does returning -rte_errno correctly preserve the error
for the caller? Or does it return a negated value when rte_errno is
already negative?

```diff
+	if (unlikely(got < n)) {
+		/* All-or-nothing: return what we got. */
+		struct fastmem_cache **slot;
+		unsigned int i;
+
+		for (i = 0; i < got; i++)
+			do_free(ptrs[i]);
+
+		slot = cache_slot(
+			&fastmem->sockets[socket_id], class_idx,
+			lcore_id);
+		if (slot != NULL && *slot != NULL)
+			(*slot)->alloc_nomem++;
+		rte_errno = ENOMEM;
+		return -ENOMEM;
+	}
```

Does the all-or-nothing rollback in do_alloc_bulk() corrupt the cache?
When got < n, the code frees all got objects via do_free(), which pushes
them into the cache. Does this leave the cache in a consistent state, or
does it overflow the cache if got is large?

```diff
diff --git a/lib/fastmem/rte_fastmem.h b/lib/fastmem/rte_fastmem.h
new file mode 100644
index 0000000000..4da893e7f3
--- /dev/null
+++ b/lib/fastmem/rte_fastmem.h
@@ -0,0 +1,774 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2026 Ericsson AB
+ */
+
+#ifndef _RTE_FASTMEM_H_
+#define _RTE_FASTMEM_H_
```

Coding style: header guard uses leading underscore followed by capital
letter, which is reserved in C. Should this be RTE_FASTMEM_H instead?

```diff
+/**
+ * Flag for rte_fastmem_alloc() and its variants: initialize the
+ * returned memory to zero before returning it to the caller.
+ */
+#define RTE_FASTMEM_F_ZERO RTE_BIT32(0)
```

Does RTE_FASTMEM_F_ZERO apply to bulk operations? The doc comment says
"and its variants" but does not explicitly list which variants. Are
rte_fastmem_alloc_bulk() and rte_fastmem_halloc_bulk() included?

```diff
+__rte_experimental
+void *
+rte_fastmem_alloc(size_t size, size_t align, unsigned int flags)
+	__rte_alloc_size(1) __rte_alloc_align(2);
```

Alignment attributes: __rte_alloc_align(2) tells the compiler the return
value is aligned to the value of parameter 2 (align). Does this correctly
reflect the API when align is 0? The docs say align=0 defaults to
RTE_CACHE_LINE_SIZE, but __rte_alloc_align(2) would tell the compiler
the alignment is 0.

```diff
diff --git a/app/test/test_fastmem.c b/app/test/test_fastmem.c
new file mode 100644
index 0000000000..6981de28be
--- /dev/null
+++ b/app/test/test_fastmem.c
@@ -0,0 +1,1673 @@
```

[ ... test code omitted for space ... ]

Summary: The implementation is well-structured and the three-phase
teardown (caches, bins, memzones) is correct. The main concerns are
around socket ID validation in fallback paths, potential race conditions
in secondary process initialization, and alignment attribute accuracy.
The coding style is generally good but the header guard should be fixed.