|SUCCESS| [RFC,7/7] ethdev: remove support for queue stats mapping

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164676

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-30

# DPDK Patch Review

**Commit:** Unknown SHA
**Author:** Stephen Hemminger <stephen at networkplumber.org>
**Subject:** [RFC 1/7] net/virtio: remove unused queue stats mapping

This is a review of a 7-patch series removing the deprecated queue stats 
mapping feature from multiple DPDK drivers and the ethdev core. The 
feature was deprecated in DPDK 25.11.

## Overall Comments

The patch series appropriately removes deprecated functionality following 
the deprecation notice. However, several patches contain issues that need 
to be addressed.

---

## Patch 1/7: net/virtio

> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 9fd746b80f..0b2574bd02 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -90,12 +90,6 @@ static int virtio_intr_disable(struct rte_eth_dev *dev);
>  static int virtio_get_monitor_addr(void *rx_queue,
>  				struct rte_power_monitor_cond *pmc);
>  
> -static int virtio_dev_queue_stats_mapping_set(
> -	struct rte_eth_dev *eth_dev,
> -	uint16_t queue_id,
> -	uint8_t stat_idx,
> -	uint8_t is_rx);
> -
>  static void virtio_notify_peers(struct rte_eth_dev *dev);
>  static void virtio_ack_link_announce(struct rte_eth_dev *dev);

This patch is straightforward and correct.

---

## Patch 2/7: net/enic

This patch is straightforward and correct - it removes explicit NULL 
initializations that are unnecessary.

---

## Patch 3/7: net/cnxk

> diff --git a/drivers/net/cnxk/cnxk_stats.c b/drivers/net/cnxk/cnxk_stats.c
> index d57659ef51..e5d29a78b3 100644
> --- a/drivers/net/cnxk/cnxk_stats.c
> +++ b/drivers/net/cnxk/cnxk_stats.c
> @@ -14,7 +14,8 @@ cnxk_nix_stats_get(struct rte_eth_dev *eth_dev, struct rte_eth_stats *stats,
>  	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
>  	struct roc_nix *nix = &dev->nix;
>  	struct roc_nix_stats nix_stats;
> -	int rc = 0, i;
> +	unsigned int i;
> +	int rc = 0;

Does the variable order change here comply with reverse Christmas tree 
ordering? The rc variable should come before the i variable in the 
declaration if following that convention.

> @@ -33,31 +34,35 @@ cnxk_nix_stats_get(struct rte_eth_dev *eth_dev, struct rte_eth_stats *stats,
>  	stats->ibytes = nix_stats.rx_octs;
>  	stats->ierrors = nix_stats.rx_err;
>  
> -	if (qstats != NULL) {
> -		for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
> [ ... ]
> +	if (qstats == NULL)
> +		goto exit;

The coding style guide indicates that NULL comparisons should be made 
explicit. Does this follow the guideline, or should it be 
`if (qstats == NULL)` instead? Actually, this is correct as written.

> +
> +	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +		struct roc_nix_stats_queue qstats_data;
> +
> +		if (i >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
> +			break;

Can this loop overflow RTE_ETHDEV_QUEUE_STAT_CNTRS in qstats array 
accesses before the break check executes?

> +		rc = roc_nix_stats_queue_get(nix, i, 0, &qstats_data);
> +		if (rc)
> +			goto exit;
> +		qstats->q_opackets[i] = qstats_data.tx_pkts;
> +		qstats->q_obytes[i] = qstats_data.tx_octs;
> +		qstats->q_errors[i] = qstats_data.tx_drop_pkts;
> +	}

---

## Patch 4/7: net/e1000

> diff --git a/drivers/net/intel/e1000/igc_ethdev.c b/drivers/net/intel/e1000/igc_ethdev.c
> index 727ea36c2b..891c810132 100644
> --- a/drivers/net/intel/e1000/igc_ethdev.c
> +++ b/drivers/net/intel/e1000/igc_ethdev.c
> @@ -1362,7 +1359,7 @@ eth_igc_dev_init(struct rte_eth_dev *dev)
>  	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
>  	struct igc_adapter *igc = IGC_DEV_PRIVATE(dev);
>  	struct e1000_hw *hw = IGC_DEV_PRIVATE_HW(dev);
> -	int i, error = 0;
> +	int error = 0;

Good cleanup of unused variable.

> @@ -2070,19 +2060,11 @@ eth_igc_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *rte_stats,
>  	/* Get per-queue statuses */
>  	if (qstats) {
>  		for (i = 0; i < IGC_QUEUE_PAIRS_NUM; i++) {
> -			/* GET TX queue statuses */
> -			int map_id = igc->txq_stats_map[i];
> -			if (map_id >= 0) {
> -				qstats->q_opackets[map_id] += queue_stats->pqgptc[i];
> -				qstats->q_obytes[map_id] += queue_stats->pqgotc[i];
> -			}
> -			/* Get RX queue statuses */
> -			map_id = igc->rxq_stats_map[i];
> -			if (map_id >= 0) {
> -				qstats->q_ipackets[map_id] += queue_stats->pqgprc[i];
> -				qstats->q_ibytes[map_id] += queue_stats->pqgorc[i];
> -				qstats->q_errors[map_id] += queue_stats->rqdpc[i];
> -			}
> +			qstats->q_opackets[i] += queue_stats->pqgptc[i];
> +			qstats->q_obytes[i] += queue_stats->pqgotc[i];
> +			qstats->q_ipackets[i] += queue_stats->pqgprc[i];
> +			qstats->q_ibytes[i] += queue_stats->pqgorc[i];
> +			qstats->q_errors[i] += queue_stats->rqdpc[i];

Can this code write beyond RTE_ETHDEV_QUEUE_STAT_CNTRS when 
IGC_QUEUE_PAIRS_NUM exceeds RTE_ETHDEV_QUEUE_STAT_CNTRS? A bounds check 
similar to the one in Patch 3 is needed here.

---

## Patch 5/7: net/ixgbe

> diff --git a/drivers/net/intel/ixgbe/ixgbe_ethdev.c b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> index 9dc015dfff..a7d995a768 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> @@ -886,96 +881,6 @@ ixgbe_reset_qstat_mappings(struct ixgbe_hw *hw)
>  	}
>  }
>  
> -
> -static int
> -ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
> [ ... ]

This patch correctly removes the queue stats mapping implementation and 
its restore function. The removal of ixgbe_restore_statistics_mapping() 
from the dev_start path is correct.

---

## Patch 6/7: net/txgbe

> diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
> index 5d360f8305..3886a2cbf4 100644
> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -2457,10 +2403,8 @@ txgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
>  {
>  	struct txgbe_hw *hw = TXGBE_DEV_HW(dev);
>  	struct txgbe_hw_stats *hw_stats = TXGBE_DEV_STATS(dev);
> -	struct txgbe_stat_mappings *stat_mappings =
> -			TXGBE_DEV_STAT_MAPPINGS(dev);
>  	struct txgbe_tx_queue *txq;
> -	uint32_t i, j;
> +	unsigned int i;

Good cleanup. The variable type change from uint32_t to unsigned int is 
appropriate for loop counters.

> @@ -2474,29 +2418,13 @@ txgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
>  	stats->obytes = hw_stats->tx_bytes;
>  
>  	if (qstats != NULL) {
> -		memset(&qstats->q_ipackets, 0, sizeof(qstats->q_ipackets));
> [ ... all the memsets snipped ... ]
>  		for (i = 0; i < TXGBE_MAX_QP; i++) {
> -			uint32_t n = i / NB_QMAP_FIELDS_PER_QSM_REG;
> [ ... ]
> +			if (i >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
> +				break;
> +			qstats->q_ipackets[i] += hw_stats->qp[i].rx_qp_packets;
> +			qstats->q_ibytes[i] += hw_stats->qp[i].rx_qp_bytes;
> +			qstats->q_opackets[i] += hw_stats->qp[i].tx_qp_packets;
> +			qstats->q_obytes[i] += hw_stats->qp[i].tx_qp_bytes;

Does removing the memset operations cause uninitialized data to be 
returned if TXGBE_MAX_QP < RTE_ETHDEV_QUEUE_STAT_CNTRS? The caller may be 
expecting all fields to be initialized.

---

## Patch 7/7: ethdev core

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index d0273e3f7b..e9bd5ba473 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -160,11 +160,6 @@ static const struct {
>  	{RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP, "FLOW_SHARED_OBJECT_KEEP"},
>  };
>  
> -enum {
> -	STAT_QMAP_TX = 0,
> -	STAT_QMAP_RX
> -};

This patch correctly removes the API functions, trace points, and 
documentation. The removal is complete and appropriate.

---

## Summary

The patch series correctly removes deprecated functionality, but has 
potential issues in patches 3, 4, and 6 regarding array bounds checking and 
initialization.