|WARNING| stack: introduce pile

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Correctness Issues

### Error: Resource leak on failure path in `__rte_stack_pile_pop`

In `lib/stack/rte_stack_pile.h`, the `fail:` label at line 309 does not free allocated `solo_first` elements when both fragmentation succeeded and solo element allocation succeeded, but the function is rolling back due to an earlier failure.

```c
fail:
	/* Failed. Roll back. */
	if (frag != NULL) {
		// ... handles frag ...
	}
	if (n_bulk > 0)
		__rte_stack_pile_bulk_push_elems(&pile->bulk, bulk_first, bulk_last, n_bulk);
	// MISSING: if (solo_first != NULL) free solo elements
	return 0;
```

**Why it matters**: If `solo_first` was successfully allocated but a subsequent operation failed (e.g., after line 231 in an earlier failure path that jumps to `fail`), those solo elements are leaked and never returned to `free_solo`.

**Fix**: Add solo element cleanup in the `fail:` path:
```c
fail:
	/* Failed. Roll back. */
	if (frag != NULL) { ... }
	if (n_bulk > 0)
		__rte_stack_pile_bulk_push_elems(&pile->bulk, bulk_first, bulk_last, n_bulk);
	if (solo_first != NULL)
		__rte_stack_lf_push_elems(&pile->free_solo, solo_first, solo_last, n_solo);
	return 0;
```

---

### Error: `memcmp` return value semantics inverted

In `app/test/test_stack.c` line 103-111:
```c
if (memcmp(&obj_table[i],
		&popped_objs[STACK_SIZE - RTE_STACK_PILE_BULK_SIZE - i],
		sizeof(void *) * RTE_STACK_PILE_BULK_SIZE) != 0) {
```

The error message prints `popped_objs[STACK_SIZE - RTE_STACK_PILE_BULK_SIZE - i]` as if it were a single pointer, but `memcmp` compared an entire bulk. The printf should indicate the range or just the start index, not pretend it's comparing a single value.

**Correction**: This is actually acceptable for a test - the message is slightly misleading but the test logic is correct. Not an error.

---

### Warning: Missing error propagation in `test_stack_basic`

In `app/test/test_stack.c` line 176-196:

```c
ret = rte_stack_push(s, obj_table, STACK_SIZE);
if (ret == 0) {
	printf("[%s():%u] All objects push failed\n", ...);
	goto fail_test;
}
```

The test expects `rte_stack_push` to return non-zero on success (number of objects pushed). However, the variable `ret` is reused and the test doesn't explicitly set `ret = -1` before the goto. Later, `ret = 0;` at line 205 would be reached if the test passes, but on the `fail_test` path, `ret` keeps its last value, which might be non-zero (the push count), misleading the test runner.

**Why it matters**: The test might return a non-zero positive value on failure, which looks like success.

**Fix**: Explicitly set `ret = -1` at line 208:
```c
fail_test:
	ret = -1;
```
(Actually, line 208-209 already has this. Reviewing again: lines 205-211 correctly handle this. Not an issue.)

---

### Error: Potential uninitialized variable use in `__rte_stack_pile_pop`

In `lib/stack/rte_stack_pile.h` line 197:
```c
unsigned int n_bulk = n / RTE_STACK_PILE_BULK_SIZE;
unsigned int n_solo = n & (RTE_STACK_PILE_BULK_SIZE - 1);
```

If `n` is 0, both `n_bulk` and `n_solo` are 0, the function returns early at line 201. However, if the early return at line 201 is taken, variables `bulk_last`, `solo_last` are never initialized but are used in the rollback path (line 314-316) if a failure occurs. 

**Correction**: Actually reviewing the code flow, if `n==0` the function returns at line 201, so no rollback path is taken. The rollback paths (line 309-317) are only reachable if allocations were attempted. The variables are initialized to NULL at declaration (line 196-197) and only used if they were set. Not an error.

---

### Error: Logic error in test expectations for pile push/pop

In `app/test/test_stack.c` line 179-187:

```c
ret = rte_stack_push(s, obj_table, STACK_SIZE);
if (ret == 0) {
	printf("[%s():%u] All objects push failed\n", ...);
	goto fail_test;
}
ret = rte_stack_push(s, obj_table, STACK_SIZE);
if (ret != 0) {
	printf("[%s():%u] Excess objects push succeeded\n", ...);
	goto fail_test;
}
```

The test pushes STACK_SIZE objects twice. The first push should succeed (fill the stack), the second should fail (stack full). But the test says "All objects push failed" when the first push returns 0, which is correct. However, it then tries to push the *same* STACK_SIZE objects again. If the first push succeeded, those objects are now on the stack, and pushing them again pushes *duplicate* pointers. When the test later pops and compares (line 81-111), it's comparing pointers that were pushed twice vs. the original `obj_table` order, which will mismatch.

**Why it matters**: The test may produce false failures because it pushes duplicate pointers.

**Fix**: After the first successful push, don't reuse `obj_table` for the second push. Either use a different array or accept that the test is checking "overfill protection" and not pointer values.

Actually, re-reading: line 191-195 pops STACK_SIZE objects after the second push attempt (which should fail). So the stack should still contain only STACK_SIZE objects from the first push. The test then pops them at line 197 and compares. This is correct. The second push *should* fail (return 0) because the stack is full. Not an error.

---

### Error: Missing NULL pointer checks after `rte_malloc` allocations

In `app/test/test_stack.c` line 149:
```c
obj_table = rte_calloc(NULL, STACK_SIZE, sizeof(void *), 0);
```
and line 156:
```c
s = rte_stack_create(STACK_NAME, STACK_SIZE, SOCKET_ID_ANY, flags);
```

Neither allocation is checked for NULL before use. If `rte_calloc` or `rte_stack_create` fails, dereferencing them will crash.

**Why it matters**: The test will segfault instead of reporting a clean failure.

**Fix**:
```c
obj_table = rte_calloc(NULL, STACK_SIZE, sizeof(void *), 0);
if (obj_table == NULL) {
	printf("Failed to allocate obj_table\n");
	return -1;
}

s = rte_stack_create(STACK_NAME, STACK_SIZE, SOCKET_ID_ANY, flags);
if (s == NULL) {
	printf("Failed to create stack\n");
	rte_free(obj_table);
	return -1;
}
```

---

### Warning: `RTE_CACHE_GUARD` used without corresponding definitions

In `lib/stack/rte_stack.h` lines 91, 93, etc., `RTE_CACHE_GUARD;` is used as a structure member. However, `RTE_CACHE_GUARD` is not a standard DPDK macro in older releases. If this patch is targeting a release where `RTE_CACHE_GUARD` is not defined, compilation will fail.

**Check**: Verify `RTE_CACHE_GUARD` is available in the target release. If not, use explicit padding.

---

## Style and API Issues

### Warning: Multiple definitions should use definition list in RST

In `doc/guides/prog_guide/stack_lib.rst` lines 12-17:
```rst
*  Create a uniquely named stack (or pile) ...
*  Push and pop a burst ...
*  Destroy a stack.
```

This is a bullet list of features. However, the existing style in this file already uses bullets, so this is acceptable for feature lists. No change needed.

---

### Info: Inconsistent spacing in comments

In `lib/stack/rte_stack_pile.h` line 82:
```c
/* Traverse the list to copy the bulks. */
```
and line 85:
```c
for (unsigned int i = 0; i < num; i++, tmp = tmp->next)
```

This is fine.

---

### Warning: Boolean comparison on unsigned int

In `lib/stack/rte_stack_pile.h` line 119:
```c
if (unlikely(n_bulk == 0)) {
```

This is an explicit comparison against 0, which is correct per DPDK style.

---

### Warning: Missing release notes update

The patch adds a significant new feature (`RTE_STACK_F_PILE`) and new API (`__rte_stack_pile_*` functions). However, I don't see a corresponding update to `doc/guides/rel_notes/release_XX_YY.rst` documenting the new feature.

**Why it matters**: Users and reviewers need to know about new features in the release notes.

**Fix**: Add a section in the appropriate release notes file:
```rst
* **Added pile stack variant.**

  A new lock-free ``RTE_STACK_F_PILE`` stack type was added,
  optimized for bulk push/pop operations. It provides significantly
  better performance than the standard lock-free stack for bulk sizes
  >= 32 objects.
```

---

### Info: `RTE_MEMPOOL_MAX_OPS_IDX` increase rationale

In `lib/mempool/rte_mempool.h` line 721:
```c
#define RTE_MEMPOOL_MAX_OPS_IDX 32  /**< Max registered ops structs */
```
changed from 16 to 32.

This change is unrelated to the pile feature itself but is presumably needed to register additional mempool handlers. The comment should explain why this increase is necessary (or it should be a separate patch).

**Suggestion**: If this change is required for pile mempool handler registration, add a comment or mention it in the commit message. Otherwise, split it into a separate preparatory patch.

---

## Summary

**Errors (must fix):**
1. Resource leak in `__rte_stack_pile_pop` fail path - `solo_first` not freed
2. Missing NULL checks after `rte_calloc` and `rte_stack_create` in test

**Warnings (should fix):**
3. Missing release notes for new `RTE_STACK_F_PILE` feature
4. `RTE_MEMPOOL_MAX_OPS_IDX` change needs justification (comment or separate patch)
5. Verify `RTE_CACHE_GUARD` availability in target release

**Info (consider):**
6. `RTE_STACK_PILE_BULK_SIZE` configuration in `rte_config.h` should be documented in a user-facing guide (not just prog_guide)

The patch introduces a valuable performance optimization but needs the correctness and documentation issues addressed before merging.