Re: [net-next,v6,10/15] ibmveth: Enable multi-queue RX receive path

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?

[ ... ]

[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?

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.

[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?

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.

[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);

[ ... ]

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

[ ... ]

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

[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?

[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?

[ ... ]

[ ... ]

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

[ ... ]

[ ... ]

[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?

[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?

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

 	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?

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.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788102125.git.mmc%40linux.ibm.com