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: Refactor buffer pool management for per-queue MQ RX
This moves the five RX buffer pools to a per-queue array and introduces
ibmveth_alloc_buffer_pools() / ibmveth_free_buffer_pools() plus the
per-queue helpers, wiring them into open() and close(). Queue 0 stays the
template for pool geometry and activation, and runtime is still
single-queue.
A few questions below about the new unwind paths and about two of the
new comments.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index 168940bfa1094..1a428ee60d58c 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -627,6 +627,142 @@ static void ibmveth_free_buffer_pool(struct
> ibmveth_adapter *adapter,
> }
> }
>
> +/**
> + * ibmveth_free_queue_buffer_pools - Free buffer pools for a single queue
[ ... ]
> +/**
> + * ibmveth_alloc_queue_buffer_pools - Allocate buffer pools for a single
> queue
> + * @adapter: ibmveth adapter structure
> + * @queue: queue index
> + *
> + * Allocates backing storage for each active pool on @queue.
> + * Inactive pools (!active) are skipped. Pool metadata must be
> + * initialized before calling this function.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int ibmveth_alloc_queue_buffer_pools(struct ibmveth_adapter *adapter,
> + int queue)
> +{
> + struct net_device *netdev = adapter->netdev;
> + int i;
> +
> + for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> + struct ibmveth_buff_pool *bpool =
> + &adapter->rx_buff_pool[queue][i];
> +
> + if (!bpool->active)
> + continue;
> +
> + if (ibmveth_alloc_buffer_pool(bpool)) {
> + netdev_err(netdev,
> + "queue %d pool %d alloc failed (size=%u
> count=%u)\n",
> + queue, i,
> + bpool->buff_size,
> + bpool->size);
> + bpool->active = 0;
> + /* Free by allocation presence, not active — the
> + * failing pool cleared active first and would be
> + * skipped by an active-only unwind.
> + */
[Severity: Low]
Is the stated rationale in this comment accurate? Freeing by allocation
presence is a fine policy for the later resize paths, but the specific
claim that an active-only unwind would skip the failing pool and leak it
does not seem to hold here, because ibmveth_alloc_buffer_pool() already
undoes its own partial allocations before returning -1:
drivers/net/ethernet/ibm/ibmveth.c:ibmveth_alloc_buffer_pool() {
pool->dma_addr = kzalloc_objs(dma_addr_t, pool->size);
if (!pool->dma_addr) {
kfree(pool->free_map);
pool->free_map = NULL;
return -1;
}
pool->skbuff = kcalloc(pool->size, sizeof(void *), GFP_KERNEL);
if (!pool->skbuff) {
kfree(pool->dma_addr);
pool->dma_addr = NULL;
kfree(pool->free_map);
pool->free_map = NULL;
return -1;
}
...
}
By the time bpool->active = 0 runs, free_map/dma_addr/skbuff are all NULL
for the failing pool, so there is nothing for an active-only unwind to
miss. The same claim appears in the changelog ("v4 fail path freed by
active and skipped the failing pool", "open-fail cannot leak partially
allocated pools"). Could the comment and changelog instead justify
free-by-presence purely in terms of the later resize paths?
> + ibmveth_free_queue_buffer_pools(adapter, queue);
> + return -ENOMEM;
> + }
> + }
> +
> + return 0;
> +}
> +
[ ... ]
> + /* Allocate actual buffers for all queues */
> + for (q = 0; q < adapter->num_rx_queues; q++) {
> + rc = ibmveth_alloc_queue_buffer_pools(adapter, q);
> + if (rc) {
> + /* Free pools for all previous queues */
> + while (--q >= 0)
> + ibmveth_free_queue_buffer_pools(adapter, q);
> + return rc;
> + }
> + }
> +
> + netdev_dbg(netdev, "allocated buffer pools for %d queue(s)\n",
> + adapter->num_rx_queues);
[Severity: Low]
This isn't a bug, but adapter->num_rx_queues is declared as
"unsigned int num_rx_queues;" in drivers/net/ethernet/ibm/ibmveth.h, so
should these two new messages use %u rather than %d? The same applies to
the netdev_dbg() in ibmveth_free_buffer_pools() below.
> + return 0;
> +}
> +
> +/**
> + * ibmveth_free_buffer_pools - Free buffer pools for all queues
> + * @adapter: ibmveth adapter structure
> + *
> + * Frees buffer pools for all queues using the helper function.
> + */
> +static void
> +ibmveth_free_buffer_pools(struct ibmveth_adapter *adapter)
> +{
> + int q;
> +
> + /* Free buffer pools for all queues */
> + for (q = 0; q < adapter->num_rx_queues; q++)
> + ibmveth_free_queue_buffer_pools(adapter, q);
> +
> + netdev_dbg(adapter->netdev, "freed buffer pools for %d queue(s)\n",
> + adapter->num_rx_queues);
> +}
> +
> /**
> * ibmveth_remove_buffer_from_pool - remove a buffer from a pool
> * @adapter: adapter instance
> @@ -851,16 +987,9 @@ static int ibmveth_open(struct net_device *netdev)
> goto out_free_tx_ltb;
> }
>
> - for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> - if (!adapter->rx_buff_pool[0][i].active)
> - continue;
> - if (ibmveth_alloc_buffer_pool(&adapter->rx_buff_pool[0][i])) {
> - netdev_err(netdev, "unable to alloc pool\n");
> - adapter->rx_buff_pool[0][i].active = 0;
> - rc = -ENOMEM;
> - goto out_free_buffer_pools;
> - }
> - }
> + rc = ibmveth_alloc_buffer_pools(adapter);
> + if (rc)
> + goto out_free_tx_ltb;
[Severity: Medium]
This isn't a gap introduced by this patch (the old inline pool loop fell
through to the same labels), but since the edge is being rewritten here:
this goto is taken after ibmveth_register_logical_lan() has already
succeeded, and the unwind it lands on never issues h_free_logical_lan():
out_free_tx_ltb:
while (--i >= 0)
ibmveth_free_tx_ltb(adapter, i);
ibmveth_cleanup_rx_resources(adapter);
out_free_filter_list:
ibmveth_free_filter_list(adapter);
ibmveth_cleanup_rx_resources() does the dma_unmap_single() of
buffer_list_dma[], the dma_free_coherent() of the RX completion queue and
the free_page() of the buffer-list page, and ibmveth_free_filter_list()
releases the filter list, all while PHYP still holds those addresses. The
adjacent request_irq() failure path does perform the handshake the driver
otherwise considers mandatory:
do {
lpar_rc =
h_free_logical_lan(adapter->vdev->unit_address);
} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
Can PHYP write the rx_no_buffer counter into the freed buffer-list page, or
post a completion into the freed RX queue, after this path returns? The
h_vio_signal(VIO_IRQ_DISABLE) issued just before registration also is not
rebalanced here.
Later patches in this series do repair this: "ibmveth: Refactor TX resource
allocation in open/close paths" retargets this edge to out_unregister_lan,
and by the end of the series ibmveth_open() allocates the pools before
ibmveth_register_rx_queues() and routes post-registration failures through
ibmveth_free_all_queues() -> h_free_logical_lan(). Would it be reasonable
to point this goto at a label that unregisters the logical LAN already in
this patch, so the intermediate commit is not bisect-visible?
>
> netdev_dbg(netdev, "registering irq 0x%x\n", netdev->irq);
> rc = request_irq(netdev->irq, ibmveth_interrupt, 0, netdev->name,
> @@ -885,11 +1014,7 @@ static int ibmveth_open(struct net_device *netdev)
> return 0;
>
> out_free_buffer_pools:
> - while (--i >= 0) {
> - if (adapter->rx_buff_pool[0][i].active)
> - ibmveth_free_buffer_pool(adapter,
> - &adapter->rx_buff_pool[0][i]);
> - }
> + ibmveth_free_buffer_pools(adapter);
> out_free_tx_ltb:
> while (--i >= 0)
> ibmveth_free_tx_ltb(adapter, i);
[Severity: Medium]
This is a pre-existing issue rather than one introduced here, but does this
hunk silently fix a TX long-term-buffer leak?
In the parent commit the pool unwind consumed i:
out_free_buffer_pools:
while (--i >= 0) {
if (adapter->rx_buff_pool[0][i].active)
ibmveth_free_buffer_pool(adapter,
&adapter->rx_buff_pool[0][i]);
}
out_free_tx_ltb:
while (--i >= 0)
ibmveth_free_tx_ltb(adapter, i);
so on reaching out_free_tx_ltb by fall-through, i was already -1 and the
first test of --i evaluated to -2, meaning no tx_ltb_ptr[]/tx_ltb_dma[]
buffer was released. With the pool unwind now a helper call that does not
touch i, i still holds the TX queue count from the completed TX allocation
loop, so the TX LTBs are freed.
Every PAGE_ALIGN(IBMVETH_MAX_TX_BUF_SIZE) TX buffer plus its DMA mapping was
leaked on each failing open, and it repeats per attempt ("ip link set <dev>
up" under memory pressure, or the close+open pair inside veth_pool_store(),
ibmveth_change_mtu() and ibmveth_reset()).
Would it make sense to split this out as its own patch with a Fixes: tag so
stable trees pick it up, or at least describe it in the changelog?
>
> @@ -928,14 +1053,10 @@ static int ibmveth_close(struct net_device *netdev)
[ ... ]
