|WARNING| [RFC,v4] NEW: pile stack and mempool driver

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This RFC patch introduces a new "pile" stack implementation and mempool driver, along with various mempool optimizations. The patch is submitted as a work-in-progress snapshot and acknowledged by the author to require separation into multiple patches.

---

## Errors

### 1. Resource Leak on Error Path (lib/mempool/rte_mempool.c:914)

**Location:** `rte_mempool_create_empty()`, error handling after memset

```c
ret = strlcpy(mp->name, name, sizeof(mp->name));
if (ret < 0 || ret >= (int)sizeof(mp->name)) {
    rte_errno = ENAMETOOLONG;
    goto exit_unlock;
}
```

**Issue:** On this error path, `mp` (allocated from memzone `mz`) is not freed. The memzone `mz` was allocated but is never freed on this error branch, causing a memzone leak.

**Fix:** The error path should free the memzone:
```c
if (ret < 0 || ret >= (int)sizeof(mp->name)) {
    rte_errno = ENAMETOOLONG;
    rte_memzone_free(mz);
    goto exit_unlock;
}
```

---

### 2. Statistics Not Updated on Mempool Cache Enqueue Overflow (drivers/net/sxe2/sxe2_txrx_vec_avx512.c)

**Location:** Line 70-74

```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;
}
```

**Issue:** When flushing the cache overflow to the mempool, the statistics counters in `cache->stats` (if enabled) are not updated. The `rte_mempool_ops_enqueue_bulk()` bypasses the normal put path that would update per-lcore stats.

**Context:** This is a driver-internal cache flush. If `RTE_LIBRTE_MEMPOOL_STATS` is enabled, the put operation should be reflected in statistics.

**Fix:** Either use the higher-level `rte_mempool_generic_put()` which handles stats, or manually update `cache->stats.put_bulk` and `cache->stats.put_objs` after the enqueue.

---

### 3. Missing `__rte_restrict` on Overlapping Pointers (lib/stack/rte_stack_std.h:80)

**Location:** `__rte_stack_std_pop()`

```c
for (index = 0; index < n; index++)
    *obj_table++ = *--stack_objs;
```

**Issue:** The function signature declares `obj_table` as `__rte_restrict`, but the implementation copies from `stack_objs` (derived from `stack->objs`) to `obj_table`. If the caller passes overlapping pointers (e.g., `obj_table` points into `stack->objs`), this violates the `restrict` contract and causes undefined behavior.

**Analysis:** The `restrict` qualifier tells the compiler that `obj_table` does not alias any other pointer used in the function. The compiler may reorder or optimize accesses based on this assumption. If `obj_table` actually overlaps with `stack->objs`, the optimization may produce incorrect results.

**Fix:** Either remove `__rte_restrict` or document that the caller must not pass overlapping pointers. For a public API, removing `restrict` is safer.

---

### 4. Unbounded Retry Loop in `__rte_stack_pile_pop()` (lib/stack/rte_stack_pile.h:214-220)

**Location:** Bulk element pop retry loop

```c
bulk_first = __rte_stack_pile_bulk_pop_elems(&pile->bulk, n_bulk, obj_table, &bulk_last);
if (unlikely(bulk_first == NULL)) {
    n_solo += RTE_STACK_PILE_BULK_SIZE;
    n_bulk--;
    if (n_bulk > 0)
        goto bulk;
    else
        goto solo;
}
```

**Issue:** Under heavy contention or starvation, this retry loop could execute `n_bulk` times (potentially hundreds of iterations if `n_bulk` is large). Each retry is a lock-free CAS operation that may fail repeatedly, causing the function to spin without yielding. This is effectively a busy-wait that could monopolize the CPU and harm overall system performance.

**Why it matters:** In a multi-threaded environment with many consumers, one thread could be starved while others continuously succeed in popping bulk elements. The starved thread would spin in this loop, wasting CPU cycles and potentially degrading cache performance for other threads.

**Fix:** Add a bounded retry count and fail the operation if retries are exhausted:
```c
unsigned int retries = 0;
const unsigned int max_retries = 16; /* or another reasonable bound */

bulk:
if (retries++ >= max_retries) {
    /* Exceeded retry limit, fail the operation */
    if (n_bulk > 0)
        __rte_stack_pile_bulk_push_elems(&pile->bulk,
                bulk_first, bulk_last, n_bulk);
    return 0;
}
bulk_first = __rte_stack_pile_bulk_pop_elems(&pile->bulk, n_bulk, obj_table, &bulk_last);
/* ... rest of retry logic ... */
```

Alternatively, insert `rte_pause()` in the retry path to reduce CPU contention.

---

### 5. Integer Overflow in Memsize Calculation (lib/stack/rte_stack_pile.c:29)

**Location:** `rte_stack_pile_get_memsize()`

```c
sz += bulk * sizeof(struct rte_stack_pile_bulk_elem);
```

**Issue:** `bulk` is `unsigned int`, and `sizeof(struct rte_stack_pile_bulk_elem)` is large (at least 256 bytes for `RTE_STACK_PILE_BULK_SIZE=32` on 64-bit). If `count` is near `UINT_MAX`, the multiplication `bulk * sizeof(...)` could overflow before assignment to `ssize_t sz`.

**Fix:** Cast to `ssize_t` before the multiply:
```c
sz += (ssize_t)bulk * sizeof(struct rte_stack_pile_bulk_elem);
```

Also add an overflow check:
```c
if (bulk > SIZE_MAX / sizeof(struct rte_stack_pile_bulk_elem))
    return -1; /* or rte_errno = EINVAL; return -1; */
```

---

### 6. Missing Release Notes for API Changes

**Issue:** The patch makes significant API changes (new mempool driver, new stack type, changes to `rte_mempool_cache` structure layout, removal of `flushthresh` field, increased `RTE_MEMPOOL_CACHE_MAX_SIZE`), but does not update the release notes.

**Required:** Update `doc/guides/rel_notes/<current_release>.rst` with:
- New features: pile stack, pile mempool driver
- API changes: `rte_mempool_cache` structure changed (removed `flushthresh`, reordered fields)
- Configuration changes: `RTE_MEMPOOL_CACHE_MAX_SIZE` increased to 1024, new `RTE_STACK_PILE_BULK_SIZE` option
- Deprecation notice for `cache->flushthresh` (if maintaining ABI compatibility)

---

### 7. `RTE_EXPORT_*` Macro Missing for New Public Functions

**Location:** `lib/stack/rte_stack_pile.c`

**Issue:** `rte_stack_pile_init()` and `rte_stack_pile_get_memsize()` are defined in a `.c` file but lack `RTE_EXPORT_SYMBOL()` or `RTE_EXPORT_INTERNAL_SYMBOL()` macros. These functions are called from `rte_stack.c`, so they are either internal (shared within the stack library) or public API.

**Analysis:** Based on usage, these appear to be internal functions (similar to `rte_stack_lf_init()` and `rte_stack_std_init()` which are also not exported). If they are internal, they should have `RTE_EXPORT_INTERNAL_SYMBOL()`. If they become part of the public API, they need `RTE_EXPORT_SYMBOL()` or `RTE_EXPORT_EXPERIMENTAL_SYMBOL()`.

**Fix:** Add the appropriate export macro before each function definition:
```c
RTE_EXPORT_INTERNAL_SYMBOL(rte_stack_pile_init)
void
rte_stack_pile_init(struct rte_stack *s, unsigned int count)
{
    /* ... */
}
```

---

## Warnings

### 1. Cache Size Alignment Requirement Not Enforced at Configure (lib/mempool/rte_mempool.c:844-850)

**Location:** `rte_mempool_create_empty()`

```c
if (cache_size & 31) {
    unsigned int rounded = RTE_ALIGN_MUL_CEIL(cache_size, 32);
    RTE_MEMPOOL_LOG(WARNING, "%s cache size %u not divisible by 32, using %u instead.",
            name, cache_size, rounded);
    cache_size = rounded;
}
```

**Issue:** The function silently modifies the requested cache size and only logs a warning. This violates the principle of least surprise -- the caller requests a specific cache size and gets a different one without an explicit indication in the return value or error code.

**Recommendation:** Either:
- Fail with `rte_errno = EINVAL` and return `NULL` if cache size is not aligned, OR
- Document this rounding behavior prominently in the Doxygen comment for the function

---

### 2. Pile Bulk Size Not Validated Against Maximum (lib/stack/rte_stack_pile.c:9)

**Issue:** `RTE_STACK_PILE_BULK_SIZE` is configured in `config/rte_config.h` but is never validated at compile time or runtime. If a user sets it to an unreasonable value (e.g., 1, 1024, or a non-power-of-2), the pile implementation will produce incorrect behavior or poor performance.

**Recommendation:** Add a compile-time check in `rte_stack_pile.h`:
```c
#if RTE_STACK_PILE_BULK_SIZE < 4 || RTE_STACK_PILE_BULK_SIZE > 256
#error "RTE_STACK_PILE_BULK_SIZE must be between 4 and 256"
#endif
#if (RTE_STACK_PILE_BULK_SIZE & (RTE_STACK_PILE_BULK_SIZE - 1)) != 0
#error "RTE_STACK_PILE_BULK_SIZE must be a power of 2"
#endif
```

---

### 3. Experimental API Not Marked (lib/stack/rte_stack.h:154)

**Location:** `RTE_STACK_F_PILE` flag definition

```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.
 *
 * @experimental
 */
#define RTE_STACK_F_PILE 0x0002
```

**Issue:** The comment says `@experimental`, but the flag is not guarded by `__rte_experimental` or marked for experimental API access. Applications using this flag should be required to opt-in to experimental API.

**Recommendation:** Either:
- Remove the `@experimental` comment if this is intended to be stable API, OR
- Document that the pile feature is experimental in the release notes and require `ALLOW_EXPERIMENTAL_API` to use it (though flags themselves are not functions, so this is harder to enforce)

---

### 4. Test Coverage for Pile Edge Cases Is Incomplete (app/test/test_stack.c:169-192)

**Location:** Pile-specific excess object push test

```c
if (s->flags & RTE_STACK_F_PILE) {
    ret = rte_stack_push(s, obj_table, STACK_SIZE * RTE_STACK_PILE_BULK_SIZE + 1);
    if (ret != 0) {
        printf("[%s():%u] Excess objects push succeeded\n",
               __func__, __LINE__);
        goto fail_test;
    }
}
```

**Issue:** This test checks that pushing more than the pile's capacity fails, but it does not test:
- Popping when the pile is partially fragmented (mixture of bulk and solo elements)
- Concurrent push/pop from multiple threads (only single-threaded test)
- Exact boundary cases (e.g., pushing `STACK_SIZE - 1` objects, then pushing 1 more, then popping)

**Recommendation:** Add multi-threaded tests similar to `test_stack_mt()` in the existing stack tests, specifically for pile behavior under contention and fragmentation.

---

### 5. Memcpy Assumption Not Enforced (lib/mempool/rte_mempool.h:1442-1447)

**Location:** `rte_mempool_do_generic_put()`, cache compaction

```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);
```

**Issue:** The code assumes `cache->objs[cache->size / 2]` is cache-line aligned, but this is only true if `cache->size` is divisible by 32 (on 64-bit with 64-byte cache line). The `__rte_assume_cache_aligned()` macro tells the compiler this is true, but if it's not, the memcpy optimization may access unaligned memory and degrade performance or cause undefined behavior on some architectures.

**Recommendation:** The earlier check at line 844-850 enforces that `cache_size` is divisible by 32, which should make this safe. However, add a compile-time assertion to make the dependency explicit:
```c
RTE_BUILD_BUG_ON((RTE_MEMPOOL_CACHE_MAX_SIZE / 2) % (RTE_CACHE_LINE_SIZE / sizeof(void *)) != 0);
```

---

### 6. Mempool Audit Checks Cache Size on Every Call (lib/mempool/rte_mempool.c:1224-1235)

**Location:** `mempool_audit_cache()`

```c
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);
    }
    /* ... */
}
```

**Issue:** This loop iterates over all `RTE_MAX_LCORE` entries on every audit call, even though `cache->size` is initialized once and never changes. This is redundant overhead in debug builds.

**Recommendation:** Move the `cache->size` consistency check to `rte_mempool_create_empty()` where the caches are initialized, rather than checking on every audit call.

---

### 7. Documentation Does Not Match Code for Pile LIFO Behavior (doc/guides/prog_guide/stack_lib.rst:111-113)

**Location:** Pile documentation

```
It is only LIFO on bulk level, not on object level; i.e. arrays of bulks are
pushed and popped in LIFO manner, but objects within each bulk are not ordered
as expected by a stack.
```

**Issue:** The code at `lib/stack/rte_stack_pile.h:166` copies bulks in **reverse order**:
```c
for (i = 0; i < n_bulk; i++, tmp_bulk = tmp_bulk->next)
    rte