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

[Jakub Kicinski <kuba@kernel.org>]

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.

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

[ ... ]

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

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.

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?

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?

> @@ -325,6 +332,7 @@ struct ibmveth_adapter {
>  	u64 tx_send_failed;
>  	u64 tx_large_packets;
>  	u64 rx_large_packets;
> +
>  	/* Ethtool settings */
>  	u8 duplex;
>  	u32 speed;