Re: [PATCH net-next v4 11/14] ibmveth: Expose per-queue buffer pool details via debugfs

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: Expose per-queue buffer pool details via debugfs
>
> This adds a read-only buffer_pools debugfs file that dumps size,
> buff_size, active and available for every RX queue and buffer pool, and
> wires creation into ibmveth_probe() and teardown into ibmveth_remove().
> The existing queue-0 poolN sysfs files are left unchanged.
>
> A few questions about the new code below.
Hi Jakub,

Thanks for the review.
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index dbb47a42b4cb5..db13747466025 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -2985,6 +2986,44 @@ static const struct net_device_ops ibmveth_netdev_ops = {
>>   #endif
>>   };
>>   
>> +static int ibmveth_buffer_pools_show(struct seq_file *m, void *v)
>> +{
>> +	struct ibmveth_adapter *adapter = m->private;
>> +	int i, j;
>> +
>> +	seq_puts(m, "Queue  Pool  Size  BuffSize  Active  Available\n");
>> +	seq_puts(m, "-----  ----  ----  --------  ------  ---------\n");
>> +
>> +	for (i = 0; i < adapter->num_rx_queues; i++) {
>> +		for (j = 0; j < IBMVETH_NUM_BUFF_POOLS; j++) {
>> +			struct ibmveth_buff_pool *pool =
>> +				&adapter->rx_buff_pool[i][j];
> [Severity: Low]
> Should this dump be taken under rtnl_lock()?
>
> ibmveth_buffer_pools_show() reads adapter->num_rx_queues plus
> pool->size, pool->buff_size, pool->active and pool->available with no
> lock held, but every writer of those fields runs under RTNL:
>
> veth_pool_store() {
> 	...
> 	rtnl_lock();
> 	...
> 	pool->active = newactive;
> 	pool->buff_size = newbuff_size;
> 	pool->size = newsize;
> 	...
> 	/* on ibmveth_open() failure, the same three fields are
> 	 * written back with the old values */
> 			pool->active = oldactive;
> 			pool->buff_size = oldbuff_size;
> 			pool->size = oldsize;
> 	...
> }
>
> ibmveth_alloc_buffer_pools() likewise copies the queue-0 geometry into
> queues 1..N-1 field by field under RTNL from ibmveth_open(), and the
> reset path goes through dev_close()/dev_open() under rtnl_lock().
>
> Can a concurrent read of buffer_pools therefore print a torn snapshot,
> for example active=1 paired with the previous size/buff_size, or a
> half-copied per-queue template, or the transient rollback state that was
> never an actual configuration?
>
> There does not appear to be a memory-safety consequence here, since
> rx_buff_pool[] is a fixed-size array embedded in struct ibmveth_adapter
> and only scalar members are read, so the concern is limited to the
> values printed.
Yes. The writers already run under RTNL, so the dump can otherwise
print a torn snapshot.

I'm planning to take rtnl_lock() around
ibmveth_buffer_pools_show() so the output stays consistent.

>> +
>> +			seq_printf(m, "%5d  %4d  %4u  %8u  %6d  %9d\n",
>> +				   i, j, pool->size, pool->buff_size,
>> +				   pool->active,
>> +				   atomic_read(&pool->available));
> [Severity: Low]
> Are Active and Available meaningful while the interface is down?
>
> ibmveth_free_buffer_pool() frees free_map, unmaps and frees all
> skbuffs, and frees dma_addr/skbuff, but leaves pool->active set and
> never decrements pool->available:
>
> 	kfree(pool->free_map);
> 	pool->free_map = NULL;
> 	...
> 	if (pool->skbuff) {
> 		kfree(pool->skbuff);
> 		pool->skbuff = NULL;
> 	}
> 	/* pool->active and pool->available untouched */
>
> available is only cleared later, in ibmveth_alloc_buffer_pool() via
> atomic_set(&pool->available, 0) on the next open.
>
> So a read after ibmveth_close() -> ibmveth_free_buffer_pools() ->
> ibmveth_free_queue_buffer_pools() -> ibmveth_free_buffer_pool() reports
> every configured pool as Active=1 with the last non-zero Available,
> even though the skbuffs and DMA mappings are gone.
>
> Related: for queues 1..N-1 the table prints all-zero rows before the
> first open, because the queue-0 template is only copied into those
> queues inside ibmveth_alloc_buffer_pools() during open.
>
> The commit message says "Per-queue runtime pressure is what debugfs
> shows" - could the down/uninitialized cases be either reflected in the
> output or documented?

Yes. The down/pre-open view needs to be more honest.

As written, the dump can present stale pool state as if it were live
runtime state after close, and it can also show all-zero subordinate
queues before the first open even though that is not the eventual
runtime shape. I'm planning to make that state more explicit by
clearing the down-state availability, treating pools without live
backing state as not active runtime pools in the dump, and adding a
short note when the adapter is not opened.

>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(ibmveth_buffer_pools);
>> +
>> +static void ibmveth_debugfs_init(struct ibmveth_adapter *adapter)
>> +{
>> +	adapter->debugfs_dir = debugfs_create_dir(adapter->netdev->name,
>> +						  NULL);
>> +	debugfs_create_file("buffer_pools", 0400, adapter->debugfs_dir,
>> +			    adapter, &ibmveth_buffer_pools_fops);
>> +}
> [Severity: Medium]
> Is the mutable interface name a safe key for a directory created
> directly in the debugfs root?
>
> Three separate things follow from passing NULL as the parent and
> netdev->name as the name.
>
> First, the name goes stale. The directory is created once, as the last
> statement of ibmveth_probe(), and the driver installs no netdev
> notifier and never calls debugfs_rename(). After the udev/systemd
> predictable-name rename that happens right after register_netdev(), or
> after any later ip link set dev ethN name foo, does the debugfs
> directory keep the obsolete name for the lifetime of the device?
>
> Second, with the debugfs root as parent the name has to be globally
> unique there. Consider the sequence: first adapter probes as eth0, udev
> renames it to enP1p1s0, a second ibmveth adapter probes and
> alloc_etherdev_mqs() names it eth0 again because eth0 is free in the
> netdev namespace. debugfs_create_dir("eth0", NULL) then collides with
> the first adapter's stale directory:
>
> fs/debugfs/inode.c:debugfs_start_creating() {
> 	...
> 	dentry = simple_start_creating(parent, name);
> 	if (IS_ERR(dentry)) {
> 		if (dentry == ERR_PTR(-EEXIST))
> 			pr_err("'%s' already exists in '%pd'\n", name, parent);
> 	...
> }
>
> The ERR_PTR is stored in adapter->debugfs_dir and handed to
> debugfs_create_file() as the parent, which returns immediately:
>
> fs/debugfs/inode.c:debugfs_start_creating() {
> 	...
> 	if (IS_ERR(parent))
> 		return parent;
> 	...
> }
>
> Does the second and every subsequent adapter then end up with no
> buffer_pools file at all, with only that debugfs pr_err as an
> indication?
>
> Third, netdev->name is read here after register_netdev() has published
> the device, without rtnl_lock(), while dev_change_name() rewrites
> net_device->name under RTNL. Can the string handed to
> debugfs_create_dir() be a partially updated name?
>
> Would a driver-owned parent directory plus a stable identifier such as
> dev_name(&dev->dev) avoid all three?

Yes. This is the main design issue in the patch.

Using `netdev->name` at the debugfs root means the directory can go
stale after rename, collide with a later adapter that reuses the old
name, and race with rename while the name is being read. I'm planning
to move this under a driver-owned `ibmveth/` parent and key each
adapter directory off the stable VIO device name instead.

I agree that once the naming is fixed, leaving debugfs creation
best-effort is much easier to justify. The bigger problem here is the
choice of key and parent, not the lack of hard failure handling.

>> +
>> +static void ibmveth_debugfs_exit(struct ibmveth_adapter *adapter)
>> +{
>> +	debugfs_remove_recursive(adapter->debugfs_dir);
>> +	adapter->debugfs_dir = NULL;
>> +}
>> +
>>   static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter,
>>   				  int pools_ready)
>>   {
>> @@ -3182,6 +3221,8 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>   
>>   	netdev_dbg(netdev, "registered\n");
>>   
>> +	ibmveth_debugfs_init(adapter);
>> +
>>   	return 0;
>>   }
> [ ... ]
>
> Cross-instance finding from sashiko-gemini (b6bfc50872782421f7178a42615ea39fe861918e607d995ed830a5a2db8a4460):
> [Severity: Medium]
> Addition of driver-private ethtool -S strings for standard queue statistic

I agree with the underlying point, but that belongs with the patch-10
stats/interface cleanup rather than this debugfs patch.

So my updated view is that the main patch-11 issue is the debugfs
naming and lifetime design, while the RTNL and down/pre-open reporting
points are real but lower-severity cleanup. I'm planning to tighten
those pieces here, but I still think debugfs is the right place for the
all-queue dump.

Thanks,
Mingming