On 8/17/26 6:47 PM, Jakub Kicinski wrote:
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: Add queue-aware RX buffer submit helper for MQ
This adds ibmveth_add_logical_lan_buffers() to pick between the legacy
single/multi buffer hcalls and the new per-queue H_ADD_LOGICAL_LAN_BUFFERS_QUEUE
hcall, threads queue_index through the RX replenish/poll helpers, and
introduces a per-queue replenish_lock plus deferred (post-unlock) failure
logging and reset scheduling.
A few questions below about the new locking scope and about some of the
error paths that changed behaviour.
diff --git a/drivers/net/ethernet/ibm/ibmveth.c
b/drivers/net/ethernet/ibm/ibmveth.c
index 8519cad503225..58a639a962a67 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
@@ -262,6 +272,7 @@ ibmveth_alloc_rx_queues(struct ibmveth_adapter *adapter,
int rxq_entries)
adapter->rx_queue[i].index = 0;
adapter->rx_queue[i].num_slots = rxq_entries;
adapter->rx_queue[i].toggle = 1;
+ spin_lock_init(&adapter->rx_queue[i].replenish_lock);
[Severity: Medium]
Is it safe to re-initialize replenish_lock here, given that
ibmveth_alloc_rx_queues() runs on every ndo_open?
The driver restarts itself directly in several places, guarded only by
netif_running(), for example ibmveth_set_csum_offload():
if (netif_running(dev)) {
restart = 1;
ibmveth_close(dev);
}
...
if (restart)
rc2 = ibmveth_open(dev);
veth_pool_store() and the MTU change path do the same. Because
__LINK_STATE_START is never cleared on these paths, netpoll_poll_disable()
is never called, and netpoll_poll_dev() only bails out on !npinfo or a held
dev_lock (dev_lock is taken solely by netpoll_poll_disable() from
__dev_open/__dev_close_many).
So can this sequence happen while netconsole is configured on this device?
CPU0 (printk -> netconsole)
ibmveth_poll_controller()
ibmveth_replenish_task(adapter, 0)
spin_lock_irqsave(&rxq->replenish_lock, flags);
CPU1 (ethtool -K restart)
ibmveth_open()
ibmveth_alloc_rx_queues()
spin_lock_init(&adapter->rx_queue[0].replenish_lock);
That resets a held spinlock to unlocked, and the later unlock on CPU0 runs
against re-initialized state.
Yes. v6 initialises the locks once in probe. open() does not touch
them. netpoll_poll_disable() on the internal restarts is still the
better general answer; it is not required for this race.
[ ... ]
-/* replenish the buffers for a pool. note that we don't need to
- * skb_reserve these since they are used for incoming...
+/**
+ * ibmveth_add_logical_lan_buffers - Add receive buffers to hypervisor
+ * @adapter: ibmveth adapter structure
+ * @descs: array of buffer descriptors to add
+ * @filled: number of valid descriptors in the array
+ * @buff_size: size of each buffer (multi-queue mode only)
+ * @queue_index: RX queue index
+ *
+ * Return: hypervisor return code
*/
-static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
- struct ibmveth_buff_pool *pool)
+static long ibmveth_add_logical_lan_buffers(struct ibmveth_adapter *adapter,
+ union ibmveth_buf_desc *descs,
+ int filled,
+ unsigned long buff_size,
+ int queue_index)
+{
+ struct vio_dev *vdev = adapter->vdev;
+ unsigned long rc;
+
+ if (adapter->multi_queue) {
+ unsigned long buffersznum = (buff_size << 32) | filled;
+ unsigned long ioba[IBMVETH_MAX_RX_PER_HCALL / 2] = {0};
+ unsigned long handle = adapter->queue_handle[queue_index];
+ int i;
[Severity: Low]
This call site passes adapter->queue_handle[queue_index], which includes
queue 0, but the kernel-doc of h_add_logical_lan_buffers_queue() in
ibmveth.h describes it as "Add buffers to subordinate queue" with
"@queue_handle: Queue handle from h_register_logical_lan_queue()", and its
Return section lists only H_SUCCESS/H_PARAMETER/H_HARDWARE.
The queue 0 handle comes from h_register_logical_lan_with_handle(), whose
own doc says the handle is "needed in multi-queue mode to use
h_add_logical_lan_buffers_queue() for all queues including queue 0", and
ibmveth_replenish_buffer_pool() has dedicated recovery for H_FUNCTION from
this hcall.
Could the h_add_logical_lan_buffers_queue() kernel-doc be updated to cover
the primary queue handle and H_FUNCTION?
Yes. v6 widens @queue_handle to cover both register helpers and adds
H_FUNCTION to the Return section. That is in the P01 kdoc.
[ ... ]
+ rc = h_add_logical_lan_buffers_queue(vdev->unit_address,
+ handle,
+ buffersznum,
+ ioba[0], ioba[1], ioba[2],
+ ioba[3], ioba[4], ioba[5]);
+ adapter->hcall_stats.add_bufs_queue++;
+ } else if (filled == 1) {
+ rc = h_add_logical_lan_buffer(vdev->unit_address,
+ descs[0].desc);
+ adapter->hcall_stats.add_buf++;
+ } else {
+ rc = h_add_logical_lan_buffers(vdev->unit_address,
+ descs[0].desc, descs[1].desc,
+ descs[2].desc, descs[3].desc,
+ descs[4].desc, descs[5].desc,
+ descs[6].desc, descs[7].desc);
+ adapter->hcall_stats.add_bufs++;
+ }
[Severity: Low]
IBMVETH_MAX_RX_PER_HCALL changes from 8 to 12 in this patch, which resizes
descs[] and correlators[] in ibmveth_replenish_buffer_pool() to 12, but this
legacy branch still hands only descs[0..7] to the 8-descriptor
h_add_logical_lan_buffers(), and the caller then accounts all "filled"
descriptors as posted (consumer_index advanced, pool->available
incremented).
The combination needed to hit that (multi_queue == 0 with
rx_buffers_per_hcall > 8) does not appear reachable, since probe now sets
IBMVETH_MAX_RX_REGULAR (8) or 1, and later in the series
ibmveth_apply_mq_fallback() clamps to IBMVETH_MAX_RX_REGULAR.
Would a clamp on filled here, or a BUILD_BUG_ON tying the legacy branch to
IBMVETH_MAX_RX_REGULAR, be worth adding so the arity contract is enforced
rather than implied?
Also, the 12-address MQ packing (ioba[4], ioba[5]) cannot be exercised at
this commit, since probe caps the batch at 8:
- adapter->rx_buffers_per_hcall = IBMVETH_MAX_RX_PER_HCALL;
+ adapter->rx_buffers_per_hcall = IBMVETH_MAX_RX_REGULAR;
Unreachable with the REGULAR cap. v6 documents the legacy branch and
static_asserts the two defines against the hand-written argument
lists. No clamp.
[ ... ]
+/* Replenish the buffers for a pool.
+ * Caller must hold the per-queue replenish_lock. Do not printk here —
+ * netconsole on the same device can re-enter replenish_task.
+ */
+static int ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
+ struct ibmveth_buff_pool *pool,
+ int queue_index,
+ struct ibmveth_replenish_fail *fail)
{
[Severity: High]
The "do not printk here" invariant is documented for this function, but can
code called from inside the critical section still printk indirectly?
dma_map_single() in the fill loop goes through iommu_map_page() on pseries,
and the driver passes attrs = 0:
arch/powerpc/kernel/iommu.c:iommu_map_page() {
...
if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
dev_info(dev, "iommu_alloc failed, tbl %p vaddr %p npages
%d\n", ...);
...
}
That is exactly the condition replenish hits (TCE table or CMO entitlement
exhausted under pressure). With netconsole on this device, the printk
recurses:
printk -> write_msg -> netpoll_send_udp -> __netpoll_send_skb
-> netpoll_poll_dev -> ibmveth_poll_controller
-> ibmveth_replenish_task -> spin_lock_irqsave(&rxq->replenish_lock)
Since the lock is already held on this CPU with interrupts disabled, does
this self-deadlock? The same re-entry also reaches
ibmveth_rxq_harvest_buffer() -> ibmveth_remove_buffer_from_pool(), which now
takes the same lock.
Yes, it self-deadlocks: the iommu warning re-enters replenish (and
harvest) while this CPU already holds the lock. v5 already moved the
driver logs out of the CS; v6 passes DMA_ATTR_NO_WARN on that map so
the iommu path cannot printk under it. Shortening the irqsave hold
is still the real fix, so the invariant does not depend on auditing
every callee. Not this series.
[ ... ]
@@ -883,21 +971,85 @@ static void ibmveth_update_rx_no_buffer(struct
ibmveth_adapter *adapter)
}
/* replenish routine */
-static void ibmveth_replenish_task(struct ibmveth_adapter *adapter)
+static void ibmveth_replenish_task(struct ibmveth_adapter *adapter,
+ int queue_index)
{
- int i;
+ struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
+ struct ibmveth_replenish_fail fail = {};
+ unsigned long flags;
+ int i, rc;
+ int need_reset = 0;
+ int batch_fallback = 0;
+ int hcall_fail = 0;
+
+ if (queue_index >= adapter->num_rx_queues) {
+ netdev_dbg(adapter->netdev,
+ "Skipping replenish for freed queue %d
(num_queues=%d)\n",
+ queue_index, adapter->num_rx_queues);
+ return;
+ }
adapter->replenish_task_cycles++;
[Severity: Medium]
Several adapter-global fields are updated while holding only a per-queue
lock, or none at all. replenish_task_cycles++ here is outside the lock, and
inside ibmveth_replenish_buffer_pool() / ibmveth_add_logical_lan_buffers()
the following are adapter-wide:
batch = adapter->rx_buffers_per_hcall;
...
adapter->rx_buffers_per_hcall = 1;
adapter->hcall_stats.add_buf/add_bufs/add_bufs_queue++;
adapter->replenish_add_buff_success/failure, replenish_no_mem
Two queues replenishing on different CPUs hold different replenish_lock
instances, so the intersection of locks protecting these is empty. Can this
lose or tear the ethtool-visible hcall_stats and replenish_* counters? Today
it is already racy between ibmveth_poll_controller() and NAPI for
replenish_task_cycles++, which is incremented before the lock is taken.
Yes. hcall_stats is gone (patch 7). replenish_* move per-queue in
patch 11, plain u64 under that queue's lock, summed on ethtool read.
No atomics. rx_buffers_per_hcall stays adapter-global; the batch-
fallback write under one queue's lock still races another queue's
read. It is a capability latch (1, 8, or 12), not a counter: on
H_FUNCTION both queues write the same value (1), so the race is
benign — one extra single-buffer replenish cycle at worst. Not
fixed this series.
- for (i = (IBMVETH_NUM_BUFF_POOLS - 1); i >= 0; i--) {
- struct ibmveth_buff_pool *pool = &adapter->rx_buff_pool[0][i];
+ spin_lock_irqsave(&rxq->replenish_lock, flags);
- if (pool->active &&
- (atomic_read(&pool->available) < pool->threshold))
- ibmveth_replenish_buffer_pool(adapter, pool);
+ for (i = (IBMVETH_NUM_BUFF_POOLS - 1); i >= 0; i--) {
+ struct ibmveth_buff_pool *pool =
+ &adapter->rx_buff_pool[queue_index][i];
+
+ if (pool->active && pool->free_map &&
+ (atomic_read(&pool->available) < pool->threshold)) {
+ rc = ibmveth_replenish_buffer_pool(adapter, pool,
+ queue_index, &fail);
[Severity: High]
How long can interrupts stay disabled here?
The lock is taken with spin_lock_irqsave() around the whole five-pool walk,
and ibmveth_replenish_buffer_pool() loops "while (remaining > 0)" doing
netdev_alloc_skb() plus dma_map_single() per buffer, then a hypercall per
batch. With pool_active { 1, 1, 0, 0, 1 } and pool_count { 256, 512, 256,
256, 256 }, a cold start posts roughly 1024 buffers, including 256 of
buff_size 64 KB.
ibmveth_open() hits this deterministically:
for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++)
ibmveth_replenish_task(adapter, i);
So every ifup does on the order of a thousand GFP_ATOMIC allocations
(some high order), a thousand IOMMU mappings and ~128 hypercalls with local
interrupts off. Previously this ran with interrupts enabled and no lock.
Can that trip the hard lockup watchdog or produce RCU stalls, and does
disabling BH/IRQ progress here also make the GFP_ATOMIC allocations more
likely to fail exactly when memory is tight? Since
ibmveth_remove_buffer_from_pool() now takes the same lock per received
packet, the NAPI consumer is also blocked for the whole region.
The changelog notes "Defer adapter-global counter atomics and irqsave
critical-section shorten to cover follow-up" - would it be better to shorten
the region in this patch, since as merged the region exists?
Yes to all three: the window is wide enough to risk watchdog/RCU
stalls, GFP_ATOMIC failures under pressure, and it blocks the NAPI
consumer for the whole fill. The lock is necessary — poll_controller
vs NAPI needs irqsave — but the window is wider than it needs to be.
Prepare-outside or per-batch unlock would shorten it but both are
lock-protocol rewrites, not a one-liner on this patch. Not this
series.
[ ... ]
+out_unlock:
ibmveth_update_rx_no_buffer(adapter);
[Severity: Low]
This helper is now called per queue, but ibmveth_update_rx_no_buffer() still
reads queue 0's buffer-list page:
__be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
Each RX queue has its own buffer-list page whose last 8 bytes hold that
queue's no-buffer drop count, so rx_no_buffer would under-report queues
1..N. This is addressed later in the series, where the helper takes
queue_index and uses buffer_list_addr[queue_index], and MQ is only turned on
by a later patch, so no wrong-page read looks reachable at this commit.
Would it be cleaner to make the helper queue-aware in the same patch that
makes its caller queue-aware?
The helper becomes queue-aware in patch 10, with MQ enablement —
the natural pairing since queue 0 is the only active queue until
then.
+
+ spin_unlock_irqrestore(&rxq->replenish_lock, flags);
+
+ /* Log and schedule reset only after dropping replenish_lock. */
+ if (need_reset == IBMVETH_REPLENISH_RESET_MAP) {
+ netdev_info(adapter->netdev,
+ "Invalid RX free_map entry on queue %d, reset\n",
+ queue_index);
+ schedule_work(&adapter->work);
[Severity: Low]
This isn't a bug introduced by this patch, but the new schedule_work() sites
add more ways to arm adapter->work from NAPI context, and ibmveth_remove()
still cancels before it stops the device:
cancel_work_sync(&adapter->work);
...
unregister_netdev(netdev);
free_netdev(netdev);
RX activity between cancel_work_sync() and the ndo_close() done by
unregister_netdev() can re-queue the work, which may then run after
free_netdev(). The ordering is swapped later in this series
(unregister_netdev() first, then cancel_work_sync()), with a comment
describing exactly this window.
Yes at this commit. Patch 11 swaps the order: unregister_netdev()
first, then cancel_work_sync(), closing the window.
+ } else if (need_reset == IBMVETH_REPLENISH_RESET_MQ) {
+ dev_err_ratelimited(&adapter->netdev->dev,
+ "MQ buffer add H_FUNCTION (q=%d, batch=%u),
reset\n",
+ queue_index, fail.batch);
+ schedule_work(&adapter->work);
+ }
+
+ if (batch_fallback)
+ dev_warn_ratelimited(&adapter->netdev->dev,
+ "Legacy batch add H_FUNCTION (batch=%u),
fallback\n",
+ fail.batch);
+
+ if (hcall_fail)
+ dev_warn_ratelimited(&adapter->netdev->dev,
+ "RX %s failed: filled=%u, rc=%lu,
batch=%u\n",
+ adapter->multi_queue ?
+ "h_add_logical_lan_buffers_queue" :
+ (fail.batch == 1 ?
+ "h_add_logical_lan_buffer" :
+ "h_add_logical_lan_buffers"),
+ fail.filled, fail.lpar_rc, fail.batch);
}
[Severity: Low]
Does this pick the wrong hcall name? The dispatch in
ibmveth_add_logical_lan_buffers() selects on filled, not batch:
} else if (filled == 1) {
rc = h_add_logical_lan_buffer(vdev->unit_address,
descs[0].desc);
If batch is 8 and the fill loop breaks early after one buffer
(netdev_alloc_skb() or dma_mapping_error() failure), the failing call was
h_add_logical_lan_buffer(), but this prints h_add_logical_lan_buffers.
Should the selector be fail.filled == 1?
Separately, one "struct ibmveth_replenish_fail fail" and the
batch_fallback/hcall_fail flags are shared by all five pool iterations, so a
later pool's failure overwrites an earlier pool's rc/filled/batch, and one
log line can mix values from two pools.
Yes. Dispatch uses filled; the fail log still selects on fail.batch,
so an early fill break can mis-name the single-buffer hcall, and one
shared fail struct can mix two pools. Log-only. Not reshuffled here.
/* empty and free ana buffer pool - also used to do cleanup in error paths */
@@ -932,6 +1084,14 @@ static void ibmveth_free_buffer_pool(struct
ibmveth_adapter *adapter,
kfree(pool->skbuff);
pool->skbuff = NULL;
}
+
+ /*
+ * 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 (TX OK, ARP/RX
+ * dead) at any queue count, including RX=8 with no -L.
+ */
+ atomic_set(&pool->available, 0);
}
[Severity: Low]
This comment describes a regression that does not exist in the tree: no
version of ibmveth_free_buffer_pool() ever cleared pool->active, and this
hunk is additive only. The same applies to the new comment in
ibmveth_rxq_correlator_valid() ("free_buffer_pool() clears skbuff but used
to leave size/active set").
References to "v3" and to lab options like "RX=8 with no -L" aren't
meaningful to a reader of mainline. Could the comments be rewritten in terms
of the code as it stands, and could the functional change here
(atomic_set(&pool->available, 0)) be mentioned in the commit message? Since
ibmveth_alloc_buffer_pool() already zeroes available on reallocation, it is
hard to tell from the patch whether this is a fix or a no-op.
Yes. v6 rewrote both comments. The message names the available=0
clear: teardown must not leave a stale count.
[Severity: High]
This is a pre-existing race, but the new locking makes it look handled while
leaving it open. ibmveth_free_buffer_pool() kfrees pool->free_map,
pool->skbuff and pool->dma_addr without taking the per-queue
replenish_lock, while ibmveth_replenish_task() checks and uses those arrays
under that lock:
if (pool->active && pool->free_map &&
(atomic_read(&pool->available) < pool->threshold)) {
So the check is a TOCTOU: replenish touches the pool arrays with
{rxq->replenish_lock} held, the free path touches and frees them with no
lock, giving an empty lock intersection.
Reachable through a direct close, which does not run
netpoll_poll_disable():
CPU0 (printk -> netconsole)
ibmveth_poll_controller() -> ibmveth_replenish_task(adapter, 0)
passes "pool->free_map != NULL", then dereferences
pool->free_map / pool->skbuff / pool->dma_addr
CPU1 (ethtool -K / veth_pool_store / MTU change)
ibmveth_close() -> ibmveth_free_buffer_pools()
-> kfree(pool->free_map)
Can that produce a use-after-free of those arrays, and post freed buffers to
the hypervisor? Should ibmveth_free_buffer_pool() take replenish_lock (or
should the pool teardown otherwise be serialized against replenish)?
Yes. The intersection is empty, so that TOCTOU is a UAF. A lock in
free_buffer_pool() is the natural fix; it is not in this series.
Taking it while replenish still holds irqsave across the whole fill
needs the same lock-protocol rewrite as the CS shorten, and close
still lacks netpoll_poll_disable. Doing the lock alone just moves
the problem.
[ ... ]
+static bool ibmveth_rxq_correlator_valid(struct ibmveth_adapter *adapter,
+ int queue_index, u64 correlator)
+{
+ unsigned int pool = correlator >> 32;
+ unsigned int index = correlator & 0xffffffffUL;
+ struct ibmveth_buff_pool *bpool;
+
+ if (pool >= IBMVETH_NUM_BUFF_POOLS)
+ return false;
+
+ bpool = &adapter->rx_buff_pool[queue_index][pool];
+
+ /* init_buffer_pool() sets size for inactive pools; free_buffer_pool()
+ * clears skbuff but used to leave size/active set. Require a live
+ * pool with allocated arrays before indexing.
+ */
+ if (!bpool->active || !bpool->skbuff || !bpool->free_map)
+ return false;
+
+ return index < bpool->size;
+}
[Severity: Medium]
Do the in-tree KUnit cases still pass with the added
"!bpool->free_map" precondition? Neither
ibmveth_remove_buffer_from_pool_test() nor ibmveth_rxq_get_buffer_test()
allocates free_map - both only kunit_kcalloc() pool->skbuff.
That makes validation fail before the skbuff[index] NULL check, so the two
-EFAULT expectations should now receive -EINVAL:
correlator = (u64)0 | 0;
pool->skbuff[0] = NULL;
KUNIT_EXPECT_EQ(test, -EFAULT,
ibmveth_remove_buffer_from_pool(adapter,
correlator, 0, false));
and the positive expectation in ibmveth_rxq_get_buffer_test() should receive
NULL:
pool->skbuff[0] = skb;
adapter->rx_queue[0].queue_addr[0].correlator = (u64)0 << 32 | 0;
KUNIT_EXPECT_PTR_EQ(test, skb, ibmveth_rxq_get_buffer(adapter, 0));
The index-out-of-range expectations also pass vacuously now, so the bound
check they were written for is no longer covered. The patch updates the
kernel-doc of both tests but not the fixtures - should the fixtures also
allocate free_map?
Yes. Both fixtures allocate a dummy free_map in v6.
[ ... ]
* Return:
* * %0 - success
* * %-EINVAL - correlator maps to pool or index out of range
* * %-EFAULT - pool and index map to null skb
*/
static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
- u64 correlator, bool reuse)
+ u64 correlator, int queue_index,
+ bool reuse)
{
[Severity: Low]
The documented meaning of -EINVAL is now incomplete: via
ibmveth_rxq_correlator_valid() this function also returns -EINVAL for an
inactive pool and for a pool whose skbuff/free_map arrays are not
allocated, which is a different condition from "out of range". The new
ibmveth_rxq_harvest_buffer() kernel-doc reinforces the narrow reading by
telling callers that -EINVAL/-EFAULT mean a "corrupt slot" safe to skip.
Could the return-code documentation be extended to cover the not-live-pool
case?
Fair. correlator_valid also rejects inactive and not-live pools, so
-EINVAL is broader than “out of range.” Not extended in this series.
+ struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
unsigned int pool = correlator >> 32;
unsigned int index = correlator & 0xffffffffUL;
unsigned int free_index;
struct sk_buff *skb;
+ unsigned long flags;
+ int rc = 0;
- if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
- WARN_ON(index >= adapter->rx_buff_pool[0][pool].size)) {
- schedule_work(&adapter->work);
- return -EINVAL;
+ spin_lock_irqsave(&rxq->replenish_lock, flags);
+
+ if (!ibmveth_rxq_correlator_valid(adapter, queue_index, correlator)) {
+ rc = -EINVAL;
+ goto out_unlock;
}
- skb = adapter->rx_buff_pool[0][pool].skbuff[index];
- if (WARN_ON(!skb)) {
- schedule_work(&adapter->work);
- return -EFAULT;
+ skb = adapter->rx_buff_pool[queue_index][pool].skbuff[index];
+ if (!skb) {
+ rc = -EFAULT;
+ goto out_unlock;
}
[ ... ]
static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
- bool reuse)
+ int queue_index, bool reuse)
{
+ struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
u64 cor;
int rc;
- struct ibmveth_rx_q *rxq = &adapter->rx_queue[0];
-
cor = rxq->queue_addr[rxq->index].correlator;
- rc = ibmveth_remove_buffer_from_pool(adapter, cor, reuse);
- if (unlikely(rc))
+ rc = ibmveth_remove_buffer_from_pool(adapter, cor, queue_index, reuse);
+ if (unlikely(rc)) {
+ /* Skip a corrupt slot without claiming pool ownership. */
+ if (rc == -EINVAL || rc == -EFAULT)
+ ibmveth_rxq_advance(rxq);
return rc;
-
- if (++adapter->rx_queue[0].index == adapter->rx_queue[0].num_slots) {
- adapter->rx_queue[0].index = 0;
- adapter->rx_queue[0].toggle = !adapter->rx_queue[0].toggle;
}
[Severity: Medium]
The reset that previously repaired this state is gone -
ibmveth_remove_buffer_from_pool() no longer calls
schedule_work(&adapter->work) for -EINVAL/-EFAULT, and here the ring is just
advanced. Does that permanently retire the buffer the hypervisor already
consumed?
On this path free_map does not regain the index, the
atomic_dec(&pool->available) is skipped, and pool->skbuff[index] plus its
DMA mapping stay owned by nobody.
Since replenish is gated on available < threshold (size * 7 / 8):
if (pool->active && pool->free_map &&
(atomic_read(&pool->available) < pool->threshold)) {
after roughly size/8 such events, does replenish stop for that pool
permanently, with the link still up and no recovery short of ifdown/up?
The poll "recycling invalid buffer" path and ibmveth_drain_rx_queue() both
swallow these errors without escalating:
/* -EINVAL/-EFAULT already advanced past the slot. */
if (rc == -EINVAL || rc == -EFAULT) {
drained++;
continue;
}
At the end of the series only the get_buffer path escalates via
ibmveth_poll_skip_bad_correlator(), and that code's own comment ("Residual
stale slot after resize") suggests stale correlators do occur. Should the
recycle and drain paths also reconcile available or schedule a reset?
Yes, a burst of bad slots can starve that pool permanently. Recycle
and drain stay advance-only: a reset is not always the right
recovery for invalid pool state, and drain runs during teardown.
available is not reconciled either — stale count is an open
limitation. Patch 9 skip_bad_correlator is the resize stale-slot
path and does reset, because that case has a known recovery.
[ ... ]
@@ -2127,34 +2340,41 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
- skb = ibmveth_rxq_get_buffer(adapter);
+ skb = ibmveth_rxq_get_buffer(adapter, queue_index);
if (unlikely(!skb))
break;
[Severity: Low]
This isn't a bug introduced by this patch, but breaking out here leaves
rxq->index and rxq->toggle untouched, so the descriptor stays pending. The
tail of ibmveth_poll() then completes NAPI, re-enables the IRQ, and:
if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
napi_schedule(napi)) {
rc = ibmveth_disable_irq(adapter, queue_index);
WARN_ON(rc);
goto restart_poll;
}
Can that spin on the same slot in softirq context when a VALID descriptor
has a correlator that fails validation, or whose pool->skbuff[index] is
NULL? The changelog says this lands with the poll harden patch ("Bad
queue_index poll path: napi_complete before return lands with poll
harden"), and later in the series
ibmveth_poll_skip_bad_correlator() schedules a reset and advances the ring.
Harvest -EINVAL/-EFAULT already advances here. A VALID correlator with
a NULL skb still breaks without advancing; P09 skip_bad closes that.
[ ... ]
diff --git a/drivers/net/ethernet/ibm/ibmveth.h
b/drivers/net/ethernet/ibm/ibmveth.h
index fae3473cc4985..d02444d5b3b89 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -259,7 +261,8 @@ static inline long h_illan_attributes(unsigned long
unit_address,
#define IBMVETH_DEFAULT_QUEUES 8U
#define IBMVETH_MAX_RX_QUEUES 1U
#define IBMVETH_DEFAULT_RX_QUEUES 1U
-#define IBMVETH_MAX_RX_PER_HCALL 8U
+#define IBMVETH_MAX_RX_REGULAR 8U
+#define IBMVETH_MAX_RX_PER_HCALL 12U
The change of IBMVETH_MAX_RX_PER_HCALL from 8 to 12 is described in the
commit message only as "up to 12 buffers" for the MQ hcall - could the
commit message also state that the existing constant is being redefined, so
that the descs[]/correlators[] resize in ibmveth_replenish_buffer_pool() and
the new IBMVETH_MAX_RX_REGULAR use in probe are easy to follow?
Yes. v6 commit message did that.
@@ -301,6 +304,7 @@ struct ibmveth_rx_q {
dma_addr_t queue_dma;
u32 queue_len;
struct ibmveth_rx_q_entry *queue_addr;
+ spinlock_t replenish_lock; /* per-queue buffer replenish */
};
This isn't a bug, but the new member uses tabs while the surrounding members
in this struct use the older space-based alignment - was the mixed style
intentional?
Not intentional. Oh.. hmm, in v6; the new member still uses tabs
while the surrounding struct members use spaces.
Regards,
Mingming
