Re: [PATCH net-next v4 02/14] ibmveth: Prepare MQ RX adapter data structures

On 8/6/26 11:36 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: Prepare MQ RX adapter data structures
>
> This converts the single-instance RX state in struct ibmveth_adapter
> (napi, rx_queue, buffer_list_addr/dma, rx_buff_pool) into queue-indexed
> arrays sized by the new IBMVETH_MAX_RX_QUEUES, adds multi_queue and
> num_rx_queues, and mechanically rewrites all existing references to
> index 0.  IBMVETH_MAX_RX_QUEUES stays at 1 in this patch.
>
> A couple of observations below, neither of which changes runtime
> behaviour today.
Hi Jakub,

Thanks for the notes.
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 73e051d26b9d8..8e758362cb263 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -482,42 +486,44 @@ static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
>>   	mb();
>>   
>> -	atomic_dec(&(adapter->rx_buff_pool[pool].available));
>> +	atomic_dec(&adapter->rx_buff_pool[0][pool].available);
>>   
>>   	return 0;
>>   }
>>   
>>   /* get the current buffer on the rx queue */
>> -static inline struct sk_buff *ibmveth_rxq_get_buffer(struct ibmveth_adapter *adapter)
>> +static inline struct sk_buff *
>> +ibmveth_rxq_get_buffer(struct ibmveth_adapter *adapter)
> [Severity: Low]
> This isn't a bug, but the patch carries a few edits that are not part of
> the stated mechanical index-0 conversion:
>
>    - dropping the redundant parentheses in the atomic_dec() call in
>      ibmveth_remove_buffer_from_pool()
>    - reflowing the ibmveth_rxq_get_buffer() prototype
>    - rewriting the kernel-doc of ibmveth_get_desired_dma() from
>      "Return value:" to "Return:"
>    - inserting a blank line in struct ibmveth_adapter after
>      rx_large_packets
>
> The commit message says:
>
>      This patch keeps behavior unchanged by mechanically switching existing
>      references to index 0
>
> Could these cosmetic edits move to a separate cleanup patch, or be
> mentioned in the changelog?  As it stands each hunk has to first be
> classified as conversion or cleanup before the "behavior unchanged" claim
> can be confirmed.
>
> [ ... ]
Agreed. Those edits are not part of the stated index-0 conversion, so
for v5 I will drop them from patch 2. If they are still worth doing, I
can send them later as a separate cleanup patch.

>> @@ -1693,8 +1704,7 @@ static void ibmveth_poll_controller(struct net_device *dev)
>>    *
>>    * @vdev: struct vio_dev for the device whose desired IO mem is to be returned
>>    *
>> - * Return value:
>> - *	Number of bytes of IO data the driver will need to perform well.
>> + * Return: Number of bytes of IO data the driver will need to perform well.
>>    */
>>   static unsigned long ibmveth_get_desired_dma(struct vio_dev *vdev)
>>   {
> [ ... ]
>
>> @@ -1898,10 +1912,10 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>   		memcpy(pool_count, pool_count_cmo, sizeof(pool_count));
>>   
>>   	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> -		struct kobject *kobj = &adapter->rx_buff_pool[i].kobj;
>> +		struct kobject *kobj = &adapter->rx_buff_pool[0][i].kobj;
>>   		int error;
>>   
>> -		ibmveth_init_buffer_pool(&adapter->rx_buff_pool[i], i,
>> +		ibmveth_init_buffer_pool(&adapter->rx_buff_pool[0][i], i,
>>   					 pool_count[i], pool_size[i],
>>   					 pool_active[i]);
>>   		error = kobject_init_and_add(kobj, &ktype_veth_pool,
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
>> index c88dbeb7cd967..8eaa3942fc40b 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
> [ ... ]
>
>> @@ -294,18 +296,23 @@ struct ibmveth_rx_q {
>> -	struct ibmveth_buff_pool rx_buff_pool[IBMVETH_NUM_BUFF_POOLS];
>> -	struct ibmveth_rx_q rx_queue;
>> +	struct ibmveth_buff_pool
>> +		rx_buff_pool[IBMVETH_MAX_RX_QUEUES][IBMVETH_NUM_BUFF_POOLS];
>> +	struct ibmveth_rx_q rx_queue[IBMVETH_MAX_RX_QUEUES];
> [Severity: Low]
> Is replicating the whole struct ibmveth_buff_pool per queue the layout you
> want here?
>
> struct ibmveth_buff_pool embeds a struct kobject, which is the per-pool
> sysfs tuning object.  Only row 0's kobjects are ever initialized and
> registered, in ibmveth_probe():
>
> 	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> 		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 only row 0's are dropped, in ibmveth_remove():
>
> 	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
> 		kobject_put(&adapter->rx_buff_pool[0][i].kobj);
>
> So every row above 0 carries a kobject that is never initialized and never
> used.

Correct. Only row 0's pool kobjects are initialized; rows 1..N-1 carry
unused embedded kobjects in the current layout.

>
> Later in the series IBMVETH_MAX_RX_QUEUES is raised to 16U, at which point
> the netdev private area unconditionally holds 16 x 5 pool structs
> regardless of num_rx_queues, of which 75 embedded kobjects are dead
> weight.
>
> The follow-on code also shows that only part of the struct is really
> per-queue: ibmveth_alloc_single_rx_queue() copies size, index, buff_size,
> threshold and active from row 0 into each new row, so the pool
> configuration is shared while free_map/dma_addr/skbuff/producer_index/
> consumer_index/available are the genuinely per-queue state.
Only part of ibmveth_buff_pool is truly per-queue; the sysfs-visible
tuning fields are effectively shared in this series.
> Would it be cleaner to split the struct into one shared, sysfs-visible
> configuration object plus a small per-queue state array, given this patch
> is the one that fixes the layout for the rest of the series?

Longer term, splitting this into a shared sysfs-visible configuration
object plus smaller per-queue runtime state would be cleaner. I am
deferring that redesign for this series.

>
> Is it also intentional that the per-pool sysfs tuning interface now
> implicitly means "queue 0 configures all queues"?  If so, could that be
> stated in the changelog?
Yes, that is intentional for this series. Queue 0 is acting as the
shared pool configuration/template surface rather than as a queue-0-only
tuning interface. In v5 I will make that explicit in the changelog and
cover letter:

- only queue 0's `poolN` sysfs nodes are registered
- those nodes represent the shared pool geometry/template policy
   (`buff_size`, `size`, `active`)
- open() and resize copy that template into each RX queue's pool row
- queues 1..N do not get separate pool sysfs controls in this series

Thanks again for the review,
Mingming