[PATCH net-next v4] ixgbe: implement get_queue_stats_rx

[PATCH net-next v4] ixgbe: implement get_queue_stats_rx

[Kshitiz Bartariya]

Hook into the netdev_stat_ops interface to expose per RX queue
statistics through the netdev generic netlink API.

The following counters are filled:
 - bytes: maps directly to bytes
 - packets: maps directly to packets
 - alloc_fail: sum of alloc_rx_page_failed and alloc_rx_buff_failed
 - csum_bad: maps directly to csum_err, which is incremented for both
   IP header and L4 checksum errors in ixgbe_rx_checksum().

The new per-queue stats can be observed with:
  $ ynltool qstats show scope queue

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Signed-off-by: Kshitiz Bartariya <kshitiz.bartariya@zohomail.in>
---
v4:
 - Changed comment format from // to /* */
 - Moved ixgbe_stat_ops declaration next to the ixgbe_netdev_ops
 Suggested by Jedrzej Jagielski.

v3:
 - Added bytes and packets stats counters
 - Implemented ixgbe_get_base_stats function
 As suggested by AI on 
 https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260603174857.78666-1-kshitiz.bartariya%40zohomail.in
 https://lore.kernel.org/lkml/20260612084605.19785-1-kshitiz.bartariya@zohomail.in/

v2:
 Amended commit message with command to get RX queue stats as 
 suggested by Jedrzej Jagielski.
 https://lore.kernel.org/lkml/20260603174857.78666-1-kshitiz.bartariya@zohomail.in/

v1: 
 https://lore.kernel.org/lkml/20260602100932.21838-1-kshitiz.bartariya@zohomail.in/

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index bc16e4c93fd4..67844e25af23 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9759,6 +9759,30 @@ static void ixgbe_get_stats64(struct net_device *netdev,
 	stats->rx_missed_errors	= netdev->stats.rx_missed_errors;
 }
 
+static void ixgbe_get_queue_stats_rx(struct net_device *dev, int idx,
+				     struct netdev_queue_stats_rx *stats)
+{
+	struct ixgbe_adapter *adapter = ixgbe_from_netdev(dev);
+	struct ixgbe_ring *ring = adapter->rx_ring[idx];
+
+	stats->bytes = ring->stats.bytes;
+	stats->packets = ring->stats.packets;
+	stats->alloc_fail = ring->rx_stats.alloc_rx_page_failed +
+			    ring->rx_stats.alloc_rx_buff_failed;
+	stats->csum_bad = ring->rx_stats.csum_err;
+}
+
+static void ixgbe_get_base_stats(struct net_device *dev,
+				 struct netdev_queue_stats_rx *rx,
+				 struct netdev_queue_stats_tx *tx)
+{
+	/* ixgbe has no inactive queues */
+	rx->bytes = 0;
+	rx->packets = 0;
+	rx->alloc_fail = 0;
+	rx->csum_bad = 0;
+}
+
 static int ixgbe_ndo_get_vf_stats(struct net_device *netdev, int vf,
 				  struct ifla_vf_stats *vf_stats)
 {
@@ -11116,6 +11140,11 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_hwtstamp_set	= ixgbe_ptp_hwtstamp_set,
 };
 
+static const struct netdev_stat_ops ixgbe_stat_ops = {
+	.get_queue_stats_rx = ixgbe_get_queue_stats_rx,
+	.get_base_stats = ixgbe_get_base_stats,
+};
+
 static void ixgbe_disable_txr_hw(struct ixgbe_adapter *adapter,
 				 struct ixgbe_ring *tx_ring)
 {
@@ -11662,6 +11691,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	hw->phy.mdio.mdio_write = ixgbe_mdio_write;
 
 	netdev->netdev_ops = &ixgbe_netdev_ops;
+	netdev->stat_ops = &ixgbe_stat_ops;
 	ixgbe_set_ethtool_ops(netdev);
 	netdev->watchdog_timeo = 5 * HZ;
 	strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name));
-- 
2.50.1 (Apple Git-155)

Re: [Intel-wired-lan] [PATCH net-next v4] ixgbe: implement get_queue_stats_rx

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
ixgbe: implement get_queue_stats_rx

This patch wires ixgbe into the netdev_stat_ops interface so that per-RX
queue counters (bytes, packets, alloc_fail, csum_bad) are exported
through the netdev generic netlink API.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index bc16e4c93fd4..67844e25af23 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -9759,6 +9759,30 @@ static void ixgbe_get_stats64(struct net_device *netdev,
>  	stats->rx_missed_errors	= netdev->stats.rx_missed_errors;
>  }
>  
> +static void ixgbe_get_queue_stats_rx(struct net_device *dev, int idx,
> +				     struct netdev_queue_stats_rx *stats)
> +{
> +	struct ixgbe_adapter *adapter = ixgbe_from_netdev(dev);
> +	struct ixgbe_ring *ring = adapter->rx_ring[idx];
> +

[High]
Can adapter->rx_ring[idx] be observed NULL here?

Elsewhere in the same driver, this slot is treated as nullable. The
writer side uses WRITE_ONCE() in both directions, for example in
ixgbe_free_q_vector():

    WRITE_ONCE(adapter->rx_ring[ring->queue_index], NULL);

and the existing readers in ixgbe_main.c pair that with READ_ONCE() and
a NULL guard, e.g. ixgbe_get_stats64():

    struct ixgbe_ring *ring = READ_ONCE(adapter->rx_ring[i]);
    if (ring) {
        ...
    }

and ixgbe_update_stats():

    struct ixgbe_ring *rx_ring = READ_ONCE(adapter->rx_ring[i]);
    if (!rx_ring)
        continue;

If a userspace caller issues a per-queue qstats request while the driver
is reconfiguring queues (e.g. ixgbe_clear_interrupt_scheme() /
ixgbe_init_interrupt_scheme() during reset), would a plain dereference
here oops?

Should this callback follow the same READ_ONCE() plus NULL-skip pattern
used in ixgbe_get_stats64() and ixgbe_update_stats()?

> +	stats->bytes = ring->stats.bytes;
> +	stats->packets = ring->stats.packets;

[Medium]
Should these 64-bit reads be wrapped in a u64_stats_sync read section?

The writer side, ixgbe_update_rx_ring_stats(), updates these counters
under ring->syncp:

    u64_stats_update_begin(&rx_ring->syncp);
    rx_ring->stats.packets += total_packets;
    rx_ring->stats.bytes += total_bytes;
    u64_stats_update_end(&rx_ring->syncp);

and the syncp is initialized at probe (u64_stats_init() on ring->syncp).
ixgbe_get_stats64() in this same file already mirrors that on the read
side:

    do {
        start = u64_stats_fetch_begin(&ring->syncp);
        packets = ring->stats.packets;
        bytes   = ring->stats.bytes;
    } while (u64_stats_fetch_retry(&ring->syncp, start));

On 32-bit kernels a 64-bit load is not atomic, so without the fetch
loop can the values returned to userspace be torn (high half from one
update, low half from another)?

> +	stats->alloc_fail = ring->rx_stats.alloc_rx_page_failed +
> +			    ring->rx_stats.alloc_rx_buff_failed;
> +	stats->csum_bad = ring->rx_stats.csum_err;
> +}
> +

[ ... ]