RE: [PATCH v2] Drivers: hv: Avoid infinite retry loop in init_vp_index()

[Michael Kelley]

From: Waiman Long <longman@xxxxxxxxxx> Sent: Wednesday, August 26, 2026 12:25 PM
> 
> There is a retry loop in init_vp_index() where the CPUs from a certain
> node are stripped out if they have already been in the allocated cpumask
> or not in HK_TYPE_MANAGED_IRQ housekeeping cpumask. If there is no
> CPU left, the allocated cpumask is ignored and the process is retried
> again. However, if the HK_TYPE_MANAGED_IRQ housekeeping cpumask turns
> out not to contain any CPU in that particular node, that will become an
> infinite retry loop.  This particular problem was reported by sashiko
> [1]. This should rarely happen, but we still need to guard against this.
> 
> Fix this infinite loop problem by skipping to the next numa node if
> the allocated cpumask has already been cleared before. Set target_cpu
> to the default VMBUS_CONNECT_CPU instead if the for loop is ending.
> 
> Link: https://sashiko.dev/#/message/20260422030903.E1BFCC2BCB0%40smtp.kernel.org [1]
> Fixes: 6640b5df1a38 ("Drivers: hv: vmbus: Don't assign VMbus channel interrupts to isolated CPUs")
> Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
> ---
>  drivers/hv/channel_mgmt.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 89d214dda360..e80ac9fb5ed3 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -793,10 +793,20 @@ static void init_vp_index(struct vmbus_channel *channel)
>  		if (cpumask_empty(available_mask)) {
>  			/*
>  			 * We have cycled through all the CPUs in the node;
> -			 * reset the allocated map.
> +			 * reset the allocated map. If the allocated map
> +			 * has already been cleared, we will try the next numa
> +			 * node. Set target_cpu to the default VMBUS_CONNECT_CPU
> +			 * instead if the for loop is going to end.
>  			 */
> -			cpumask_clear(allocated_mask);
> -			goto retry;
> +			if (!cpumask_empty(allocated_mask)) {
> +				cpumask_clear(allocated_mask);
> +				goto retry;
> +			}

As you describe in the commit message, the core problem is a NUMA
node that has no housekeeping CPUs. But rather than add a test
here for that condition, consider that there is a "while (true)" loop a
few lines above that cycles thru the NUMA nodes and already skips
NUMA nodes that have no CPUs. What if that loop was enhanced so
that NUMA nodes with no housekeeping CPUs are skipped? Then all
the existing machinery just works. I think this approach also
addresses the two Sashiko comments.

> +			if (i > ncpu) {
> +				target_cpu = VMBUS_CONNECT_CPU;
> +				break;

Can this condition really happen? Won't the "for" loop always find
a housekeeping CPU in some NUMA node? If you want some
additional robustness in case the "for" loop doesn't pick a
CPU, I think initializing target_cpu to VMBUS_CONNECT_CPU up
in the local variable definitions would be sufficient.

Michael

> +			}
> +			continue;	/* Try next numa node */
>  		}
> 
>  		target_cpu = cpumask_first(available_mask);
> --
> 2.55.0
>