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 choose between the legacy
single/multi-buffer hcalls and the new per-queue buffer hcall, threads a
queue_index argument through the RX accessors, harvest and replenish
helpers, and adds a per-queue replenish_lock plus reworked H_FUNCTION
handling.
A few questions below about the new failure handling, the new lock and the
logging changes.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index 9d1242dbcb19d..cb93659fc0576 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -588,11 +598,75 @@ static inline void ibmveth_flush_buffer(void *addr,
> unsigned long length)
> asm("dcbf %0,%1,1" :: "b" (addr), "r" (offset));
> }
>
> +/**
> + * 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 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) {
[ ... ]
> + 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: Medium]
These hcall_stats fields are plain non-atomic u64 counters on the
adapter, but the only serialization this patch adds is the per-queue
rx_queue[i].replenish_lock taken in ibmveth_replenish_task().
Two NAPI instances replenishing different queues hold different locks, so
can these increments lose counts once MQ is enabled later in the series and
num_rx_queues becomes larger than 1?
The same question applies to the other adapter-wide counters reached from
this path:
adapter->replenish_task_cycles++;
adapter->replenish_add_buff_success += filled;
adapter->replenish_add_buff_failure += filled;
and to ibmveth_update_rx_no_buffer(), which ibmveth_replenish_task() calls
while holding queue N's lock only, although it rewrites adapter-global
state for every queue:
adapter->rx_no_buffer = 0;
for (i = 0; i < adapter->num_rx_queues; i++) {
...
adapter->rx_qstats[i].no_buffer_drops = drops;
adapter->rx_no_buffer += drops;
}
Would rx_no_buffer and rx_qstats[].no_buffer_drops be clobbered when two
queues replenish concurrently? ibmveth_close() calls the same helper with
no lock held at all.
> /* replenish the buffers for a pool. note that we don't need to
> * skb_reserve these since they are used for incoming...
> */
> static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
> - struct ibmveth_buff_pool *pool)
> + struct ibmveth_buff_pool *pool,
> + int queue_index)
> {
> union ibmveth_buf_desc descs[IBMVETH_MAX_RX_PER_HCALL] = {0};
> u32 remaining = pool->size - atomic_read(&pool->available);
> @@ -678,24 +752,16 @@ static void ibmveth_replenish_buffer_pool(struct
> ibmveth_adapter *adapter,
[ ... ]
> + lpar_rc = ibmveth_add_logical_lan_buffers(adapter, descs,
> + filled,
> + pool->buff_size,
> + queue_index);
> +
> if (lpar_rc != H_SUCCESS) {
> dev_warn_ratelimited(dev,
> - "RX h_add_logical_lan failed:
> filled=%u, rc=%lu, batch=%u\n",
> + "RX h_add_logical_lan %s failed:
> filled=%u, rc=%lu, batch=%u\n",
> + adapter->multi_queue ?
> + "_queue" : "",
> filled, lpar_rc, batch);
> goto hcall_failure;
> }
[Severity: Low]
The interpolated hcall name never matches an actual symbol. In MQ mode
this prints
RX h_add_logical_lan _queue failed: ...
with a stray space, and in legacy mode
RX h_add_logical_lan failed: ...
with a double space. Would a full literal per branch read better?
The two new H_FUNCTION messages added below also print batch with %d,
although batch is declared u32 in this function ("u32 i, filled, batch;"),
while this warning uses %u for the same variable.
> @@ -736,24 +802,23 @@ static void ibmveth_replenish_buffer_pool(struct
> ibmveth_adapter *adapter,
> }
> adapter->replenish_add_buff_failure += filled;
>
> - /*
> - * If multi rx buffers hcall is no longer supported by FW
> - * e.g. in the case of Live Partition Migration
> - */
> - if (batch > 1 && lpar_rc == H_FUNCTION) {
> - /*
> - * Instead of retry submit single buffer individually
> - * here just set the max rx buffer per hcall to 1
> - * buffers will be respleshed next time
> - * when ibmveth_replenish_buffer_pool() is called again
> - * with single-buffer case
> - */
> - netdev_info(adapter->netdev,
> - "RX Multi buffers not supported by FW,
> rc=%lu\n",
> - lpar_rc);
> - adapter->rx_buffers_per_hcall = 1;
> - netdev_info(adapter->netdev,
> - "Next rx replesh will fall back to
> single-buffer hcall\n");
> + if (lpar_rc == H_FUNCTION) {
> + if (adapter->multi_queue) {
> + netdev_err(adapter->netdev,
> + "MQ buffer add H_FUNCTION (q=%d,
> batch=%d)\n",
> + queue_index, batch);
> + } else if (batch > 1) {
[Severity: High]
The multi_queue arm logs and breaks without changing any state:
adapter->multi_queue stays set, rx_buffers_per_hcall is untouched, there is
no fallback to h_add_logical_lan_buffer()/h_add_logical_lan_buffers(), and
no schedule_work(&adapter->work).
ibmveth_add_logical_lan_buffers() picks the hcall purely on
adapter->multi_queue, and that field is only written in ibmveth_probe(),
which is not re-run on resume (ibmveth_resume() only calls
ibmveth_schedule_rx_queue()).
So once the hypervisor stops implementing H_ADD_LOGICAL_LAN_BUFFERS_QUEUE
- exactly the Live Partition Migration case the legacy arm below exists for
- can this queue ever post another RX buffer again? pool->available stays
at 0, PHYP drops every inbound frame, and the interface still reports the
link up, so recovery would need an administrative down/up.
Every other unrecoverable condition in this driver schedules the reset
work (ibmveth_remove_buffer_from_pool(), ibmveth_rxq_get_buffer(), and the
new enable_irq failure path in ibmveth_poll()). Should this arm do the
same, or fall back to the legacy hcalls the way the legacy arm downgrades
batch?
Separately, netdev_err() here is not rate limited, while the warning for
the same failure a few lines above uses dev_warn_ratelimited(), so a
dried-out pool can emit several console lines per poll cycle per pool.
This arm appears unchanged through the end of the series.
> + /*
> + * Live Partition Migration may drop multi-
> + * buffer support. Fall back to single-buffer
> + * on the next replenish; do not continue with
> + * a stale local batch size (infinite loop).
> + */
> + netdev_warn(adapter->netdev,
> + "Legacy batch add H_FUNCTION
> (batch=%d), fallback\n",
> + batch);
> + adapter->rx_buffers_per_hcall = 1;
> + }
> }
> break;
> }
> @@ -775,21 +840,33 @@ 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)
> {
> + struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
> + unsigned long flags;
> int i;
>
> + if (queue_index >= adapter->num_rx_queues)
> + return;
> +
> adapter->replenish_task_cycles++;
>
> + spin_lock_irqsave(&rxq->replenish_lock, flags);
> +
> for (i = (IBMVETH_NUM_BUFF_POOLS - 1); i >= 0; i--) {
> - struct ibmveth_buff_pool *pool = &adapter->rx_buff_pool[0][i];
> + struct ibmveth_buff_pool *pool =
> + &adapter->rx_buff_pool[queue_index][i];
>
> if (pool->active &&
> (atomic_read(&pool->available) < pool->threshold))
> - ibmveth_replenish_buffer_pool(adapter, pool);
> + ibmveth_replenish_buffer_pool(adapter, pool,
> + queue_index);
> }
>
> ibmveth_update_rx_no_buffer(adapter);
> +
> + spin_unlock_irqrestore(&rxq->replenish_lock, flags);
> }
[Severity: High]
Can this lock deadlock against itself when netconsole runs over this
ibmveth device?
ibmveth_replenish_buffer_pool() printk()s from inside the locked region:
netdev_info() on an invalid map index, dev_warn_ratelimited() on hcall
failure, and the two new H_FUNCTION messages. With netconsole on this
interface a printk from there re-enters the driver:
netconsole write_msg()
netpoll_send_udp()
netpoll_send_skb()
__netpoll_send_skb()
netpoll_poll_dev()
ops->ndo_poll_controller() -> ibmveth_poll_controller()
ibmveth_replenish_task(adapter, i)
__netpoll_send_skb() calls netpoll_poll_dev() whenever the xmit does not
go through on the first try, and netpoll_poll_dev() only declines when
down_trylock(&ni->dev_lock) fails, when !netif_running(dev), or when
netif_local_xmit_active(dev):
if (!ni || down_trylock(&ni->dev_lock))
return;
if (!netif_running(dev) || netif_local_xmit_active(dev)) {
up(&ni->dev_lock);
return;
}
...
if (ops->ndo_poll_controller)
ops->ndo_poll_controller(dev);
None of those covers the replenish path, so ibmveth_poll_controller() loops
ibmveth_replenish_task() over every queue and re-takes the same
replenish_lock on the same CPU, with interrupts already disabled.
Before this patch ibmveth_poll_controller() also called
ibmveth_replenish_task(), but with no lock the recursion was harmless.
This code appears unchanged through the end of the series.
[Severity: Medium]
Does this need to be spin_lock_irqsave()? The lock now brackets the whole
multi-pool loop, so every netdev_alloc_skb(), every dma_map_single() and
every H_ADD_LOGICAL_LAN_BUFFER[S][_QUEUE] hypervisor call for all five
pools runs with local interrupts hard-disabled.
On a cold fill from ibmveth_open() or the first NAPI poll that is roughly
768 allocations and DMA maps and around 96 hcalls per queue at the default
MTU (pool_count[] = { 256, 512, 256, 256, 256 }), and more when the large
pools are active.
None of the protected state is written from hardirq context, and the commit
message says the lock exists only "so later concurrent NAPI/resize paths
can serialize buffer posting", so would spin_lock_bh(), dropping and
re-acquiring per batch, or preparing buffers outside the critical section
be enough?
>
> /* empty and free ana buffer pool - also used to do cleanup in error paths */
[ ... ]
> @@ -1004,50 +1083,55 @@ static int ibmveth_remove_buffer_from_pool(struct
> ibmveth_adapter *adapter,
[ ... ]
> - free_index = adapter->rx_buff_pool[0][pool].producer_index;
> - adapter->rx_buff_pool[0][pool].producer_index++;
> - if (adapter->rx_buff_pool[0][pool].producer_index >=
> - adapter->rx_buff_pool[0][pool].size)
> - adapter->rx_buff_pool[0][pool].producer_index = 0;
> - adapter->rx_buff_pool[0][pool].free_map[free_index] = index;
> + free_index = adapter->rx_buff_pool[queue_index][pool].producer_index;
> + adapter->rx_buff_pool[queue_index][pool].producer_index++;
> + if (adapter->rx_buff_pool[queue_index][pool].producer_index >=
> + adapter->rx_buff_pool[queue_index][pool].size)
> + adapter->rx_buff_pool[queue_index][pool].producer_index = 0;
> + adapter->rx_buff_pool[queue_index][pool].free_map[free_index] = index;
>
> mb();
>
> - atomic_dec(&adapter->rx_buff_pool[0][pool].available);
> + atomic_dec(&adapter->rx_buff_pool[queue_index][pool].available);
[Severity: High]
This isn't a bug introduced by this patch, but the new replenish_lock only
covers the producer half of this bookkeeping, so it is worth asking whether
the consumer half is still unprotected.
ibmveth_remove_buffer_from_pool() mutates free_map[], producer_index,
skbuff[], dma_addr[] and available for the same pool that
ibmveth_replenish_buffer_pool() mutates under replenish_lock, but takes no
lock:
producer: ibmveth_poll_controller()
ibmveth_replenish_task() /* holds replenish_lock */
ibmveth_replenish_buffer_pool()
consumer: ibmveth_poll() /* holds nothing */
ibmveth_rxq_harvest_buffer()
ibmveth_remove_buffer_from_pool()
netpoll_poll_dev() serializes netpoll users against each other via
ni->dev_lock, and __napi_poll()'s netpoll_poll_lock() only stops netpoll
from re-polling a NAPI instance that is already running; neither prevents
ndo_poll_controller from running on one CPU while the same queue's NAPI
poll runs on another.
Can free_map[]/producer_index/consumer_index then hand the same pool index
out twice, so a buffer is posted to the hypervisor while its skb has
already gone up the stack, or lose a free index permanently?
Since the future work described in the commit message ("so later concurrent
NAPI/resize paths can serialize buffer posting") builds on this lock, should
the consumer side take it as well?
[ ... ]
> @@ -1993,34 +2076,45 @@ static void ibmveth_rx_csum_helper(struct sk_buff
> *skb,
>
> static int ibmveth_poll(struct napi_struct *napi, int budget)
> {
> - struct ibmveth_adapter *adapter =
> - container_of(napi, struct ibmveth_adapter, napi[0]);
> - struct net_device *netdev = adapter->netdev;
> + struct net_device *netdev = napi->dev;
> + struct ibmveth_adapter *adapter = netdev_priv(netdev);
> int frames_processed = 0;
> unsigned long lpar_rc;
> + int queue_index, rc;
> u16 mss = 0;
>
> + queue_index = napi - adapter->napi;
> +
> + if (WARN_ON(queue_index < 0 || queue_index >= adapter->num_rx_queues))
> + return 0;
> +
[ ... ]
> @@ -2097,14 +2197,19 @@ static int ibmveth_poll(struct napi_struct *napi, int
> budget)
> /* We think we are done - reenable interrupts,
> * then check once more to make sure we are done.
> */
> - lpar_rc = ibmveth_enable_irq(adapter, 0);
> - if (WARN_ON(lpar_rc != H_SUCCESS)) {
> + lpar_rc = ibmveth_enable_irq(adapter, queue_index);
> + if (lpar_rc != H_SUCCESS) {
> + netdev_err(netdev,
> + "Failed to enable IRQ for queue %d (rc=0x%lx),
> scheduling reset\n",
> + queue_index, lpar_rc);
> schedule_work(&adapter->work);
> goto out;
> }
>
> - if (ibmveth_rxq_pending_buffer(adapter) && napi_schedule(napi)) {
> - lpar_rc = ibmveth_disable_irq(adapter, 0);
> + if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
> + napi_schedule(napi)) {
> + lpar_rc = ibmveth_disable_irq(adapter, queue_index);
> + WARN_ON(lpar_rc != H_SUCCESS);
> goto restart_poll;
> }
[Severity: Low]
Two diagnostic changes here are not described in the commit message, which
only says callers were updated "so arity stays consistent": the WARN_ON()
around the ibmveth_enable_irq() failure is replaced by an unconditional
netdev_err(), which drops the taint and backtrace that test harnesses key
on, and a new WARN_ON() is added on the ibmveth_disable_irq() result.
Would it help to mention or split out that change?
Also, the new guard added at the top of ibmveth_poll()
if (WARN_ON(queue_index < 0 || queue_index >= adapter->num_rx_queues))
return 0;
returns less than budget from a NAPI poll callback without calling
napi_complete_done(), which leaves the instance scheduled with the
hypervisor IRQ still masked. The guard is unreachable at this commit,
since IBMVETH_MAX_RX_QUEUES is 1 and queue_index is always 0, but the same
early return is still there at the end of the series. Should it complete
NAPI before returning?
[ ... ]
> @@ -2229,9 +2334,13 @@ static int ibmveth_change_mtu(struct net_device *dev,
> int new_mtu)
> static void ibmveth_poll_controller(struct net_device *dev)
> {
> struct ibmveth_adapter *adapter = netdev_priv(dev);
> + int i;
>
> - ibmveth_replenish_task(adapter);
> - ibmveth_schedule_rx_queue(adapter, 0);
> + for (i = 0; i < adapter->num_rx_queues; i++)
> + ibmveth_replenish_task(adapter, i);
> +
> + for (i = 0; i < adapter->num_rx_queues; i++)
> + ibmveth_schedule_rx_queue(adapter, i);
> }
> #endif
[ ... ]
