Re: [net-next,v6,08/15] ibmveth: Add queue-aware RX buffer submit helper for MQ

[mingming cao <mmc@linux.ibm.com>]

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 2 (1) · Low: 3
>
> - [High] (Discarded — resolved later in this series.) In ibmveth_poll()
>    at this commit, when ibmveth_rxq_get_buffer() rejects a correlator it…
> - [Medium] Removing the WARN_ON()+schedule_work() recovery from
>    ibmveth_remove_buffer_from_pool() leaves one live RX path with no…
> - [Medium] ibmveth_replenish_task() wraps the entire multi-pool replenish
>    loop in spin_lock_irqsave(&rxq->replenish_lock, flags), so up to…
> - [Low] The patch contains a user-visible robustness fix (replacing the
>    correlator WARN_ON()s that could take the partition down under…
> - [Low] The hcall-failure log in ibmveth_replenish_task() reconstructs
>    the wrapper name from fail.batch (`fail.batch == 1 ?
> - [Low] The kernel-doc Return block of ibmveth_remove_buffer_from_pool()
>    still documents only '-EINVAL - correlator maps to pool or index out…
>
> Pre-existing issues:
> - [Medium] (Discarded — reachability closed later in this series.) The
>    new per-queue replenish_lock is taken by the replenish producer and by…
>
> --
>
> ibmveth: Add queue-aware RX buffer submit helper for MQ
>
> This adds ibmveth_add_logical_lan_buffers() so RX pool replenishment can
> go through H_ADD_LOGICAL_LAN_BUFFERS_QUEUE in multi-queue mode, and
> parameterises the RX accessors, harvest and replenish paths by queue
> index. It also adds a per-queue replenish_lock, converts
> ibmveth_replenish_buffer_pool() to return an outcome that is logged after
> the unlock, and replaces the correlator WARN_ON()s with
> ibmveth_rxq_correlator_valid().
>
> A few questions on the correlator error handling, the replenish critical
> section and the failure log below.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 465330464f916..46f68f5b52e70 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -712,11 +722,100 @@ 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);
>> +	}
> [ ... ]
>
>> @@ -899,21 +1000,85 @@ 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;
>> +	struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
>> +	struct ibmveth_replenish_fail fail = {};
>> +	unsigned long flags;
>> +	int i, rc;
>> +	int need_reset = 0;
>> +	int batch_fallback = 0;
>> +	int hcall_fail = 0;
>> +
>> +	if (queue_index >= adapter->num_rx_queues) {
>> +		netdev_dbg(adapter->netdev,
>> +			   "Skipping replenish for freed queue %d (num_queues=%u)\n",
>> +			   queue_index, adapter->num_rx_queues);
>> +		return;
>> +	}
>>   
>>   	adapter->replenish_task_cycles++;
>>   
>> -	for (i = (IBMVETH_NUM_BUFF_POOLS - 1); i >= 0; i--) {
>> -		struct ibmveth_buff_pool *pool = &adapter->rx_buff_pool[0][i];
>> +	spin_lock_irqsave(&rxq->replenish_lock, flags);
> [Severity: Medium]
> Is it intended that the whole multi-pool replenish loop now runs with
> hardware interrupts disabled?
>
> The lock is taken here and released only after all five pools have been
> walked, and ibmveth_replenish_buffer_pool() allocates and maps every
> buffer inside that region:
>
> 	skb = netdev_alloc_skb(adapter->netdev, pool->buff_size);
> 	...
> 	dma_addr = dma_map_single_attrs(dev, skb->data, pool->buff_size,
> 					DMA_FROM_DEVICE, DMA_ATTR_NO_WARN);
>
> With pool_count[] = { 256, 512, 256, 256, 256 } that is up to roughly
> 1536 allocations plus iommu mappings with interrupts masked on a cold
> refill, for example ibmveth_open() -> ibmveth_restart_rx_queue() ->
> ibmveth_replenish_task() where available is 0 for every pool. Before this
> patch replenish ran from NAPI with interrupts enabled.
>
> There is also a second-order concern about the netconsole rationale in
> the commit message. ibmveth_poll_controller() calls
> ibmveth_replenish_task(adapter, 0), and netpoll_send_skb() has already
> done local_irq_save() by then, so this CPU can enter the region from
> netpoll. DMA_ATTR_NO_WARN and moving the logging out of the lock cover
> the iommu and the driver printks, but netdev_alloc_skb() for the large
> pools takes the __alloc_skb()/slab path, which is not blanket
> __GFP_NOWARN. If that allocator warning is emitted while netconsole is
> bound to this device, can it re-enter ndo_poll_controller ->
> ibmveth_replenish_task() -> spin_lock_irqsave() on the lock this CPU
> already holds?
Yes, it can: the whole fill still runs under
irqsave. v6 already moved
the driver logs out of the lock and passed
DMA_ATTR_NO_WARN so the iommu path cannot printk
under it. netdev_alloc_skb() can still warn, and that
can re-enter.

Shortening irqsave here is the same leftover as
locking free_buffer_pool() and calling
netpoll_poll_disable() on close. That is a
lock-protocol rewrite, not two conditions.
poll_controller already runs with IRQs off, so
this is not a drop-in lock_bh. Doing one of
those alone just moves the race, so I am
taking them together after this series.
>>   
>> -		if (pool->active &&
>> -		    (atomic_read(&pool->available) < pool->threshold))
>> -			ibmveth_replenish_buffer_pool(adapter, pool);
>> +	for (i = (IBMVETH_NUM_BUFF_POOLS - 1); i >= 0; i--) {
>> +		struct ibmveth_buff_pool *pool =
>> +			&adapter->rx_buff_pool[queue_index][i];
>> +
>> +		if (pool->active && pool->free_map &&
>> +		    (atomic_read(&pool->available) < pool->threshold)) {
>> +			rc = ibmveth_replenish_buffer_pool(adapter, pool,
>> +							   queue_index, &fail);
>> +			switch (rc) {
>> +			case IBMVETH_REPLENISH_RESET_MAP:
>> +			case IBMVETH_REPLENISH_RESET_MQ:
>> +				need_reset = rc;
>> +				goto out_unlock;
>> +			case IBMVETH_REPLENISH_BATCH_FALLBACK:
>> +				batch_fallback = 1;
>> +				break;
>> +			case IBMVETH_REPLENISH_HCALL_FAIL:
>> +				hcall_fail = 1;
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +		}
>>   	}
>>   
>> +out_unlock:
>>   	ibmveth_update_rx_no_buffer(adapter);
>> +
>> +	spin_unlock_irqrestore(&rxq->replenish_lock, flags);
> [ ... ]
>
>> +	if (hcall_fail)
>> +		dev_warn_ratelimited(&adapter->netdev->dev,
>> +				     "RX %s failed: filled=%u, rc=%lu, batch=%u\n",
>> +				     adapter->multi_queue ?
>> +				     "h_add_logical_lan_buffers_queue" :
>> +				     (fail.batch == 1 ?
>> +				      "h_add_logical_lan_buffer" :
>> +				      "h_add_logical_lan_buffers"),
>> +				     fail.filled, fail.lpar_rc, fail.batch);
> [Severity: Low]
> Should the wrapper name here be selected from fail.filled rather than
> fail.batch?
>
> ibmveth_add_logical_lan_buffers() dispatches on filled:
>
> 	} else if (filled == 1) {
> 		rc = h_add_logical_lan_buffer(vdev->unit_address,
> 					      descs[0].desc);
>
> So when adapter->rx_buffers_per_hcall is 8 (firmware with
> IBMVETH_ILLAN_RX_MULTI_BUFF_SUPPORT) and the fill loop stops at
> filled == 1 because remaining was 1, netdev_alloc_skb() failed or
> dma_mapping_error() hit after the first descriptor, the call that failed
> is h_add_logical_lan_buffer() but the log prints
> h_add_logical_lan_buffers. fail.filled already records the right
> discriminator.
>
> Related: fail is a single struct reused across the pool loop while
> batch_fallback and hcall_fail are sticky flags, so if two pools fail in
> one invocation, do the printed filled/rc/batch values only describe the
> last failure?
v7 names the wrapper from filled, so a
last-buffer single add prints the single
hcall. One fail struct is still shared, so
two pool failures leave only the last
filled/rc/batch. That struct stays as it
is in this patch.
> This is still the case at the end of the series.
>
>>   }
>>   
>>   /* empty and free ana buffer pool - also used to do cleanup in error paths */
>> @@ -948,6 +1113,12 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,
>>   		kfree(pool->skbuff);
>>   		pool->skbuff = NULL;
>>   	}
>> +
>> +	/*
>> +	 * Keep probe/sysfs geometry (active, size, buff_size, threshold).
>> +	 * Only tear down runtime allocations; open reuses active pools.
>> +	 */
>> +	atomic_set(&pool->available, 0);
>>   }
> [Severity: Medium]
> This is a pre-existing pattern and not introduced by this patch, but with
> the new replenish_lock it is worth a note.
>
> ibmveth_free_buffer_pool() kfree()s free_map, skbuff and dma_addr and
> does atomic_set(&pool->available, 0) without taking
> rxq->replenish_lock, while the new tests in ibmveth_replenish_task()
> and ibmveth_rxq_correlator_valid():
>
> 	if (pool->active && pool->free_map &&
> 	...
> 	if (!bpool->active || !bpool->skbuff || !bpool->free_map)
>
> are check-then-use against that teardown.
>
> Tracing it through, the window looks closed at the end of the series:
> ibmveth_poll_controller() returns early on !adapter->opened,
> ibmveth_close() clears opened first, then calls
> ibmveth_cleanup_rx_interrupts() and synchronize_net() before
> ibmveth_free_buffer_pools(), and netpoll runs ndo_poll_controller with
> hard interrupts disabled, so a netpoll section that observed
> opened == true is waited out by that synchronize_net(). Would it be worth
> documenting that free_buffer_pool() relies on the opened flag plus
> synchronize_net() rather than on replenish_lock, since it writes state
> the lock otherwise protects?
I am leaving that undocumented for now.
free_buffer_pool() still does not take
replenish_lock, so a comment that says it
relies on opened plus synchronize_net() would
overclaim. close() does clear opened and wait before
it frees the pools, but that is not the same as
holding the lock. Taking the lock in free belongs
with shortening irqsave, not a kdoc line here.

>>   
>>   /**
>> @@ -1088,35 +1259,74 @@ ibmveth_free_buffer_pools(struct ibmveth_adapter *adapter)
>>   		   adapter->num_rx_queues);
>>   }
>>   
>> +static bool ibmveth_rxq_correlator_valid(struct ibmveth_adapter *adapter,
>> +					 int queue_index, u64 correlator)
>> +{
>> +	unsigned int pool = correlator >> 32;
>> +	unsigned int index = correlator & 0xffffffffUL;
>> +	struct ibmveth_buff_pool *bpool;
>> +
>> +	if (pool >= IBMVETH_NUM_BUFF_POOLS)
>> +		return false;
>> +
>> +	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;
>> +
>> +	return index < bpool->size;
>> +}
> [Severity: Low]
> This isn't a bug, but this helper is a standalone robustness fix bundled
> into an MQ feature patch. The commit message says a malformed correlator
> with panic_on_warn set "would take the partition down", which is a
> user-visible fix that stable trees would want, and the WARN_ON()s being
> replaced here and in ibmveth_rxq_get_buffer() came from commit
> 2c91e2319ed9 ("net: ibmveth: Reset the adapter when unexpected states are
> detected").
>
> Could the correlator validation be split into its own patch with a
> Fixes: 2c91e2319ed9 tag, so it can be identified independently of the
> queue plumbing?
I am keeping the helper here. It is wired into
the queue-indexed harvest, remove, and get_buffer
paths, so a Fixes: peel would not be a clean
cherry-pick of just the WARN_ON() change. There is
no Fixes: tag. The commit message already says
WARN_ON() was the wrong tool.
>> +
>> +static void ibmveth_rxq_advance(struct ibmveth_rx_q *rxq)
>> +{
>> +	if (++rxq->index == rxq->num_slots) {
>> +		rxq->index = 0;
>> +		rxq->toggle = !rxq->toggle;
>> +	}
>> +}
>> +
>>   /**
>>    * ibmveth_remove_buffer_from_pool - remove a buffer from a pool
>>    * @adapter: adapter instance
>>    * @correlator: identifies pool and index
>> + * @queue_index: RX queue index (0..num_rx_queues-1)
>>    * @reuse: whether to reuse buffer
>>    *
>> + * Context: may run concurrently with netpoll replenish_task on the same
>> + * queue; takes per-queue replenish_lock to serialize free_map /
>> + * producer_index / available against the producer.
>> + *
>>    * Return:
>>    * * %0       - success
>>    * * %-EINVAL - correlator maps to pool or index out of range
>>    * * %-EFAULT - pool and index map to null skb
> [Severity: Low]
> Does this Return block still describe all the -EINVAL cases?
>
> ibmveth_rxq_correlator_valid() also returns false, mapped to -EINVAL
> here, when bpool->active is 0 or when bpool->skbuff or bpool->free_map
> are NULL. That is a deactivated or torn-down pool, not a correlator
> "out of range", and callers react to -EINVAL by advancing the ring as
> if the slot were corrupt, so the distinction is visible in behaviour.
>
> The two updated tests, ibmveth_remove_buffer_from_pool_test() and
> ibmveth_rxq_get_buffer_test(), also exercise none of the three new
> predicates: they allocate free_map and rely on the pool being active, so
> they pass identically with or without the new checks. Would it be worth
> adding cases for active == 0 and for skbuff/free_map being NULL?
>
> The doc block is unchanged at the end of the series.
Yes — -EINVAL is also an inactive or not-live pool,
not only "out of range." v7 names that on the
Return line. Callers already skip both the same
way. I am not adding extra KUnit cases for
active==0 / NULL-array; v6 already gives the
fixtures a dummy free_map so the range and
-EFAULT paths are live again.

>>    */
>>   static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
>> -					   u64 correlator, bool reuse)
>> +					   u64 correlator, int queue_index,
>> +					   bool reuse)
>>   {
>> +	struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
>>   	unsigned int pool  = correlator >> 32;
>>   	unsigned int index = correlator & 0xffffffffUL;
>>   	unsigned int free_index;
>>   	struct sk_buff *skb;
>> +	unsigned long flags;
>> +	int rc = 0;
>>   
>> -	if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
>> -	    WARN_ON(index >= adapter->rx_buff_pool[0][pool].size)) {
>> -		schedule_work(&adapter->work);
>> -		return -EINVAL;
>> +	spin_lock_irqsave(&rxq->replenish_lock, flags);
>> +
>> +	if (!ibmveth_rxq_correlator_valid(adapter, queue_index, correlator)) {
>> +		rc = -EINVAL;
>> +		goto out_unlock;
>>   	}
>>   
>> -	skb = adapter->rx_buff_pool[0][pool].skbuff[index];
>> -	if (WARN_ON(!skb)) {
>> -		schedule_work(&adapter->work);
>> -		return -EFAULT;
>> +	skb = adapter->rx_buff_pool[queue_index][pool].skbuff[index];
>> +	if (!skb) {
>> +		rc = -EFAULT;
>> +		goto out_unlock;
>>   	}
> [Severity: Medium]
> Can this leak RX pool capacity permanently on a bad correlator?
>
> The goto out_unlock skips both the free_map push and the
> atomic_dec(&pool->available) that the success path performs, and the
> schedule_work(&adapter->work) that used to accompany the WARN_ON()s is
> gone from this function.
>
> The callers treat -EINVAL/-EFAULT as "slot skipped, keep going".
> ibmveth_rxq_harvest_buffer() advances the ring anyway:
>
> 	if (unlikely(rc)) {
> 		/* Skip a corrupt slot without claiming pool ownership. */
> 		if (rc == -EINVAL || rc == -EFAULT)
> 			ibmveth_rxq_advance(rxq);
> 		return rc;
> 	}
>
> and ibmveth_drain_rx_queue() counts it as drained:
>
> 		/* -EINVAL/-EFAULT already advanced past the slot. */
> 		if (rc == -EINVAL || rc == -EFAULT) {
> 			drained++;
> 			continue;
> 		}
>
> The invalid-buffer recycle branch of ibmveth_poll() is the concrete live
> path, since it never calls ibmveth_rxq_get_buffer() and so nothing
> schedules a reset there.
>
> Given that ibmveth_replenish_buffer_pool() computes
>
> 	u32 remaining = pool->size - atomic_read(&pool->available);
>
> and ibmveth_replenish_task() only refills while available < threshold,
> does each occurrence permanently shrink the usable ring, until replenish
> stops firing for that pool altogether? For the -EINVAL case the
> DMA-mapped skb also stays in pool->skbuff[] with its free_map entry left
> at IBM_VETH_INVALID_MAP until teardown.
>
> The pre-patch code performed a close/open reset here, which reconciled
> the driver bookkeeping with PHYP. Should these paths still request a
> reset, or correct available, when the correlator is rejected?
On -EINVAL/-EFAULT the helper does not push free_map
or drop available. Harvest still advances the ring.
I am not putting schedule_work() back in
remove_buffer_from_pool: drain runs from close and
resize, and a reset from there is wrong. Patch 9
skip_bad already resets when get_buffer() returns
NULL. Recycle and drain stay advance-only.
> At the end of the series this is unchanged in effect:
> ibmveth_poll_harvest_slot() returns true for -EINVAL/-EFAULT and
> ibmveth_poll_recycle_invalid() schedules nothing, while only
> ibmveth_poll_skip_bad_correlator() schedules the reset.
>
>>   
>>   	/* if we are going to reuse the buffer then keep the pointers around
>> @@ -1127,75 +1337,88 @@ 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);
>>   
>> -	return 0;
>> +out_unlock:
>> +	spin_unlock_irqrestore(&rxq->replenish_lock, flags);
>> +	return rc;
>>   }
>>   
>>   /* get the current buffer on the rx queue */
>> -static inline struct sk_buff *ibmveth_rxq_get_buffer(struct ibmveth_adapter *adapter)
>> +static struct sk_buff *
>> +ibmveth_rxq_get_buffer(struct ibmveth_adapter *adapter,
>> +		       int queue_index)
>>   {
>> -	struct ibmveth_rx_q *rxq = &adapter->rx_queue[0];
>> +	struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
>>   	u64 correlator = rxq->queue_addr[rxq->index].correlator;
>>   	unsigned int pool = correlator >> 32;
>>   	unsigned int index = correlator & 0xffffffffUL;
>>   
>> -	if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
>> -	    WARN_ON(index >= adapter->rx_buff_pool[0][pool].size)) {
>> +	if (!ibmveth_rxq_correlator_valid(adapter, queue_index, correlator)) {
>>   		schedule_work(&adapter->work);
>>   		return NULL;
>>   	}
> [ ... ]
>
>> @@ -2161,34 +2384,41 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
> [ ... ]
>
>> -			skb = ibmveth_rxq_get_buffer(adapter);
>> +			skb = ibmveth_rxq_get_buffer(adapter, queue_index);
>>   			if (unlikely(!skb))
>>   				break;
> [ ... ]
>
>> @@ -2265,14 +2501,18 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
> [ ... ]
>
>> -	if (ibmveth_rxq_pending_buffer(adapter) && napi_schedule(napi)) {
>> -		ibmveth_disable_irq(adapter, 0);
>> +	if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
>> +	    napi_schedule(napi)) {
>> +		ibmveth_disable_irq(adapter, queue_index);
>>   		goto restart_poll;
>>   	}
> [Severity: High]
> At this commit, can ibmveth_poll() spin here forever after
> ibmveth_rxq_get_buffer() rejects a correlator?
>
> Unlike ibmveth_rxq_harvest_buffer(), ibmveth_rxq_get_buffer() returns
> NULL without advancing the ring, and the caller just breaks:
>
> 			skb = ibmveth_rxq_get_buffer(adapter, queue_index);
> 			if (unlikely(!skb))
> 				break;
>
> rxq->index and rxq->toggle are untouched, so
> ibmveth_rxq_pending_buffer() is still true, napi_schedule() succeeds and
> poll jumps back to restart_poll on the same slot. The reset queued by
> ibmveth_rxq_get_buffer() cannot break the cycle either, because
> ibmveth_reset() -> dev_close() -> napi_disable() waits for this poll to
> finish.
>
> The following patch in the series, "ibmveth: Harden RX poll path with
> helpers", replaces this bare break with
> ibmveth_poll_skip_bad_correlator(), which schedules the reset and calls
> ibmveth_poll_harvest_slot() so the slot is advanced, so the loop is gone
> by the end of the series. Would it be better to fold that handling into
> this patch, so the intermediate commit is not bisectable into a softirq
> loop?
>
> [ ... ]
Folded into this patch: when get_buffer() returns
NULL, ibmveth_rxq_advance() advances past the slot
before breaking, so poll makes progress and does not
spin at this commit. Patch 9 then refactors that path
into the dedicated skip_bad_correlator helper.

Thanks,
Mingming