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: Implement incremental MQ RX queue resize
Adds per-queue alloc/register/irq-setup/free helpers plus
ibmveth_resize_rx_queues_incremental() so the live RX queue count can grow
or shrink without tearing down surviving queues, and wires
ibmveth_resize_rx_channels() into set_channels() when the interface is up.
A few questions on the new resize paths below.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index db13747466025..b57c7df92853f 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -1307,6 +1494,51 @@ static int ibmveth_rxq_harvest_buffer(struct
> ibmveth_adapter *adapter,
> return 0;
> }
>
> +/**
> + * ibmveth_drain_rx_queue - Drain pending buffers from an RX queue
> + * @adapter: ibmveth adapter structure
> + * @queue_index: Queue index to drain
> + *
> + * Recycles all pending buffers back to the per-queue buffer pools.
> + * Must be called with NAPI disabled for this queue.
> + *
> + * Return: Number of buffers drained
> + */
> +static int
> +ibmveth_drain_rx_queue(struct ibmveth_adapter *adapter, int queue_index)
> +{
> + struct net_device *netdev = adapter->netdev;
> + int drained = 0;
> + int limit = adapter->rx_queue[queue_index].num_slots;
> + int rc;
> +
> + netdev_dbg(netdev, "Draining RX queue %d (limit: %d slots)\n",
> + queue_index, limit);
> +
> + while (drained < limit &&
> + ibmveth_rxq_pending_buffer(adapter, queue_index)) {
> + /* Match poll-side order before harvesting completion state. */
> + smp_rmb();
> + rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, true);
> + if (rc) {
> + netdev_err(netdev,
> + "Failed to harvest buffer from queue %d
> during drain: %d\n",
> + queue_index, rc);
> + break;
> + }
[Severity: Low]
Can this error arm ever run? ibmveth_rxq_harvest_buffer() normalizes
every failure to a 0 return:
rc = ibmveth_remove_buffer_from_pool(adapter, cor, queue_index, reuse);
if (unlikely(rc)) {
if (rc == -EINVAL || rc == -EFAULT)
goto advance;
return rc;
}
advance:
ibmveth_rxq_advance(rxq);
return 0;
and ibmveth_remove_buffer_from_pool() only ever returns 0, -EINVAL or
-EFAULT, so both non-zero values take the goto advance path. The
netdev_err()/break looks like dead code.
> + drained++;
> + }
> +
> + if (drained > 0)
> + netdev_dbg(netdev, "Drained %d buffer(s) from RX queue %d\n",
> + drained, queue_index);
> + else
> + netdev_dbg(netdev, "No buffers to drain from RX queue %d\n",
> + queue_index);
> +
> + return drained;
> +}
> +
[ ... ]
> @@ -1562,6 +1794,227 @@ ibmveth_register_single_rx_queue(struct
> ibmveth_adapter *adapter,
> return 0;
> }
>
[ ... ]
> +static int
> +ibmveth_resize_rx_queues_incremental(struct ibmveth_adapter *adapter,
> + int new_count, int rxq_entries)
> +{
[ ... ]
> + for (i = old_count; i < new_count; i++) {
> + rc = ibmveth_alloc_single_rx_queue(adapter, i,
> + rxq_entries);
[ ... ]
> + /*
> + * Fully ready before PHYP delivery, matching open():
> + * publish -> replenish -> napi_enable -> enable_irq.
> + * That way ibmveth_interrupt() cannot run on an
> + * unpublished, empty, or NAPI-disabled queue.
> + */
> + adapter->num_rx_queues = i + 1;
[Severity: Medium]
Should this publish store be ordered? adapter->num_rx_queues is the only
gate that makes rx_queue[i].queue_addr/num_slots/toggle/replenish_lock,
rx_buff_pool[i][*] and buffer_list_addr[i] visible to lockless readers,
and it is written with a plain store here while the queues added in
earlier iterations are already running NAPI with PHYP unmasked.
The consumers also use plain loads from hardirq and softirq context:
ibmveth_interrupt():
if (WARN_ON(qindex < 0 || qindex >= adapter->num_rx_queues))
return IRQ_NONE;
ibmveth_poll():
if (WARN_ON(queue_index < 0 || queue_index >= adapter->num_rx_queues))
return 0;
and ibmveth_update_rx_no_buffer() dereferences buffer_list_addr[i] for
every i below the count. On powerpc, can a reader see the incremented
count while still observing stale contents of that slot (for example a
buffer_list_addr[i] left NULL by an earlier scale-down), or the converse
stale count for the queue whose interrupt was just unmasked? Would
WRITE_ONCE()/READ_ONCE() plus a release barrier before the publish be
appropriate here?
> + ibmveth_replenish_task(adapter, i);
> + napi_enable(&adapter->napi[i]);
> +
> + rc = ibmveth_enable_irq(adapter, i);
[Severity: Medium]
After the unmask, should the queue be re-checked for descriptors that
PHYP enqueued while delivery was masked? Buffers are posted by
ibmveth_replenish_task() before ibmveth_enable_irq(), so PHYP can write
descriptors during the masked window. ibmveth_poll() does exactly this
compensation:
lpar_rc = ibmveth_enable_irq(adapter, queue_index);
...
if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
napi_schedule(napi)) {
ibmveth_disable_irq(adapter, queue_index);
goto restart_poll;
}
Without an equivalent pending check plus napi_schedule() here (and in the
scale-down rollback loop further down, which is even more exposed since
PHYP may have enqueued during the disable/drain window), can the new
queue sit with unharvested descriptors until unrelated traffic raises the
next interrupt?
> + if (rc) {
> + netdev_err(netdev,
> + "Failed to enable IRQ for queue %d:
> %d\n",
> + i, rc);
> + adapter->num_rx_queues = i;
> + napi_disable(&adapter->napi[i]);
> + ibmveth_cleanup_single_rx_interrupt(adapter, i);
> + ibmveth_deregister_single_rx_queue(adapter, i);
> + ibmveth_free_single_rx_queue(adapter, i);
> + goto cleanup_new_queues;
> + }
[Severity: Medium]
Is rc an errno at this point? ibmveth_enable_irq() -> ibmveth_toggle_irq()
returns the raw plpar_hcall_norets() result and only normalizes
H_PARAMETER to 0, so rc can be H_HARDWARE (-1), H_FUNCTION (-2),
H_PRIVILEGE (-3) or the positive H_BUSY (1). That value is returned
unchanged through ibmveth_resize_rx_channels() -> ibmveth_set_channels()
into the ethtool ioctl, so userspace sees EPERM/ENOENT/ESRCH, or for a
positive code a positive ioctl return that ethtool reads as success even
though the resize failed and unwound.
The pre-existing ibmveth_setup_rx_interrupts() converts the same failure
with rc = -EIO; should this path do the same?
> + }
> +
> + rc = netif_set_real_num_rx_queues(netdev, new_count);
> + if (rc) {
> + netdev_err(netdev, "Failed to set real RX queues to %d:
> %d\n",
> + new_count, rc);
> + goto cleanup_new_queues;
> + }
> + } else {
> + netdev_dbg(netdev, "Scale-down: removing queues %d-%d\n",
> + new_count, old_count - 1);
> +
> + /*
> + * Mask PHYP delivery before napi_disable/drain. Otherwise
> + * ibmveth_interrupt returns IRQ_HANDLED without masking when
> + * NAPI is disabled, and the HV can storm during drain.
> + */
> + for (i = new_count; i < old_count; i++) {
> + ibmveth_disable_irq(adapter, i);
> + synchronize_irq(adapter->queue_irq[i]);
> + }
> +
> + for (i = new_count; i < old_count; i++)
> + napi_disable(&adapter->napi[i]);
[Severity: Medium]
Does the mask actually hold across the drain? synchronize_irq() waits
for the hardirq handler, not for an ibmveth_poll() already in flight on
another CPU, and that poll re-arms delivery unconditionally:
CPU0 (resize)
ibmveth_disable_irq(adapter, i);
synchronize_irq(adapter->queue_irq[i]); /* handler idle, returns */
napi_disable(&adapter->napi[i]); /* returns after
napi_complete_done() */
CPU1 (ibmveth_poll for queue i, scheduled before the mask)
if (!napi_complete_done(napi, frames_processed))
goto out;
lpar_rc = ibmveth_enable_irq(adapter, queue_index); /* unmasked */
so the drain can run with PHYP delivery re-enabled. With NAPI disabled,
ibmveth_schedule_rx_queue() fails napi_schedule_prep() and returns
without masking, and once adapter->num_rx_queues is lowered each further
delivery hits WARN_ON(qindex >= adapter->num_rx_queues) in
ibmveth_interrupt() and returns IRQ_NONE. Is the storm rule stated in
the comment still guaranteed? The same ordering is repeated in the
cleanup_new_queues label below.
> +
> + for (i = new_count; i < old_count; i++)
> + ibmveth_drain_rx_queue(adapter, i);
> +
> + synchronize_net();
> +
> + rc = netif_set_real_num_rx_queues(netdev, new_count);
> + if (rc) {
> + netdev_err(netdev, "Failed to set real RX queues to %d:
> %d\n",
> + new_count, rc);
> + for (i = new_count; i < old_count; i++) {
> + ibmveth_replenish_task(adapter, i);
> + napi_enable(&adapter->napi[i]);
> + ibmveth_enable_irq(adapter, i);
> + }
> + return rc;
> + }
[Severity: Medium]
Should the ibmveth_enable_irq() return code be checked in this rollback
loop? This is the only new call site that discards it.
If the H_VIOCTL enable fails for one of these queues, the queue stays in
the live set (adapter->num_rx_queues is still old_count) with NAPI
enabled and buffers posted, but with PHYP delivery masked, so that RX
queue is silently stalled until a close/open or another resize.
Elsewhere the driver treats this as actionable, e.g. ibmveth_poll():
lpar_rc = ibmveth_enable_irq(adapter, queue_index);
if (lpar_rc != H_SUCCESS) {
...
schedule_work(&adapter->work);
}
> +
> + adapter->num_rx_queues = new_count;
[Severity: Medium]
Do the interface statistics go backwards here? ibmveth_get_stats64(),
ibmveth_get_ethtool_stats(), ibmveth_sum_rx_invalid_buffers() and
ibmveth_sum_rx_large_packets() all sum rx_qstats[0..num_rx_queues-1]:
if (adapter->rx_qstats) {
for (i = 0; i < adapter->num_rx_queues; i++) {
stats->rx_packets += adapter->rx_qstats[i].packets;
so lowering the count drops the packets/bytes already accumulated by the
removed queues from the reported totals.
In the other direction, ibmveth_alloc_single_rx_queue() resets
rx_queue[queue_idx].index/num_slots/toggle and copies the pool geometry
but never clears adapter->rx_qstats[queue_idx], so re-adding a previously
removed index resurrects its old counters and the totals jump up.
Should the retired queues' counts be folded into an accumulator and the
slot cleared on (re-)allocation, so rtnl_link_stats64 stays monotonic?
> +
> + for (i = new_count; i < old_count; i++) {
> + ibmveth_cleanup_single_rx_interrupt(adapter, i);
> + ibmveth_deregister_single_rx_queue(adapter, i);
> + ibmveth_free_single_rx_queue(adapter, i);
> + }
> + }
> +
> + netdev_info(netdev, "Successfully resized to %d RX queues
> (incremental)\n",
> + adapter->num_rx_queues);
> +
> + if (firmware_has_feature(FW_FEATURE_CMO))
> + vio_cmo_set_dev_desired(adapter->vdev,
> + ibmveth_get_desired_dma(adapter->vdev));
[Severity: Medium]
Is the entitlement raised too late on CMO LPARs? All the CMO-charged DMA
work for the new queues has already happened by the time this runs:
dma_map_single() of the buffer-list page and dma_alloc_coherent() of the
descriptor ring in ibmveth_alloc_single_rx_queue(), plus every RX skb
mapping done by ibmveth_replenish_task(). Those run while the device's
desired entitlement still reflects the old, smaller queue count, and
vio_dma_iommu_alloc_coherent() refuses the allocation when
vio_cmo_alloc() cannot cover it.
It is also skipped on every failure and rollback return, so a resize that
fails leaves the entitlement untouched.
The driver's own convention is the opposite order, in
ibmveth_change_mtu():
vio_cmo_set_dev_desired(viodev, ibmveth_get_desired_dma(viodev));
...
ibmveth_open(...)
Note that simply moving the call is not enough, since
ibmveth_get_desired_dma() itself loops over adapter->num_rx_queues, so
the target count has to be accounted before the allocations and rolled
back on failure.
> +
> + return 0;
> +
> +cleanup_new_queues:
> + failed_queue = i;
> + netdev_err(netdev,
> + "Scale-up failed at queue %d, cleaning up queues %d-%d\n",
> + failed_queue, old_count, failed_queue - 1);
[Severity: Low]
When the first newly added queue is the one that fails, failed_queue ==
old_count, so this prints an inverted empty range such as "cleaning up
queues 4-3" while the three loops below correctly iterate zero times.
> + for (i = old_count; i < failed_queue; i++) {
> + ibmveth_disable_irq(adapter, i);
> + synchronize_irq(adapter->queue_irq[i]);
> + }
> +
> + for (i = old_count; i < failed_queue; i++)
> + napi_disable(&adapter->napi[i]);
> +
> + for (i = old_count; i < failed_queue; i++)
> + ibmveth_drain_rx_queue(adapter, i);
> +
> + synchronize_net();
> +
> + for (i = old_count; i < failed_queue; i++) {
> + ibmveth_cleanup_single_rx_interrupt(adapter, i);
> + ibmveth_deregister_single_rx_queue(adapter, i);
> + ibmveth_free_single_rx_queue(adapter, i);
> + }
> + adapter->num_rx_queues = old_count;
[Severity: High]
Can a concurrent NAPI poll dereference the pages this loop just freed?
adapter->num_rx_queues is restored only after the free loop, so for the
duration of that loop the indices [old_count, failed_queue) are freed but
still inside the published live range.
ibmveth_free_single_rx_queue() unmaps buffer_list_dma[i], frees the
coherent ring and then does:
free_page((unsigned long)adapter->buffer_list_addr[queue_idx]);
adapter->buffer_list_addr[queue_idx] = NULL;
Meanwhile every surviving queue's poll reaches
ibmveth_replenish_task() -> ibmveth_update_rx_no_buffer(), which walks the
published range with no NULL or liveness check:
for (i = 0; i < adapter->num_rx_queues; i++) {
__be64 *p = adapter->buffer_list_addr[i] + 4096 - 8;
u64 drops = be64_to_cpup(p);
So for a 2->4 scale-up where queue 2 came up and queue 3 fails, a poll on
queue 0 or 1 reads buffer_list_addr[2] either as a just-freed page or as
NULL + 0xff8, in softirq context. ibmveth_poll_controller() reaches the
same range from netpoll without rtnl, and additionally posts buffers into
pools that are being freed.
The guard in ibmveth_replenish_task():
if (queue_index >= adapter->num_rx_queues) {
does not help while the count is still the higher value. The inline
enable_irq failure handler above and the successful scale-down path both
lower adapter->num_rx_queues before freeing; should this label do the
same, i.e. set adapter->num_rx_queues = old_count before the free loop?
> + netdev_warn(netdev, "Keeping %d queues after scale-up failure\n",
> + old_count);
> + return rc;
> +}
> +
[ ... ]
> @@ -2217,12 +2670,62 @@ static void ibmveth_get_channels(struct net_device
> *netdev,
> channels->rx_count = adapter->num_rx_queues;
> }
>
[ ... ]
> +static int ibmveth_resize_rx_channels(struct ibmveth_adapter *adapter,
> + unsigned int goal_rx)
> +{
[ ... ]
> + if (goal_rx == old_rx)
> + return 0;
> +
> + if (!(netdev->flags & IFF_UP)) {
> + adapter->num_rx_queues = goal_rx;
> + return 0;
> + }
> +
[Severity: Low]
This isn't a bug in the final series, but at this commit the stash branch
is unreachable: the only caller, ibmveth_set_channels(), already returns
early on the same !IFF_UP condition before reaching
ibmveth_resize_rx_channels(), so ethtool -L <dev> rx N on a down
interface returns 0 while the requested count is discarded. The next
patch in the series ("ibmveth: Wire ethtool set_channels to MQ RX queue
resize") replaces that early return and makes this branch live, so this
only shows up at this bisect point.
> + rxq_entries = adapter->rx_queue[0].num_slots;
> + rc = ibmveth_resize_rx_queues_incremental(adapter, goal_rx,
> + rxq_entries);
> + if (rc)
> + netdev_err(netdev, "Failed to resize RX queues: %d\n", rc);
> + return rc;
> +}
> +
> static int ibmveth_set_channels(struct net_device *netdev,
> struct ethtool_channels *channels)
> {
> struct ibmveth_adapter *adapter = netdev_priv(netdev);
> unsigned int old = netdev->real_num_tx_queues,
> goal = channels->tx_count;
> + unsigned int goal_rx = channels->rx_count;
> int rc, i;
>
> /* If ndo_open has not been called yet then don't allocate, just set
> @@ -2231,6 +2734,13 @@ static int ibmveth_set_channels(struct net_device
> *netdev,
> if (!(netdev->flags & IFF_UP))
> return netif_set_real_num_tx_queues(netdev, goal);
>
> + /* Resize RX first while UP so ibmveth_resize_rx_channels() is used
> + * in this patch. !IFF_UP RX stash ordering lands next.
> + */
> + rc = ibmveth_resize_rx_channels(adapter, goal_rx);
> + if (rc)
> + return rc;
> +
[Severity: Medium]
Should the RX side be unwound when the TX phase below fails? The RX
resize is committed here first, and then ibmveth_allocate_tx_ltb() can
return -ENOMEM or netif_set_real_num_tx_queues() can fail, in which case
the function ends with return rc and no compensating
ibmveth_resize_rx_channels(adapter, old_rx).
ETHTOOL_SCHANNELS then reports failure while half of the requested
configuration is in effect: on scale-down the excess RX queues have
already been deregistered from PHYP and freed, on scale-up new queues are
registered with IRQs installed and DMA memory pinned, and ethtool -l
afterwards reports the new rx_count.
> /* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
> * but we may need to alloc/free the ltb's.
> */
[ ... ]
