|WARNING| [v2,9/9] ethdev: remove support for queue stats mapping

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-23

# DPDK Patch Review

## PATCH 1/9: net/virtio: remove unused queue stats mapping

### Info
- **Purpose:** Removes unused `queue_stats_mapping_set` operation and related code
- **Justification:** States feature hasn't been needed since 25.11 release

**Findings:** No issues found. The removal is clean and complete.

---

## PATCH 2/9: app/testpmd: remove unused function prototype

### Findings

#### Error: Missing Release Notes
The patch removes user-facing command-line help text but does not update release notes. Changes to application behavior visible to users require documentation.

**Suggested fix:**
Add a section to `doc/guides/rel_notes/release_26_11.rst`:
```rst
* **Removed testpmd queue statistics mapping command.**

  The deprecated ``set stat_qmap`` command has been removed from testpmd.
```

---

## PATCH 3/9: net/enic: remove unneeded ops initialization

### Findings

#### Warning: Patch Purpose Mismatch
The commit message describes this as removing "unneeded ops initialization" to avoid needing updates when fields change. However, the removed fields were never part of queue stats mapping functionality--they are unrelated device operations (`dev_set_link_up`, `dev_set_link_down`, `vlan_filter_set`, etc.).

This patch conflates two unrelated cleanups:
1. Removing explicit NULL assignments for unused ops (valid cleanup)
2. The queue stats mapping removal theme of the series (not applicable here)

**Suggested fix:** Either split this into a separate series or clarify in the commit message that this is a general cleanup of unnecessary explicit NULL initializations in the enic driver, unrelated to queue stats mapping.

---

## PATCH 4/9: net/cnxk: remove queue stats mapping

### Findings

#### Error: Logic Change Without Testing Documentation
The patch changes stats reporting behavior:

**Before:**
```c
for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
    if (dev->txq_stat_map[i] & (1U << 31)) {
        qidx = dev->txq_stat_map[i] & 0xFFFF;
        /* ... fetch stats for mapped queue qidx into slot i ... */
    }
}
```

**After:**
```c
for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
    if (i >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
        break;
    /* ... fetch stats for queue i into slot i ... */
}
```

The old code supported arbitrary queue-to-stats-slot mapping (e.g., physical queue 7 could report into stats slot 2). The new code enforces 1:1 mapping (queue N - slot N).

If applications relied on custom mappings, they will now see different (possibly zero) counters for those slots.

**Impact:** ABI-compatible (no structure changes) but behavioral change. Should be noted in release notes as a behavioral change that applications using custom queue stats mappings will observe.

**Suggested addition to release notes:**
```rst
* **cnxk driver now uses 1:1 queue-to-stats mapping.**

  The cnxk driver previously supported custom queue-to-stats-slot mappings via
  the deprecated ``queue_stats_mapping_set`` operation. Statistics now use
  direct queue index mapping (queue N reports in stats slot N, up to
  ``RTE_ETHDEV_QUEUE_STAT_CNTRS``). Applications that configured custom mappings
  will observe different queue statistics.
```

---

## PATCH 5/9: net/e1000: remove queue stats mapping

### Findings

#### Error: Same Logic Change as PATCH 4/9
Similar to cnxk, this changes from arbitrary queue-stats-slot mapping to 1:1 mapping:

**Before:**
```c
int map_id = igc->txq_stats_map[i];
if (map_id >= 0) {
    qstats->q_opackets[map_id] += queue_stats->pqgptc[i];
    /* ... */
}
```

**After:**
```c
qstats->q_opackets[i] += queue_stats->pqgptc[i];
```

The old code initialized `txq_stats_map[i]` to `-1` (see removed init loop in `eth_igc_dev_init`), meaning unmapped queues were skipped. The new code unconditionally reports all queues in their natural index.

**Impact:** Applications see different behavior--previously unmapped queues now appear in stats, and custom mappings are lost.

**Suggested addition to release notes:**
```rst
* **e1000 (igc) driver now uses 1:1 queue-to-stats mapping.**

  Custom queue statistics mappings are no longer supported. Queue statistics
  now directly map queue index to stats array index.
```

---

## PATCH 6/9: net/ixgbe: remove queue stats mapping

### Findings

#### Warning: Hardware Register Writes Removed
This patch removes `ixgbe_restore_statistics_mapping()`, which wrote `stat_mappings->rqsmr[n]` and `stat_mappings->tqsm[n]` to hardware registers `IXGBE_RQSMR(n)` and `IXGBE_TQSM(n)` during `dev_start`.

**Question for maintainer:** Does the hardware require these registers to be configured for basic queue stats collection to work, or were they only needed for custom mapping? If basic stats collection relies on these registers being non-zero, removing the writes could break per-queue statistics entirely.

**Impact:** Potentially breaks per-queue statistics collection if hardware requires configuration. Needs driver maintainer verification.

---

## PATCH 7/9: net/txgbe: remove queue stats mapping

### Findings

#### Error: Same Logic Change as Previous Patches
Changes from `stat_mappings->tqsm[]` lookup to direct 1:1 mapping:

**Before:**
```c
q_map = (stat_mappings->rqsm[n] >> offset) & QMAP_FIELD_RESERVED_BITS_MASK;
j = (q_map < RTE_ETHDEV_QUEUE_STAT_CNTRS ? q_map : q_map % RTE_ETHDEV_QUEUE_STAT_CNTRS);
qstats->q_ipackets[j] += hw_stats->qp[i].rx_qp_packets;
```

**After:**
```c
qstats->q_ipackets[i] += hw_stats->qp[i].rx_qp_packets;
```

**Impact:** Custom mappings lost, behavior changes for applications that configured them.

---

## PATCH 8/9: net/sxe2: remove queue stats mapping

### Findings

#### Warning: Capability Flag Handling Inconsistent
The patch removes `SXE2_DEV_CAPS_OFFLOAD_Q_MAP` from capability mask assignment but does not remove the definition of the constant itself. If the constant is defined in a driver header (not shown in the patch context), it becomes dead code.

**Suggested cleanup:** Remove the `SXE2_DEV_CAPS_OFFLOAD_Q_MAP` definition from the driver header if it exists, since it is no longer used.

#### Info: Driver Command Channel Cleanup
The patch removes driver-to-hardware communication for queue mapping (`SXE2_DRV_CMD_RX_MAP_SET`, `SXE2_DRV_CMD_TX_MAP_SET`, etc.). This is appropriate if the hardware/firmware commands are no longer used. Ensure that firmware compatibility is maintained if older firmware versions expected these commands.

---

## PATCH 9/9: ethdev: remove support for queue stats mapping

### Findings

#### Error: Missing ABI Version Bump Documentation
The patch removes public API functions (`rte_eth_dev_set_tx_queue_stats_mapping`, `rte_eth_dev_set_rx_queue_stats_mapping`). While these were marked `__rte_deprecated`, their removal is an ABI break that must be documented in the ABI policy tracking.

**Required addition:**
In `doc/guides/rel_notes/release_26_11.rst` under **ABI Changes**:
```rst
* **ethdev: Removed deprecated queue stats mapping API.**

  The following functions were removed:

  * ``rte_eth_dev_set_tx_queue_stats_mapping``
  * ``rte_eth_dev_set_rx_queue_stats_mapping``

  The corresponding ``queue_stats_mapping_set`` device operation was removed
  from ``struct eth_dev_ops``.
```

#### Warning: Features Documentation Not Updated
`doc/guides/nics/features.rst` previously documented queue stats mapping under the **Per-queue stats** section:
```rst
May also support configuring per-queue stat counter mapping.
Used by some drivers to workaround HW limitations.

* **[implements] eth_dev_ops**: ``queue_stats_mapping_set``.
* **[related]    API**: ``rte_eth_dev_set_rx_queue_stats_mapping()``,
  ``rte_eth_dev_set_tx_queue_stats_mapping()``.
```

The patch correctly removes this text. **No issue here.**

#### Info: Trace Point Removal
The patch removes trace points for the deleted functions. This is correct, but note that applications using these trace points will need to remove the corresponding trace configuration.

---

## Series-Level Issues

### Error: Inconsistent Behavioral Changes Across Drivers
Patches 4, 5, 7 change driver behavior from custom queue-to-stats-slot mapping to 1:1 mapping. Patch 6 (ixgbe) additionally removes hardware register configuration. These are **not pure removals**--they change observable behavior.

**Required:** Release notes must document behavioral changes for each affected driver (cnxk, igc, txgbe, ixgbe, sxe2) so users can update test scripts and monitoring tools.

### Warning: No Mention of virtio Secondary Process Removal
Patch 1 removes `queue_stats_mapping_set` from `virtio_user_secondary_eth_dev_ops`. If secondary processes relied on this (unlikely, since the API was deprecated), this is a behavioral change. Suggest noting this in release notes.

---

## Summary

### Errors (Must Fix)
1. **PATCH 2/9:** Missing release notes for testpmd command removal
2. **PATCH 4/9, 5/9, 7/9:** Missing release notes documenting behavioral change (custom mapping - 1:1 mapping)
3. **PATCH 9/9:** Missing ABI changes documentation in release notes

### Warnings (Should Fix)
1. **PATCH 3/9:** Commit message does not match patch content (unrelated cleanup conflated with queue stats removal)
2. **PATCH 6/9:** Verify with driver maintainer that removing `ixgbe_restore_statistics_mapping()` does not break per-queue stats collection
3. **PATCH 8/9:** Check if `SXE2_DEV_CAPS_OFFLOAD_Q_MAP` definition should be removed from header

### Info
1. **PATCH 9/9:** Trace point removal will affect applications using those trace configurations (expected consequence)

---

## Suggested Release Notes Additions

Add to `doc/guides/rel_notes/release_26_11.rst`:

**Under "Removed Items":**
```rst
* **Removed testpmd queue statistics mapping command.**

  The deprecated ``set stat_qmap`` command has been removed.

* **Removed custom queue-to-statistics-slot mapping in drivers.**

  The following drivers changed from custom queue statistics mapping to 1:1
  (queue index N reports in statistics slot N):

  * cnxk
  * igc (e1000)
  * ixgbe
  * txgbe
  * sxe2

  Applications that configured custom mappings via the deprecated API will
  observe different queue statistics counters.
```

**Under "ABI Changes":**
```rst
* **ethdev: Removed deprecated queue stats mapping API.**

  The following functions were removed:

  * ``rte_eth_dev_set_tx_queue_stats_mapping``
  * ``rte_eth_dev_set_rx_queue_stats_mapping``

  The corresponding ``queue_stats_mapping_set`` device operation was removed
  from ``struct eth_dev_ops``.
```