Hi Mattias,
It would be great to get fastmem into DPDK 26.11.
I have reviewed the patch for features and usage. Not for correctness.
Here are my comments, in somewhat random order.
-
Export symbols for 26.11, not 24.11.
-
Generally, consider setting rte_errno and returning -1, instead of returning
-ESOMETHING.
Then, e.g. in bin_alloc_one(), if slab_acquire() returns NULL, grow_socket()
(called by slab_acquire()) has already set rte_errno.
-
+/*
+ * Slab header at offset 0 of each 2 MiB slab. Either free (linked
+ * via next_free) or assigned to a bin (linked via list).
+ */
+struct fastmem_slab {
+ struct fastmem_bin *bin;
+ void *free_head;
+ uint32_t free_count;
+ uint32_t n_slots;
+ struct fastmem_slab *next_free;
+ TAILQ_ENTRY(fastmem_slab) list;
+ rte_iova_t iova_base;
+} __rte_aligned(FASTMEM_SLAB_HEADER_SIZE);
I wonder if rte_fastmem_virt2iova() is going to be a hot function?
Then it should be inline, and the iova_base field should be first in the
fastmem_slab for easier access (and possibly faster on architectures without
"load with offset" instructions).
The rte_fastmem_virt2iova() function can be inline without exposing fastmem
internals by using opaque pointer magic.
The opaque pointer magic should be protected by a bunch of static_assert()'s in
rte_fastmem.c, mentioning that rte_fastmem_virt2iova() depends on these.
Something like:
static inline rte_iova_t
rte_fastmem_virt2iova(const void *ptr)
{
// struct fastmem_slab *slab;
void *slab;
// slab = slab_of((void *)(uintptr_t)ptr);
// slab = (uintptr_t)ptr & ~(uintptr_t)FASTMEM_SLAB_MASK;
slab = ((uintptr_t)ptr & ~(uintptr_t)((1 << 21) - 1));
// rte_fastmem.c: static_assert(FASTMEM_SLAB_MASK == (1 << 21) - 1),
"Mismatch in rte_fastmem_virt2iova()");
// return slab->iova_base + ((uintptr_t)ptr - (uintptr_t)slab);
return *(rte_iova_t *)slab + ((uintptr_t)ptr - (uintptr_t)slab);
// rte_fastmem.c: static_assert(offsetof(struct fastmem_slab,
iova_base) == 0,
"Mismatch in rte_fastmem_virt2iova()");
}
-
Generally, statistics counting should be build-time configurable enable/disable
for performance.
It's acceptable to keep the variables in the structs, and just not update them.
Consider (with statistics disabled) removing the statistics variables from the
fastmem_cache structure, so the objects at the bottom of the stack are in the
same cache line as the count/capacity fields. (If it makes any practical
difference.)
-
+struct fastmem_cache {
+ uint32_t count;
+ uint32_t capacity;
+ uint32_t target;
+ uint64_t alloc_cache_hits;
+ uint64_t alloc_cache_misses;
+ uint64_t alloc_nomem;
+ uint64_t free_cache_hits;
+ uint64_t free_cache_misses;
+ void *objs[];
+} __rte_cache_aligned;
"target" is a shadow of "capacity / 2".
It is rarely used without also accessing "capacity".
Consider dropping it, and just using capacity / 2.
It might even improve performance in some locations, where "capacity" is
already loaded into a CPU register or will be used shortly thereafter. (My
performance comment is pure speculation, not measured.) (For reference, the
mempool lib uses cache->size / 2.)
-
Initialization should be done at the proper DPDK startup stage, not lazily at
all fastmem functions.
In the fastmem functions, calling fastmem_assure() should be replaced by a
simple RTE_ASSERT(fastmem != NULL).
I was wondering how do other libs handle the situation where a secondary
process depends on init performed by the primary function?
It looks like they do something similiar to fastmem_assure(). This sucks!
That's an important feature gap in DPDK, which I already registered as a bug on
the wishlist: https://bugs.dpdk.org/show_bug.cgi?id=1913
Until we get that wish fixed, we should make eal depend on fastmem, and call
the fastmem init function from inside rte_eal_init().
-
Do you foresee any other flags than RTE_FASTMEM_F_ZERO?
I'd prefer not passing a flags parameter to the fastmem public alloc APIs, and
instead expose two variants of the alloc APIs.
It's OK to pass around the zero flag (or a boolean) internally; it will be
optimized away because it is known at build time from the two alloc API
variants.
-
A fastmem handle "rte_fastmem_handle_t" is not an object handle, so the name
could be misleading.
It's rather a sort of accelerator or helper for faster alloc/free.
Also, it doesn't support SOCKET_ID_ANY (-1).
Would it be beneficial to support SOCKET_ID_ANY, so the socket_id part gets
resolved when using the fastmem handle?
Or would that defeat the performance gain of fastmem handles?
And are there relevant use cases for fastmem handle supporting SOCKET_ID_ANY,
or am I pointlessly feature creeping?
-
+#define FASTMEM_MIN_SIZE ((size_t)1 << FASTMEM_MIN_CLASS_LOG2)
+#define FASTMEM_MAX_ALLOC_SIZE ((size_t)1 << FASTMEM_MAX_CLASS_LOG2)
Min and Max names should be similar.
-
rte_fastmem_cache_flush():
The function name could indicate that it operates on the current lcore only.
Makes code using the function easier to read, for reviewers unfamiliar with the
fastmem lib.
-
In grow_socket():
+ if (socket->n_memzones == FASTMEM_MAX_MEMZONES_PER_SOCKET) {
+ FASTMEM_LOG(ERR,
+ "reached per-socket memzone cap (%u) on socket %d",
cap -> limit
-
In bin_push_locked():
+ struct fastmem_slab *slab = (struct fastmem_slab *)
+ ((uintptr_t)obj & ~(uintptr_t)FASTMEM_SLAB_MASK);
-> slab_of()
-Morten
