Re: [PATCH net-next v3 13/15] ibmveth: Implement incremental MQ RX queue resize

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

On 7/14/26 6:03 AM, Simon Horman 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.
> Full review at: https://sashiko.dev
> ---
> Date:   Mon Jul 6 19:36:01 2026 +0000
>
> ibmveth: Implement incremental MQ RX queue resize
>      
> Add helpers to register/deregister individual RX queues and scale
> queue count up or down while the netdev stays up. Surviving queues
> keep PHYP handles, buffer pools, and IRQ state across resize.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index ecc472ee8f71f..ac4d89a66a8db 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -1080,6 +1132,138 @@ static void ibmveth_free_buffer_pools(struct ibmveth_adapter *adapter)
>> +static int
>> +ibmveth_alloc_single_rx_queue(struct ibmveth_adapter *adapter, int queue_idx,
>> +			      int rxq_entries)
>> +{
> [ ... ]
>
>> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> +		adapter->rx_buff_pool[queue_idx][i].size =
>> +			adapter->rx_buff_pool[0][i].size;
>> +		adapter->rx_buff_pool[queue_idx][i].buff_size =
>> +			adapter->rx_buff_pool[0][i].buff_size;
>> +		adapter->rx_buff_pool[queue_idx][i].threshold =
>> +			adapter->rx_buff_pool[0][i].threshold;
>> +		adapter->rx_buff_pool[queue_idx][i].active =
>> +			adapter->rx_buff_pool[0][i].active;
>> +	}
> [Severity: Critical]
> Does this loop miss initializing pool->index?
>
> If pool->index remains 0 for all buffer pools on the new queue, when
> replenishing, 0 is shifted into the correlator.
>
> Then when receiving, ibmveth_remove_buffer_from_pool() will extract pool=0,
> which fetches from the wrong pool and could corrupt the free map of pool 0,
> leading to a use-after-free or leak.
Thanks — this was a real bug. We discovered this issue while doing 
stress tests, and identified
this issue, Agreed copies
*

pool->index when cloning pools for incrementally added queues; I have 
verified the fix. Thanks again,

*
>
> [ ... ]
>
>> @@ -1192,6 +1376,49 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
>> +static int
>> +ibmveth_drain_rx_queue(struct ibmveth_adapter *adapter, int queue_index)
>> +{
> [ ... ]
>
>> +	while (drained < limit &&
>> +	       ibmveth_rxq_pending_buffer(adapter, queue_index)) {
>> +		rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, true);
> Simon says: I think we can consider the issue below in the context of
>              possible follow-up, because I believe the use of this pattern
> 	    predates this patch.
>
> [Severity: High]
> Is a read memory barrier needed between reading the toggle bit in
> ibmveth_rxq_pending_buffer() and reading the correlator in
> ibmveth_rxq_harvest_buffer()?
>
> Since the hypervisor could be actively writing new descriptors while this is
> called, could a weakly-ordered CPU reorder the correlator load before the
> toggle bit load, causing us to recycle the wrong buffer?
>
> [ ... ]
Ah, *

as this toggle/correlator ordering pattern

**

pre-dates the resize work. I’ll treat adding an explicit barrier

in the drain path in v4

*
>> @@ -1491,6 +1718,175 @@ ibmveth_deregister_single_rx_queue(struct ibmveth_adapter *adapter,
>> +static int
>> +ibmveth_resize_rx_queues_incremental(struct ibmveth_adapter *adapter,
>> +				     int new_count, int rxq_entries)
>> +{
> [ ... ]
>
>> +			rc = ibmveth_enable_irq(adapter, i);
>> +			if (rc) {
>> +				netdev_err(netdev,
>> +					   "Failed to enable IRQ for queue %d: %d\n",
>> +					   i, rc);
>> +				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;
>> +			}
>> +
>> +			napi_enable(&adapter->napi[i]);
>> +		}
>> +
>> +		adapter->num_rx_queues = new_count;
> [Severity: High]
> What happens if an interrupt fires on the newly added queue before
> adapter->num_rx_queues is updated?
*agreed, v3 published the new queue count too late.*
> If an interrupt fires between ibmveth_enable_irq() and the update of
> adapter->num_rx_queues, ibmveth_interrupt() might evaluate:
>      WARN_ON(qindex < 0 || qindex >= adapter->num_rx_queues)
> to true and return IRQ_NONE without masking the interrupt.
>
> Could this lead to an unhandled interrupt storm?
*

possible

**

I will work on in v4 so that each new queue is published and replenished

**

before PHYP enable and napi_enable(), so ibmveth_interrupt() will not

**

return IRQ_NONE on a queue we have not published yet, and NAPI

does not run on an empty queue.

*
>
> [ ... ]
>
>> +	} else {
>> +		netdev_dbg(netdev, "Scale-down: removing queues %d-%d\n",
>> +			   new_count, old_count - 1);
>> +
>> +		for (i = new_count; i < old_count; i++)
>> +			napi_disable(&adapter->napi[i]);
>> +
>> +		for (i = new_count; i < old_count; i++)
>> +			ibmveth_drain_rx_queue(adapter, i);
>> +
>> +		synchronize_net();
> [Severity: High]
> Can disabling NAPI before disabling the hardware interrupts create an
> interrupt storm window?
*

  Agreed — with the handler only masking on successful NAPI

  schedule, napi_disable before PHYP mask can storm during drain.

*
>
> If the hypervisor asserts an interrupt during synchronize_net(), the hardware
> IRQ handler runs. Since NAPI is disabled, it returns IRQ_HANDLED without
> masking the interrupt source at the hypervisor, leading immediately to an
> IRQ storm and CPU lockup.
*

  That is a load-dependent race (tests can pass and still miss it).

  In v4 I will disable_irq + synchronize_irq before

  napi_disable/drain on scale-down (and on scale-up fail cleanup).

*

>> +
>> +		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++)
>> +				napi_enable(&adapter->napi[i]);
>> +			return rc;
>> +		}
> [Severity: High]
> Is it possible for this error path to permanently stall the RX queues?
>
> It calls napi_enable() but doesn't call ibmveth_replenish_task(). Since the
> queues were completely drained of their buffers by ibmveth_drain_rx_queue(),
> will the hypervisor have any buffers to place incoming packets?
>
> Without buffers, no interrupts will fire for these queues, and NAPI will
> never run again.
*

  Good catch — after drain, that set_real_num_rx failure path must

  replenish before re-enabling IRQ/NAPI, or those queues stall with

  empty pools. Real rollback bug (rare path). Will fix in v4.

*
Thanks,
Mingming