Re: [PATCH net-next v5 07/15] ibmveth: Add RX queue register helpers for MQ

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

On 8/17/26 6:47 PM, Jakub Kicinski wrote:
> 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 RX queue register helpers for MQ
>
> This extracts the queue-0 control-plane registration and teardown out of
> ibmveth_open()/ibmveth_close() into ibmveth_register_rx_queues() and
> ibmveth_free_all_queues(), and adds adapter->hcall_stats for register/free
> path accounting. Runtime behaviour stays single-queue since multi_queue is
> still false.
>
> A few questions on the teardown error handling and on the changelog text.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index b39e8c53cbfd1..8519cad503225 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -1270,13 +1275,27 @@ static int ibmveth_register_logical_lan(struct ibmveth_adapter *adapter,
>>   	 * try again, but only once.
>>   	 */
>>   retry:
>> -	rc = h_register_logical_lan(adapter->vdev->unit_address,
>> -				    adapter->buffer_list_dma[0], rxq_desc.desc,
>> -				    adapter->filter_list_dma, mac_address);
>> +	/* In multi-queue mode, obtain a queue handle for queue 0 so all RX
>> +	 * queues can use the same per-queue buffer hypercalls.
>> +	 */
>> +	if (adapter->multi_queue) {
>> +		rc = h_register_logical_lan_with_handle(ua, buf_dma,
>> +							rxq_desc.desc,
>> +							filter_dma,
>> +							mac_address,
>> +							&qh0);
>> +		if (rc == H_SUCCESS)
>> +			adapter->queue_handle[0] = qh0;
>> +	} else {
>> +		rc = h_register_logical_lan(ua, buf_dma, rxq_desc.desc,
>> +					    filter_dma, mac_address);
>> +	}
>> +	adapter->hcall_stats.reg_lan++;
>>   
>>   	if (rc != H_SUCCESS && try_again) {
>>   		do {
>>   			rc = h_free_logical_lan(adapter->vdev->unit_address);
>> +			adapter->hcall_stats.free_lan++;
>>   		} while (H_IS_LONG_BUSY(rc) || (rc == H_BUSY));
>>   
>>   		try_again = 0;
> [ ... ]
>
>> +static void ibmveth_free_all_queues(struct ibmveth_adapter *adapter)
>> +{
>> +	unsigned long lpar_rc;
>> +	int i;
>> +
>> +	netdev_dbg(adapter->netdev, "freeing all RX queues at once\n");
>> +
>> +	do {
>> +		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>> +		adapter->hcall_stats.free_lan++;
>> +	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>> +
>> +	if (lpar_rc != H_SUCCESS) {
>> +		netdev_err(adapter->netdev,
>> +			   "h_free_logical_lan failed: %ld\n", lpar_rc);
>> +	}
>> +
>> +	for (i = 0; i < adapter->num_rx_queues; i++)
>> +		adapter->queue_handle[i] = 0;
>> +}
> [Severity: High]
> This isn't a bug introduced by this patch - the baseline ibmveth_close()
> already had the same log-and-continue loop ("h_free_logical_lan failed with
> %lx, continuing") followed by ibmveth_free_buffer_pools() - but the new
> helper makes it harder to fix later, so a question about it.
>
> The retry loop here only retries H_BUSY and H_IS_LONG_BUSY. Any other
> status (H_PARAMETER, H_STATE, H_HARDWARE) leaves the loop, is logged, and
> then the helper returns void, so neither ibmveth_close() nor the
> ibmveth_open() unwind can see that the logical LAN may still be registered.
>
> Right after the netdev_err(), queue_handle[] is cleared unconditionally:
>
> 	for (i = 0; i < adapter->num_rx_queues; i++)
> 		adapter->queue_handle[i] = 0;
>
> Should that clearing be conditional on H_SUCCESS? Once multi_queue is
> enabled later in the series, ibmveth_deregister_single_rx_queue() bails out
> on "if (!qh)", so zeroing the handles after a failed free removes the only
> reference a per-queue H_FREE_LOGICAL_LAN_QUEUE retry would need.
>
> Would it make sense for this helper to return the hypercall status so the
> callers can decide, rather than returning void?
Yes. Clearing handles after a failed free throws away the retry
reference. Returning status without a caller policy is worse than
matching SQ close — callers would have to keep the mappings too, and
resize has to match. Halfway is worse. v6 documents the void return
and the unconditional clear in the kdoc.
>
> [ ... ]
>
>> @@ -1309,37 +1407,23 @@ static int ibmveth_open(struct net_device *netdev)
>>   	if (rc)
>>   		goto out_free_filter_list;
>>   
>> -	mac_address = ether_addr_to_u64(netdev->dev_addr);
>> -
>> -	rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
>> -					adapter->rx_queue[0].queue_len;
>> -	rxq_desc.fields.address = adapter->rx_queue[0].queue_dma;
>> -
>> -	adapter->queue_irq[0] = netdev->irq;
>> -	ibmveth_disable_irq(adapter, 0);
>> -
>> -	lpar_rc = ibmveth_register_logical_lan(adapter, rxq_desc, mac_address);
>> -
>> -	if (lpar_rc != H_SUCCESS) {
>> -		netdev_err(netdev, "h_register_logical_lan failed with %ld\n",
>> -			   lpar_rc);
>> -		netdev_err(netdev, "buffer TCE:0x%llx filter TCE:0x%llx rxq "
>> -			   "desc:0x%llx MAC:0x%llx\n",
>> -				     adapter->buffer_list_dma[0],
>> -				     adapter->filter_list_dma,
>> -				     rxq_desc.desc,
>> -				     mac_address);
>> -		rc = -ENONET;
>> +	rc = ibmveth_alloc_buffer_pools(adapter);
>> +	if (rc)
>>   		goto out_free_queue_mem;
>> -	}
>>   
>> -	rc = ibmveth_alloc_buffer_pools(adapter);
>> +	rc = ibmveth_register_rx_queues(adapter, mac_address);
>>   	if (rc)
>> -		goto out_unregister_lan;
>> +		goto out_free_buffer_pools;
>> +
>> +	rc = netif_set_real_num_rx_queues(netdev, adapter->num_rx_queues);
>> +	if (rc) {
>> +		netdev_err(netdev, "failed to set number of rx queues\n");
>> +		goto out_unregister_queues;
>> +	}
>>   
>>   	rc = ibmveth_setup_rx_interrupts(adapter);
>>   	if (rc)
>> -		goto out_unregister_lan;
>> +		goto out_free_all_queues; /* setup already disposed IRQs */
> [Severity: Low]
> The changelog says:
>
>    "failures after successful LAN registration enter
>     out_unregister_queues, then fall through to out_free_buffer_pools"
>
> Is that the label graph the code actually has? Only the
> netif_set_real_num_rx_queues() failure jumps to out_unregister_queues. The
> ibmveth_setup_rx_interrupts() failure goes straight to
> out_free_all_queues, and the ibmveth_alloc_tx_resources() failure goes to
> out_cleanup_rx_interrupts which then does "goto out_free_all_queues".
>
> All three do reach ibmveth_free_all_queues() and
> ibmveth_free_buffer_pools(), so the "RX pools are not leaked" part still
> holds; only the description of which label the paths enter looks off.
> Could the changelog be adjusted to match?
Only set_real_num_rx_queues() enters that label. v6 dropped the walk
from the message.
>>   
>>   	netdev_dbg(netdev, "initial replenish cycle\n");
>>   	ibmveth_schedule_rx_queue(adapter, 0);
>> @@ -1357,10 +1441,12 @@ static int ibmveth_open(struct net_device *netdev)
>>   
>>   out_cleanup_rx_interrupts:
>>   	ibmveth_cleanup_rx_interrupts(adapter);
>> -out_unregister_lan:
>> -	do {
>> -		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>> -	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>> +	goto out_free_all_queues; /* cleanup already disposed IRQs */
>> +out_unregister_queues:
>> +	ibmveth_dispose_subordinate_irq_mappings(adapter);
>> +out_free_all_queues:
>> +	ibmveth_free_all_queues(adapter);
>> +out_free_buffer_pools:
>>   	ibmveth_free_buffer_pools(adapter);
>>   out_free_queue_mem:
>>   	ibmveth_cleanup_rx_resources(adapter);
> [Severity: High]
> Same pre-existing concern as above, now on the open-failure unwind: since
> ibmveth_free_all_queues() cannot report failure, this path unconditionally
> continues into ibmveth_free_buffer_pools() (dma_unmap_single() plus
> dev_kfree_skb_any() of every posted RX buffer) and
> ibmveth_cleanup_rx_resources() (dma_unmap of the buffer-list page,
> free_page(), dma_free_coherent() of the RX ring).
>
> If H_FREE_LOGICAL_LAN returned something other than H_SUCCESS and the LAN
> is still registered, can PHYP keep writing received frames and the
> rx_no_buffer counter into those unmapped or freed pages?
Yes. If h_free does not succeed, PHYP can still write into those
pages. Same hazard as baseline close, and the same reason as above:
returning status without a caller policy is worse than matching SQ
close.
>> @@ -1395,15 +1480,7 @@ static int ibmveth_close(struct net_device *netdev)
>>   	synchronize_net();
>>   
>>   	ibmveth_update_rx_no_buffer(adapter);
>> -	/* Full LAN teardown (subordinates arrive with register helpers). */
>> -	do {
>> -		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>> -	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>> -	if (lpar_rc != H_SUCCESS) {
>> -		netdev_err(adapter->netdev,
>> -			   "h_free_logical_lan failed with %lx, continuing\n",
>> -			   lpar_rc);
>> -	}
>> +	ibmveth_free_all_queues(adapter);
> [Severity: High]
> This is the pre-existing log-and-continue behaviour moved into the helper,
> so the same question applies on the close path: after
> ibmveth_free_all_queues() the function proceeds to
> ibmveth_free_tx_resources(), ibmveth_free_buffer_pools() and
> ibmveth_cleanup_rx_resources() regardless of the hypercall result.
>
> The changelog states:
>
>    "free_all_queues() runs before free_buffer_pools() on open failure
>     and close, so PHYP cannot retain a registered LAN while its
>     DMA-backed RX pools are released."
>
> Does that hold when the hypercall does not succeed? The ordering only
> gives that guarantee on H_SUCCESS; on a non-busy failure the LAN may still
> be registered when the pools go away. Could the changelog note that, or
> could the helper propagate the status so close() can keep the DMA
> mappings?
>
> I also checked the end of the series (1f83fd44d867) and
> ibmveth_free_all_queues() is unchanged there, so this is not addressed by
> a later patch in the series.
No. Only on H_SUCCESS. v6 dropped that sentence; the kdoc records the
limitation. Still unchanged at tip.
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
>> index bf5dd5703773d..fae3473cc4985 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
>> @@ -268,6 +268,17 @@ static int pool_active[] = { 1, 1, 0, 0, 1};
>>   
>>   #define IBM_VETH_INVALID_MAP ((u16)0xffff)
>>   
>> +struct ibmveth_hcall_stats {
>> +	u64 reg_lan_queue;	/* H_REG_LOGICAL_LAN_QUEUE */
>> +	u64 reg_lan;		/* H_REGISTER_LOGICAL_LAN */
>> +	u64 add_bufs_queue;	/* H_ADD_LOGICAL_LAN_BUFFERS_QUEUE */
>> +	u64 add_bufs;		/* H_ADD_LOGICAL_LAN_BUFFERS */
>> +	u64 add_buf;		/* H_ADD_LOGICAL_LAN_BUFFER */
>> +	u64 free_lan_queue;	/* H_FREE_LOGICAL_LAN_QUEUE */
>> +	u64 free_lan;		/* H_FREE_LOGICAL_LAN */
>> +	u64 send_lan;		/* H_SEND_LOGICAL_LAN */
>> +};
> [Severity: Low]
> This isn't a bug, just a note on what was checked. At this commit only
> reg_lan and free_lan have writers and none of the eight counters has a
> reader, even though h_add_logical_lan_buffer(),
> h_add_logical_lan_buffers() and h_send_logical_lan() already exist in the
> driver.
>
> Looking at the end of the series (1f83fd44d867), all eight counters get
> both producers and a consumer through ethtool -S ("hcall_reg_lan_queue",
> "hcall_reg_lan", "hcall_add_bufs_queue", "hcall_add_bufs", "hcall_add_buf",
> "hcall_free_lan_queue", "hcall_free_lan", "hcall_send_lan"), so this
> resolves within the series and needs no action.
>
> For completeness: reg_lan is incremented even when registration fails, and
> free_lan is incremented once per H_BUSY retry. Given the field comments
> name the hypercalls and the struct is described as hypercall statistics,
> invocation counts look like the intent, so no change is requested here
> either.

They were invocation counts, so no semantic correction needed. The
incomplete-producers issue also resolves: v6 deletes all eight rather
than finishing them. No struct, no ethtool keys.

Thanks,

Mingming