Re: [PATCH net-next v3 09/15] ibmveth: Add queue-aware RX buffer submit helper for MQ
From: Simon Horman <horms@kernel.org>
Date: 2026-07-14 12:53:40
Also in:
netdev
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev --- ibmveth: Add queue-aware RX buffer submit helper for MQ This patch updates the ibmveth driver's buffer replenishment path to be queue-aware for multi-queue receive support. It adds a helper to select the correct hypervisor call and handles fallback to single-buffer mode if batch operations are unsupported.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c index fa2d4777ffc7b..b3b3886c3eedb 100644 --- a/drivers/net/ethernet/ibm/ibmveth.c +++ b/drivers/net/ethernet/ibm/ibmveth.c@@ -597,11 +597,73 @@ static inline void ibmveth_flush_buffer(void *addr, unsigned long length)
[ ... ]
+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};
+ int i;
+
+ /* Pack descriptor addresses into ioba pairs.
+ * Each ioba holds two 32-bit addresses packed into 64 bits:
+ * - Even descriptors (0,2,4...) go in high 32 bits
+ * - Odd descriptors (1,3,5...) go in low 32 bits
+ */
+ for (i = 0; i < filled && i < IBMVETH_MAX_RX_PER_HCALL; i++) {
+ int pair_idx = i / 2; /* Which pair: 0-5 */
+ int is_high = (i % 2 == 0); /* High or low 32 bits */
+
+ if (is_high)
+ ioba[pair_idx] = (unsigned long)descs[i].fields.address << 32;
+ else
+ ioba[pair_idx] |= descs[i].fields.address;
+ }
+
+ rc = h_add_logical_lan_buffers_queue(vdev->unit_address,
+ adapter->queue_handle[queue_index],
+ buffersznum,
+ ioba[0], ioba[1], ioba[2],
+ ioba[3], ioba[4], ioba[5]);
+ adapter->hcall_stats.add_bufs_queue++;[Severity: Medium] Will this counter increment lose updates when multiple NAPI poll routines run concurrently on different CPUs? Since multi-queue enables concurrent execution, it seems global statistic counters like hcall_stats.add_bufs_queue and replenish_add_buff_success might need atomic or per-CPU operations to avoid read-modify-write data races.
+ } 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++;
+ }
+
+ return rc;
+}[ ... ]
quoted hunk ↗ jump to hunk
@@ -745,24 +798,19 @@ 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) { - /*
[ ... ]
+ if (lpar_rc == H_FUNCTION) {
+ if (adapter->multi_queue) {
+ netdev_err(adapter->netdev,
+ "Unexpected H_FUNCTION from multi-queue buffer add (queue=%d, batch=%d)\n",
+ queue_index, batch);
+ break;
+ } else if (batch > 1) {
+ netdev_warn(adapter->netdev,
+ "H_FUNCTION from legacy batch buffer add (batch=%d), falling back to single buffer mode\n",
+ batch);
+ adapter->rx_buffers_per_hcall = 1;
+ continue;
[Severity: High]
Does this continue statement cause an infinite loop?
Looking at ibmveth_replenish_buffer_pool(), the local batch variable is set
before the while loop:
batch = adapter->rx_buffers_per_hcall;
while (remaining > 0) {
...
If the fallback updates adapter->rx_buffers_per_hcall to 1 and issues a continue,
the loop will restart but the local batch variable will retain its original
value.
Would this lead to the loop repeatedly attempting the unsupported batch size
and failing with H_FUNCTION forever?
quoted hunk ↗ jump to hunk
+ } } break; }@@ -784,18 +832,24 @@ 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; + if (queue_index >= adapter->num_rx_queues) + return; + adapter->replenish_task_cycles++;
[Severity: Medium] Similar to the hcall_stats counters above, could this increment suffer from data races when multiple queues are processed concurrently across different CPUs?