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

[mingming cao <mmc@linux.ibm.com>]

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.
>> 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
> [ ... ]
>
>> @@ -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.
>> +		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;
>> +}
>> +
> [ ... ]
>
>> @@ -1562,6 +1794,227 @@ ibmveth_register_single_rx_queue(struct ibmveth_adapter *adapter,
>>   	return 0;
>>   }
>>   
> [ ... ]
>
>> +static int
>> +ibmveth_resize_rx_queues_incremental(struct ibmveth_adapter *adapter,
>> +				     int new_count, int rxq_entries)
>> +{
> [ ... ]
>
>> +		for (i = old_count; i < new_count; i++) {
>> +			rc = ibmveth_alloc_single_rx_queue(adapter, i,
>> +							   rxq_entries);
> [ ... ]
>
>> +			/*
>> +			 * 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.

>> +			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.
>> +			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.
>> +		}
>> +
>> +		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.

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

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

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

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

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

>> +	netdev_warn(netdev, "Keeping %d queues after scale-up failure\n",
>> +		    old_count);
>> +	return rc;
>> +}
>> +
> [ ... ]
>
>> @@ -2217,12 +2670,62 @@ static void ibmveth_get_channels(struct net_device *netdev,
>>   	channels->rx_count = adapter->num_rx_queues;
>>   }
>>   
> [ ... ]
>
>> +static int ibmveth_resize_rx_channels(struct ibmveth_adapter *adapter,
>> +				      unsigned int goal_rx)
>> +{
> [ ... ]
>
>> +	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.

>> +	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
>>   	/* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
>>   	 * but we may need to alloc/free the ltb's.
>>   	 */
> [ ... ]