Re: [PATCH net-next v4 09/14] ibmveth: Enable multi-queue RX receive path

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: Enable multi-queue RX receive path

This patch turns on the multi-queue RX path: probe latches multi_queue and
num_rx_queues from the H_ILLAN_ATTRIBUTES MQ bit, subordinate queues are
registered with H_REG_LOGICAL_LAN_QUEUE, and open replenishes every queue
before setup_rx_interrupts() unmasks PHYP. A few questions below on the
new ordering, the CMO entitlement accounting and the probe unwind.

[ ... ]

[Severity: Low]
Should hcall_send_lan be exported through ethtool -S?  It is incremented
once per successfully transmitted packet on the ibmveth_send() success
path:

drivers/net/ethernet/ibm/ibmveth.c:ibmveth_send() {
	...
	adapter->hcall_stats.send_lan++;
	return 0;
}

That makes it the same value as the standard rtnl_link_stats64 tx_packets
counter reported by ndo_get_stats64 / ip -s link, and
Documentation/networking/statistics.rst asks that counters with a standard
uAPI not be duplicated as driver-private ethtool -S strings.  These
strings become uAPI once merged.

The commit message subject is "Enable multi-queue RX receive path" and
does not mention that eight new statistics strings are being added.  Could
the stats additions be split out or at least described?

[Severity: Low]
This isn't a bug, but do the "scale-down and scale-up fail paths" exist at
this commit?  ibmveth_resize_rx_queues_incremental() and
ibmveth_setup_single_rx_interrupt() arrive in a later patch, and the only
caller here is the bulk helper bounded by num_rx_queues.

The same applies to the comment added above ibmveth_get_strings() in this
patch:

	/*
	 * Sum per-queue counters for rare ethtool reads. Do not write adapter
	 * globals on the hot path (ibmvnic-style); with qstats allocated for the
	 * adapter lifetime, these sums remain meaningful across ifdown/up.
	 */

ibmveth_get_strings() only memcpy()s string tables here, and no
rx_qstats/tx_qstats members exist in struct ibmveth_adapter until the
following patch.  Could both comments move to the patches that add the
code they describe, so the intermediate commits read correctly?

+	for (i = 0; i < num; i++)
+		napi_enable(&adapter->napi[i]);
+
+	if (adapter->multi_queue && num > 1) {
+		for (i = 0; i < num; i++) {
+			rc = ibmveth_enable_irq(adapter, i);
+			if (rc) {
+				netdev_err(netdev,
+					   "Failed to enable IRQ for queue %d, rc=%d\n",
+					   i, rc);
+				while (--i >= 0)
+					ibmveth_disable_irq(adapter, i);
+				rc = -EIO;
+				goto err_disable_napi;
+			}
+		}
+	}
+
 	return 0;
 
+err_disable_napi:
+	/* PHYP unmask was rolled back above; disable NAPI before free_irq */
+	for (i = 0; i < num; i++)
+		napi_disable(&adapter->napi[i]);
+	for (i = 0; i < num; i++) {
+		if (adapter->queue_irq[i])
+			free_irq(adapter->queue_irq[i], &adapter->napi[i]);
+	}
+	goto err_dispose_mappings;

[Severity: Low]
Can the rollback loop above be undone by an in-flight poll?  Queues 0..i-1
were already unmasked, so a poll may be running when
ibmveth_enable_irq() fails for queue i:

ibmveth_poll() {
	...
	if (!napi_complete_done(napi, frames_processed))
		goto out;
	...
	lpar_rc = ibmveth_enable_irq(adapter, queue_index);
}

There is no synchronize_irq() between the while (--i >= 0)
ibmveth_disable_irq() loop and napi_disable(), so a poll that completes
after the mask re-enables PHYP delivery.  napi_disable() waits for the
poll but does not undo that re-enable, and the following free_irq() plus
ibmveth_dispose_subordinate_irq_mappings() then leave the hypervisor
source enabled for a hwirq whose Linux mapping is gone.

The scale-down path added later does disable_irq() followed by
synchronize_irq() for exactly this reason.  Should this path do the same?

[ ... ]

[Severity: High]
Is this cross-queue aggregation safe against the RX queue resize path?
The only lock held here is the caller's own per-queue lock:

ibmveth_replenish_task(adapter, queue_index) {
	spin_lock_irqsave(&rxq->replenish_lock, flags);
	...
	ibmveth_update_rx_no_buffer(adapter);
	spin_unlock_irqrestore(&rxq->replenish_lock, flags);
}

That lock does not cover other queues' buffer_list_addr[] entries, nor
num_rx_queues.  The ethtool -L scale-down path added later in the series
publishes the new count and then frees the pages:

	adapter->num_rx_queues = new_count;

	for (i = new_count; i < old_count; i++) {
		...
		ibmveth_free_single_rx_queue(adapter, i);   /* free_page() + NULL */
	}

If a surviving queue's poll already loaded the older, larger
num_rx_queues, can this loop dereference NULL + 4096 - 8, or read a page
that was just freed, from softirq context?  A READ_ONCE() of the count
plus a NULL check, or serializing the aggregation against the resize
path, would make this explicit.

Separately, two concurrent polls on different queues both execute
adapter->rx_no_buffer = 0 followed by += drops on the same field.  Can
that lose increments and expose a transient 0 through
rx_no_buffer / ethtool -S?

[Severity: Low]
With several RX queues now polling concurrently, do the adapter-wide plain
u64 counters need serialization?  replenish_task_cycles is incremented
before the per-queue lock is taken, and replenish_no_mem,
replenish_add_buff_success/failure and the new hcall_stats fields are
touched under different per-queue locks, so the lockset intersection for
these adapter-global fields is empty.  hcall_stats.send_lan is also
incremented from ibmveth_start_xmit() on up to 16 TX queues in parallel.
The visible effect is lost increments in the ethtool -S values.

[ ... ]

[Severity: Low]
Should this hcall retry on a busy return?  Every other long-running hcall
in the driver loops:

	do {
		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));

including h_free_logical_lan_queue() in the IRQ-mapping failure path a few
lines below this call, and h_send_logical_lan() in ibmveth_send().  As
written, a transient H_BUSY / H_LONG_BUSY_ORDER_* here becomes -EIO from
ibmveth_register_single_rx_queue() and a failed ifup.

+	/*
+	 * H_FUNCTION means firmware rejected this subordinate register
+	 * (MQ unsupported). That is a hard open failure: do not clear
+	 * multi_queue or claim single-queue fallback. Keep a specific
+	 * log, then the generic failure lines below (no early return).
+	 */
+	if (lpar_rc == H_FUNCTION)
+		netdev_err(adapter->netdev,
+			   "h_reg_logical_lan_queue H_FUNCTION for queue %d (firmware MQ unsupported)\n",
+			   queue_index);

[Severity: High]
Can the adapter recover if firmware stops honouring the MQ register after
probe?  multi_queue and num_rx_queues are latched once in ibmveth_probe()
from the H_ILLAN_ATTRIBUTES bit and are never re-read or reduced, so a
H_FUNCTION here fails ibmveth_open() and every subsequent open the same
way.

The reset work item only closes and reopens, it never re-reads the
attributes:

ibmveth_reset() {
	rtnl_lock();
	dev_close(adapter->netdev);
	dev_open(adapter->netdev, NULL);
	rtnl_unlock();
}

ethtool -L ethX rx 1 while down does not restore RX either: multi_queue
stays 1, so rx_buffers_per_hcall remains IBMVETH_MAX_RX_QUEUE and
ibmveth_add_logical_lan_buffers() keeps calling
h_add_logical_lan_buffers_queue(), whose H_FUNCTION path has no fallback
when multi_queue is set.

The driver already assumes PHYP capabilities can disappear across LPM:

	/*
	 * Live Partition Migration may drop multi-
	 * buffer support. Fall back to single-buffer
	 * on the next replenish; ...
	 */

Would clearing multi_queue and falling back to one queue on H_FUNCTION be
preferable to leaving the interface permanently unable to open?

+static int
+ibmveth_register_single_rx_queue(struct ibmveth_adapter *adapter,
+				 int queue_idx, u64 mac_address)
+{
+	struct net_device *netdev = adapter->netdev;
+	union ibmveth_buf_desc rxq_desc;
+	long lpar_rc;
+
+	(void)mac_address;

[Severity: Low]
This isn't a bug, but is the mac_address parameter worth keeping?
H_REG_LOGICAL_LAN_QUEUE has no MAC operand and
ibmveth_register_logical_lan_queue() has no MAC parameter, so callers
compute ether_addr_to_u64(netdev->dev_addr) only to feed a parameter that
is voided here.  The (void)x; suppression cast is also not needed under
the kernel's warning set.

[ ... ]

[Severity: Medium]
Is PHYP delivery for subordinate queues guaranteed to be masked in this
window?  Queue 0 is explicitly masked before registration:

ibmveth_register_rx_queues() {
	adapter->queue_irq[0] = netdev->irq;
	rc = ibmveth_disable_irq(adapter, 0);
	...
}

but queues 1..N get no equivalent H_VIOCTL/H_DISABLE_VIO_INTERRUPT after
H_REG_LOGICAL_LAN_QUEUE plus irq_create_mapping().  Buffers are now
posted for every queue here, before any handler exists, and
ibmveth_setup_rx_interrupts() was also reordered so request_irq() for all
queues completes before any napi_enable():

	for (i = 0; i < num; i++) {
		...
		rc = request_irq(adapter->queue_irq[i], ibmveth_interrupt, ...);
	}

	for (i = 0; i < num; i++)
		napi_enable(&adapter->napi[i]);

If an interrupt arrives in that window, ibmveth_schedule_rx_queue() finds
NAPI still disabled:

	if (napi_schedule_prep(napi)) {
		lpar_rc = ibmveth_disable_irq(adapter, qindex);
		WARN_ON(lpar_rc != H_SUCCESS);
		__napi_schedule(napi);
	}

so the handler returns IRQ_HANDLED without masking the hypervisor source
and without scheduling a poll.  That is the same storm window described in
ibmveth_cleanup_rx_interrupts() ("Mask PHYP before napi_disable so
ibmveth_interrupt cannot return IRQ_HANDLED without masking").  The
err_free_irqs path has the same gap: it frees handlers and disposes
subordinate mappings without masking already-registered subordinate
queues.

The later scale-up path uses register -> request_irq -> publish ->
replenish -> napi_enable -> enable_irq and documents why.  Would enabling
NAPI before request_irq (as the baseline did), or explicitly masking each
subordinate queue right after registration, close this window?

[ ... ]

[Severity: Medium]
Does the buffer-list term still need scaling here?  The unchanged line
above this loop counts it once:

	ret = IBMVETH_BUFF_LIST_SIZE + IBMVETH_FILT_LIST_SIZE;

while multi-queue allocates and dma_map_single()s a distinct 4096-byte
buffer-list page per queue, which ibmveth_cleanup_rx_resources() unmaps
per index.  That leaves the desired value short by (num_rx_queues - 1)
buffer-list pages.

Also, is anything updating the CMO entitlement for the new default queue
count?  The VIO core samples it before the driver's probe runs:

arch/powerpc/platforms/pseries/vio.c:vio_cmo_bus_probe() {
	viodev->cmo.desired =
		IOMMU_PAGE_ALIGN(viodrv->get_desired_dma(viodev), tbl);
}

with dev_get_drvdata() still NULL, so the single-queue sized
IBMVETH_IO_ENTITLEMENT_DEFAULT is used for an adapter that will open with
up to 8 RX queues, and neither ibmveth_probe() nor ibmveth_open() calls
vio_cmo_set_dev_desired().  On a CMO partition, can this make
dma_map_single()/dma_alloc_coherent() fail during open on adapters that
worked in single-queue mode?

+
+		for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
+			/* add the size of the active receive buffers */
+			struct ibmveth_buff_pool *bpool =
+				&adapter->rx_buff_pool[q][i];

[Severity: Medium]
Is rx_buff_pool[q][i] populated for q >= 1 when this runs?  Pool metadata
for queues 1..N is only filled in by ibmveth_alloc_buffer_pools(), which
runs from ndo_open; ibmveth_probe() initializes only rx_buff_pool[0][i],
and the private area is zeroed, so before the first open these entries
have size = active = buff_size = 0.

ibmveth_change_mtu() pushes the result straight into firmware accounting:

	vio_cmo_set_dev_desired(viodev, ibmveth_get_desired_dma(viodev));

Can an MTU change while the interface is down therefore lower the
entitlement to roughly the single-queue value, right before an open that
allocates buffers for up to 8 queues?

[ ... ]

[Severity: Medium]
Now that num_rx_queues can be greater than one here, does
ibmveth_resume() need updating too?  It still kicks queue 0 only:

ibmveth_resume() {
	...
	ibmveth_schedule_rx_queue(adapter, 0);
	return 0;
}

The kick exists because the poll routine is what replenishes buffers and
re-arms the PHYP interrupt via napi_complete_done() ->
ibmveth_enable_irq().  After a suspend/resume or partition migration,
queues 1..N-1 would then have no buffers posted and no interrupt
re-armed until an ifdown/ifup.

[Severity: Medium]
Is it safe to free_netdev() immediately after ibmveth_put_pool_kobjs()?
The kobjects are embedded in netdev_priv(netdev), and ktype_veth_pool has
no release callback:

static struct kobj_type ktype_veth_pool = {
	.release        = NULL,
	...
};

With CONFIG_DEBUG_KOBJECT_RELEASE=y the final put defers cleanup using a
work item stored inside the kobject itself:

lib/kobject.c:kobject_release() {
#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	unsigned long delay = HZ + HZ * get_random_u32_below(4);
	...
	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
	schedule_delayed_work(&kobj->release, delay);
#else
	kobject_cleanup(kobj);
#endif
}

free_netdev() then releases that memory with no window, so
kobject_delayed_cleanup() would read kobj->state_in_sysfs, kobj->parent
and kobj->name from freed storage.  The same shape applies to the two
other new put_pool_kobjs() call sites below.

[ ... ]