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

[Jakub Kicinski <kuba@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.
---
ibmveth: Add per-queue RX and TX statistics collection

This adds per-queue RX and TX counter arrays that are allocated at probe
and freed at remove, updates them from the poll/IRQ/xmit paths, and
exposes the values through ethtool -S plus a new ndo_get_stats64
implementation. It also adds the hcall_* counters to the ethtool string
table and reworks the probe error paths and ibmveth_remove() teardown
order.

A few questions below.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 36f0926711e60..3202b657c9b8f 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -98,7 +98,17 @@ static struct ibmveth_stat ibmveth_stats[] = {
>  	{ "fw_enabled_ipv6_csum", IBMVETH_STAT_OFF(fw_ipv6_csum_support) },
>  	{ "tx_large_packets", IBMVETH_STAT_OFF(tx_large_packets) },
>  	{ "rx_large_packets", IBMVETH_STAT_OFF(rx_large_packets) },
> -	{ "fw_enabled_large_send", IBMVETH_STAT_OFF(fw_large_send_support) }
> +	{ "fw_enabled_large_send", IBMVETH_STAT_OFF(fw_large_send_support) },
> +	{ "hcall_reg_lan_queue", IBMVETH_STAT_OFF(hcall_stats.reg_lan_queue) },
> +	{ "hcall_reg_lan", IBMVETH_STAT_OFF(hcall_stats.reg_lan) },
> +	{ "hcall_add_bufs_queue",
> +	  IBMVETH_STAT_OFF(hcall_stats.add_bufs_queue) },
> +	{ "hcall_add_bufs", IBMVETH_STAT_OFF(hcall_stats.add_bufs) },
> +	{ "hcall_add_buf", IBMVETH_STAT_OFF(hcall_stats.add_buf) },
> +	{ "hcall_free_lan_queue",
> +	  IBMVETH_STAT_OFF(hcall_stats.free_lan_queue) },
> +	{ "hcall_free_lan", IBMVETH_STAT_OFF(hcall_stats.free_lan) },
> +	{ "hcall_send_lan", IBMVETH_STAT_OFF(hcall_stats.send_lan) },
>  };

[Severity: Low]
Can these hcall_* values lose increments once they become user visible?

The fields in adapter->hcall_stats are plain u64 bumped with an
unserialized read-modify-write. ibmveth_replenish_task() takes only the
queue-local lock:

	spin_lock_irqsave(&rxq->replenish_lock, flags);

so two NAPI instances replenishing different RX queues both reach

	adapter->hcall_stats.add_bufs_queue++;

with no shared serialization, and separate TX queues both reach

	adapter->hcall_stats.send_lan++;

in ibmveth_send(). Two CPUs can read N and both store N+1.

Would per-queue counters summed on read (the same approach used for the
new rx_qstats/tx_qstats), or atomic64_t, be preferable here?

[ ... ]

> @@ -1011,7 +1075,23 @@ static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter,
>  	p = adapter->buffer_list_addr[queue_index] + 4096 - 8;
>  	drops = be64_to_cpup(p);
>  
> -	adapter->rx_no_buffer = drops;
> +	/*
> +	 * PHYP's buffer-list page counter is absolute for that page. A new
> +	 * page (reopen / queue reuse after -L) starts near zero; fold the
> +	 * previous absolute into retired so adapter-level sums stay
> +	 * monotonic.
> +	 */
> +	if (adapter->rx_qstats) {
> +		u64 *slot = &adapter->rx_qstats[queue_index].no_buffer_drops;
> +
> +		if (drops < *slot)
> +			adapter->rx_no_buffer_retired += *slot;
> +		*slot = drops;
> +	} else {
> +		if (drops < adapter->rx_no_buffer)
> +			adapter->rx_no_buffer_retired += adapter->rx_no_buffer;
> +		adapter->rx_no_buffer = drops;
> +	}
>  }

[Severity: Low]
Does the no_buffer_drops slot lose the final PHYP value when a queue is
retired by a channel shrink?

This makes rx_qstats[i].no_buffer_drops a mirror of the page-absolute
counter, and folding into rx_no_buffer_retired only happens when a later
read observes a decrease. ibmveth_update_rx_no_buffer() has only two call
sites, the replenish path and ibmveth_close(), and it early-returns for

	queue_index >= ibmveth_get_num_rx_queues(adapter)

Once the incremental resize patch later in this series lands, scale-down
does:

	ibmveth_publish_num_rx_queues(adapter, new_count);
	...
	for (i = new_count; i < old_count; i++)
		ibmveth_destroy_subordinate_rx_queue(adapter, i);

which reaches ibmveth_free_single_rx_queue() and dma_unmap_single() plus
free_page() on buffer_list_addr[i]. The live count is already lowered, so
a late harvest is rejected by the guard above, and everything PHYP
recorded in that page since the last replenish read is dropped from both
the per-queue slot and the adapter sum.

Would harvesting the absolute before lowering the published queue count
and freeing the page fix this?

> @@ -2239,22 +2319,158 @@ static int ibmveth_set_features(struct net_device *dev,

[ ... ]

>  static void ibmveth_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>  {
> +	struct ibmveth_adapter *adapter = netdev_priv(dev);
> +	u8 *p = data;
>  	int i;
>  
>  	if (stringset != ETH_SS_STATS)
>  		return;
>  
> -	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++, data += ETH_GSTRING_LEN)
> -		memcpy(data, ibmveth_stats[i].name, ETH_GSTRING_LEN);
> +	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++) {
> +		memcpy(p, ibmveth_stats[i].name, ETH_GSTRING_LEN);
> +		p += ETH_GSTRING_LEN;
> +	}
> +
> +	for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++) {
> +		ethtool_sprintf(&p, "rx%d_packets", i);
> +		ethtool_sprintf(&p, "rx%d_bytes", i);
> +		ethtool_sprintf(&p, "rx%d_interrupts", i);
> +		ethtool_sprintf(&p, "rx%d_polls", i);
> +		ethtool_sprintf(&p, "rx%d_large_packets", i);
> +		ethtool_sprintf(&p, "rx%d_invalid_buffers", i);
> +		ethtool_sprintf(&p, "rx%d_no_buffer_drops", i);
> +	}
> +
> +	for (i = 0; i < dev->real_num_tx_queues; i++) {
> +		ethtool_sprintf(&p, "tx%d_packets", i);
> +		ethtool_sprintf(&p, "tx%d_bytes", i);
> +		ethtool_sprintf(&p, "tx%d_large_packets", i);
> +		ethtool_sprintf(&p, "tx%d_dropped_packets", i);
> +		ethtool_sprintf(&p, "tx%d_send_failures", i);
> +		ethtool_sprintf(&p, "tx%d_checksum_offload", i);
> +	}

[Severity: Medium]
Should the per-queue packet, byte and drop counters go through the
standard per-queue statistics interface instead of private ethtool
strings?

rx%d_packets, rx%d_bytes, tx%d_packets, tx%d_bytes and
tx%d_dropped_packets map directly onto existing fields:

include/net/netdev_queues.h
	struct netdev_stat_ops {
		void (*get_queue_stats_rx)(struct net_device *dev, int idx,
					   struct netdev_queue_stats_rx *stats);
		...

The driver adds only .ndo_get_stats64 (device-wide) and never sets
netdev->stat_ops, so the newly collected per-queue values are reachable
only through the private ethtool blob, which cannot be removed once
shipped. The genuinely driver-specific counters (interrupts, polls,
invalid_buffers, no_buffer_drops, send_failures, checksum_offload) look
fine in ethtool -S.

Could the packets/bytes/dropped set be exposed via netdev_stat_ops
qstats instead?

> +
> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> +		ethtool_sprintf(&p, "pool%d_size", i);
> +		ethtool_sprintf(&p, "pool%d_active", i);
> +		ethtool_sprintf(&p, "pool%d_available", i);
> +	}
>  }
>  
>  static int ibmveth_get_sset_count(struct net_device *dev, int sset)
>  {
> +	struct ibmveth_adapter *adapter = netdev_priv(dev);
> +
>  	switch (sset) {
>  	case ETH_SS_STATS:
> -		return ARRAY_SIZE(ibmveth_stats);
> +		return ARRAY_SIZE(ibmveth_stats) +
> +		       ibmveth_get_num_rx_queues(adapter) *
> +		       IBMVETH_NUM_RX_QSTATS +
> +		       dev->real_num_tx_queues * IBMVETH_NUM_TX_QSTATS +
> +		       IBMVETH_NUM_BUFF_POOLS * 3;
>  	default:
>  		return -EOPNOTSUPP;
>  	}

> @@ -2263,11 +2479,44 @@ static int ibmveth_get_sset_count(struct net_device *dev, int sset)
>  static void ibmveth_get_ethtool_stats(struct net_device *dev,
>  				      struct ethtool_stats *stats, u64 *data)
>  {

[ ... ]

> +	for (j = 0; j < IBMVETH_NUM_BUFF_POOLS; j++) {
> +		data[i++] = adapter->rx_buff_pool[0][j].size;
> +		data[i++] = adapter->rx_buff_pool[0][j].active;
> +		data[i++] = atomic_read(&adapter->rx_buff_pool[0][j].available);
> +	}
>  }

[Severity: Medium]
This is a pre-existing issue rather than something this patch creates for
the lifetime part below, but the commit message describes only per-queue
RX/TX statistics and the hcall_* strings, while the patch also carries two
other changes.

First, three new ethtool -S entries per buffer pool are added:

	ethtool_sprintf(&p, "pool%d_size", i);
	ethtool_sprintf(&p, "pool%d_active", i);
	ethtool_sprintf(&p, "pool%d_available", i);

and counted as IBMVETH_NUM_BUFF_POOLS * 3 in ibmveth_get_sset_count().
The values are read only from adapter->rx_buff_pool[0][j], but
rx_buff_pool is declared as

	struct ibmveth_buff_pool
		rx_buff_pool[IBMVETH_MAX_RX_QUEUES][IBMVETH_NUM_BUFF_POOLS];

so in multi-queue mode queues 1..N-1 are not represented and the string
names carry no queue index. pool size and active are also already
available through the per-pool sysfs attributes (veth_size_attr,
veth_num_attr, veth_active_attr). Should these strings be dropped, or at
least given a queue index and mentioned in the commit message?

Second, ibmveth_remove() reorders unregister_netdev() ahead of
cancel_work_sync(), and the new ibmveth_probe_cleanup() clears vio
drvdata before free_netdev(). The in-code comments describe both as
fixing existing problems (a worker running after free_netdev(), and CMO
get_desired_dma() seeing a freed netdev on rebind), neither of which the
commit message body mentions and neither of which has a Fixes: tag.
Could these be split into their own patches?

[ ... ]

> @@ -3107,6 +3410,7 @@ static const struct net_device_ops ibmveth_netdev_ops = {
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_set_mac_address    = ibmveth_set_mac_addr,
>  	.ndo_features_check	= ibmveth_features_check,
> +	.ndo_get_stats64	= ibmveth_get_stats64,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller	= ibmveth_poll_controller,
>  #endif

> @@ -3121,6 +3425,23 @@ static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter,
>  		kobject_put(&adapter->rx_buff_pool[0][i].kobj);
>  }
>  
> +static void ibmveth_probe_cleanup(struct ibmveth_adapter *adapter,
> +				  int pools_ready)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +
> +	cancel_work_sync(&adapter->work);
> +	ibmveth_put_pool_kobjs(adapter, pools_ready);
> +
> +	ibmveth_free_tx_qstats(adapter);
> +	ibmveth_free_rx_qstats(adapter);
> +	/* Probe failure never reaches ibmveth_remove(); clear before free so
> +	 * CMO get_desired_dma() cannot see a freed netdev on rebind.
> +	 */
> +	dev_set_drvdata(&adapter->vdev->dev, NULL);
> +	free_netdev(netdev);
> +}
> +

[ ... ]

> @@ -3332,12 +3651,20 @@ static void ibmveth_remove(struct vio_dev *dev)
>  	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>  	int i;
>  
> -	cancel_work_sync(&adapter->work);
> -
>  	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
>  		kobject_put(&adapter->rx_buff_pool[0][i].kobj);
>  
> +	/*
> +	 * Unregister first so NAPI/xmit cannot re-arm reset work after we
> +	 * cancel it. cancel_work_sync() before unregister left a window
> +	 * where poll could schedule_work() and the worker ran after
> +	 * free_netdev().
> +	 */
>  	unregister_netdev(netdev);
> +	cancel_work_sync(&adapter->work);
> +
> +	ibmveth_free_tx_qstats(adapter);
> +	ibmveth_free_rx_qstats(adapter);
>  
>  	free_netdev(netdev);
>  	dev_set_drvdata(&dev->dev, NULL);

[Severity: High]
With cancel_work_sync() now running after unregister_netdev(), can the
reset worker re-open an already unregistered netdev?

adapter->work is queued from several error paths, for example
ibmveth_replenish_task() on an invalid free_map entry or on MQ H_FUNCTION
after LPM, and from ibmveth_poll_skip_bad_correlator():

	schedule_work(&adapter->work);

The handler has no registration or running check:

drivers/net/ethernet/ibm/ibmveth.c:ibmveth_reset() {
	rtnl_lock();

	dev_close(adapter->netdev);
	dev_open(adapter->netdev, NULL);

	rtnl_unlock();
}

A worker already queued (or already blocked in rtnl_lock() while
unregister_netdev() holds rtnl) proceeds once unregistration finishes.
dev_close() is then a no-op, and the re-open is not rejected:

net/core/dev.c:netif_open() {
	if (dev->flags & IFF_UP)
		return 0;

	ret = __dev_open(dev, extack);
}

net/core/dev.c:__dev_open() {
	ASSERT_RTNL();
	dev_addr_check(dev);

	if (!netif_device_present(dev)) {
	...
	if (!ret && ops->ndo_open)
		ret = ops->ndo_open(dev);
}

IFF_UP was cleared by unregister and __LINK_STATE_PRESENT is never
cleared (the driver does not call netif_device_detach()), so
ibmveth_open() runs again on the unregistered device: it re-requests the
RX IRQs with dev_id = &adapter->napi[i], re-registers the logical LAN
with PHYP, and re-arms DMA into freshly allocated buffer lists.

cancel_work_sync() then returns and remove() continues into
ibmveth_free_tx_qstats() / ibmveth_free_rx_qstats() and free_netdev(),
so the installed IRQ handler and the hypervisor are left pointing at
freed memory. The next RX interrupt would execute

	adapter->rx_qstats[qindex].interrupts++;

in ibmveth_interrupt() against a freed adapter and a freed qstat array.

Would a netif_running() or reg_state check inside ibmveth_reset(), or a
"removing" flag set before unregister_netdev(), close this?