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: Expose per-queue buffer pool details via debugfs
This adds a read-only buffer_pools file under
/sys/kernel/debug/ibmveth/<dev_name>/ that dumps size, buff_size, active
and available for every RX queue and buffer pool. The per-adapter
directory lives under a driver-owned root created lazily from
ibmveth_probe(), and the historical queue-0 poolN sysfs attributes are
left untouched.
A few questions below about the dumped values and about the lazy creation
of the driver root.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index 5d4ca4b3d3d07..1b58a3c6ce77a 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -3421,6 +3422,68 @@ 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;
> +
> + /*
> + * Writers (veth_pool_store, open template copy, reset close/open)
> + * update these fields under RTNL. Take the same lock so the dump
> + * is not a torn scalar snapshot. Not required for
> + * memory safety — embedded arrays only.
> + */
> + rtnl_lock();
[Severity: Low]
Is this comment accurate for all of the dumped fields? It says the
writers of "these fields" run under RTNL, but pool->available is also
printed here and is updated from the NAPI/softirq RX path:
ibmveth_replenish_buffer_pool()
atomic_add(buffers_added, &(pool->available));
ibmveth_remove_buffer_from_pool()
atomic_dec(&adapter->rx_buff_pool[queue_index][pool].available);
The second one runs under the per-queue replenish_lock, as its own
kernel-doc describes, and neither takes RTNL.
So rtnl_lock() does not serialize the Available column against the
geometry columns; only atomic_read() keeps that value from tearing.
Could the comment scope the RTNL claim to the geometry/active fields and
say that available is merely read atomically? As written, a later change
that adds a non-atomic read of a NAPI-updated pool field under this
rtnl_lock() would look safe when it is not.
> +
> + seq_puts(m, "Queue Pool Size BuffSize Active Available\n");
> + seq_puts(m, "----- ---- ---- -------- ------ ---------\n");
[Severity: Low]
This isn't a bug, but does the "Size" column name collide with the
existing poolN sysfs naming, with the opposite meaning?
The debugfs table prints pool->size (a buffer count) under "Size" and
pool->buff_size (bytes per buffer) under "BuffSize", while veth_pool_show()
maps the attributes the other way around:
else if (attr == &veth_num_attr)
return sprintf(buf, "%d\n", pool->size);
else if (attr == &veth_size_attr)
return sprintf(buf, "%d\n", pool->buff_size);
veth_pool_store() confirms this by range checking the size attribute
against IBMVETH_MAX_BUF_SIZE and num against IBMVETH_MAX_POOL_COUNT.
So "size" means a count in debugfs and a byte length in sysfs for the same
pool objects. Would naming the column "Count" or "Num" avoid that?
> + if (!adapter->opened)
> + seq_puts(m, "# down: Active/Available 0 unless allocated\n");
> +
> + for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++) {
> + for (j = 0; j < IBMVETH_NUM_BUFF_POOLS; j++) {
> + struct ibmveth_buff_pool *pool =
> + &adapter->rx_buff_pool[i][j];
[Severity: Low]
Can the Size/BuffSize columns print zero or stale values for queues
1..N here?
ibmveth_probe() publishes the queue count up front:
ibmveth_publish_num_rx_queues(adapter,
min(num_online_cpus(),
IBMVETH_DEFAULT_QUEUES));
but only initializes the queue-0 pools with
ibmveth_init_buffer_pool(&adapter->rx_buff_pool[0][i], ...). The copy to
the other queues happens later, in ibmveth_alloc_buffer_pools():
/* Initialize pool metadata for queues 1..N from queue 0 settings */
for (q = 1; q < ibmveth_get_num_rx_queues(adapter); q++) {
Reading buffer_pools before the first open therefore prints rows with
i > 0 showing Size 0 and BuffSize 0.
After an open/close cycle there is a second case: veth_pool_store() while
the device is down updates only the queue-0 pool and skips the reopen:
pool->active = newactive;
pool->buff_size = newbuff_size;
pool->size = newsize;
if (netif_running(netdev)) {
rc = ibmveth_open(netdev);
so queues 1..N keep the old geometry and the dump disagrees with queue 0.
The only hint emitted is "# down: Active/Available 0 unless allocated",
which covers just those two columns. Should the note also cover
Size/BuffSize for not-yet-templated queues, or should untemplated rows be
skipped?
> + bool live = pool->skbuff && pool->free_map;
> + int active = live ? pool->active : 0;
[Severity: Low]
Does the "Active" column here report something different from the
identically named sysfs attribute and ethtool statistic?
ibmveth_free_buffer_pool() deliberately keeps pool->active set when the
allocation goes away:
* Keep probe/sysfs geometry (active, size, buff_size, threshold).
* Clearing active here was a v3 ifdown/up regression: open skips
* !active pools, so reopen posted no RX buffers
while both existing surfaces print the raw flag:
veth_pool_show()
if (attr == &veth_active_attr)
return sprintf(buf, "%d\n", pool->active);
ibmveth_get_ethtool_stats()
data[i++] = adapter->rx_buff_pool[0][j].active;
With the interface down, .../pool0/active and poolN_active read 1 while
buffer_pools shows Active 0 for the same pool. Would a separate column
name such as "Alloc" or "Live" avoid overloading the existing meaning of
"active"?
The commit message says the file "lists size, buff_size, active, and
available for every RX queue and buffer pool" without mentioning that the
Active column is really an allocation indicator; only the stripped v5
changelog notes it. Could the commit message state that?
> + int available = live ? atomic_read(&pool->available)
> + : 0;
> +
> + seq_printf(m, "%5d %4d %4u %8u %6d %9d\n",
> + i, j, pool->size, pool->buff_size,
> + active, available);
> + }
> + }
> +
> + rtnl_unlock();
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(ibmveth_buffer_pools);
> +
> +/* Driver-owned root so per-adapter dirs use a stable vio name, not the
> + * mutable netdev->name (avoids stale names / eth0 collisions after rename).
> + */
> +static struct dentry *ibmveth_dbg_root;
> +
> +static void ibmveth_debugfs_init(struct ibmveth_adapter *adapter)
> +{
> + if (!ibmveth_dbg_root)
> + ibmveth_dbg_root =
> + debugfs_create_dir(ibmveth_driver_name, NULL);
> +
> + adapter->debugfs_dir =
> + debugfs_create_dir(dev_name(&adapter->vdev->dev),
> + ibmveth_dbg_root);
> + debugfs_create_file("buffer_pools", 0400, adapter->debugfs_dir,
> + adapter, &ibmveth_buffer_pools_fops);
> +}
[Severity: Medium]
Is this test-then-assign of ibmveth_dbg_root safe against concurrent
probes? ibmveth_debugfs_init() is called once per device from
ibmveth_probe(), and probe of two different vio devices is serialized only
by the per-device device_lock, so a DLPAR IO add racing with another add
(or with driver_attach during module load) can have both threads observe
NULL:
CPU0 CPU1
if (!ibmveth_dbg_root) if (!ibmveth_dbg_root)
debugfs_create_dir("ibmveth") debugfs_create_dir("ibmveth")
-> valid dentry -> ERR_PTR(-EEXIST)
ibmveth_dbg_root = dentry
ibmveth_dbg_root = ERR_PTR(-EEXIST)
fs/debugfs/inode.c returns ERR_PTR(-EEXIST) for the duplicate name
(debugfs_start_creating() -> simple_start_creating() with LOOKUP_EXCL,
then the dentry == ERR_PTR(-EEXIST) branch), and an ERR_PTR parent is
passed straight back out:
if (IS_ERR(parent))
return parent;
So after the race, every later per-adapter directory and buffer_pools file
is created with an ERR_PTR parent and silently fails, and the handle to the
real "ibmveth" directory is gone.
Would creating the root once in ibmveth_module_init(), before any probe can
run, avoid this?
> +
> +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)
> {
> @@ -3647,6 +3710,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;
> }
>
[ ... ]
> @@ -3891,6 +3958,8 @@ static int __init ibmveth_module_init(void)
> static void __exit ibmveth_module_exit(void)
> {
> vio_unregister_driver(&ibmveth_driver);
> + debugfs_remove_recursive(ibmveth_dbg_root);
> + ibmveth_dbg_root = NULL;
> }
[Severity: Medium]
Following on from the probe race above, does this leak the "ibmveth"
directory? If ibmveth_dbg_root holds ERR_PTR(-EEXIST),
debugfs_remove_recursive() returns immediately for IS_ERR_OR_NULL, so the
real directory and the simple_pin_fs reference it holds survive module
unload, and a later module load hits -EEXIST for the root every time.
