Thanks for the review feedback.
On 9/3/26 11:10 AM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 11 potential
issue(s) to consider:
Critical: 0 · High: 1 (2) · Medium: 4 · Low: 6
- [High] In multi-queue mode ibmveth_setup_rx_interrupts() already
unmasks the PHYP interrupt for every live RX queue, and ibmveth_open()…
- [Medium] Adapter-wide RX counters are not converted to per-queue
storage while this commit enables up to min(nr_cpus,8) live RX queues:…
- [Medium] ibmveth_apply_mq_fallback() publishes adapter->num_rx_queues =
1 at the top of ibmveth_open() but deliberately leaves…
- [Medium] ibmveth_open() now runs ibmveth_replenish_task() for every
queue, and that function can queue reset work…
- [Medium] The new probe error paths call kobject_put() on the pool
kobjects, which are embedded in struct ibmveth_adapter inside…
- [Low] ibmveth_get_channels() begins advertising max_rx/rx_count > 1 (up
to min(num_online_cpus(), IBMVETH_DEFAULT_QUEUES)) while its peer…
- [Low] Probe enables min(num_online_cpus(), IBMVETH_DEFAULT_QUEUES) RX
queues whenever the firmware MQ bit is present; each queue clones…
- [Low] The commit message itself states that this commit ships known-
wrong statistics and new schedule_work() producers whose remove-path…
- [Low] The pool sysfs kobject creation loop in ibmveth_probe() changes
from 'ignore kobject_init_and_add() failure and continue probing' to…
- [Low] The ibmveth_setup_rx_interrupts() kernel-doc still describes the
pre-MQ open sequence: "Single-queue open leaves PHYP masked here and…
- [Low] ibmveth_register_single_rx_queue() logs a signed `long lpar_rc`
with the unsigned conversion "rc=0x%lx".
Pre-existing issues:
- [High] This commit makes the MQ reset producer live
(adapter->multi_queue was hardcoded 0 before) and adds a new…
- [High] ibmveth_poll_controller() now replenishes and schedules every
published RX queue instead of only queue 0, multiplying the exposure…
--
ibmveth: Enable multi-queue RX receive path
This turns on the multi-queue RX path when firmware advertises
IBMVETH_ILLAN_RX_MULTI_QUEUE_SUPPORT, raises IBMVETH_MAX_RX_QUEUES from 1
to 16, and wires subordinate queue registration, IRQ setup and per-queue
replenish into open()/close(). Probe now selects min(num_online_cpus(),
IBMVETH_DEFAULT_QUEUES) RX queues and refreshes the VIO CMO entitlement.
A few questions on the commit message first.
[Severity: Low]
The changelog says:
Two things are knowingly left for patch 11. RX counters are racy:
adapter->rx_no_buffer is assigned rather than summed from one queue's
buffer-list page, so it reports whichever queue replenished last and
can go backwards, while rx_packets, rx_bytes, rx_invalid_buffer and
rx_large_packets are plain read-modify-writes now reached from several
NAPI instances at once, so they can lose counts
and:
This commit also adds schedule_work() producers, and the remove-path
unregister / cancel_work_sync reorder and reset reg_state gate that
make them safe land there too.
Would it be preferable to reorder so the counter conversion and the
remove-path fix precede the commit that makes the MQ path live, rather
than shipping an intermediate commit that is documented as incorrect in
those two respects?
The same paragraph documents a third, unrelated change:
On probe failure after pool kobjects were created, put them before
free_netdev(). The leak is pre-existing and unrelated to multi-queue,
so a standalone version also goes to net; the probe_cleanup helper
lands in patch 11.
Since this is described as unrelated and is being posted separately, could
it be dropped from this patch instead of open-coded on four error paths
here and then replaced by a helper later in the series?
The remove-path fix (unregister_netdev then cancel_work_sync)
and the NETREG_REGISTERED gate are folded into this patch,
alongside the buffer-add H_FUNCTION reset producer.
The counter conversion stays in Patch 11: we keep activation in
this patch so all MQ bring-up logic is exercised together at
this commit, and explicitly document the transient counter race
resolved in Patch 11. If you prefer a Prepare/Enable split to
keep intermediate counters strictly race-free, we can separate
activation if requested.
The pool kobject puts stay here: dropping them
leaks on probe fail until Patch 11 extracts
probe_cleanup. That is not the unwind
standalone (Fixes: d43732ce021f). v7 waits
for release before free_netdev().
diff --git a/drivers/net/ethernet/ibm/ibmveth.c
b/drivers/net/ethernet/ibm/ibmveth.c
index f726d1e3f0b11..2e8896ea5af2e 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
@@ -992,11 +1014,21 @@ static int ibmveth_replenish_buffer_pool(struct
ibmveth_adapter *adapter,
* because there was not a buffer in the buffer list capable of holding
* the frame.
*/
-static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
+static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter,
+ int queue_index)
{
- __be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
+ __be64 *p;
+ u64 drops;
- adapter->rx_no_buffer = be64_to_cpup(p);
+ if (queue_index < 0 ||
+ queue_index >= ibmveth_get_num_rx_queues(adapter) ||
+ !adapter->buffer_list_addr[queue_index])
+ return;
+
+ p = adapter->buffer_list_addr[queue_index] + 4096 - 8;
+ drops = be64_to_cpup(p);
+
+ adapter->rx_no_buffer = drops;
}
[Severity: Medium]
ibmveth_update_rx_no_buffer() now takes a queue index but still ends in a
plain assignment to the single adapter-wide field:
adapter->rx_no_buffer = drops;
With more than one live queue, does each call simply overwrite the value
harvested for the previous queue? In ibmveth_close() the new loop calls it
for every live queue back to back:
for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++)
ibmveth_update_rx_no_buffer(adapter, i);
so the reported counter ends up being whichever queue was visited last, and
can move backwards between reads.
Similarly, ibmveth_poll_deliver_frame() does unlocked read-modify-writes:
netdev->stats.rx_packets++;
netdev->stats.rx_bytes += length;
and the replenish counters (replenish_task_cycles, replenish_add_buff_success
/ _failure, replenish_no_mem) are serialized only by the per-queue
rx_queue[i].replenish_lock, which does not serialize different queues against
each other. Can these lose counts once several NAPI instances run
concurrently?
Yes, at this commit. Patch 11 already moves replenish_* and
the packet/byte counters to per-queue u64, summed on read.
rx_no_buffer gets a per-queue carry there too.
The changelog notes that the following patch moves these counters to
per-queue storage summed on read, which does resolve it; the question is
whether this intermediate state is worth carrying.
@@ -1062,6 +1094,7 @@ static void ibmveth_replenish_task(struct ibmveth_adapter
*adapter,
dev_err_ratelimited(&adapter->netdev->dev,
"MQ buffer add H_FUNCTION (q=%d, batch=%u),
reset\n",
queue_index, fail.batch);
+ adapter->mq_fallback = true;
schedule_work(&adapter->work);
}
[Severity: High]
This isn't a bug introduced by this patch, but this commit makes the reset
producer reachable for the first time (adapter->multi_queue was hardcoded to
0 before) and adds a second schedule_work() site here.
At this commit ibmveth_reset() has no registration-state check:
rtnl_lock();
dev_close(adapter->netdev);
dev_open(adapter->netdev, NULL);
rtnl_unlock();
and ibmveth_remove() still runs cancel_work_sync(&adapter->work) before
unregister_netdev(). If a live queue hits MQ buffer-add H_FUNCTION after
the cancel, can the requeued work then run dev_close()/dev_open() against a
netdev that unregister_netdev()/free_netdev() is tearing down?
Folded into this patch: unregister_netdev() moves
ahead of cancel_work_sync(), and ibmveth_reset()
is gated on NETREG_REGISTERED here alongside the
buffer-add H_FUNCTION reset producer. Patch 11
then frees the per-queue statistics arrays between
cancel_work_sync() and free_netdev().
Both halves are fixed later in this series (ibmveth_remove() is reordered to
unregister_netdev() before cancel_work_sync(), and ibmveth_reset() gains the
netdev->reg_state != NETREG_REGISTERED gate), so the exposure is limited to
this intermediate commit.
@@ -1081,6 +1114,27 @@ static void ibmveth_replenish_task(struct
ibmveth_adapter *adapter,
fail.filled, fail.lpar_rc, fail.batch);
}
+/**
+ * ibmveth_restart_rx_queue - Post buffers and ensure Q can take RX
+ * @adapter: ibmveth adapter
+ * @qindex: RX queue index
+ *
+ * SQ open leaves PHYP masked until the first poll. If schedule_prep fails,
+ * NAPI never runs and the queue stays masked (TX OK, RX/ARP dead) until
+ * reload. Replenish first so an enable_irq fallback can actually deliver.
+ * Also used after every open (SQ and MQ) and after scale-down so a
+ * queue is not left idle+masked.
+ */
+static void ibmveth_restart_rx_queue(struct ibmveth_adapter *adapter,
+ int qindex)
+{
+ ibmveth_replenish_task(adapter, qindex);
+ if (ibmveth_schedule_rx_queue(adapter, qindex))
+ return;
+
+ ibmveth_enable_irq(adapter, qindex);
+}
[Severity: High]
Is the unconditional ibmveth_enable_irq() here safe in multi-queue mode?
ibmveth_schedule_rx_queue() returns false in two different situations:
if (napi_schedule_prep(napi)) {
ibmveth_disable_irq(adapter, qindex);
__napi_schedule(napi);
return true;
}
return false;
The second case is "NAPI already claimed", which is exactly what the IRQ
handler does after it has masked PHYP. In MQ mode
ibmveth_setup_rx_interrupts() has already unmasked every queue:
if (adapter->multi_queue && num > 1) {
for (i = 0; i < num; i++) {
rc = ibmveth_enable_irq(adapter, i);
so by the time open() runs its restart loop an interrupt may already have
claimed NAPI and masked the queue. restart then re-unmasks it under the
in-flight poll, and when that poll finishes ibmveth_poll() calls
ibmveth_enable_irq() again on an already-enabled subordinate interrupt.
ibmveth_toggle_irq() folds H_PARAMETER only on the disable side:
if (h_rc == H_PARAMETER && !enable) {
dev_warn_ratelimited(...);
return 0;
}
so the redundant enable returns -EIO, and ibmveth_poll() escalates that to
schedule_work(&adapter->work), i.e. a full dev_close()/dev_open() of an
otherwise healthy adapter.
Since the PHYP mask/unmask hcalls are not reference counted, should restart
skip the enable when schedule_rx_queue() failed because NAPI was already
scheduled, rather than when prep failed for lack of a pending descriptor?
The same enable-then-restart sequence appears later in the ethtool -L
scale-up path of ibmveth_resize_rx_queues_incremental():
rc = ibmveth_enable_irq(adapter, i);
...
ibmveth_restart_rx_queue(adapter, i);
[ ... ]
It should not skip the enable. When prep fails,
NAPI is already scheduled, so enabling there is
deliberate; masking on that path left the queue
idle and masked.
+static int
+ibmveth_register_single_rx_queue(struct ibmveth_adapter *adapter,
+ int queue_idx)
+{
+ struct net_device *netdev = adapter->netdev;
+ union ibmveth_buf_desc rxq_desc;
+ long lpar_rc;
+
+ if (WARN_ON(queue_idx < 1 || queue_idx >= IBMVETH_MAX_RX_QUEUES))
+ return -EINVAL;
+
+ rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
+ adapter->rx_queue[queue_idx].queue_len;
+ rxq_desc.fields.address = adapter->rx_queue[queue_idx].queue_dma;
+
+ lpar_rc = ibmveth_register_logical_lan_queue(adapter, rxq_desc,
+ queue_idx);
+ if (lpar_rc != H_SUCCESS) {
+ netdev_err(netdev, "Failed to register queue %d: rc=0x%lx\n",
+ queue_idx, lpar_rc);
[Severity: Low]
lpar_rc is a signed long here, and ibmveth_register_logical_lan_queue()
returns -EINVAL when irq_create_mapping() fails, so that case prints as
rc=0xffffffffffffffea. Would %ld (or splitting the errno and hypervisor
cases) read better? Note this will not produce a build warning, since the
signed/unsigned format mismatch is only diagnosed under
-Wformat-signedness, which the kernel does not enable.
[ ... ]
v7 uses %ld. The hypervisor fail one function
up already prints that way.
+static void ibmveth_apply_mq_fallback(struct ibmveth_adapter *adapter)
+{
+ struct net_device *netdev = adapter->netdev;
+
+ if (!adapter->mq_fallback)
+ return;
+
+ adapter->mq_fallback = false;
+
+ if (!adapter->multi_queue && ibmveth_get_num_rx_queues(adapter) == 1)
+ return;
+
+ netdev_warn(netdev,
+ "Falling back to single RX queue (firmware MQ
unavailable)\n");
+ adapter->multi_queue = false;
+ ibmveth_publish_num_rx_queues(adapter, 1);
+ /* real_num_rx_queues is set later in open after resources exist. */
[Severity: Medium]
Can the two counts be left permanently out of sync here?
ibmveth_apply_mq_fallback() runs at the very top of ibmveth_open() and
publishes adapter->num_rx_queues = 1, but netdev->real_num_rx_queues is only
updated further down. Four failure exits sit in between:
ibmveth_alloc_filter_list()
ibmveth_alloc_rx_queues()
ibmveth_alloc_buffer_pools()
ibmveth_register_rx_queues()
...
netif_set_real_num_rx_queues(netdev, ibmveth_get_num_rx_queues(adapter))
If the fallback open fails at any of those (for example -ENOMEM from pool
allocation, or -ENONET from queue-0 registration), the device stays down with
adapter->num_rx_queues == 1 while netdev->real_num_rx_queues is still the old
multi-queue value, so /sys/class/net/ethX/queues/rx-1..7 remain present for
queues the driver will never drive.
That contradicts the invariant the probe path documents for exactly this
case:
* alloc_etherdev_mqs() sized real_num_rx_queues to MAX. Match the
* advertised default (or SQ 1) before register_netdev so down-state
* readers agree with adapter->num_rx_queues / ethtool -l.
Would it be better to lower real_num_rx_queues in the fallback path itself,
or to restore the published count on the open failure paths?
real_num is set after the resources exist. If this
open fails first, the old real_num can sit while
num_rx_queues is already 1. The next successful
open calls set_real. The fallback helper only
publishes the count.
@@ -1651,6 +1900,8 @@ static int ibmveth_open(struct net_device *netdev)
netdev_dbg(netdev, "open starting\n");
+ ibmveth_apply_mq_fallback(adapter);
+
for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
rxq_entries += adapter->rx_buff_pool[0][i].size;
@@ -1670,18 +1921,34 @@ static int ibmveth_open(struct net_device *netdev)
if (rc)
goto out_free_buffer_pools;
- rc = netif_set_real_num_rx_queues(netdev, adapter->num_rx_queues);
+ rc = netif_set_real_num_rx_queues(netdev,
+ ibmveth_get_num_rx_queues(adapter));
+
if (rc) {
netdev_err(netdev, "failed to set number of rx queues\n");
goto out_unregister_queues;
}
+ /*
+ * Post buffers before setup_rx_interrupts(). MQ setup then unmasks
+ * PHYP; SQ setup leaves PHYP masked. Scheduling NAPI only when a
+ * descriptor is already pending is not enough: after ifdown/up
+ * (RX=8, no -L) NAPI can be idle with nothing pending and the
+ * queue stays dead (TX OK, ARP/RX fail).
+ * restart_rx_queue() replenishes, schedules NAPI, and unmasks if
+ * prep fails.
+ */
+ for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++) {
+ netdev_dbg(netdev, "initial replenish cycle for queue %d\n", i);
+ ibmveth_replenish_task(adapter, i);
+ }
+
rc = ibmveth_setup_rx_interrupts(adapter);
if (rc)
goto out_free_all_queues; /* setup already disposed IRQs */
- netdev_dbg(netdev, "initial replenish cycle\n");
- ibmveth_schedule_rx_queue(adapter, 0);
+ for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++)
+ ibmveth_restart_rx_queue(adapter, i);
[Severity: Medium]
This replenish loop can queue reset work, and ibmveth_replenish_task()
returns void, so open() cannot see it:
} else if (need_reset == IBMVETH_REPLENISH_RESET_MQ) {
...
adapter->mq_fallback = true;
schedule_work(&adapter->work);
}
If a buffer-add hcall returns H_FUNCTION here and the open then fails at
ibmveth_setup_rx_interrupts() or ibmveth_alloc_tx_resources(), userspace gets
an error back from ndo_open and the device is left down, but the pending
ibmveth_reset() only checks registration state:
rtnl_lock();
if (netdev->reg_state != NETREG_REGISTERED) {
rtnl_unlock();
return;
}
dev_close(adapter->netdev);
dev_open(adapter->netdev, NULL);
so it brings the interface up anyway a moment later. Should the reset worker
also check that the device was successfully opened before reopening it?
No. After a failed open the worker can still
dev_open(); the netdev is registered. mq_fallback
is already latched, so that open comes up
single-queue. That is the H_FUNCTION recovery.
[Severity: Low]
This isn't a bug, but the kernel-doc on ibmveth_setup_rx_interrupts() still
describes the pre-MQ ordering:
* Single-queue open leaves PHYP masked
* here and kicks NAPI afterward (classic path: first poll posts then
* enables).
After this change open() posts buffers for every live queue (queue 0
included, in single-queue mode) before calling setup_rx_interrupts(), and
ibmveth_restart_rx_queue() posts again, so the "first poll posts" behaviour no
longer applies to either mode. Could that comment be refreshed?
v7 refreshes it. Open posts first in both
modes; restart_rx_queue still kicks NAPI
after setup.
@@ -1714,6 +1981,7 @@ static int ibmveth_open(struct net_device *netdev)
static int ibmveth_close(struct net_device *netdev)
{
struct ibmveth_adapter *adapter = netdev_priv(netdev);
+ int i;
/* Gate on opened, not IFF_UP: pool_store/change_mtu close+open can
* leave IFF_UP set after a failed reopen.
@@ -1734,7 +2002,8 @@ static int ibmveth_close(struct net_device *netdev)
/* Wait for softirq/poll that already passed shutdown checks. */
synchronize_net();
- ibmveth_update_rx_no_buffer(adapter);
+ for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++)
+ ibmveth_update_rx_no_buffer(adapter, i);
ibmveth_free_all_queues(adapter);
[ ... ]
@@ -2527,6 +2796,7 @@ static int ibmveth_poll_deliver_frame(struct napi_struct
*napi,
ibmveth_rx_csum_helper(skb, adapter);
}
+ skb_record_rx_queue(skb, queue_index);
napi_gro_receive(napi, skb);
netdev->stats.rx_packets++;
[ ... ]
@@ -2750,9 +3021,14 @@ static int ibmveth_change_mtu(struct net_device *dev,
int new_mtu)
static void ibmveth_poll_controller(struct net_device *dev)
{
struct ibmveth_adapter *adapter = netdev_priv(dev);
+ unsigned int num = ibmveth_get_num_rx_queues(adapter);
+ int i;
- ibmveth_replenish_task(adapter, 0);
- ibmveth_schedule_rx_queue(adapter, 0);
+ for (i = 0; i < num; i++)
+ ibmveth_replenish_task(adapter, i);
+
+ for (i = 0; i < num; i++)
+ ibmveth_schedule_rx_queue(adapter, i);
}
#endif
[Severity: High]
This is a pre-existing race, not one introduced here, but walking every
published queue widens it considerably.
The in-driver callers of ibmveth_close() (veth_pool_store(),
ibmveth_set_csum_offload(), ibmveth_set_tso(), ibmveth_change_mtu()) call
ndo_stop directly rather than through dev_close(), so
netpoll_poll_disable() is never called and netif_running() stays true.
netpoll_poll_dev() can therefore call ndo_poll_controller() while close is
freeing the pools.
The replenish side samples the pool and then dereferences it:
if (pool->active && pool->free_map &&
(atomic_read(&pool->available) < pool->threshold)) {
rc = ibmveth_replenish_buffer_pool(adapter, pool,
queue_index, &fail);
while the freeing side takes no replenish_lock at all:
kfree(pool->free_map);
pool->free_map = NULL;
Can this access freed free_map[]/skbuff[]/dma_addr[] arrays, and the freed
buffer-list page in ibmveth_update_rx_no_buffer()?
A later patch in the series adds an "if (!adapter->opened) return;" check to
ibmveth_poll_controller(), but ibmveth_close() clears opened before it frees
anything and does not wait for an in-flight poll_controller, so the window
appears to remain.
Yes, at this commit. Patch 15 already returns if !opened.
That does not wait for an in-flight poll_controller.
close still skips netpoll_poll_disable, and
free_buffer_pool still skips the lock. Those two
stay with shortening irqsave after this series.
@@ -2781,23 +3056,35 @@ static unsigned long ibmveth_get_desired_dma(struct
vio_dev *vdev)
adapter = netdev_priv(netdev);
- ret = IBMVETH_BUFF_LIST_SIZE + IBMVETH_FILT_LIST_SIZE;
+ /* One buffer list page per RX queue; filter list is shared. */
+ ret = IBMVETH_BUFF_LIST_SIZE * ibmveth_get_num_rx_queues(adapter) +
+ IBMVETH_FILT_LIST_SIZE;
ret += IOMMU_PAGE_ALIGN(netdev->mtu, tbl);
/* add size of mapped tx buffers */
ret += IOMMU_PAGE_ALIGN(IBMVETH_MAX_TX_BUF_SIZE, tbl);
+ for (q = 0; q < ibmveth_get_num_rx_queues(adapter); q++) {
+ int rxqentries = 1;
+
+ for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
+ struct ibmveth_buff_pool *bpool =
+ &adapter->rx_buff_pool[0][i];
+
+ if (bpool->active)
+ ret += bpool->size *
+ IOMMU_PAGE_ALIGN(bpool->buff_size, tbl);
+ rxqentries += bpool->size;
+ }
[ ... ]
@@ -2954,16 +3253,30 @@ static int ibmveth_probe(struct vio_dev *dev, const
struct vio_device_id *id)
netdev->features |= NETIF_F_FRAGLIST;
}
- /* Initialize queue count - always 1 for now */
- adapter->multi_queue = 0;
- adapter->num_rx_queues = IBMVETH_DEFAULT_RX_QUEUES;
+ if (ret == H_SUCCESS &&
+ (ret_attr & IBMVETH_ILLAN_RX_MULTI_QUEUE_SUPPORT)) {
+ adapter->multi_queue = true;
+ ibmveth_publish_num_rx_queues(adapter,
+ min(num_online_cpus(),
+ IBMVETH_DEFAULT_QUEUES));
[ ... ]
@@ -2981,25 +3294,62 @@ static int ibmveth_probe(struct vio_dev *dev, const
struct vio_device_id *id)
for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
struct kobject *kobj = &adapter->rx_buff_pool[0][i].kobj;
- int error;
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,
- &dev->dev.kobj, "pool%d", i);
- if (!error)
- kobject_uevent(kobj, KOBJ_ADD);
+ rc = kobject_init_and_add(kobj, &ktype_veth_pool,
+ &dev->dev.kobj, "pool%d", i);
+ if (rc) {
+ dev_err(&dev->dev,
+ "failed to create pool%d kobject: %d\n", i, rc);
+ /* init_and_add takes a ref even on failure */
+ kobject_put(kobj);
+ ibmveth_put_pool_kobjs(adapter, pools_ready);
+ dev_set_drvdata(&dev->dev, NULL);
+ free_netdev(netdev);
+ return rc;
+ }
[Severity: Medium]
Can this free the memory the kobjects live in before their cleanup runs?
The pool kobjects are embedded in struct ibmveth_adapter, which is
netdev_priv(netdev), and ktype_veth_pool has .release = NULL. With
CONFIG_DEBUG_KOBJECT_RELEASE=y the last kobject_put() goes to
kobject_release(), which defers the cleanup:
lib/kobject.c:kobject_release() {
...
#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
schedule_delayed_work(&kobj->release, delay);
...
}
The delayed_work lives inside the kobject, i.e. inside the storage that
free_netdev() releases immediately on this path (reg_state is
NETREG_UNINITIALIZED), and the pool%d directories stay published against it
until the work fires a few seconds later.
The same put-then-free sequence is repeated on the
netif_set_real_num_tx_queues(), netif_set_real_num_rx_queues() and
register_netdev() failure paths below, and survives into the later
ibmveth_probe_cleanup() helper. The changelog lists "pool kobj vs
DEBUG_KOBJECT_RELEASE" as a leftover - would adding a real release() with a
container that outlives the kobjects, or creating them only after the last
failure point, be the fix?
v7 waits. A real release completes, and last
put waits before free_netdev(). Patch 11
probe_cleanup and remove use that helper.
[Severity: Low]
This isn't a bug, but the policy here changes from "ignore
kobject_init_and_add() failure and keep probing" to "fail the probe", so a
kernfs name collision or -ENOMEM now leaves the interface unavailable
instead of merely missing the pool%d tuning knobs. Checking the return
value is the right thing to do; could the changelog mention the behaviour
change, since it currently only describes adding cleanup?
Failing the probe on kobject_init_and_add() is intended.
The commit message already names the puts.
+ /*
+ * VIO CMO entitlement was set before probe (netdev NULL, so default).
+ * Recompute now that num_rx_queues and pool 0 metadata are known.
+ */
+ if (firmware_has_feature(FW_FEATURE_CMO))
+ vio_cmo_set_dev_desired(dev, ibmveth_get_desired_dma(dev));
+
[Severity: Low]
This isn't a bug, but probe unconditionally selects min(num_online_cpus(),
IBMVETH_DEFAULT_QUEUES) RX queues whenever the firmware bit is set, and each
queue clones queue 0's full pool geometry plus its own buffer-list page and
RX ring. ibmveth_get_desired_dma() therefore multiplies the desired DMA
entitlement by num_rx_queues, and vio_cmo_set_dev_desired() returns void, so
an entitlement request that cannot be granted is not visible here.
On a CMO partition with many CPUs, is defaulting to eight RX queues the
behaviour you want out of the box, or would defaulting to one and letting
ethtool -L opt in be safer? The series does add the runtime opt-out in a
later patch.
Yes. The default is min(num_online_cpus(), 8), same cap as
TX. ethtool -L can raise or lower it later in the series.
CMO is Power9 and earlier; the MQ firmware bit is
Power11+. Those partitions do not coexist, so this is not a CMO
tradeoff. get_desired_dma() still adds one TX LTB,
but that is only the CMO desired hint. The real
TX LTBs already scale with real_num_tx_queues
(kzalloc + dma_map_single). Those two CMO leftovers
come off the v7 cover. For the same reason v7
drops the probe-time CMO desired refresh; a CMO
partition keeps its single-queue default.
rc = netif_set_real_num_tx_queues(netdev, min(num_online_cpus(),
IBMVETH_DEFAULT_QUEUES));
if (rc) {
netdev_dbg(netdev, "failed to set number of tx queues rc=%d\n",
rc);
+ ibmveth_put_pool_kobjs(adapter, pools_ready);
+ dev_set_drvdata(&dev->dev, NULL);
+ free_netdev(netdev);
+ return rc;
+ }
+
+ /*
+ * alloc_etherdev_mqs() sized real_num_rx_queues to MAX. Match the
+ * advertised default (or SQ 1) before register_netdev so down-state
+ * readers agree with adapter->num_rx_queues / ethtool -l.
+ */
+ rc = netif_set_real_num_rx_queues(netdev,
+ ibmveth_get_num_rx_queues(adapter));
[Severity: Low]
With real_num_rx_queues now published as up to eight before
register_netdev(), ibmveth_get_channels() starts reporting max_rx and
rx_count greater than one:
channels->max_rx = netdev->real_num_rx_queues;
channels->rx_count = netdev->real_num_rx_queues;
but ibmveth_set_channels() at this commit only reads channels->tx_count.
Does "ethtool -L ethX rx 4" then pass the core's max_rx check, get silently
discarded by the driver and report success?
Yes, at this commit. set_channels only reads
tx_count, so rx 4 is discarded and the call
succeeds. Patch 12 rejects an RX count change
with -EOPNOTSUPP; Patch 14 implements live
resize.
Thanks,
Mingming
The changelog acknowledges this ("This commit does not implement
set_channels / rx_count") and patch 14 wires rx_count up, so the question is
whether the advertisement should wait for the implementation.
[ ... ]
