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

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.

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

[ ... ]

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

[ ... ]

 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;
 	}

[ ... ]

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

[ ... ]

[ ... ]

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