|WARNING| [v4,16/16] test/pmd_ring: test per-queue xstats

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Series: virtio queue stats mapping removal and related cleanup

Reviewed 16 patches total.

---

## Patch 1/16: net/virtio: remove unused queue stats mapping

**No issues found.**

The removal is correct. The callback `virtio_dev_queue_stats_mapping_set` did nothing
and was unused since the queue mapping functions were deprecated in 25.11.

---

## Patch 2/16: app/testpmd: remove leftover set qmap

**No issues found.**

Cleans up the leftover `set qmap` help text and prototype after the function was removed.

---

## Patch 3/16: net/enic: remove unneeded ops initialization

**No issues found.**

Initializing function pointers to NULL is unnecessary; the compiler does it automatically.

---

## Patch 4/16: net/cnxk: fix Tx drops added to Rx queue errors

**No issues found.**

Correctness bug fixed: `q_errors` is an Rx stat (rx_qN_errors), so the Tx drop assignment
was wrong. The fix correctly stores only the Rx drop count.

---

## Patch 5/16: net/cnxk: remove queue stats mapping

**Errors:**

1. **Line 39 (cnxk_stats.c): loop index out of range check removed (Error)**

   The original code checked `if (i >= RTE_ETHDEV_QUEUE_STAT_CNTRS) break;`
   to prevent writing past the end of the qstats arrays.
   The new code removes this check. Without it, if `dev->data->nb_rx_queues`
   or `dev->data->nb_tx_queues` is larger than `RTE_ETHDEV_QUEUE_STAT_CNTRS`,
   the loops will write out of bounds.

   **Fix**: Restore the bounds check:
   ```c
   for (i = 0; i < dev->data->nb_tx_queues &&
           i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
   ```

---

## Patch 6/16: net/e1000: remove queue stats mapping

**No issues found.**

---

## Patch 7/16: net/ixgbe: remove queue stats mapping

**No issues found.**

The patch removes the `queue_stats_mapping_set` op and the mapping storage,
but retains the initialization-time default mapping (`ixgbe_reset_qstat_mappings`),
which is called once in `ixgbe_dev_start`.

---

## Patch 8/16: net/txgbe: remove queue stats mapping

**No issues found.**

---

## Patch 9/16: net/sxe2: fix null dereference in stats get

**No issues found.**

Correctly adds a NULL check for `qstats` in both the primary and multiprocess paths
before dereferencing it.

---

## Patch 10/16: net/sxe2: remove queue stats mapping

**Errors:**

1. **Line 72 (sxe2_mp.c): `static_assert` on `SXE2_MP_MAX_QSTATS` (Error)**

   The assertion checks that the shared qstats array is large enough.
   `SXE2_MP_MAX_QSTATS` is defined as `SXE2_TXQ_STATS_MAP_MAX_NUM`,
   and the assertion compares it against both Rx and Tx max queues.
   But if `SXE2_RXQ_STATS_MAP_MAX_NUM > SXE2_TXQ_STATS_MAP_MAX_NUM`,
   the assertion will fail at compile time.

   **Fix**: Define `SXE2_MP_MAX_QSTATS` as the maximum of the two:
   ```c
   #define SXE2_MP_MAX_QSTATS \
       RTE_MAX(SXE2_RXQ_STATS_MAP_MAX_NUM, SXE2_TXQ_STATS_MAP_MAX_NUM)
   ```

---

## Patch 11/16: ethdev: remove support for queue stats mapping

**No issues found.**

The removal is correct and complete. The deprecated API functions and their
internal helpers are deleted, and the release notes are updated.

---

## Patch 12/16: net/mvpp2: fix out of range Tx queue stats write

**No issues found.**

Correctly adds the missing `continue` after logging the error,
preventing an out-of-bounds write.

---

## Patch 13/16: net/ntnic: fix Tx errors reported as Rx queue errors

**No issues found.**

Removes the incorrect assignment of Tx errors to `q_errors`, which is an Rx stat.

---

## Patch 14/16: net/xsc: fix Tx errors added to Rx queue errors

**No issues found.**

Removes the incorrect accumulation of Tx errors into `q_errors`.

---

## Patch 15/16: ethdev: remove queue stats counter limit

**Warnings:**

1. **Line 507 (eth_basic_stats_get, ethdev/rte_ethdev.c): memory allocation failure not logged (Warning)**

   ```c
   queue_stats = calloc(nb_q, sizeof(*queue_stats));
   if (queue_stats == NULL)
       return -ENOMEM;
   ```

   If `calloc` fails, the function returns `-ENOMEM` silently.
   In the user-facing `rte_eth_xstats_get` wrapper (which calls this),
   the return code is converted to positive via `eth_err`, so the caller
   will see `ENOMEM`. However, there is no logging here.
   Most allocation failures in ethdev log an error before returning.

   **Suggested fix**: Add a log line:
   ```c
   if (queue_stats == NULL) {
       RTE_ETHDEV_LOG_LINE(ERR, "No memory for queue stats");
       return -ENOMEM;
   }
   ```

---

## Patch 16/16: test/pmd_ring: test per-queue xstats

**Warnings:**

1. **Line 618 (test_queue_xstats, app/test/test_pmd_ring.c): cleanup on error may leak rings (Warning)**

   The test creates `QSTATS_NB_RINGS` rings, then calls `rte_eth_from_rings`
   which takes ownership of the ring pointers. If `from_rings` succeeds
   but a later step fails (e.g., configure, queue setup, xstats get),
   the error path calls `rte_eth_dev_close(port)` then `rte_ring_free(qrings[q])`
   on all rings.

   If `rte_eth_dev_close` already freed the rings (which it may do since it owns them),
   the `rte_ring_free` calls are use-after-free. If it did not, the rings are leaked.

   **Suggested fix**: After `from_rings` succeeds, set `qrings[q] = NULL` for all rings
   passed to it, so that the cleanup path only frees rings that were not handed off:
   ```c
   port = rte_eth_from_rings("net_ringq", qrings, QSTATS_NB_RXQ,
           qrings, QSTATS_NB_TXQ, SOCKET0);
   if (port < 0) {
       printf("failed to create port\n");
       goto out;
   }
   /* Port now owns the rings, don't free them on error path. */
   for (q = 0; q < QSTATS_NB_RINGS; q++)
       qrings[q] = NULL;
   ```

---

## Summary

**Errors (must fix):**
- Patch 5: Out-of-bounds write in `cnxk_stats.c` (removed bounds check).
- Patch 10: Compile-time assertion failure if Rx max > Tx max.

**Warnings (should fix):**
- Patch 15: Missing error log on allocation failure.
- Patch 16: Potential double-free or leak of rings in test cleanup.

**Info:**
None.