|WARNING| [v2, 5/5] app/test/mempool_perf: adjust test to work with many cores

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/5: app/test/reciprocal_division: make it a fast test

### Errors

None.

### Warnings

None.

### Info

- The patch refactors a performance test into a proper unit test using the TEST_ASSERT framework, which is appropriate for a correctness test becoming a fast test.
- `rte_rand_max(qmax)` when `qmax` could be 0 (when `d == UINT32_MAX` or `UINT64_MAX` since `qmax = UINT_MAX / d` gives 1, then `++qmax` makes it 2, but the initial condition has `d = UINT32_MAX` leading to `qmax = 0` before increment) - however, inspection shows `d` is constrained to be at least 1 by `rte_rand_max(UINT32_MAX - 1) + 1`, so `qmax` is never zero. The edge case `d = 1` gives `qmax = UINT32_MAX` which is handled by the `if (d != 1)` check.
- Test structure follows DPDK functional test patterns correctly with proper use of `unit_test_suite_runner` and `REGISTER_FAST_TEST`.

---

## Patch 2/5: app/test/reciprocal_division_perf: reduce test time

### Errors

None.

### Warnings

None.

### Info

- Simple constant change to reduce test iteration count, appropriate for a performance test optimization.

---

## Patch 3/5: app/test/test_rcu_qsbr_perf: call quiescent more often

### Errors

None.

### Warnings

None.

### Info

- Moving `rte_rcu_qsbr_quiescent()` inside the loop is correct - it reduces backlog and improves test performance without changing test validity.

---

## Patch 4/5: app/test/test_pmd_perf: skip if no device available

### Errors

None.

### Warnings

None.

### Info

- Correctly returns `TEST_SKIPPED` when the test cannot run due to insufficient ports, which is the appropriate behavior.

---

## Patch 5/5: app/test/mempool_perf: adjust test to work with many cores

### Errors

1. **Resource leak on error path** (line 506, 518, 528, 549, 552, 574, 577):
   When mempool creation/population fails and the function goes to `err:`, previously created mempools are not freed. Multiple `goto err` statements exit without cleaning up `mp_nocache`, `mp_cache`, `default_pool_nocache`, and `default_pool_cache` that may have been successfully created.
   
   **Fix**: Add cleanup before each error return:
   ```c
   if (mp_nocache == NULL) {
       printf("cannot allocate mempool (without cache)\n");
       ret = TEST_SKIPPED;
       goto err;  /* OK - nothing to clean up yet */
   }
   
   if (mp_cache == NULL) {
       printf("cannot allocate mempool (with cache)\n");
       ret = TEST_SKIPPED;
       rte_mempool_free(mp_nocache);  /* ADD THIS */
       goto err;
   }
   
   if (default_pool_nocache == NULL) {
       printf("cannot allocate %s mempool (without cache)\n", default_pool_ops);
       ret = TEST_SKIPPED;
       rte_mempool_free(mp_cache);     /* ADD THIS */
       rte_mempool_free(mp_nocache);   /* ADD THIS */
       goto err;
   }
   
   /* Similar pattern for all other error paths before err: label */
   ```
   
   Alternatively, restructure the `err:` label to free all non-NULL mempools:
   ```c
   err:
       if (default_pool_cache)
           rte_mempool_free(default_pool_cache);
       if (default_pool_nocache)
           rte_mempool_free(default_pool_nocache);
       if (mp_cache)
           rte_mempool_free(mp_cache);
       if (mp_nocache)
           rte_mempool_free(mp_nocache);
       return ret;
   ```

2. **Variable initialization for error path safety** (lines 502-503):
   When using the restructured error handler pattern above, initialize mempool pointers to NULL at declaration:
   ```c
   struct rte_mempool *mp_cache = NULL, *mp_nocache = NULL;
   struct rte_mempool *default_pool_cache = NULL, *default_pool_nocache = NULL;
   ```

### Warnings

1. **Mempool sizing calculation change** (line 502):
   The patch changes from `MEMPOOL_SIZE` (using `rte_lcore_count()`) to `mempool_size` (using the `cores` parameter). This is correct for the intent, but worth noting that the function signature changed from implicitly testing all cores to accepting a `cores` parameter. The calculation `cores * (MAX_KEEP + RTE_MEMPOOL_CACHE_MAX_SIZE * 2) - 1` mirrors the old macro definition, so the formula is consistent.

2. **Return value handling** (lines 658-660):
   The function propagates `TEST_SKIPPED` from `do_all_mempool_perf_tests()`, which is correct behavior. However, any negative return from `do_all_mempool_perf_tests()` (indicating a test failure rather than skip) is also propagated directly, which is appropriate.

### Info

- The reduction in bulk size tables for `cores > MANYCORE_THRESHOLD` (lines 406-408) is a reasonable performance optimization.
- The change to limit mempool operations per `test_loop()` call (lines 152-164) and return object count is correct and addresses the timeout issue described.
- The reordering of tests to run largest-core-count first (lines 653-656) is a good optimization for quickly skipping when resources are insufficient.
- The printing and flushing of progress (line 348) is helpful for debugging timeout issues.
- The patch correctly updates all mempool allocations to use `mempool_size` instead of the old `MEMPOOL_SIZE` macro.

---

## Summary

**Patches 1-4:** No issues found. These are straightforward test improvements that follow DPDK coding standards.

**Patch 5:** Contains resource leak errors on multiple error paths. The mempools allocated before an error occurs are not freed when jumping to the `err:` label. This must be fixed before merging.