Re: [Intel-wired-lan] [PATCH net-next v4] ixgbe: implement get_queue_stats_rx
From: Simon Horman <horms@kernel.org>
Date: 2026-06-15 11:58:26
Also in:
intel-wired-lan, lkml
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.
quoted hunk ↗ jump to hunk
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; +} +
[ ... ]