|WARNING| [RFC,v11] pile stack and mempool driver

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/167506

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-11

# DPDK Patch Review

## Summary
This is a comprehensive patch adding a new "pile" stack and mempool driver optimized for bulk operations. The patch contains numerous correctness issues, style violations, and API design concerns that must be addressed.

---

## ERRORS (Must Fix)

### Correctness Bugs

**1. Race condition in `__rte_stack_pile_pop()` fragmentation path**

When fragmentation succeeds but freeing the excess objects fails, the code attempts to roll back by pushing the fragmentation element back to the bulk list. However, the fragmentation element's object array has been partially consumed (first `n_solo` objects copied out). Rolling back without restoring these objects corrupts the pile state.

```c
/* Get n_solo objects from the fragmentation element. */
for (i = 0; i < n_solo; i++)
    obj_table[n_bulk * RTE_STACK_PILE_BULK_SIZE + i] = obj_frag[i];
```

After this, `obj_frag[0..n_solo-1]` are in `obj_table`, but on rollback failure:

```c
fail:
    if (frag != NULL) {
        /* No further action than this is required to roll the fragmentation
         * element back into the pile of bulk elements, as the objects in
         * the fragmentation element are intact.  <-- WRONG
         */
```

The comment claims objects are intact but they're not. If the allocation of free solo elements fails, the fragmentation element is pushed back with corrupted contents.

**Fix:** Before rollback, restore the consumed objects to the fragmentation element, or ensure the frag element is freed rather than pushed back if allocation fails.

---

**2. Resource leak in `mempool_cache_create()` on error path**

```c
struct rte_mempool_cache *
rte_mempool_cache_create(uint32_t size, int socket_id)
{
    /* ... validation adjusting size ... */
    if (size == 0 || size > RTE_MEMPOOL_CACHE_MAX_SIZE) {
        rte_errno = EINVAL;
        return NULL;  /* rte_errno set but no cleanup needed yet - OK */
    }

    cache = rte_zmalloc_socket("MEMPOOL_CACHE", ..., socket_id);
    if (cache == NULL) {
        rte_errno = ENOMEM;
        return NULL;  /* OK - allocation failed, nothing to free */
    }

    mempool_cache_init(cache, size);
    return cache;
}
```

Actually, no leak here. My initial concern was premature. All error paths before allocation return early; the only path that allocates either succeeds or returns NULL after setting `rte_errno`. No leak exists.

**Correction: This is not an error.** Retracted.

---

**3. Statistics counter assignment instead of accumulation (driver fix incomplete)**

In `drivers/net/sxe2/sxe2_txrx_vec_avx512.c`:

```c
if (cache->len >= cache->size) {
    (void)rte_mempool_ops_enqueue_bulk(mp,
            &cache->objs[cache->size], cache->len - cache->size);
    cache->len = cache->size;  /* Assignment OK here - resetting cache */
}
```

This is actually correct -- it's resetting the cache length after flushing, not accumulating statistics. The guideline applies to statistics counters (`stats->rx_packets += nb_rx`), not to cache management (`cache->len = cache->size` after flush).

**Correction: This is not an error.** Retracted.

---

**4. Missing `RTE_EXPORT_SYMBOL` for new public functions**

The new pile functions `rte_stack_pile_init()` and `rte_stack_pile_get_memsize()` are defined in `rte_stack_pile.c` without any export macros. These functions are declared in the public header `rte_stack_pile.h` (which is in `indirect_headers`, but still installed), yet lack `RTE_EXPORT_SYMBOL` or `RTE_EXPORT_INTERNAL_SYMBOL`.

Looking at the declarations:

```c
/**
 * @internal Initialize a pile stack.
 */
void
rte_stack_pile_init(struct rte_stack *s, unsigned int count);

/**
 * @internal Return the memory required for a pile stack.
 */
ssize_t
rte_stack_pile_get_memsize(unsigned int count);
```

Marked `@internal`, so they should use `RTE_EXPORT_INTERNAL_SYMBOL`:

```c
RTE_EXPORT_INTERNAL_SYMBOL(rte_stack_pile_init)
void
rte_stack_pile_init(struct rte_stack *s, unsigned int count)
{
    /* ... */
}

RTE_EXPORT_INTERNAL_SYMBOL(rte_stack_pile_get_memsize)
ssize_t
rte_stack_pile_get_memsize(unsigned int count)
{
    /* ... */
}
```

Without this, the symbols won't be exported on shared library builds.

---

**5. Mempool cache alignment assumption violated when cache size < 16**

In `rte_mempool_do_generic_put()`:

```c
const size_t move = RTE_ALIGN_MUL_CEIL(
        sizeof(void *) * (cache->len - cache->size / 2), 32);
__rte_assume(move >= 32);
__rte_assume((move & 31) == 0);
rte_memcpy(cache->objs, __rte_assume_cache_aligned(&cache->objs[cache->size / 2]),
        move);
```

The `__rte_assume_cache_aligned()` asserts that `&cache->objs[cache->size / 2]` is cache-line aligned (64 bytes on x86). On 64-bit systems with 64-byte cache lines, this requires `cache->size / 2 * sizeof(void *)` to be a multiple of 64, i.e., `cache->size / 2 * 8` must be divisible by 64, so `cache->size` must be divisible by 16.

However, the code only enforces that `cache->size` is divisible by 32:

```c
if (size & 31) {
    uint32_t rounded = RTE_ALIGN_MUL_FLOOR(size, 32);
    /* ... */
    size = rounded;
}
```

If `cache->size = 32`, then `cache->size / 2 = 16`, and `&cache->objs[16]` is at offset `16 * 8 = 128` bytes from `cache->objs`. If `cache->objs` is at offset 0 in the cache-aligned structure, this is 128-byte aligned (OK). But if the structure layout changes or on 128-byte cache line systems, the assumption may not hold.

More critically: on x86-64 with 64-byte cache lines, if `cache->size = 32`, the offset is 128 bytes which is 64-byte aligned (OK). But the comment says:

```c
/* Note: For cache->objs[cache->size / 2] to be cache line aligned, cache->size
 * must be divisible by 32 on 32-bit architecture with 64-byte cache line,
 * divisible by 32 on 64-bit architecture with 128-byte cache line, and
 * be divisible by 16 on 64-bit architecture with 64-byte cache line.
 */
```

The actual code requires divisibility by 32, but the comment says 64-bit/64-byte needs divisibility by 16. This is a discrepancy. The code is more conservative (32) which is safe, but the `__rte_assume_cache_aligned()` might still be wrong if the structure is not laid out as assumed.

**Actually, looking closer:** `cache->objs` is declared `alignas(RTE_CACHE_LINE_SIZE)`, so it's cache-line aligned. On 64-byte systems, if `cache->size = 32`, then `&cache->objs[16]` = `cache->objs + 128 bytes` = `<cache-line-aligned-base> + 128`. Since 128 is a multiple of 64, this is also cache-line aligned. The code is correct **if** `cache->size >= 32`. But the code rounds down tiny caches to 0, disabling them:

```c
if (rounded == 0)
    RTE_MEMPOOL_LOG(WARNING,
            "Tiny cache size %u not divisible by 32, disabling cache.",
            cache_size);
```

So cache sizes 1-31 are disabled. This is fine. The only risk is if someone bypasses the creation check and forces a cache with size 16-31. That would violate the alignment assumption on 64-bit/64-byte systems. But the patch explicitly prevents this.

**Correction: This is not an error** given the enforcement of cache size divisibility and disabling of tiny caches.

---

**6. Shared variable access without atomics in `mempool_audit_cache()`**

```c
static void
mempool_audit_cache(const struct rte_mempool *mp)
{
    unsigned lcore_id;
    const uint32_t cache_size = mp->cache_size;

    /* ... */

    for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
        const struct rte_mempool_cache *cache;
        cache = &mp->local_cache[lcore_id];
        if (cache->size != cache_size) {
            RTE_MEMPOOL_LOG(CRIT, "badness on cache[%u] size", lcore_id);
            rte_panic("MEMPOOL: invalid cache[%u] size\n", lcore_id);
        }
        if (cache->len > cache_size) {
            RTE_MEMPOOL_LOG(CRIT, "badness on cache[%u] len", lcore_id);
            rte_panic("MEMPOOL: invalid cache[%u] len\n", lcore_id);
        }
    }
}
```

This function reads `cache->len` from all lcores without synchronization. If another lcore is concurrently modifying `cache->len` (during `rte_mempool_put` or `get`), this is a data race. The comment in `mempool_audit_cookies()` acknowledges this for stack checks but doesn't address it. `cache->len` should be read atomically:

```c
uint32_t len = rte_atomic_load_explicit(&cache->len, rte_memory_order_relaxed);
if (len > cache_size) {
    /* ... */
}
```

But `cache->len` is declared `uint32_t`, not `RTE_ATOMIC(uint32_t)`. This is an ABI change. For now, this is a data race, though likely benign in practice (reading a 32-bit aligned value is usually atomic on modern architectures). However, per the guidelines, shared variable access must use atomics.

**Actually:** Audit functions are typically called when the mempool is quiescent (e.g., at shutdown or in tests). The patch doesn't introduce this issue; it exists in the original code. The new `mempool_audit_cache()` check of `cache->size` is new, but the `cache->len` check replaces the old check on `cache->len > RTE_DIM(cache->objs)` which had the same race.

If the audit is meant to be called during active use, it's a correctness bug. If it's only called when quiescent, it's acceptable. Given the `rte_panic()` on failure, this seems like a debug/sanity check not a runtime check.

**Conclusion:** Not a correctness bug introduced by this patch. The race existed; the patch just changes the check. Audit is assumed to run when mempool is idle.

---

### Process and API Violations

**7. Experimental API missing `__rte_experimental` tag**

`RTE_STACK_F_PILE` is documented as experimental:

```c
/**
 * The stack-like pile uses lock-free push and pop functions.
 * It is optimized for bulks of objects, and is not strictly LIFO.
 * This flag is only supported on x86_64 or arm64 platforms, currently.
 *
 * @warning
 * @b EXPERIMENTAL: this API may change, or be removed, without prior notice.
 */
#define RTE_STACK_F_PILE 0x0002
```

But the inline functions `__rte_stack_pile_push()`, `__rte_stack_pile_pop()`, `__rte_stack_pile_count()` in `rte_stack_pile.h` are not marked `__rte_experimental`. Nor is the struct `rte_stack_pile_bulk_elem`.

Wait, actually: these are static inline functions, not exported symbols. They don't need `__rte_experimental` because they're not ABI. The flag `RTE_STACK_F_PILE` itself doesn't require tagging; it's a macro. The warning in the doc comment is sufficient.

However, looking at `rte_stack.h`:

```c
static __rte_always_inline unsigned int
rte_stack_push(struct rte_stack *s, void * const *obj_table, unsigned int n)
{
    /* ... */
    if (s->flags & RTE_STACK_F_LF)
        return __rte_stack_lf_push(s, obj_table, n);
    else if (s->flags & RTE_STACK_F_PILE)
        return __rte_stack_pile_push(s, obj_table, n);  /* Calling experimental code */
```

The `rte_stack_push()` function itself is not experimental. By adding this branch without marking `rte_stack_push()` as experimental when used with `RTE_STACK_F_PILE`, the API is inconsistent. But since it's controlled by a flag, this is acceptable -- the flag's experimental warning covers it.

**Correction: Not an error.** The experimental warning on the flag is sufficient.

---

**8. Mempool driver missing from default build configuration comment**

`config/rte_config.h` changes the default mempool ops:

```c
#define RTE_MBUF_DEFAULT_MEMPOOL_OPS "pile" /* FIXME: Test only. Default: "ring_mp_mc" */
```

The comment says "Test only" but this change should not be in a production patch. This is acceptable for an RFC, but must be reverted before merging.

**Not a correctness bug, but a process error** for production inclusion.

---

---

## WARNINGS (Should Fix)

### API and Documentation

**1. Missing release notes**

The patch adds:
- New pile stack implementation with `RTE_STACK_F_PILE` flag
- New "pile" mempool driver
- Changes to mempool cache behavior (rounding, size validation)
- Changes to `RTE_MEMPOOL_CACHE_MAX_SIZE` (512 -> 1024)

No `doc/guides/rel_notes/release_*.rst` file is included. The commit message says "release notes must be added", but this must be done before merging (not just mentioned).

---

**2. Changes to `RTE_MEMPOOL_NAMESIZE` calculation**

```c
-#define RTE_MEMPOOL_NAMESIZE (RTE_RING_NAMESIZE - \
-              sizeof(RTE_MEMPOOL_MZ_PREFIX) + 1)
+#define RTE_MEMPOOL_NAMESIZE (RTE_MEMZONE_NAMESIZE - \
+        (sizeof(RTE_MEMPOOL_DRIVER_REPRESENTATIVE_MZ_PREFIX) - 1) - \
+        (sizeof(RTE_MEMPOOL_MZ_PREFIX) - 1))
```

The new calculation uses `RTE_MEMZONE_NAMESIZE` instead of `RTE_RING_NAMESIZE`, which is more correct (mempools use memzones, not rings directly). However, this changes the maximum allowed mempool name length. The comment explains the rationale, but this is a behavior change that should be documented in release notes and carefully considered for ABI impact.

The old definition was:
- `RTE_RING_NAMESIZE` = `RTE_MEMZONE_NAMESIZE - sizeof("RG_") + 1` = `RTE_MEMZONE_NAMESIZE - 3 + 1` = `RTE_MEMZONE_NAMESIZE - 2`
- Old `