Message-ID:
 <SN6PR02MB41577ABE2174E5630703F842D41A2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Thu, 16 Jan 2025 17:20:12 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Hamza Mahfooz <hamzamahfooz@...ux.microsoft.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>
CC: Boqun Feng <boqun.feng@...il.com>, Wei Liu <wei.liu@...nel.org>, "K. Y.
 Srinivasan" <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>,
	Dexuan Cui <decui@...rosoft.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v4 2/2] drivers/hv: add CPU offlining support

From: Hamza Mahfooz <hamzamahfooz@...ux.microsoft.com> Sent: Thursday, January 16, 2025 5:42 AM
> 
> Currently, it is tedious to offline CPUs. Since, most CPUs will have
> vmbus channels attached to them that a user would have to manually
> rebind elsewhere. 

Cleaning up the punctuation and wording a bit:

Currently, it is tedious to offline CPUs in a Hyper-V VM since CPUs may
have VMBus channels attached to them that a user would have to manually
rebind elsewhere.

> So, as made mention of in
> commit d570aec0f2154 ("Drivers: hv: vmbus: Synchronize init_vp_index()
> vs. CPU hotplug"), rebind channels associated with CPUs that a user is
> trying to offline to a new "randomly" selected CPU.
> 
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Michael Kelley <mhklinux@...look.com>
> Cc: Wei Liu <wei.liu@...nel.org>
> Signed-off-by: Hamza Mahfooz <hamzamahfooz@...ux.microsoft.com>
> ---
> v2: remove cpus_read_{un,}lock() from hv_pick_new_cpu() and add
>     lockdep_assert_cpus_held().
> 
> v3: use for_each_cpu_wrap() in hv_pick_new_cpu().
> 
> v4: store get_random_u32_below() in start.
> ---
>  drivers/hv/hv.c | 65 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 36d9ba097ff5..4388c0030a59 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -433,13 +433,48 @@ static bool hv_synic_event_pending(void)
>  	return pending;
>  }
> 
> +static int hv_pick_new_cpu(struct vmbus_channel *channel,
> +			   unsigned int current_cpu)
> +{
> +	int ret = 0;
> +	int start;
> +	int cpu;
> +
> +	lockdep_assert_cpus_held();
> +	lockdep_assert_held(&vmbus_connection.channel_mutex);
> +
> +	/*
> +	 * We can't assume that the relevant interrupts will be sent before
> +	 * the cpu is offlined on older versions of hyperv.
> +	 */
> +	if (vmbus_proto_version < VERSION_WIN10_V5_3)
> +		return -EBUSY;
> +
> +	start = get_random_u32_below(nr_cpu_ids);
> +
> +	for_each_cpu_wrap(cpu, cpu_online_mask, start) {
> +		if (cpu == current_cpu || cpu == VMBUS_CONNECT_CPU)
> +			continue;
> +
> +		ret = vmbus_channel_set_cpu(channel, cpu);
> +

Avoid the blank line between setting "ret" and testing it. If you look through
other Linux kernel code, you'll see that the set and test are almost always
grouped together with no intervening blank line.

> +		if (!ret)
> +			break;
> +	}

The loop looks good as a solution to filtering isolated CPUs!  But there's
a subtle bug. Consider a VM with two CPUs. One of the CPUs is the
VMBUS_CONNECT_CPU, and the other CPU is being taken offline.
The loop will exit without ever having called vmbus_channel_set_cpu(),
so "ret" will still be zero. The check below will *not* call
vmbus_channel_set_cpu(), and the CPU will be offlined with the
channel interrupt still assigned.

I think the easy fix is to initialize "ret" to -EBUSY, but double-check
my thinking.

> +
> +	if (ret)
> +		ret = vmbus_channel_set_cpu(channel, VMBUS_CONNECT_CPU);
> +
> +	return ret;
> +}
> +
>  /*
>   * hv_synic_cleanup - Cleanup routine for hv_synic_init().
>   */
>  int hv_synic_cleanup(unsigned int cpu)
>  {
>  	struct vmbus_channel *channel, *sc;
> -	bool channel_found = false;
> +	int ret = 0;
> 
>  	if (vmbus_connection.conn_state != CONNECTED)
>  		goto always_cleanup;
> @@ -456,31 +491,31 @@ int hv_synic_cleanup(unsigned int cpu)
> 
>  	/*
>  	 * Search for channels which are bound to the CPU we're about to
> -	 * cleanup.  In case we find one and vmbus is still connected, we
> -	 * fail; this will effectively prevent CPU offlining.
> -	 *
> -	 * TODO: Re-bind the channels to different CPUs.
> +	 * cleanup.
>  	 */
>  	mutex_lock(&vmbus_connection.channel_mutex);
>  	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
>  		if (channel->target_cpu == cpu) {
> -			channel_found = true;
> -			break;
> +			ret = hv_pick_new_cpu(channel, cpu);
> +

As above, avoid the blank line here.

> +			if (ret) {
> +				mutex_unlock(&vmbus_connection.channel_mutex);
> +				return ret;
> +			}
>  		}
>  		list_for_each_entry(sc, &channel->sc_list, sc_list) {
>  			if (sc->target_cpu == cpu) {
> -				channel_found = true;
> -				break;
> +				ret = hv_pick_new_cpu(channel, cpu);

The first argument to hv_pick_new_cpu() should be "sc", not "channel".
Currently you'll be updating the primary channel, not the intended
sub-channel.

Also, you don't really need to pass the "cpu" as an argument to
hv_pick_new_cpu(). Just have that function take the channel as its
only argument.  It can get the cpu as the target_cpu field in the channel.

> +

Avoid the blank line here too.

> +				if (ret) {
> +					mutex_unlock(&vmbus_connection.channel_mutex);
> +					return ret;
> +				}
>  			}
>  		}
> -		if (channel_found)
> -			break;
>  	}
>  	mutex_unlock(&vmbus_connection.channel_mutex);
> 
> -	if (channel_found)
> -		return -EBUSY;
> -
>  	/*
>  	 * channel_found == false means that any channels that were previously

Since the "channel_found" local variable has been removed, this comment
needs some cleanup.

>  	 * assigned to the CPU have been reassigned elsewhere with a call of
> @@ -497,5 +532,5 @@ int hv_synic_cleanup(unsigned int cpu)
> 
>  	hv_synic_disable_regs(cpu);
> 
> -	return 0;
> +	return ret;
>  }
> --
> 2.47.1

Powered by blists - more mailing lists