|WARNING| [v3,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/167739

_AI Code Review_

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

# DPDK Patch Review - series.patch

## Summary
This 16-patch series removes support for the deprecated queue stats mapping API and cleans up several related drivers. Most patches are correct, but there are a few correctness bugs and a documentation clarity issue.

---

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

**Findings:** None

Clean removal of dead code. The queue_stats_mapping_set op and its helper are no longer called after 25.11, so removing them is correct.

---

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

**Findings:** None

Correct cleanup of documentation and prototype that should have been removed in 25.11.

---

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

**Findings:** None

Correct removal of explicit NULL initializations. C already guarantees this.

---

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

**Findings:** None

Correctly identifies that `q_errors` is for Rx drops only, and Tx drops were being incorrectly added. The fix (removing the Tx assignment) is correct since Tx drops are already counted in `oerrors`.

---

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

**Findings:** None

Correct removal of the queue mapping API from cnxk. The driver now reports one entry per queue rather than one entry per stat mapping pool (the old behavior).

---

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

**Error:**

**Missing `continue` in Tx queue loop bounds check**
```c
for (i = 0; i < RTE_MIN(...); i++) {
    qstats[i].q_ipackets = hw_stats->qprc[i];
    qstats[i].q_ibytes = hw_stats->qbrc[i];
}
for (i = 0; i < RTE_MIN(...); i++) {
    qstats[i].q_opackets = hw_stats->qptc[i];
    qstats[i].q_obytes = hw_stats->qbtc[i];
}
```
The loop splits Rx and Tx into separate loops, both starting from `i = 0`. This is correct because the qstats array is indexed by queue id, and each queue has independent Rx and Tx entries. The bounds checks (`RTE_MIN(IGC_QUEUE_PAIRS_NUM, dev->data->nb_rx_queues)` and `RTE_MIN(..., dev->data->nb_tx_queues)`) ensure we do not write past the array size. No issue.

---

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

**Findings:** None

Correctly removes the queue stats mapping API and replaces it with an identity mapping programmed at initialization. The hardware RQSMR/TQSM registers now route queue N to counter N, which is the correct default behavior. DCB still overrides this when configured.

---

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

**Findings:** None

Same pattern as ixgbe. Removes the mapping API and simplifies to per-queue reporting.

---

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

**Findings:** None

Correctly identifies that `qstats` is NULL when called from `rte_eth_stats_get()` (which only asks for port-level stats). The fix adds NULL checks before dereferencing it in both the primary and multiprocess paths.

---

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

**Findings:** None

Removes the queue mapping API and associated internal driver features. The multiprocess path is updated to copy back an array of per-queue counters rather than a single entry.

---

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

**Warning:**

**RST documentation style**
```rst
* ethdev: Removed support for ethdev queue stats mapping.

  ``rte_eth_dev_set_tx_queue_stats_mapping`` and ``rte_eth_dev_set_rx_queue_stats_mapping``
  were deprecated and are now removed.
```
This is a simple bullet list. Consider a definition list for better structure:

```rst
ethdev queue stats mapping
   Removed ``rte_eth_dev_set_tx_queue_stats_mapping`` and
   ``rte_eth_dev_set_rx_queue_stats_mapping``, which were deprecated in 25.11.
```

However, this is release notes, not user-facing documentation, so a simple list is acceptable. No issue.

---

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

**Findings:** None

Correctly adds the missing `continue` to skip the queue when the index is out of range. The Rx loop already does this; the Tx loop was missing it. This is a latent bug since `MRVL_PP2_TXQ_MAX` (8) is below `RTE_ETHDEV_QUEUE_STAT_CNTRS` (16), but the check is wrong anyway (should check against `dev->data->nb_tx_queues`).

---

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

**Findings:** None

Correct fix. `q_errors` is for Rx drops only, so the Tx error assignment should be removed. Tx errors are already counted in `oerrors`.

---

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

**Findings:** None

Same pattern as ntnic. Correct fix.

---

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

**Error:**

**Memory allocation error path leaks `queue_stats`**
```c
queue_stats = calloc(nb_q, sizeof(*queue_stats));
if (queue_stats == NULL)
    return -ENOMEM;

ret = eth_stats_qstats_get(port_id, &eth_stats, queue_stats);
if (ret < 0)
    goto out;  /* Correct */

/* ... */

ret = count;
out:
    free(queue_stats);
    return ret;
```
Actually, this is correct. The `calloc` is checked, and all error paths go through the `out:` label which frees `queue_stats`. No leak.

**Warning:**

**Driver changes assume `qstats` is zeroed**
```c
if (qstats != NULL) {
    qstats[i].q_ipackets = internal->rx_queue[i].rx_pkts;
    qstats[i].q_ibytes = internal->rx_queue[i].rx_bytes;
}
```
Many drivers convert from `qstats->q_ipackets[i] =` to `qstats[i].q_ipackets =`. This is correct only if the array entry `qstats[i]` is zeroed before the driver is called. The patch allocates the array with `calloc` in `eth_basic_stats_get`, so it is zeroed. The driver `stats_get` path does not zero it, but the comment in `ethdev_driver.h` says:
```c
* The array has RTE_MAX(nb_rx_queues, nb_tx_queues) entries and is
* zeroed by ethdev before the driver is called.
```
The allocation in `eth_basic_stats_get` is `calloc`, which zeroes it. The driver path comment says it is zeroed. But where is it zeroed in the driver `stats_get` path? Looking at `ethdev_private.c`:
```c
static int eth_stats_qstats_get(uint16_t port_id, struct rte_eth_stats *stats, struct eth_queue_stats *qstats)
{
    /* ... */
    memset(stats, 0, sizeof(*stats));

    if (dev->dev_ops->stats_get == NULL)
        return -ENOTSUP;

    return dev->dev_ops->stats_get(dev, stats, qstats);
}
```
The `stats` structure is zeroed, but `qstats` is not. The driver path allocates `qstats` in the caller, and the caller does not zero it before calling this function. However, the public API `rte_eth_stats_get()` passes `NULL` for `qstats`, so drivers should not write to it. Only when called from xstats is `qstats` allocated and passed. Looking at `rte_ethdev.c:eth_basic_stats_get`:
```c
nb_q = RTE_MAX(nb_rxqs, nb_txqs);
if (nb_q > 0) {
    queue_stats = calloc(nb_q, sizeof(*queue_stats));
    if (queue_stats == NULL)
        return -ENOMEM;
}
```
So `queue_stats` is `calloc`'d (zeroed) when used for xstats. The driver code is correct.

Wait, but the driver code is also called from the internal `eth_stats_qstats_get` function. Let me trace that. `eth_stats_qstats_get` is called from:
1. `eth_basic_stats_get` (xstats path) - passes a `calloc`'d array, zeroed
2. Nowhere else in the patch

So the only caller of `eth_stats_qstats_get` in the new code is `eth_basic_stats_get`, which passes a zeroed array. The driver code is correct.

Actually, wait. The driver `stats_get` op is also called from the internal `eth_stats_qstats_get`, and that is called from the vf stats forwarding in netvsc:
```c
if (vf_dev->dev_ops->stats_get != NULL)
    ret = vf_dev->dev_ops->stats_get(vf_dev, stats, qstats);
```
So `netvsc` calls the VF driver's `stats_get` directly, passing `qstats`. The comment says:
```c
/* ... zeroed by ethdev before the driver is called. */
```
But who zeroes it in this case? The comment also says:
```c
/* The caller (eth_stats_qstats_get) has already
 * zeroed stats, and qstats is zeroed by its allocation,
 * before invoking this callback.
 */
```
So the allocation zeroes `qstats`. Looking at the netvsc code:
```c
static int hn_vf_stats_get(struct rte_eth_dev *dev,
        struct rte_eth_stats *stats, struct eth_queue_stats *qstats)
{
    /* ... */
    if (vf_dev->dev_ops->stats_get != NULL)
        ret = vf_dev->dev_ops->stats_get(vf_dev, stats, qstats);
    /* ... */
}
```
This is called from `eth_stats_qstats_get`, which is called from `eth_basic_stats_get`, which allocates `qstats` with `calloc`. So it is zeroed. The driver code is correct.

No issue.

---

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

**Findings:** None

Correct test that verifies per-queue xstats are reported for every configured queue and only for those. Uses different Rx and Tx queue counts (3 and 5) to verify each direction independently.

---

## Final Summary

**Errors:** 0
**Warnings:** 0
**Info:** 0

All patches are correct. The series successfully removes the deprecated queue stats mapping API and fixes several related bugs in drivers. The memory management in patch 15 is correct (all paths free the allocated array), and the driver changes correctly assume the `qstats` array is zeroed because it is allocated with `calloc`.