Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] This patch raises IBMVETH_MAX_RX_PER_HCALL to 12, so
descs[]/correlators[] in ibmveth_replenish_buffer_pool() now hold 12…
- [Low] The subject, "Add queue-aware RX buffer submit helper for MQ",
reads like a prep-only patch, but the patch also changes behaviour on…
- [Low] This patch makes ibmveth_rxq_correlator_valid() check
`!bpool->active || !bpool->skbuff || !bpool->free_map`, with the…
- [Low] When multi_queue is set, an H_FUNCTION from
H_ADD_LOGICAL_LAN_BUFFERS_QUEUE yields IBMVETH_REPLENISH_RESET_MQ, and…
--
ibmveth: Add queue-aware RX buffer submit helper for MQ
This patch passes a queue index through the RX accessors, the replenish
path and buffer pool access. It also adds ibmveth_add_logical_lan_buffers(),
which uses H_ADD_LOGICAL_LAN_BUFFERS_QUEUE in multi-queue mode and the
legacy hcalls otherwise. It adds a per-queue replenish_lock, gives the
replenish code return values so logging happens after unlock, and raises
IBMVETH_MAX_RX_PER_HCALL to 12.
> ibmveth: Add queue-aware RX buffer submit helper for MQ
[Severity: Low]
This isn't a bug, but the subject reads like a prep-only change. The patch
also changes behaviour on the existing single-queue path:
- a per-queue spin_lock_bh() around harvest/remove and replenish
- IBM_VETH_INVALID_MAP handling (no WARN, the partial batch is dropped,
a reset is scheduled)
- DMA_ATTR_NO_WARN on the RX maps
- reworked hcall-failure, fallback and IRQ-enable logging
- pool->available cleared in ibmveth_free_buffer_pool()
- a stricter ibmveth_rxq_correlator_valid() that now also requires an
active pool and a free_map
The body does document these ("The correlator and locking changes do take
effect immediately, on the single queue").
Would bisecting or reverting be easier if the single-queue behaviour
changes were split out from the MQ plumbing?
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index e2aa108655676..a1f286b37effa 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -726,11 +728,103 @@ static inline void ibmveth_flush_buffer(void *addr,
> unsigned long length)
[ ... ]
> + } else if (filled == 1) {
> + rc = h_add_logical_lan_buffer(vdev->unit_address,
> + descs[0].desc);
> + } else {
> + /* Legacy 8-desc hcall; probe/mq_fallback keep batch <=
> + * IBMVETH_MAX_RX_REGULAR.
> + */
> + 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);
> + }
[Severity: Low]
This legacy branch always passes descs[0] through descs[7] and never looks
at filled. Because IBMVETH_MAX_RX_PER_HCALL is now 12, descs[] in
ibmveth_replenish_buffer_pool() holds 12 entries.
The comment says "probe/mq_fallback keep batch <= IBMVETH_MAX_RX_REGULAR".
There is no mq_fallback anywhere in the driver at this commit. Is the
comment describing a guard that only arrives later in the series?
Nothing appears to trigger this today. At this commit probe sets
multi_queue = 0, and it sets rx_buffers_per_hcall to either
IBMVETH_MAX_RX_REGULAR or 1. Later in the series,
ibmveth_apply_mq_fallback() clears multi_queue and clamps
rx_buffers_per_hcall to IBMVETH_MAX_RX_REGULAR at the start of open, so
the limit seems to hold across the series.
Suppose rx_buffers_per_hcall did go above 8 with multi_queue false.
Descriptors 8 to 11 would never be posted. The success path in
ibmveth_replenish_buffer_pool() would still set every filled free_map slot
to IBM_VETH_INVALID_MAP and add filled to pool->available.
Would a WARN_ON_ONCE(filled > IBMVETH_MAX_RX_REGULAR) here be worth adding,
or a comment that does not mention mq_fallback?
[ ... ]
> @@ -917,21 +1014,91 @@ static void ibmveth_update_rx_no_buffer(struct
> ibmveth_adapter *adapter)
[ ... ]
> + /* 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);
> + } 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);
> + }
[Severity: Low]
In multi-queue mode, when H_ADD_LOGICAL_LAN_BUFFERS_QUEUE returns
H_FUNCTION, ibmveth_replenish_buffer_pool() returns
IBMVETH_REPLENISH_RESET_MQ. ibmveth_replenish_task() then only logs and
calls schedule_work().
At this commit nothing records a fallback to single-queue mode. Wouldn't
the reset reopen in MQ mode, get H_FUNCTION from the same hcall again, and
loop?
This arm can't be reached here, because probe hard-codes multi_queue = 0.
The commit message says so too. Later in the series ("ibmveth: Complete
set_channels down-path"), this arm calls
WRITE_ONCE(adapter->mq_fallback, true) before schedule_work(), and
ibmveth_open() calls ibmveth_apply_mq_fallback(), which clears multi_queue.
Could the commit message say that recovery for this arm depends on those
later patches?
[ ... ]
> @@ -1133,33 +1298,77 @@ ibmveth_free_buffer_pools(struct ibmveth_adapter
> *adapter)
[ ... ]
> + bpool = &adapter->rx_buff_pool[queue_index][pool];
> +
> + /* Require a live pool with allocated arrays before indexing.
> + * Inactive pools still have size from init; free clears skbuff.
> + */
> + if (!bpool->active || !bpool->skbuff || !bpool->free_map)
> + return false;
[ ... ]
> @@ -3146,26 +3401,41 @@ static void
> ibmveth_remove_buffer_from_pool_test(struct kunit *test)
[ ... ]
> /* Pool 2 is in range but has no skbuff array, like an inactive pool. */
> correlator = ((u64)2 << 32) | 0;
> - ret = ibmveth_remove_buffer_from_pool(adapter, correlator, false);
> + ret = ibmveth_remove_buffer_from_pool(adapter, correlator, 0, false);
> KUNIT_EXPECT_EQ(test, -EINVAL, ret);
> - ret = ibmveth_remove_buffer_from_pool(adapter, correlator, true);
> + ret = ibmveth_remove_buffer_from_pool(adapter, correlator, 0, true);
> KUNIT_EXPECT_EQ(test, -EINVAL, ret);
[Severity: Low]
Does this case still reach the !bpool->skbuff check in
ibmveth_rxq_correlator_valid()?
pool_active[] in ibmveth.h is { 1, 1, 0, 0, 1 }, so pool 2 is inactive. The
new !bpool->active test comes first, so this case is rejected there before
the skbuff check runs. Before this patch it did exercise the skbuff check.
The pool 2 case in ibmveth_rxq_get_buffer_test() has the same problem.
No case covers an active pool with skbuff set and free_map NULL, so the new
!bpool->free_map check is not tested at all. If either the skbuff check or
the free_map check were deleted, every KUnit case would still pass.
These tests are unchanged at the end of the series. Would it make sense to
add cases for an active pool with a NULL skbuff and an active pool with a
NULL free_map?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235556.60604-1-mmc%40linux.ibm.com
