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

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

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

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

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