Re: [PATCH net-next v3 11/15] ibmveth: Add per-queue RX and TX statistics collection and reporting

[mingming cao <mmc@linux.ibm.com>]

On 7/14/26 5:59 AM, Simon Horman wrote:
> 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.
>
>> 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
> [ ... ]
>> @@ -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?
>
> [ ... ]
*

  Confirmed — that goto skips RX interrupt cleanup. Will fix the

  open() unwind order in v4 so TX-stat allocation failure still

  disables NAPI and frees IRQs before tearing down queues.

*
>> @@ -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?
Confirmed.
>
> 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?
>
> [ ... ]
*

  Good point. ndo_get_stats64 can run without RTNL. In v4 I’ll give

  the qstat arrays adapter lifetime (probe/remove) instead of

  open/close, so close cannot free them under concurrent readers.

*
>> @@ -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?
yes
> 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?
>
> [ ... ]
*

  Agreed this regresses persistence vs pre-MQ.

**

In v4 per-queue qstats will stay for the adapter lifetime, and adapter-level

ethtool strings are summed on read (ibmvnic-style). No dual-updating

globals on the hot path, and no aggregate_* that wipes history on reopen.

*

>> +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.
*

  Thanks — **

*I can move those to standard*

*

netdev queue stats and drop the ethtool duplicates in a follow-up. For

now I'll keep device-specific counters (interrupts, polls, invalid, …)

as private ethtool strings.

*


*
>> 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?


*Will add cacheline alignment for the per-queue stats structs in v4*

*Thanks again for your valuable feedback!*

*Mingming*