Re: [PATCH v3] Drivers: hv: Avoid infinite retry loop in init_vp_index()

[Waiman Long <longman@redhat.com>]

On 8/30/26 7:17 PM, Michael Kelley wrote:
> From: Waiman Long <longman@redhat.com> Sent: Friday, August 28, 2026 6:22 PM
>> On 8/27/26 5:10 PM, Michael Kelley wrote:
>>> From: Waiman Long <longman@redhat.com> Sent: Thursday, August 27, 2026 12:38 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 also skipping NUMA node that has no
>>>> housekeeping CPU in the inner while loop of init_vp_index(). As the outer
>>>> for loop will only be reached if the housekeeping cpumask isn't empty,
>>>> a NUMA node with housekeeping CPUs will eventually be found.
>>>>
>>>> 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@redhat.com>
>>>> ---
>>>>    drivers/hv/channel_mgmt.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>>>> index 89d214dda360..ed121d74d73f 100644
>>>> --- a/drivers/hv/channel_mgmt.c
>>>> +++ b/drivers/hv/channel_mgmt.c
>>>> @@ -752,6 +752,7 @@ static void init_vp_index(struct vmbus_channel *channel)
>>>>    	u32 i, ncpu = num_online_cpus();
>>>>    	cpumask_var_t available_mask;
>>>>    	struct cpumask *allocated_mask;
>>>> +	const struct cpumask *node_mask;
>>>>    	const struct cpumask *hk_mask = housekeeping_cpumask(HK_TYPE_MANAGED_IRQ);
>>>>    	u32 target_cpu;
>>>>    	int numa_node;
>>>> @@ -780,14 +781,16 @@ static void init_vp_index(struct vmbus_channel *channel)
>>>>    				next_numa_node_id = 0;
>>>>    				continue;
>>>>    			}
>>>> -			if (cpumask_empty(cpumask_of_node(numa_node)))
>>>> +			node_mask = cpumask_of_node(numa_node);
>>>> +			if (cpumask_empty(node_mask) ||
>>>> +			    !cpumask_intersects(node_mask, hk_mask))
>>> The cpumask_empty() test looks to be redundant. The
>>> cpumask_intersects() test will catch the case where
>>> node_mask is empty.
>>>
>>> Otherwise, I think this looks good as a solution to the core
>>> problem.
>>>
>>> Michael
>> Yes, I am aware that cpumask_empty() test is redundant and can be
>> skipped. I keep it just to make it easier to read. I can certainly drop
>> the cpumask_empty() statement.
>>
> The flip side is people like me try to figure out "why is this
> here?", thinking there must be a reason. :-)  I'd say drop the
> code and add a comment like "Also catches an empty node_mask".

Fair, will do that.

Thanks,
Longman

> Michael