|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

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

**Severity: Info**

No issues found. The removal is correct:
- `queue_stats_mapping_set` was deprecated in 25.11
- Removes dead code from both `virtio_eth_dev_ops` and `virtio_user_secondary_eth_dev_ops`
- Removes the unused `virtio_dev_queue_stats_mapping_set()` implementation

---

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

**Severity: Info**

No issues found.
- Correctly removes the obsolete help text for the `set stat_qmap` command
- Removes the dangling prototype for `set_qmap`
- The Fixes tag references the correct deprecation commit
- Cc: stable is appropriate since this cleans up dead code

---

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

**Severity: Info**

No issues found.
- Removing explicit NULL initializations allows the compiler to zero-initialize the structure
- This is cleaner and reduces maintenance burden when the ops table evolves

---

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

**Severity: Error**

**Correctness bug: Tx errors incorrectly accumulated into Rx error counter**

The Tx branch assigns `qstats->q_errors[i] = qstats_data.tx_drop_pkts`, then the Rx branch adds to it with `+=`, so the same `q_errors` entry holds both Tx and Rx drops. Since `q_errors` is the Rx per-queue drop counter, only Rx drops should be stored there. Tx drops are already counted in `oerrors`.

The fix correctly changes the Rx branch from `+=` to `=`.

---

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

**Severity: Info**

No issues found.
- Removes the deprecated queue stats mapping support
- Simplifies the stats collection loop to report one counter per configured queue
- The loop bounds are correct: `i < eth_dev->data->nb_tx_queues` and `i < eth_dev->data->nb_rx_queues`

---

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

**Severity: Info**

No issues found.
- Removes the queue stats mapping operation
- The loop bounds are correct: one entry per queue, capped by `IGC_QUEUE_PAIRS_NUM`

---

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

**Severity: Info**

No issues found.
- Removes the mapping operation and the `ixgbe_stat_mapping_registers` structure
- Replaces the reset-to-zero mapping with an identity mapping so hardware counters are usable without application involvement
- DCB can still override the mapping when configured

---

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

**Severity: Info**

No issues found.
- Removes the mapping operation and the `txgbe_stat_mappings` structure
- Simplifies stats collection to report one counter per queue

---

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

**Severity: Error**

**Correctness bug: NULL pointer dereference when qstats is NULL**

`rte_eth_stats_get()` calls the driver with `qstats = NULL` when only port-level stats are requested. Both the primary and secondary paths dereference `qstats` unconditionally:

```c
/* Primary path */
ret = sxe2_drv_queue_info_get_update(adapter, qstats);
/* qstats dereferenced inside */

/* Secondary path */
memcpy(qstats, &mz_data->payload.stats_blk.qstats, sizeof(*qstats));
```

The fix adds the missing `if (qstats != NULL)` guards. Correct.

---

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

**Severity: Info**

No issues found.
- Removes the deprecated mapping API and its command channel support
- Adjusts the multi-process path to allocate a qstats array sized for all queues
- The secondary process copies back the count in use, which is correct

---

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

**Severity: Info**

No issues found.
- Removes the public API functions `rte_eth_dev_set_tx_queue_stats_mapping` and `rte_eth_dev_set_rx_queue_stats_mapping`
- Removes the `queue_stats_mapping_set` operation from `eth_dev_ops`
- Updates documentation to reflect the removal
- Removes the trace points for the mapping functions

---

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

**Severity: Error**

**Correctness bug: missing continue after bounds check logs error**

The Tx loop logs an error when the queue index exceeds the array size, but then writes to `qstats->q_opackets[idx]` and `qstats->q_obytes[idx]` anyway (out of bounds write):

```c
if (unlikely(idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS)) {
    MRVL_LOG(ERR, "...");
    /* BUG: no continue, falls through to write qstats[idx] */
}
```

The fix adds the missing `continue;`. The Rx loop already has this.

---

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

**Severity: Error**

**Correctness bug: Tx errors incorrectly assigned to `q_errors` (Rx counter)**

```c
if (qstats != NULL && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
    qstats->q_opackets[i] = internals->txq_scg[i].tx_pkts;
    qstats->q_obytes[i] = internals->txq_scg[i].tx_bytes;
    qstats->q_errors[i] = internals->txq_scg[i].err_pkts;  /* BUG */
}
```

`q_errors` is the Rx per-queue drop counter. Tx errors are already counted in `oerrors`. The fix removes the assignment. Correct.

---

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

**Severity: Error**

**Correctness bug: same as net/ntnic**

The Rx loop fills `q_errors` correctly, then the Tx loop adds Tx errors to it:

```c
qstats->q_errors[idx] += txq->stats.tx_errors;
```

This causes Rx queue error counters to hold both Rx and Tx errors. The fix removes the assignment. Correct.

---

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

**Severity: Info**

**Extensive changes to per-queue stats infrastructure.**

### Key changes:
1. Replaces the fixed-size arrays `q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS]` etc. with one `struct eth_queue_stats` per queue, allocated by ethdev
2. Drivers now write `qstats[i].q_ipackets` instead of `qstats->q_ipackets[i]`
3. Removes `RTE_ETHDEV_QUEUE_STAT_CNTRS` (was 16)
4. Removes the `q_errors` counter (Rx drops are still in `ierrors` and `imissed`)

### Driver changes reviewed:
- **af_packet, af_xdp, ark, atlantic, avp, axgbe, bnxt, cnxk, dpaa2, ena, hinic, hinic3, igc, fm10k, ixgbe, ionic, mana, memif, mlx4, mlx5, mvpp2, nbl, netvsc, nfb, nfp, ntnic, null, octeon_ep, pcap, qede, ring, rnp, sxe2, tap, thunderx, txgbe, vhost, virtio, vmxnet3, xsc, zxdh**: All correctly migrated to the new API.

**Observations:**

1. **nfp representor allocation**: The qstats array is dynamically allocated in `nfp_flower_repr_init()` and freed in `nfp_flower_repr_uninit()`. This is correct since the representor doesn't use ethdev's allocation.

2. **nfp_net allocation**: Both `nfp_net_init()` and `nfp_netvf_init()` allocate `eth_qstats_base` and free it on close. This matches the pattern of `eth_xstats_base`. Correct.

3. **ethdev core**:
   - `eth_stats_qstats_get()` no longer zeroes `qstats` before calling the driver (allocation zeroes it)
   - `eth_basic_stats_get()` allocates `calloc(nb_q, sizeof(*queue_stats))` where `nb_q = RTE_MAX(nb_rxqs, nb_txqs)` and frees it at the end. Correct.

### No correctness issues found.

---

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

**Severity: Info**

No issues found.
- Creates a port with 3 Rx and 5 Tx queues
- Verifies that xstats are reported for every configured queue
- Verifies that queues beyond the configured count are not reported
- Good coverage of the new per-queue stats API

---

# Summary

## Errors (must fix)

1. **Patch 4 (net/cnxk)**: Tx drops incorrectly accumulated into `q_errors` (Rx counter). Fixed by changing `+=` to `=` in the Rx branch.

2. **Patch 9 (net/sxe2)**: NULL pointer dereference when `qstats` is NULL in `rte_eth_stats_get()`. Fixed by adding null checks in both primary and secondary paths.

3. **Patch 12 (net/mvpp2)**: Missing `continue` after Tx queue index bounds check, causing out-of-bounds write. Fixed by adding the continue.

4. **Patch 13 (net/ntnic)**: Tx errors incorrectly assigned to `q_errors` (Rx counter). Fixed by removing the assignment.

5. **Patch 14 (net/xsc)**: Tx errors incorrectly added to `q_errors` (Rx counter). Fixed by removing the assignment.

## Warnings

None.

## Info

- Patches 1, 2, 3, 5-8, 10-11, 15-16: No issues. Clean removal of deprecated API and migration to the new per-queue stats infrastructure.