Re: [PATCH net-next v4 04/14] ibmveth: Refactor buffer pool management for per-queue MQ RX

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

On 8/6/26 11:37 AM, Jakub Kicinski wrote:
> his is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ibmveth: Refactor buffer pool management for per-queue MQ RX
>
> This reworks the five RX buffer pools into per-queue arrays
> (rx_buff_pool[queue][pool]) with new alloc/free helpers, wires those
> helpers into ibmveth_open()/ibmveth_close(), and clones pool geometry from
> queue 0 to queues 1..N.  It also flips the default active flag of the
> 64 KiB pool to 0.
>
> A few questions below.
Hi Jakub,

Thanks for the careful read.
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 1007dd95cde02..7a2ed49cad4f1 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -625,6 +625,145 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,
>>   	}
>>   }
>>   
>> +/**
>> + * ibmveth_alloc_queue_buffer_pools - Allocate buffer pools for a single queue
>> + * @adapter: ibmveth adapter structure
>> + * @queue: queue index
>> + *
>> + * Allocates all active buffer pools for the specified queue.
>> + * Pool metadata must be initialized before calling this function.
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +static int ibmveth_alloc_queue_buffer_pools(struct ibmveth_adapter *adapter,
>> +					    int queue)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +	int i;
>> +
>> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> +		struct ibmveth_buff_pool *bpool =
>> +			&adapter->rx_buff_pool[queue][i];
>> +
>> +		if (!bpool->active)
>> +			continue;
>> +
>> +		if (ibmveth_alloc_buffer_pool(bpool)) {
>> +			netdev_err(netdev,
>> +				   "pool %d/%d alloc failed (size=%u count=%u)\n",
>> +				   i, queue,
>> +				   bpool->buff_size,
>> +				   bpool->size);
>> +			bpool->active = 0;
>> +
>> +			/* Free pools allocated so far for this queue */
>> +			while (--i >= 0) {
>> +				struct ibmveth_buff_pool *fpool =
>> +					&adapter->rx_buff_pool[queue][i];
>> +
>> +				if (fpool->active)
>> +					ibmveth_free_buffer_pool(adapter,
>> +								 fpool);
>> +			}
> [Severity: Low]
>
> Is the free predicate here meant to differ from the one used by the sibling
> helper added in the same patch?
>
> This unwind path keys off fpool->active, while
> ibmveth_free_queue_buffer_pools() deliberately frees by allocation
> presence:
>
> 	if (pool->free_map || pool->dma_addr || pool->skbuff)
> 		ibmveth_free_buffer_pool(adapter, pool);
>
> and the changelog states "free paths release by real allocations
> (free_map/dma_addr/skbuff), not only pool->active".
>
> So on the queue that fails, a pool holding allocations with active == 0 is
> never released, which is exactly the case the allocation-based predicate
> was added for.  Since a failing ndo_open means ibmveth_close() is never
> called, would those free_map/dma_addr/skbuff arrays be leaked?
>
> ibmveth_free_buffer_pool() already NULLs all three pointers and is
> idempotent, so could the unwind just call ibmveth_free_queue_buffer_pools()
> on the same queue (or use the same predicate)?  The same helper is reused
> by the incremental resize path later in the series, where pools can hold
> memory while active is clear.
Agreed. That unwind is wrong because allocation state and `active` can
diverge on the failure path.  I will make the failure unwind match
`ibmveth_free_queue_buffer_pools()` by freeing on allocation presence
rather than `active`, so partially allocated pools are not skipped just
because `active` was cleared early.
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ibmveth_free_queue_buffer_pools - Free buffer pools for a single queue
>> + * @adapter: ibmveth adapter structure
>> + * @queue: queue index
>> + *
>> + * Frees all active buffer pools for the specified queue.
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> [Severity: Low]
>
> The kernel-doc summary says "all active buffer pools", but the body
> intentionally ignores the active flag and frees on allocation presence, and
> the in-body comment says the opposite of the summary.  Could the summary be
> reworded to say pools that have allocations?  Later MQ patches reuse this
> helper and depend on that distinction.
Agreed. I will update the kerneldoc to match the actual
allocation-presence free rule.
>> + */
>> +static void ibmveth_free_queue_buffer_pools(struct ibmveth_adapter *adapter,
>> +					    int queue)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> +		struct ibmveth_buff_pool *pool =
>> +			&adapter->rx_buff_pool[queue][i];
>> +
>> +		/* Free pool if it has allocated memory, regardless of
>> +		 * active flag. Pools may have memory allocated but not
>> +		 * marked active during queue scale-up, so we must check
>> +		 * for actual allocations.
>> +		 */
>> +		if (pool->free_map || pool->dma_addr || pool->skbuff)
>> +			ibmveth_free_buffer_pool(adapter, pool);
>> +	}
>> +}
>> +
> [ ... ]
>
>> +static int
>> +ibmveth_alloc_buffer_pools(struct ibmveth_adapter *adapter)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +	int i, q, rc;
>> +
>> +	/* Initialize pool metadata for queues 1-15 from queue 0 settings */
>                                              ^^^^^^^^^^
>
> [Severity: Low]
>
> IBMVETH_MAX_RX_QUEUES is 1U at this commit, so queues 1-15 do not exist yet
> and this loop body cannot run.  Should the comment say "queues 1..N" until
> the bound is raised later in the series?
Agreed. I will make that comment generic rather than hard-coding a queue
range.
>> +	for (q = 1; q < adapter->num_rx_queues; q++) {
>> +		for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> +			struct ibmveth_buff_pool *src =
>> +				&adapter->rx_buff_pool[0][i];
>> +			struct ibmveth_buff_pool *dst =
>> +				&adapter->rx_buff_pool[q][i];
>> +
>> +			dst->size = src->size;
>> +			dst->index = src->index;
>> +			dst->buff_size = src->buff_size;
>> +			dst->threshold = src->threshold;
>> +			dst->active = src->active;
>> +		}
>> +	}
> [Severity: Low]
>
> This turns the per-pool sysfs knobs into a per-queue template.  Is that
> intended to stay undocumented?
>
> Only queue 0 gets sysfs objects, in ibmveth_probe():
>
> 	struct kobject *kobj = &adapter->rx_buff_pool[0][i].kobj;
> 	...
> 	error = kobject_init_and_add(kobj, &ktype_veth_pool, &dev->dev.kobj,
> 				     "pool%d", i);
>
> and veth_pool_show()/veth_pool_store() read and write that single instance.
> After this change the queue-0 values are cloned above and each queue
> allocates its own copy, so writing num = 4096 to one pool multiplies the
> real allocation by adapter->num_rx_queues while sysfs still reports the
> single-queue value.
>
> The multiplier is 1 everywhere in this series (IBMVETH_DEFAULT_RX_QUEUES is
> 1), so nothing changes today.  Should these knobs stay adapter-wide (counts
> divided across queues), gain per-queue nodes, or should the multiplier be
> documented?
For this series, queue 0's pool sysfs nodes remain the shared
configuration/template interface: they define the pool geometry copied
into each RX queue rather than queue-0-only state. Later in the series I
add debugfs visibility for per-queue runtime information, but not
separate per-queue pool sysfs controls. I will make that split explicit
in the patch and cover letter so the multiplier effect is documented
rather than implicit.
>> +
>> +	/* Allocate actual buffers for all queues */
>> +	for (q = 0; q < adapter->num_rx_queues; q++) {
>> +		rc = ibmveth_alloc_queue_buffer_pools(adapter, q);
>> +		if (rc) {
>> +			/* Free pools for all previous queues */
>> +			while (--q >= 0)
>> +				ibmveth_free_queue_buffer_pools(adapter, q);
>> +			return rc;
>> +		}
>> +	}
>> +
>> +	netdev_dbg(netdev, "allocated buffer pools for %d queue(s)\n",
>> +		   adapter->num_rx_queues);
>> +	return 0;
>> +}
>> +
> [ ... ]
>
>> @@ -927,14 +1055,10 @@ static int ibmveth_close(struct net_device *netdev)
>>   
>>   	ibmveth_update_rx_no_buffer(adapter);
>>   
>> +	ibmveth_free_buffer_pools(adapter);
>>   	ibmveth_cleanup_rx_resources(adapter);
>>   	ibmveth_free_filter_list(adapter);
>>   
>> -	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
>> -		if (adapter->rx_buff_pool[0][i].active)
>> -			ibmveth_free_buffer_pool(adapter,
>> -						 &adapter->rx_buff_pool[0][i]);
>> -
> [Severity: Low]
>
> The teardown order changed here, which the changelog does not mention (it
> only says the helpers are wired into open()/close()).
>
> Baseline:
>
> 	ibmveth_update_rx_no_buffer()
> 	ibmveth_cleanup_rx_resources()
> 	ibmveth_free_filter_list()
> 	free active pools
>
> New:
>
> 	ibmveth_update_rx_no_buffer()
> 	ibmveth_free_buffer_pools()
> 	ibmveth_cleanup_rx_resources()
> 	ibmveth_free_filter_list()
>
> h_free_logical_lan(), napi_disable() and free_irq() have all run by this
> point, so neither PHYP nor NAPI still references the pool buffers or the RX
> queue, and the swap looks safe.  Could the reorder be called out in the
> changelog, since a future per-queue drain may need the RX completion queue
> still present while pools are released?
Agreed. The reorder looks safe in the current shutdown path, since LAN
teardown, NAPI shutdown, and IRQ teardown have already happened by then,
but it is still a real teardown-sequencing change. I will call that out
in the changelog, especially since later per-queue drain/free ordering
may care whether the RX completion queue is still present when pools are
released.
>>   	for (i = 0; i < netdev->real_num_tx_queues; i++)
>>   		ibmveth_free_tx_ltb(adapter, i);
>>   
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
>> index 8eaa3942fc40b..abcd4054c67e2 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
>> @@ -265,7 +265,7 @@ static inline long h_illan_attributes(unsigned long unit_address,
>>   static int pool_size[] = { 512, 1024 * 2, 1024 * 16, 1024 * 32, 1024 * 64 };
>>   static int pool_count[] = { 256, 512, 256, 256, 256 };
>>   static int pool_count_cmo[] = { 256, 512, 256, 256, 64 };
>> -static int pool_active[] = { 1, 1, 0, 0, 1};
>> +static int pool_active[] = { 1, 1, 0, 0, 0};
>                                            ^^^
>
> [Severity: High]
>
> Does this drop large-receive support for existing single-queue users?
>
> Pools 2 (16 KiB) and 3 (32 KiB) are already inactive by default, so with
> the 64 KiB pool off the largest buffer ever posted at MTU 1500 becomes
> 2048 bytes.  ibmveth_probe() seeds each pool from this array:
>
> 	ibmveth_init_buffer_pool(&adapter->rx_buff_pool[0][i], i,
> 				 pool_count[i], pool_size[i], pool_active[i]);
>
> and the new helper skips inactive pools, so nothing larger is handed to
> PHYP:
>
> 	if (!bpool->active)
> 		continue;
>
> ibmveth_poll() does receive hypervisor-aggregated frames longer than the
> MTU:
>
> 	if ((length > netdev->mtu + ETH_HLEN) || lrg_pkt ||
> 	    iph_check == 0xffff) {
> 		ibmveth_rx_mss_helper(skb, mss, lrg_pkt);
> 		adapter->rx_large_packets++;
> 	}
>
> Those arrive as single non-scatter buffers, and TSO/large-send is enabled
> by default in ibmveth_probe() when firmware reports
> IBMVETH_ILLAN_LRG_SND_SUPPORT.  With no buffer larger than 2 KiB posted,
> are such frames simply dropped by PHYP and counted in the no-buffer counter
> read by ibmveth_update_rx_no_buffer()?
Agreed. With the default RX queue count still at 1, disabling the 64
KiB pool here changes the historical single-queue default rather than
only reducing future MQ memory use. That makes any large-receive
dependency on the 64 KiB pool a regression risk for existing
single-queue users, not just an MQ tuning choice.

>
> The changelog says:
>
>      "MTU changes activate it when required; leaving it enabled would pin
>      about 16 MiB per RX queue in MQ mode."
>
> Is the first half accurate?  ibmveth_change_mtu() activates pools only up
> to the first pool whose buff_size covers the new MTU:
>
> 	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> 		adapter->rx_buff_pool[0][i].active = 1;
> 		if (new_mtu_oh <= adapter->rx_buff_pool[0][i].buff_size) {
> 			...
> 			return 0;
> 		}
> 	}
>
> At MTU 1500 that stops at the 2 KiB pool, so the 64 KiB pool is never
> re-enabled; ibmveth_set_tso() does not touch pool->active either.  Recovery
> then requires a manual write to pool4/active in sysfs.
You are right that the changelog claim about MTU changes re-enabling it
was not accurate for the MTU 1500 case: `ibmveth_change_mtu()` stops
once the first pool covers the MTU, so pool 4 does not come back
automatically there.
>
> The second half describes MQ, but IBMVETH_DEFAULT_RX_QUEUES is 1 both at
> this commit and at the end of the series (a3781f4ae789 still has
> pool_active[] = { 1, 1, 0, 0, 0}), so the only configurations affected are
> today's single-queue ones.  Would it be better to keep the default at 1 and
> scale the 64 KiB count (or deactivate it) when RX queues are actually
> scaled up?
Yes. If MQ memory pressure still needs separate handling, that should be
done only when RX queues are actually scaled up, not by changing the
single-queue default.
>
> Note also that commit cd7c7ec3687986 ("ibmveth: change rx buffer default
> allocation for CMO") enabled this pool on purpose and added
> pool_count_cmo[] = { 256, 512, 256, 256, 64 } specifically to reduce the
> 64 KiB count under CMO, which suggests the default was deliberate.
>
> As a side effect, ibmveth_get_desired_dma() sums only active pools:
>
> 	if (adapter->rx_buff_pool[0][i].active)
> 		ret += adapter->rx_buff_pool[0][i].size *
> 		       IOMMU_PAGE_ALIGN(adapter->rx_buff_pool[0][i].buff_size,
> 					tbl);
>
> so does the CMO entitlement the driver requests shrink silently as well?
Yes. Because desired DMA is summed only over active pools, disabling the

64 KiB pool here also shrinks the requested CMO footprint....

and the earlier CMO change also suggests the 64 KiB default was

intentional rather than incidental.

In v5 I will restore the default pool activation so the 64 KiB pool
remains enabled in the normal single-queue baseline.

Thanks again for the careful review,
Mingming

>>   
>>   #define IBM_VETH_INVALID_MAP ((u16)0xffff)