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

[Kshitiz Bartariya]

Hi Simon,

For the review generated by AI, I had done changes on that line previously but Jakub Kicinski <kuba@xxxxxxxxxx> pointed out that such checks are redundant https://lore.kernel.org/lkml/20260525085356.5c336491@xxxxxxxxxx/ .

Kindly let me know according to whom I've to make the final changes.

Thanks,
Kshitiz

From: Simon Horman <horms@xxxxxxxxxx>
To: <intel-wired-lan@xxxxxxxxxx>
Cc: "Simon Horman"<horms@xxxxxxxxxx>, <anthony.l.nguyen@xxxxxxxxx>, <przemyslaw.kitszel@xxxxxxxxx>, <andrew+netdev@xxxxxxx>, <davem@xxxxxxxxxxxxx>, <edumazet@xxxxxxxxxx>, <kuba@xxxxxxxxxx>, <pabeni@xxxxxxxxxx>, <jedrzej.jagielski@xxxxxxxxx>, <kshitiz.bartariya@xxxxxxxxxxx>, <intel-wired-lan@xxxxxxxxxxxxxxxx>, <netdev@xxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>
Date: Mon, 15 Jun 2026 17:28:06 +0530
Subject: Re: [Intel-wired-lan] [PATCH net-next v4] ixgbe: implement get_queue_stats_rx

 > 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;
 > > +}
 > > +
 > 
 > [ ... ]
 >