Re: [PATCH net-next v4 12/14] ibmveth: Implement incremental MQ RX queue resize

On 8/6/26 11:37 AM, Jakub Kicinski 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.
---
ibmveth: Implement incremental MQ RX queue resize

Adds per-queue alloc/register/irq-setup/free helpers plus
ibmveth_resize_rx_queues_incremental() so the live RX queue count can grow
or shrink without tearing down surviving queues, and wires
ibmveth_resize_rx_channels() into set_channels() when the interface is up.

A few questions on the new resize paths below.

Hi Jakub,

Thanks for the review.

quoted

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index db13747466025..b57c7df92853f 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

quoted

@@ -1307,6 +1494,51 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
  	return 0;
  }
  
+/**
+ * ibmveth_drain_rx_queue - Drain pending buffers from an RX queue
+ * @adapter: ibmveth adapter structure
+ * @queue_index: Queue index to drain
+ *
+ * Recycles all pending buffers back to the per-queue buffer pools.
+ * Must be called with NAPI disabled for this queue.
+ *
+ * Return: Number of buffers drained
+ */
+static int
+ibmveth_drain_rx_queue(struct ibmveth_adapter *adapter, int queue_index)
+{
+	struct net_device *netdev = adapter->netdev;
+	int drained = 0;
+	int limit = adapter->rx_queue[queue_index].num_slots;
+	int rc;
+
+	netdev_dbg(netdev, "Draining RX queue %d (limit: %d slots)\n",
+		   queue_index, limit);
+
+	while (drained < limit &&
+	       ibmveth_rxq_pending_buffer(adapter, queue_index)) {
+		/* Match poll-side order before harvesting completion state. */
+		smp_rmb();
+		rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, true);
+		if (rc) {
+			netdev_err(netdev,
+				   "Failed to harvest buffer from queue %d during drain: %d\n",
+				   queue_index, rc);
+			break;
+		}

[Severity: Low]
Can this error arm ever run?  ibmveth_rxq_harvest_buffer() normalizes
every failure to a 0 return:

	rc = ibmveth_remove_buffer_from_pool(adapter, cor, queue_index, reuse);
	if (unlikely(rc)) {
		if (rc == -EINVAL || rc == -EFAULT)
			goto advance;
		return rc;
	}

advance:
	ibmveth_rxq_advance(rxq);

	return 0;

and ibmveth_remove_buffer_from_pool() only ever returns 0, -EINVAL or
-EFAULT, so both non-zero values take the goto advance path.  The
netdev_err()/break looks like dead code.

Yes, that error arm is effectively dead on the mailed patch.

I’ll make harvest return `-EINVAL` / `-EFAULT` honestly and keep drain
tolerant of those slot-advanced cases.

quoted

+		drained++;
+	}
+
+	if (drained > 0)
+		netdev_dbg(netdev, "Drained %d buffer(s) from RX queue %d\n",
+			   drained, queue_index);
+	else
+		netdev_dbg(netdev, "No buffers to drain from RX queue %d\n",
+			   queue_index);
+
+	return drained;
+}
+

[ ... ]

quoted

@@ -1562,6 +1794,227 @@ ibmveth_register_single_rx_queue(struct ibmveth_adapter *adapter,
  	return 0;
  }
  

[ ... ]

quoted

+static int
+ibmveth_resize_rx_queues_incremental(struct ibmveth_adapter *adapter,
+				     int new_count, int rxq_entries)
+{

[ ... ]

quoted

+		for (i = old_count; i < new_count; i++) {
+			rc = ibmveth_alloc_single_rx_queue(adapter, i,
+							   rxq_entries);

[ ... ]

quoted

+			/*
+			 * Fully ready before PHYP delivery, matching open():
+			 * publish -> replenish -> napi_enable -> enable_irq.
+			 * That way ibmveth_interrupt() cannot run on an
+			 * unpublished, empty, or NAPI-disabled queue.
+			 */
+			adapter->num_rx_queues = i + 1;

[Severity: Medium]
Should this publish store be ordered?  adapter->num_rx_queues is the only
gate that makes rx_queue[i].queue_addr/num_slots/toggle/replenish_lock,
rx_buff_pool[i][*] and buffer_list_addr[i] visible to lockless readers,
and it is written with a plain store here while the queues added in
earlier iterations are already running NAPI with PHYP unmasked.

The consumers also use plain loads from hardirq and softirq context:

ibmveth_interrupt():
	if (WARN_ON(qindex < 0 || qindex >= adapter->num_rx_queues))
		return IRQ_NONE;

ibmveth_poll():
	if (WARN_ON(queue_index < 0 || queue_index >= adapter->num_rx_queues))
		return 0;

and ibmveth_update_rx_no_buffer() dereferences buffer_list_addr[i] for
every i below the count.  On powerpc, can a reader see the incremented
count while still observing stale contents of that slot (for example a
buffer_list_addr[i] left NULL by an earlier scale-down), or the converse
stale count for the queue whose interrupt was just unmasked?  Would
WRITE_ONCE()/READ_ONCE() plus a release barrier before the publish be
appropriate here?

Yes, the publication model needs to be explicit here

`num_rx_queues` is the visibility boundary for lockless readers, so I'm
planning to publish it with ordering only after the per-queue state is
ready, and use matching read-side accessors on the IRQ, poll, and
other lockless reader paths.

quoted

+			ibmveth_replenish_task(adapter, i);
+			napi_enable(&adapter->napi[i]);
+
+			rc = ibmveth_enable_irq(adapter, i);

[Severity: Medium]
After the unmask, should the queue be re-checked for descriptors that
PHYP enqueued while delivery was masked?  Buffers are posted by
ibmveth_replenish_task() before ibmveth_enable_irq(), so PHYP can write
descriptors during the masked window.  ibmveth_poll() does exactly this
compensation:

	lpar_rc = ibmveth_enable_irq(adapter, queue_index);
	...
	if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
	    napi_schedule(napi)) {
		ibmveth_disable_irq(adapter, queue_index);
		goto restart_poll;
	}

Without an equivalent pending check plus napi_schedule() here (and in the
scale-down rollback loop further down, which is even more exposed since
PHYP may have enqueued during the disable/drain window), can the new
queue sit with unharvested descriptors until unrelated traffic raises the
next interrupt?

Yes, after unmask the queue needs the same pending-work kick.

The right rationale is that PHYP may already have posted descriptors
while delivery was masked. I’ll kick the queue after scale-up
enable_irq and after scale-down rollback unmask so pending work is
serviced promptly.

quoted

+			if (rc) {
+				netdev_err(netdev,
+					   "Failed to enable IRQ for queue %d: %d\n",
+					   i, rc);
+				adapter->num_rx_queues = i;
+				napi_disable(&adapter->napi[i]);
+				ibmveth_cleanup_single_rx_interrupt(adapter, i);
+				ibmveth_deregister_single_rx_queue(adapter, i);
+				ibmveth_free_single_rx_queue(adapter, i);
+				goto cleanup_new_queues;
+			}

[Severity: Medium]
Is rc an errno at this point?  ibmveth_enable_irq() -> ibmveth_toggle_irq()
returns the raw plpar_hcall_norets() result and only normalizes
H_PARAMETER to 0, so rc can be H_HARDWARE (-1), H_FUNCTION (-2),
H_PRIVILEGE (-3) or the positive H_BUSY (1).  That value is returned
unchanged through ibmveth_resize_rx_channels() -> ibmveth_set_channels()
into the ethtool ioctl, so userspace sees EPERM/ENOENT/ESRCH, or for a
positive code a positive ioctl return that ethtool reads as success even
though the resize failed and unwound.

The pre-existing ibmveth_setup_rx_interrupts() converts the same failure
with rc = -EIO; should this path do the same?

Yes, that should return a Linux errno, not raw `H_*` status.

I'm planning to keep the helper contract normalized to errno and make
the resize path return `-EIO` on enable_irq failure rather than leaking
hypervisor status upward.

quoted

+		}
+
+		rc = netif_set_real_num_rx_queues(netdev, new_count);
+		if (rc) {
+			netdev_err(netdev, "Failed to set real RX queues to %d: %d\n",
+				   new_count, rc);
+			goto cleanup_new_queues;
+		}
+	} else {
+		netdev_dbg(netdev, "Scale-down: removing queues %d-%d\n",
+			   new_count, old_count - 1);
+
+		/*
+		 * Mask PHYP delivery before napi_disable/drain. Otherwise
+		 * ibmveth_interrupt returns IRQ_HANDLED without masking when
+		 * NAPI is disabled, and the HV can storm during drain.
+		 */
+		for (i = new_count; i < old_count; i++) {
+			ibmveth_disable_irq(adapter, i);
+			synchronize_irq(adapter->queue_irq[i]);
+		}
+
+		for (i = new_count; i < old_count; i++)
+			napi_disable(&adapter->napi[i]);

[Severity: Medium]
Does the mask actually hold across the drain?  synchronize_irq() waits
for the hardirq handler, not for an ibmveth_poll() already in flight on
another CPU, and that poll re-arms delivery unconditionally:

CPU0 (resize)
	ibmveth_disable_irq(adapter, i);
	synchronize_irq(adapter->queue_irq[i]);   /* handler idle, returns */
	napi_disable(&adapter->napi[i]);          /* returns after
						     napi_complete_done() */

CPU1 (ibmveth_poll for queue i, scheduled before the mask)
	if (!napi_complete_done(napi, frames_processed))
		goto out;

	lpar_rc = ibmveth_enable_irq(adapter, queue_index);  /* unmasked */

so the drain can run with PHYP delivery re-enabled.  With NAPI disabled,
ibmveth_schedule_rx_queue() fails napi_schedule_prep() and returns
without masking, and once adapter->num_rx_queues is lowered each further
delivery hits WARN_ON(qindex >= adapter->num_rx_queues) in
ibmveth_interrupt() and returns IRQ_NONE.  Is the storm rule stated in
the comment still guaranteed?  The same ordering is repeated in the
cleanup_new_queues label below.

Yes, the drain and rollback comments were too strong as written.

Teardown-first is still the primary fix, but poll also needs to stop
re-enabling delivery while the queue is being stopped. I'm planning to
tighten that stopping behavior so the drain window cannot remask PHYP
during teardown.

quoted

+
+		for (i = new_count; i < old_count; i++)
+			ibmveth_drain_rx_queue(adapter, i);
+
+		synchronize_net();
+
+		rc = netif_set_real_num_rx_queues(netdev, new_count);
+		if (rc) {
+			netdev_err(netdev, "Failed to set real RX queues to %d: %d\n",
+				   new_count, rc);
+			for (i = new_count; i < old_count; i++) {
+				ibmveth_replenish_task(adapter, i);
+				napi_enable(&adapter->napi[i]);
+				ibmveth_enable_irq(adapter, i);
+			}
+			return rc;
+		}

[Severity: Medium]
Should the ibmveth_enable_irq() return code be checked in this rollback
loop?  This is the only new call site that discards it.

If the H_VIOCTL enable fails for one of these queues, the queue stays in
the live set (adapter->num_rx_queues is still old_count) with NAPI
enabled and buffers posted, but with PHYP delivery masked, so that RX
queue is silently stalled until a close/open or another resize.

Elsewhere the driver treats this as actionable, e.g. ibmveth_poll():

	lpar_rc = ibmveth_enable_irq(adapter, queue_index);
	if (lpar_rc != H_SUCCESS) {
		...
		schedule_work(&adapter->work);
	}

Yes, rollback should not discard that return code.

I’ll check it there, log it, schedule reset work on failure, and still
kick pending work on success so rollback does not leave a queue
silently stalled.

quoted

+
+		adapter->num_rx_queues = new_count;

[Severity: Medium]
Do the interface statistics go backwards here?  ibmveth_get_stats64(),
ibmveth_get_ethtool_stats(), ibmveth_sum_rx_invalid_buffers() and
ibmveth_sum_rx_large_packets() all sum rx_qstats[0..num_rx_queues-1]:

	if (adapter->rx_qstats) {
		for (i = 0; i < adapter->num_rx_queues; i++) {
			stats->rx_packets += adapter->rx_qstats[i].packets;

so lowering the count drops the packets/bytes already accumulated by the
removed queues from the reported totals.

In the other direction, ibmveth_alloc_single_rx_queue() resets
rx_queue[queue_idx].index/num_slots/toggle and copies the pool geometry
but never clears adapter->rx_qstats[queue_idx], so re-adding a previously
removed index resurrects its old counters and the totals jump up.

Should the retired queues' counts be folded into an accumulator and the
slot cleared on (re-)allocation, so rtnl_link_stats64 stays monotonic?

Yes, that is a real integration issue with the stats model.

I'm planning to preserve monotonic aggregate stats across resize,
either by folding retired queues into a persistent accumulator or by
otherwise ensuring removed queues do not disappear from the reported
totals and then reappear later.

quoted

+
+		for (i = new_count; i < old_count; i++) {
+			ibmveth_cleanup_single_rx_interrupt(adapter, i);
+			ibmveth_deregister_single_rx_queue(adapter, i);
+			ibmveth_free_single_rx_queue(adapter, i);
+		}
+	}
+
+	netdev_info(netdev, "Successfully resized to %d RX queues (incremental)\n",
+		    adapter->num_rx_queues);
+
+	if (firmware_has_feature(FW_FEATURE_CMO))
+		vio_cmo_set_dev_desired(adapter->vdev,
+					ibmveth_get_desired_dma(adapter->vdev));

[Severity: Medium]
Is the entitlement raised too late on CMO LPARs?  All the CMO-charged DMA
work for the new queues has already happened by the time this runs:
dma_map_single() of the buffer-list page and dma_alloc_coherent() of the
descriptor ring in ibmveth_alloc_single_rx_queue(), plus every RX skb
mapping done by ibmveth_replenish_task().  Those run while the device's
desired entitlement still reflects the old, smaller queue count, and
vio_dma_iommu_alloc_coherent() refuses the allocation when
vio_cmo_alloc() cannot cover it.

It is also skipped on every failure and rollback return, so a resize that
fails leaves the entitlement untouched.

The driver's own convention is the opposite order, in
ibmveth_change_mtu():

	vio_cmo_set_dev_desired(viodev, ibmveth_get_desired_dma(viodev));
	...
	ibmveth_open(...)

Note that simply moving the call is not enough, since
ibmveth_get_desired_dma() itself loops over adapter->num_rx_queues, so
the target count has to be accounted before the allocations and rolled
back on failure.

Yes, on CMO systems that entitlement update is too late.

I’ll raise the desired entitlement for the target RX queue count before
scale-up allocation, and restore it on failure so the entitlement
tracks the surviving geometry.

quoted

+
+	return 0;
+
+cleanup_new_queues:
+	failed_queue = i;
+	netdev_err(netdev,
+		   "Scale-up failed at queue %d, cleaning up queues %d-%d\n",
+		   failed_queue, old_count, failed_queue - 1);

[Severity: Low]
When the first newly added queue is the one that fails, failed_queue ==
old_count, so this prints an inverted empty range such as "cleaning up
queues 4-3" while the three loops below correctly iterate zero times.

Yes, that log should not print an inverted empty range.

I’ll log the cleanup range only when there is actually something to
clean up, and otherwise say there is nothing to clean up.

quoted

+	for (i = old_count; i < failed_queue; i++) {
+		ibmveth_disable_irq(adapter, i);
+		synchronize_irq(adapter->queue_irq[i]);
+	}
+
+	for (i = old_count; i < failed_queue; i++)
+		napi_disable(&adapter->napi[i]);
+
+	for (i = old_count; i < failed_queue; i++)
+		ibmveth_drain_rx_queue(adapter, i);
+
+	synchronize_net();
+
+	for (i = old_count; i < failed_queue; i++) {
+		ibmveth_cleanup_single_rx_interrupt(adapter, i);
+		ibmveth_deregister_single_rx_queue(adapter, i);
+		ibmveth_free_single_rx_queue(adapter, i);
+	}
+	adapter->num_rx_queues = old_count;

[Severity: High]
Can a concurrent NAPI poll dereference the pages this loop just freed?
adapter->num_rx_queues is restored only after the free loop, so for the
duration of that loop the indices [old_count, failed_queue) are freed but
still inside the published live range.

ibmveth_free_single_rx_queue() unmaps buffer_list_dma[i], frees the
coherent ring and then does:

	free_page((unsigned long)adapter->buffer_list_addr[queue_idx]);
	adapter->buffer_list_addr[queue_idx] = NULL;

Meanwhile every surviving queue's poll reaches
ibmveth_replenish_task() -> ibmveth_update_rx_no_buffer(), which walks the
published range with no NULL or liveness check:

	for (i = 0; i < adapter->num_rx_queues; i++) {
		__be64 *p = adapter->buffer_list_addr[i] + 4096 - 8;
		u64 drops = be64_to_cpup(p);

So for a 2->4 scale-up where queue 2 came up and queue 3 fails, a poll on
queue 0 or 1 reads buffer_list_addr[2] either as a just-freed page or as
NULL + 0xff8, in softirq context.  ibmveth_poll_controller() reaches the
same range from netpoll without rtnl, and additionally posts buffers into
pools that are being freed.

The guard in ibmveth_replenish_task():

	if (queue_index >= adapter->num_rx_queues) {

does not help while the count is still the higher value.  The inline
enable_irq failure handler above and the successful scale-down path both
lower adapter->num_rx_queues before freeing; should this label do the
same, i.e. set adapter->num_rx_queues = old_count before the free loop?

Yes, that is a serious publish-before-free hazard in the mailed patch.

The published live range needs to be shrunk before those queues are
freed, otherwise lockless readers can still walk freed queue state. I'm
planning to restore the old published count before the destroy loop in
`cleanup_new_queues`, just like the successful scale-down path.

quoted

+	netdev_warn(netdev, "Keeping %d queues after scale-up failure\n",
+		    old_count);
+	return rc;
+}
+

[ ... ]

quoted

@@ -2217,12 +2670,62 @@ static void ibmveth_get_channels(struct net_device *netdev,
  	channels->rx_count = adapter->num_rx_queues;
  }
  

[ ... ]

quoted

+static int ibmveth_resize_rx_channels(struct ibmveth_adapter *adapter,
+				      unsigned int goal_rx)
+{

[ ... ]

quoted

+	if (goal_rx == old_rx)
+		return 0;
+
+	if (!(netdev->flags & IFF_UP)) {
+		adapter->num_rx_queues = goal_rx;
+		return 0;
+	}
+

[Severity: Low]
This isn't a bug in the final series, but at this commit the stash branch
is unreachable: the only caller, ibmveth_set_channels(), already returns
early on the same !IFF_UP condition before reaching
ibmveth_resize_rx_channels(), so ethtool -L <dev> rx N on a down
interface returns 0 while the requested count is discarded.  The next
patch in the series ("ibmveth: Wire ethtool set_channels to MQ RX queue
resize") replaces that early return and makes this branch live, so this
only shows up at this bisect point.

Yes, that is a series-split artifact rather than a final-series bug.

I’ll fold the down-interface stash handling into the next patch so this
intermediate commit does not accept and then discard the requested
count.

quoted

+	rxq_entries = adapter->rx_queue[0].num_slots;
+	rc = ibmveth_resize_rx_queues_incremental(adapter, goal_rx,
+						  rxq_entries);
+	if (rc)
+		netdev_err(netdev, "Failed to resize RX queues: %d\n", rc);
+	return rc;
+}
+
  static int ibmveth_set_channels(struct net_device *netdev,
  				struct ethtool_channels *channels)
  {
  	struct ibmveth_adapter *adapter = netdev_priv(netdev);
  	unsigned int old = netdev->real_num_tx_queues,
  		     goal = channels->tx_count;
+	unsigned int goal_rx = channels->rx_count;
  	int rc, i;
  
  	/* If ndo_open has not been called yet then don't allocate, just set

@@ -2231,6 +2734,13 @@ static int ibmveth_set_channels(struct net_device *netdev,
  	if (!(netdev->flags & IFF_UP))
  		return netif_set_real_num_tx_queues(netdev, goal);
  
+	/* Resize RX first while UP so ibmveth_resize_rx_channels() is used
+	 * in this patch. !IFF_UP RX stash ordering lands next.
+	 */
+	rc = ibmveth_resize_rx_channels(adapter, goal_rx);
+	if (rc)
+		return rc;
+

[Severity: Medium]
Should the RX side be unwound when the TX phase below fails?  The RX
resize is committed here first, and then ibmveth_allocate_tx_ltb() can
return -ENOMEM or netif_set_real_num_tx_queues() can fail, in which case
the function ends with return rc and no compensating
ibmveth_resize_rx_channels(adapter, old_rx).

ETHTOOL_SCHANNELS then reports failure while half of the requested
configuration is in effect: on scale-down the excess RX queues have
already been deregistered from PHYP and freed, on scale-up new queues are
registered with IRQs installed and DMA memory pinned, and ethtool -l
afterwards reports the new rx_count.

Yes, the combined RX/TX transaction needs a rollback story.

I'm planning to roll RX back to the old count if the later TX phase
cannot reach the requested configuration, so `set_channels()` does not
report failure after committing only half of the change.

So my updated view is that patch 12 got materially better in follow-up
on several of the biggest resize-path issues: publication ordering,
teardown ordering, errno normalization, post-unmask pending handling,
rollback robustness, and CMO timing. I would still avoid claiming the
live-resize model is fully closed out until the remaining publication,
teardown, and transaction story is tighter.

Thanks,
Mingming

quoted

  	/* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
  	 * but we may need to alloc/free the ltb's.
  	 */

[ ... ]