* Re: [PATCH net v2] ixgbe: implement get_queue_stats_rx
2026-05-26 7:47 [PATCH net v2] ixgbe: implement get_queue_stats_rx Kshitiz Bartariya
@ 2026-05-27 21:08 ` Jacob Keller
2026-05-28 8:54 ` [Intel-wired-lan] " Loktionov, Aleksandr
2026-05-28 17:26 ` Simon Horman
2 siblings, 0 replies; 4+ messages in thread
From: Jacob Keller @ 2026-05-27 21:08 UTC (permalink / raw)
To: Kshitiz Bartariya, kuba, anthony.l.nguyen, przemyslaw.kitszel,
andrew+netdev, davem, edumazet, pabeni
Cc: intel-wired-lan, netdev, linux-kernel
On 5/26/2026 12:47 AM, Kshitiz Bartariya wrote:
> Hook into the netdev_stat_ops interface to expose per RX queue
> statistics through the netdev generic netlink API.
>
> The following counters are filled:
>
> - 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().
>
> Signed-off-by: Kshitiz Bartariya <kshitiz.bartariya@zohomail.in>
This probably should target net-next since its not a regression but a
new feature implementation. For future contributions, you should check
the guidelines at Documentation/process/maintainer-netdev.rst,
particularly the section describing the differences between the net and
net-next trees. Intel networking patches do also have their own mailing
list (Intel Wired LAN) which we use to have our validation team do
explicit testing on patches before we submit to netdev. Not being a long
standing contributor, its quite understandable you might not be aware of
that.
The implementation seems fine, and this is a nice improvement to get the
driver supporting the more modern stat interfaces. Thanks!
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> v2:
> - Removed setting of hw_gro counters as ixgbe doesn't advertise it
> - Removed idx and ring checks as they are already checked against
> real_num_rx_queues
> Thanks Jakub Kicinski for the review comments.
>
> v1: https://lore.kernel.org/lkml/20260523144022.36484-1-kshitiz.bartariya@zohomail.in/
>
>
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 2646ee6f295f..2184213727c7 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];
> +
> + 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 const struct netdev_stat_ops ixgbe_stat_ops = {
> + .get_queue_stats_rx = ixgbe_get_queue_stats_rx,
> +};
> +
> static int ixgbe_ndo_get_vf_stats(struct net_device *netdev, int vf,
> struct ifla_vf_stats *vf_stats)
> {
> @@ -11643,6 +11658,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));
^ permalink raw reply [flat|nested] 4+ messages in thread* RE: [Intel-wired-lan] [PATCH net v2] ixgbe: implement get_queue_stats_rx
2026-05-26 7:47 [PATCH net v2] ixgbe: implement get_queue_stats_rx Kshitiz Bartariya
2026-05-27 21:08 ` Jacob Keller
@ 2026-05-28 8:54 ` Loktionov, Aleksandr
2026-05-28 17:26 ` Simon Horman
2 siblings, 0 replies; 4+ messages in thread
From: Loktionov, Aleksandr @ 2026-05-28 8:54 UTC (permalink / raw)
To: Kshitiz Bartariya, kuba@kernel.org, Nguyen, Anthony L,
Kitszel, Przemyslaw, andrew+netdev@lunn.ch, davem@davemloft.net,
edumazet@google.com, pabeni@redhat.com
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kshitiz Bartariya via Intel-wired-lan
> Sent: Tuesday, May 26, 2026 9:48 AM
> To: kuba@kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>;
> andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> pabeni@redhat.com
> Cc: Kshitiz Bartariya <kshitiz.bartariya@zohomail.in>; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH net v2] ixgbe: implement
> get_queue_stats_rx
>
> Hook into the netdev_stat_ops interface to expose per RX queue
> statistics through the netdev generic netlink API.
>
> The following counters are filled:
>
> - 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().
>
> Signed-off-by: Kshitiz Bartariya <kshitiz.bartariya@zohomail.in>
> ---
> v2:
> - Removed setting of hw_gro counters as ixgbe doesn't advertise it
> - Removed idx and ring checks as they are already checked against
> real_num_rx_queues
> Thanks Jakub Kicinski for the review comments.
>
> v1: https://lore.kernel.org/lkml/20260523144022.36484-1-
> kshitiz.bartariya@zohomail.in/
>
>
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 2646ee6f295f..2184213727c7 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];
> +
> + 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 const struct netdev_stat_ops ixgbe_stat_ops = {
> + .get_queue_stats_rx = ixgbe_get_queue_stats_rx,
> +};
> +
> static int ixgbe_ndo_get_vf_stats(struct net_device *netdev, int vf,
> struct ifla_vf_stats *vf_stats)
> {
> @@ -11643,6 +11658,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)
I think it should go to net-next branch
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net v2] ixgbe: implement get_queue_stats_rx
2026-05-26 7:47 [PATCH net v2] ixgbe: implement get_queue_stats_rx Kshitiz Bartariya
2026-05-27 21:08 ` Jacob Keller
2026-05-28 8:54 ` [Intel-wired-lan] " Loktionov, Aleksandr
@ 2026-05-28 17:26 ` Simon Horman
2 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2026-05-28 17:26 UTC (permalink / raw)
To: kshitiz.bartariya
Cc: 'Simon Horman', kuba, anthony.l.nguyen,
przemyslaw.kitszel, andrew+netdev, davem, edumazet, pabeni,
intel-wired-lan, netdev, linux-kernel
From: 'Simon Horman' <horms@kernel.org>
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.
> +}
> +
^ permalink raw reply [flat|nested] 4+ messages in thread