|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

## Summary

This patch series removes deprecated queue stats mapping features, cleans up related PMD code, fixes several correctness bugs where queue stats were misreported, and lifts the RTE_ETHDEV_QUEUE_STAT_CNTRS=16 limit on per-queue statistics reporting. The series is well-structured and addresses multiple longstanding issues.

---

## Patch-by-Patch Review

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

**Clean** -- No issues found. Removes dead code (queue_stats_mapping_set op) that was deprecated in 25.11.

---

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

**Clean** -- No issues found. Removes documentation and header for the deprecated `set_qmap` command removed in 25.11.

---

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

**Clean** -- No issues found. Removes explicit NULL initialization of eth_dev_ops fields, relying on compiler zero-init. Improves maintainability.

---

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

**Error**

**q_errors field used for the wrong purpose:**
Line 45 in the original code initializes `q_errors[i]` with Tx drops, then line 58 adds Rx drops.
This is a correctness bug: `q_errors` counts received packets dropped (from the Rx path), but the code was adding Tx drops to it as well, causing the `rx_qN_errors` xstat to report both directions.

The fix correctly removes the Tx assignment (line 48 deleted). Tx drops are already counted in the port-level `oerrors` field.

However, **after the patch**, line 57 sets `q_errors[i] = qstats_data.rx_drop_pkts` directly. This is correct **only if** `i` iterates over Rx queues exclusively. Verify the loop bounds:

- The loop runs `for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++)`
- Inside, it checks `if (dev->txq_stat_map[i] & (1U << 31))` for Tx, and `if (dev->rxq_stat_map[i] & (1U << 31))` for Rx
- So `i` is a stat-pool index, not a queue index, and Tx and Rx can both write to `q_errors[i]`

This means **the bug is only half-fixed**. If the same stat pool index is mapped to both a Tx queue and an Rx queue, the Rx block (lines 51-58) will still overwrite `q_errors[i]` with the Rx value, losing any prior data. But since the Tx block no longer writes `q_errors`, this is now **mostly correct** (Rx data is all that should be there). The only remaining issue is if Tx and Rx share a pool index, the Rx value overwrites instead of being the only value. Since stats mapping is being removed in later patches, this is acceptable for a backportable fix.

**Suggested commit message tweak (optional):**
The commit message says "Tx drops are already counted in oerrors, so drop the assignment." Add: "The Rx branch stores the Rx drop count directly, replacing the incorrect `+=`."

**Final verdict:** Acceptable as-is for a stable backport. The correctness bug (Tx drops added to Rx errors) is fixed. The minor sequencing issue (Tx block runs first, then Rx overwrites) is benign because Tx no longer writes `q_errors`.

---

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

**Errors**

1. **Loop iteration past `nb_tx_queues` / `nb_rx_queues`:**

   - Line 42: `for (i = 0; i < eth_dev->data->nb_tx_queues; i++)`
   - Line 50: `for (i = 0; i < eth_dev->data->nb_rx_queues; i++)`

   But the hardware stats arrays are `hw_stats->qp[i]`, where the size is device-specific (not necessarily equal to the configured queue count). If `nb_tx_queues` or `nb_rx_queues` exceeds the hardware queue limit, this reads uninitialized memory or out-of-bounds.

   **Fix:** Cap the loop at the device queue limit:
   ```c
   for (i = 0; i < RTE_MIN(eth_dev->data->nb_tx_queues, dev->nb_txq); i++)
   ```
   (Substitute the correct max-queue constant or struct field for the cnxk device.)

2. **Missing NULL check before dereferencing `qstats`:**

   Lines 45-46 and 53-54 write `qstats[i].*` without checking `if (qstats != NULL)`.
   This causes a **NULL pointer dereference** when `rte_eth_stats_get()` is called (which passes `qstats=NULL`).

   **Fix:** Wrap the qstats writes in `if (qstats != NULL)`.

**Example corrected version:**
```c
if (qstats != NULL) {
    for (i = 0; i < RTE_MIN(eth_dev->data->nb_tx_queues, nix->nb_tx_queues); i++) {
        struct roc_nix_stats_queue qstats_data;
        rc = roc_nix_stats_queue_get(nix, i, 0, &qstats_data);
        if (rc)
            goto exit;
        qstats[i].q_opackets = qstats_data.tx_pkts;
        qstats[i].q_obytes = qstats_data.tx_octs;
    }
    /* Same for Rx */
}
```

---

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

**Warning**

Line 2063: `for (i = 0; i < RTE_MIN(IGC_QUEUE_PAIRS_NUM, dev->data->nb_rx_queues); i++)`

This is **correct** after the patch, but verify that `IGC_QUEUE_PAIRS_NUM` is the hardware queue limit. If it's larger than the hardware supports, accessing `queue_stats->pqgprc[i]` or `queue_stats->rqdpc[i]` for large `i` could read garbage. Review the definition of `struct igc_hw_queue_stats` to ensure the arrays are sized to `IGC_QUEUE_PAIRS_NUM`.

Assuming the stats arrays are correctly sized, no issue.

---

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

**Error**

**Dead code after `return` statement:**

Line 2650 (after patch):
```c
ixgbe_reset_qstat_mappings(hw);
```

This call is **unreachable** because the function returned at line 2647:
```c
err = ixgbe_flow_ctrl_enable(dev, hw);
if (err < 0) {
    PMD_INIT_LOG(ERR, "...flow control enable failed...");
    return err;
}
/* Dead code */
ixgbe_reset_qstat_mappings(hw);
```

Every code path above line 2647 that reaches the flow-control call can fail, and the `if (err < 0) return err;` is **unconditional** (no way to skip it). So the stats reset never executes.

**Fix:** The commit message says "Keep the existing queue stats mapping that happens on device initialization." But this code path is `dev_start`, not `dev_init`. The correct fix depends on intent:

- If the mapping reset is needed here, move it **before** the flow-control call.
- If it should only run on success, add `if (err == 0) ixgbe_reset_qstat_mappings(hw);` before the return.
- If it's truly initialization-only, remove the call entirely (and update the commit message).

**Recommendation:** Move it before the flow-control call, or delete it and clarify the commit message that the reset is only done in `dev_configure` (not `dev_start`).

---

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

**Error**

**NULL pointer dereference:**

Lines 2455-2460 (after patch):
```c
if (qstats != NULL) {
    for (i = 0; i < TXGBE_MAX_QP && i < dev->data->nb_rx_queues; i++) {
        qstats[i].q_ipackets += hw_stats->qp[i].rx_qp_packets;
        qstats[i].q_ibytes += hw_stats->qp[i].rx_qp_bytes;
    }
    /* Same for Tx */
}
```

This is **correct**. No issue.

---

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

**Clean** -- No issues found. Adds missing `if (qstats != NULL)` checks before dereferencing `qstats` in both the primary and secondary (multi-process) paths. Fixes a NULL pointer dereference when `rte_eth_stats_get()` is called.

---

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

**Warning**

**Multiprocess queue stats array sizing:**

Line 10 (in `sxe2_mp.h`):
```c
#define SXE2_MP_MAX_QSTATS	SXE2_TXQ_STATS_MAP_MAX_NUM
```

The comment says the array must cover every queue the primary can report stats for. The code uses `SXE2_TXQ_STATS_MAP_MAX_NUM` (presumably the Tx max). Verify:
- Is `SXE2_RXQ_STATS_MAP_MAX_NUM` equal to or smaller than `SXE2_TXQ_STATS_MAP_MAX_NUM`?
- If Rx can have more queues than Tx, use `RTE_MAX(SXE2_RXQ_STATS_MAP_MAX_NUM, SXE2_TXQ_STATS_MAP_MAX_NUM)`.

The `static_assert` at line 18 (in `sxe2_mp.c`) checks both:
```c
static_assert(SXE2_RXQ_STATS_MAP_MAX_NUM <= SXE2_MP_MAX_QSTATS &&
              SXE2_TXQ_STATS_MAP_MAX_NUM <= SXE2_MP_MAX_QSTATS, ...);
```

So **if `SXE2_TXQ_STATS_MAP_MAX_NUM >= SXE2_RXQ_STATS_MAP_MAX_NUM`**, the define is correct. If not, the `static_assert` will catch it at compile time. No runtime issue.

**Final verdict:** Acceptable. The `static_assert` guards the array size.

---

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

**Errors**

1. **Removed deprecated notice section is not empty:**

   The patch deletes lines from `doc/guides/rel_notes/deprecation.rst` (the deprecated queue stats mapping notice). However, it does **not** verify that the deprecation notice file is otherwise well-formed. If other deprecation notices depend on the removed section's formatting (e.g., RST list structure), the doc build could break.

   **Review the context:** Ensure the removal does not orphan a list or break RST syntax. The diff shows only the deleted lines; the surrounding context is not visible. Assuming the notice was a standalone paragraph, this is fine.

2. **Release notes wording:**

   Line 83 (in `release_26_11.rst`):
   ```
   ``rte_eth_dev_set_tx_queue_stats_mapping`` and ``rte_eth_dev_set_rx_queue_stats_mapping``
   were deprecated and are now removed.
   ```

   Passive voice is acceptable here, but for consistency with other entries, consider:
   ```
   Removed the deprecated functions ``rte_eth_dev_set_tx_queue_stats_mapping`` and
   ``rte_eth_dev_set_rx_queue_stats_mapping``.
   ```

   This is a style nit, not an error.

**Final verdict:** Clean, with a minor style suggestion.

---

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

**Clean** -- No issues found. Adds missing `continue` after logging an error for an out-of-range Tx queue index. Matches the Rx loop pattern. The code was writing `qstats[idx]` even after logging "tx queue stats out of range", which is a correctness bug (latent today since `MRVL_PP2_TXQ_MAX=8 < RTE_ETHDEV_QUEUE_STAT_CNTRS=16`, but wrong).

---

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

**Clean** -- No issues found. Removes the line that wrote Tx errors to `qstats->q_errors[i]`, which was causing Tx errors to be reported as `rx_qN_errors`. Tx errors are already in the port-level `oerrors` field. The commit message and fix are correct.

---

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

**Clean** -- No issues found. Same pattern as patch 13: removes Tx error accumulation in `q_errors`, which was causing `rx_qN_errors` to report both Rx and Tx drops. Correct fix.

---

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

**Errors**

1. **Out-of-bounds write risk in drivers:**

   The patch changes `struct eth_queue_stats` from arrays of `[RTE_ETHDEV_QUEUE_STAT_CNTRS]` to per-queue members. Ethdev now allocates one `struct eth_queue_stats` per queue. Drivers are instructed to write `qstats[i].q_ipackets` instead of `qstats->q_ipackets[i]`.

   However, **drivers that were not updated** (or that have bugs) could still write past the end of the array. For example, if a driver loops `for (i = 0; i < hw_queue_limit; i++)` and `hw_queue_limit > nb_rx_queues`, it writes `qstats[i].*` where `i >= nb_rx_queues`, which is out-of-bounds of the allocated array.

   **The patch updates many drivers**, but any driver **not** in the diff is at risk. Reviewing the entire driver list is impractical in this review, but the patch should be tested with ASAN to catch any out-of-bounds writes.

   **Recommendation:** Add to the commit message: "Drivers must not write past `nb_rx_queues` for Rx fields or `nb_tx_queues` for Tx fields. Drivers updated in this patch are verified; others are subject to review."

2. **Incorrect loop bounds in several drivers** (after the patch):

   - **drivers/net/atlantic/atl_ethdev.c**, line 958:
     ```c
     for (i = 0; i < dev->data->nb_rx_queues && i < AQ_HW_MAX_RX_QUEUES; i++)
     ```
     This reads `swstats->rxq[i]` which is sized `[AQ_HW_MAX_RX_QUEUES]`. If `nb_rx_queues > AQ_HW_MAX_RX_QUEUES`, the loop stops at `AQ_HW_MAX_RX_QUEUES`, but the `&&` condition is backwards. Should be:
     ```c
     for (i = 0; i < RTE_MIN(dev->data->nb_rx_queues, AQ_HW_MAX_RX_QUEUES); i++)
     ```
     Or split the conditions to avoid ambiguity. The code is **correct** (both conditions are checked), but the `&&` style is less clear.

   - **drivers/net/cnxk/cnxk_stats.c**, lines 42 and 50 (same as patch 05 comment):
     No bounds check on the hardware queue limit. If `nb_tx_queues`