Re: [PATCH net-next v3 11/15] ibmveth: Add per-queue RX and TX statistics collection and reporting
From: Simon Horman <horms@kernel.org>
Date: 2026-07-14 13:00:05
Also in:
linuxppc-dev
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 --- ibmveth: Add per-queue RX and TX statistics collection and reporting This commit introduces per-queue statistics tracking for both receive and transmit paths in the ibmveth driver. The counters are aggregated for legacy interfaces and exposed individually via ethtool stringsets.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c index 863e5c68b42c7..4e3f49b6346fd 100644 --- a/drivers/net/ethernet/ibm/ibmveth.c +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1655,6 +1655,10 @@ static int ibmveth_open(struct net_device *netdev) if (rc) goto out_cleanup_rx_interrupts; + rc = ibmveth_alloc_tx_qstats(adapter); + if (rc) + goto out_free_tx_resources; + netif_tx_start_all_queues(netdev); netdev_dbg(netdev, "open complete\n");
[ ... ]
out_cleanup_rx_interrupts: ibmveth_cleanup_rx_interrupts(adapter); out_free_tx_resources: ibmveth_free_tx_resources(adapter);
[Severity: High] Does the goto out_free_tx_resources on ibmveth_alloc_tx_qstats() failure bypass the cleanup for RX interrupts? Since out_free_tx_resources is physically below out_cleanup_rx_interrupts in ibmveth_open(), failing here leaves the IRQs requested and NAPI enabled. If an interrupt subsequently fires, could the handler access the freed RX queue memory and cause a kernel panic? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1699,6 +1699,7 @@ static int ibmveth_close(struct net_device *netdev) } } + ibmveth_free_tx_qstats(adapter); ibmveth_free_tx_resources(adapter); ibmveth_cleanup_rx_interrupts(adapter); ibmveth_update_rx_no_buffer(adapter);
[ ... ]
ibmveth_free_rx_qstats(adapter);
[Severity: High] If ibmveth_close() is called to bring down the interface, does this unconditionally free adapter->tx_qstats and adapter->rx_qstats without a grace period? Since ibmveth_get_stats64() can be called concurrently under RCU, and ibmveth_start_xmit() can run concurrently if the datapath is not completely synchronized, could this lead to a use-after-free or NULL pointer dereference when these functions access the freed arrays? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1967,22 +1967,107 @@ static int ibmveth_set_features(struct net_device *dev,
[ ... ]
+static void ibmveth_aggregate_rx_qstats(struct ibmveth_adapter *adapter)
+{
+ u64 total_invalid = 0;
+ u64 total_large = 0;
+ int i;
+
+ if (!adapter->rx_qstats)
+ return;
+
+ for (i = 0; i < adapter->num_rx_queues; i++) {
+ total_invalid += adapter->rx_qstats[i].invalid_buffers;
+ total_large += adapter->rx_qstats[i].large_packets;
+ }
+
+ adapter->rx_invalid_buffer = total_invalid;
+ adapter->rx_large_packets = total_large;
+}[Severity: High] Does overwriting adapter->rx_invalid_buffer and other persistent global counters with the dynamically allocated adapter->rx_qstats values cause a permanent loss of interface statistics? When the interface is brought down and back up, ibmveth_close() frees the queue stats arrays and ibmveth_open() reallocates them as zeroed. Will this cause all historical statistics to be wiped out on every interface reset, breaking standard persistence behavior? [ ... ]
+static void ibmveth_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{[ ... ]
+ for (i = 0; i < adapter->num_rx_queues; i++) {
+ ethtool_sprintf(&p, "rx%d_packets", i);
+ ethtool_sprintf(&p, "rx%d_bytes", i);[Severity: Low] Are these private ethtool stringsets duplicating standard kernel statistics? According to the subsystem guidelines, per-queue packet and byte statistics that have a standard UAPI representation (like struct netdev_queue_stats_rx exported via netlink) should not be duplicated as private ethtool strings.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h index f7b20fd01acb5..390c660af979e 100644 --- a/drivers/net/ethernet/ibm/ibmveth.h +++ b/drivers/net/ethernet/ibm/ibmveth.h@@ -316,9 +316,21 @@ struct ibmveth_rx_queue_stats { u64 no_buffer_drops; }; +struct ibmveth_tx_queue_stats { + u64 packets; + u64 bytes; + u64 large_packets; + u64 dropped_packets; + u64 send_failures; + u64 checksum_offload; +};
[Severity: Medium] Will this structure layout prevent cache line false sharing? The commit message mentions tracking transmit counters per TX queue to avoid cache line contention, but struct ibmveth_tx_queue_stats appears to be exactly 48 bytes and lacks cacheline alignment annotations like ____cacheline_aligned. When allocated as a contiguous array, will multiple CPUs processing different queues end up writing to the same hardware cacheline?