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.

quoted

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

[ ... ]

quoted

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

quoted

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

quoted

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

quoted

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