|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

Reviewing patch series 'app/test: reciprocal_division and other test improvements' (5 patches).

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

This is a well-structured refactoring that converts the reciprocal division test from a performance test to a fast functional test using the modern test infrastructure. The changes are appropriate:

- Reduces `MAX_ITERATIONS` from 2^32 to 2^20 (still thorough for a fast test)
- Adopts `TEST_ASSERT` macros and `unit_test_suite_runner`
- Uses `REGISTER_FAST_TEST` with correct `NOHUGE_OK` and `ASAN_OK` flags
- Tests boundary values systematically (edge divisors, values around multiples)
- The structured approach testing `val-1`, `val`, `val+1` around `q*d` is a good strategy for catching off-by-one errors

The division-by-zero avoidance (`rte_rand_max(UINT32_MAX - 1) + 1`) is correct. The `qmax` calculation and increment logic is mathematically sound.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

Straightforward reduction of iteration counts for the performance test to prevent timeout. `MAX_ITERATIONS` reduced from 2^32 to 2^24, `DIVIDE_ITER` from 2^28 to 2^10. This is reasonable for a perf test that measures relative performance rather than exhaustive correctness.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The change moves `rte_rcu_qsbr_quiescent()` inside the loop (called every iteration) instead of outside (called once per outer loop). This prevents the quiescent action backlog from growing unbounded and causing excessive test runtime. The change is functionally correct: calling quiescent more frequently is safe and actually better models real-world usage where quiescent points occur regularly during processing.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

Correct use of `TEST_SKIPPED` return value when a prerequisite (available device) is not met. This is the appropriate handling per the test infrastructure guidelines.

---

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

### Errors

1. **Inconsistent `MEMPOOL_SIZE` removal creates correctness risk**

   The patch removes the `MEMPOOL_SIZE` macro and introduces a local variable `mempool_size` computed in `do_all_mempool_perf_tests()`. However, `test_loop()` and `test_loop_random()` both had logic referencing the total pool size `N` defined as `128 * MAX_KEEP`. After this patch, when `cores` is reduced (e.g., to 1 or 2), `mempool_size` shrinks, but `N` remains `128 * MAX_KEEP` = 4,194,304 objects. The functions now attempt to get/put objects from a pool that may be smaller than `N`, potentially causing underflow or incorrect rate calculations.

   **Example scenario:**
   - `cores = 1`, so `mempool_size = 1 * (32768 + 2*1024) - 1 = 34815` objects
   - `N = 128 * 32768 = 4194304` objects
   - `test_loop_random()` tries to operate on `N` objects, but the pool only has 34815

   **Suggested fix:** Define `N` based on the actual `mempool_size` parameter passed to the test, not a compile-time constant. Either pass `mp->size` into the test loop functions or redefine `N` dynamically. For example:

   ```c
   /* in per_lcore_mempool_test or earlier */
   const unsigned int n_total = mp->size / rte_lcore_count();
   /* pass n_total to test_loop() and test_loop_random() instead of N */
   ```

   Alternatively, ensure `N <= mempool_size / cores` always holds by adjusting the `N` definition.

---

2. **`MAX_OPS` limit in `test_loop()` changes semantics and may break rate calculation**

   The patch adds a cap on mempool operations per `test_loop()` call:

   ```c
   iter = MAX_OPS / (x_keep / x_get_bulk + x_keep / x_put_bulk);
   if (iter == 0)
       iter = 1;
   ```

   It then returns `iter * x_keep` as the number of objects processed. However:

   - The `enq_count` accumulation in `per_lcore_mempool_test()` now adds the return value:
     ```c
     stats[lcore_id].enq_count += ret;
     ```
   - But the rate calculation still assumes `enq_count` represents the number of complete put operations. With the new logic, `enq_count` is incremented by `iter * x_keep` each call, but the number of actual `rte_mempool_generic_put()` calls is `iter * (x_keep / x_put_bulk)`.
   - The comment says "return the number of objects handled so the reported rate stays correct", but the rate is computed as `enq_count * hz / duration_cycles`, which now measures objects, not operations. This is a semantic change.

   **Potential issue:** The reported "rate_persec" now means "objects per second" instead of "put operations per second" (or "enqueue operations per second"). This may be intentional, but it's a subtle change that could confuse readers of the test output or break comparisons with historical data.

   **Suggested fix:** Document this semantic change in the commit message or a code comment. Alternatively, if the original intent was to measure put operations per second, adjust the return value to `iter * (x_keep / x_put_bulk)` and update the comment accordingly.

---

### Warnings

1. **Potential integer division by zero in `test_loop()` if `x_keep < x_get_bulk`**

   ```c
   iter = MAX_OPS / (x_keep / x_get_bulk + x_keep / x_put_bulk);
   ```

   If `x_keep < x_get_bulk`, then `x_keep / x_get_bulk` (integer division) is 0. Similarly for `x_put_bulk`. If both are 0, the denominator is 0, causing division by zero.

   The launch code has a guard:
   ```c
   if (*keep_ptr < *get_bulk_ptr || *keep_ptr < *put_bulk_ptr)
       continue;
   ```

   But this only skips the test when `n_keep < n_get_bulk` **or** `n_keep < n_put_bulk`. It does not guarantee `n_keep >= n_get_bulk` **and** `n_keep >= n_put_bulk` simultaneously in all cases (though in practice the loop structure likely ensures it). If `x_keep < x_get_bulk` ever occurs due to a future code change, `iter` calculation will divide by zero.

   **Suggested fix:** Add an assertion or a defensive check in `test_loop()`:

   ```c
   RTE_ASSERT(x_keep >= x_get_bulk && x_keep >= x_put_bulk);
   /* or: */
   if (x_keep < x_get_bulk || x_keep < x_put_bulk)
       return 0;
   ```

---

2. **Launch overhead measurement may be inaccurate when `cores > MANYCORE_THRESHOLD`**

   The patch notes that "launch overhead dominates with many cores" and switches to a reduced bulk size table for `cores > 2`. However, the 2 ms vs 1 second launch cost measurement mentioned in the commit message is a one-time overhead per test point, not per mempool operation. The reported `rate_persec` already accounts for this by dividing total objects by total duration (including launch overhead). Reducing the test matrix for many cores is a pragmatic choice for total test time, but the comment "launch overhead dominates" may be misleading--it's the *cumulative* launch overhead over many test points that dominates total time, not the per-operation cost in the rate measurement.

   **Suggested clarification:** Reword the comment or commit message to say "launch overhead per test point is significant (>1s with 32 cores), so we reduce the number of test points when `cores > MANYCORE_THRESHOLD` to keep total test time bounded."

---

3. **Test may still timeout on very large core counts**

   With 32 cores and the reduced tables, the test completes in ~200s (down from timeout). However, the patch does not add any hard bound on the maximum number of cores tested. On a system with 128 cores, even with reduced tables, the test could still take excessive time or exceed memory. Consider adding a skip condition like:

   ```c
   if (cores > 32) {
       printf("Skipping mempool perf test for >32 cores\n");
       return TEST_SKIPPED;
   }
   ```

   Or document in the commit message that systems with >32 cores are expected to either pass `--lcores` to limit the test or accept longer test times.

---

### Info

The patch makes several improvements to handle many-core systems:

- Sizes mempools by the number of cores actually tested rather than `rte_lcore_count()`
- Returns `TEST_SKIPPED` instead of failure when memory allocation fails
- Reduces `TIME_S` to `TIME_MS = 200`
- Reduces the number of test points (fewer `n_keep`, `n_get_bulk`, `n_put_bulk` values)
- Uses a reduced bulk size table when `cores > MANYCORE_THRESHOLD`
- Prints elapsed time per test point for visibility during timeout
- Runs the largest core count first so systems that can't fit the mempool skip immediately

These are all reasonable changes for a performance test in CI that needs to complete in a bounded time.

The `MAX_OPS` limit per `test_loop()` call is a clever solution to the problem of small bulk sizes causing one call to take >50 seconds with high core contention. However, the correctness issues noted above (mismatch between `N` and `mempool_size`, potential division by zero) need to be addressed.

---

## Summary

**Patches 1-4:** No issues. Well done.

**Patch 5:** Two correctness errors and three warnings that should be addressed before merging. The core idea (reducing test time for many cores) is sound, but the implementation has subtle bugs related to pool size assumptions and loop iteration bounds.