|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

1. **Good improvement**: Converting to `unit_test_suite_runner` and `TEST_ASSERT` macros aligns with current best practices for functional tests.

2. **Iteration reduction is reasonable**: Reducing `MAX_ITERATIONS` from 2^32 to 2^20 is appropriate for a fast test, especially since the new approach tests boundary values systematically rather than purely random inputs.

3. **Boundary value testing**: The approach of testing `val-1`, `val`, and `val+1` around multiples of the divisor is a good correctness strategy and more valuable than purely random testing.

---

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

### Errors

None.

### Warnings

None.

### Info

1. **Appropriate for perf test**: Reducing `MAX_ITERATIONS` from 2^32 to 2^24 and `DIVIDE_ITER` from 2^28 to 2^10 is reasonable for a performance test that measures relative performance rather than exhaustive correctness.

---

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

### Errors

None.

### Warnings

None.

### Info

1. **Correct change**: Moving `rte_rcu_qsbr_quiescent()` inside the loop per hash lookup iteration (after each lock/unlock cycle) instead of once per outer loop prevents unbounded backlog accumulation. This is semantically correct and improves test behavior.

---

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

### Errors

None.

### Warnings

None.

### Info

1. **Good change**: Returning `TEST_SKIPPED` instead of `-1` (failure) when insufficient ports are available is the correct test framework usage. This allows CI systems to distinguish "test cannot run" from "test ran and failed."

---

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

### Errors

None.

### Warnings

1. **`rte_mempool_avail_count(mp) != mp->size` comparison**: At line 351 in the new code, the check `if (rte_mempool_avail_count(mp) != mp->size)` directly compares against `mp->size`. This is correct for the local `mp` pointer which is the mempool being tested, so this is not an issue. (Mentioning only to confirm it was reviewed.)

2. **Error handling consistency**: The patch adds `ret = TEST_SKIPPED;` before `goto err;` in multiple allocation failure paths. This is good practice. However, the existing `goto err;` paths for `rte_mempool_set_ops_byname()` failures do not set `ret` and will still return `-1`. For consistency, those should also set `ret = TEST_SKIPPED;` or appropriate error code. (Minor inconsistency, not a correctness bug.)

### Info

1. **Good approach**: Dynamically sizing the mempool by `cores * (MAX_KEEP + RTE_MEMPOOL_CACHE_MAX_SIZE * 2) - 1` instead of using `rte_lcore_count()` directly is correct, since the test only exercises the specified number of cores.

2. **Iteration bounding improvement**: Limiting mempool operations per `test_loop()` call via `MAX_OPS` instead of a fixed object count prevents excessive per-call latency with small bulk sizes on many cores. The returned object count keeps the rate calculation correct.

3. **Reduced bulk size tables above `MANYCORE_THRESHOLD`**: Using `bulk_tab_get_manycore` and `bulk_tab_put_manycore` with fewer sizes when `cores > 2` is a reasonable heuristic to reduce test time when launch overhead dominates.

4. **Test order change**: Running the largest (all-core) configuration first so a memory shortfall is detected immediately is a good user experience improvement.

5. **Time constant**: `TIME_MS = 200` (reduced from `TIME_S = 1`) is appropriate given the iteration bounding changes and should still provide stable performance measurements.

---

## Summary

All five patches are correct and represent good improvements to test infrastructure:

- **Patch 1**: Modernizes reciprocal division test to use unit test framework and boundary-value testing.
- **Patch 2**: Reduces performance test time to practical levels.
- **Patch 3**: Fixes RCU quiescent state backlog issue.
- **Patch 4**: Correct use of `TEST_SKIPPED` when hardware unavailable.
- **Patch 5**: Makes mempool performance test practical on many-core systems by reducing redundancy, bounding per-call work, and reporting skipped status on memory shortfall.

No correctness bugs identified. One minor inconsistency in error handling in patch 5 (noted above) could be improved for uniformity but does not affect correctness.