From: 'Simon Horman' <[email protected]>
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://sashiko.dev
---
ixgbe: implement get_queue_stats_rx
This commit hooks into the netdev_stat_ops interface to expose per RX queue
statistics 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 2646ee6f295f0..2184213727c7d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[ ... ]
> @@ -9740,6 +9740,21 @@ 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];
> +
Simon says: The issue flagged below does seem to warrant investigation.
[Severity: High]
Does this code risk a NULL pointer dereference?
The generic netlink queue stat dump executes under RCU without holding the
rtnl_lock. During device reconfigurations (e.g., changing the number of channels
via ethtool -L), the driver temporarily tears down the queues and sets
adapter->rx_ring[idx] to NULL.
Because this happens while the interface is still marked up and before
dev->real_num_rx_queues is updated, a concurrent queue stats dump could invoke
ixgbe_get_queue_stats_rx() while the ring pointer is NULL.
> + 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;
Simon says: While it seems to me that the heart of the issue flagged below is
a subjective decision around the scope of this work.
[Severity: Medium]
Are we missing the packet and byte counts here?
The driver tracks these fundamental metrics in ring->stats.packets and
ring->stats.bytes, and they can be safely read using the ring->syncp
u64_stats_sync lock.
Since they are omitted from struct netdev_queue_stats_rx, the generic netlink
API will treat them as NETDEV_STAT_NOT_SET, leaving userspace tools without
packet and byte counts for the RX queues.
> +}
> +
