RE: [PATCH v3 10/12] Drivers: hv: vmbus: Clean up hv_sock channels by force upon suspend

From: Michael Kelley <mikelley@microsoft.com>
To: Dexuan Cui <decui@microsoft.com>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Sasha Levin <Alexander.Levin@microsoft.com>,
	"sashal@kernel.org" <sashal@kernel.org>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	KY Srinivasan <kys@microsoft.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3 10/12] Drivers: hv: vmbus: Clean up hv_sock channels by force upon suspend
Date: Fri, 23 Aug 2019 20:02:12 +0000	[thread overview]
Message-ID: <DM5PR21MB013722B88011C7D587BB6182D7A40@DM5PR21MB0137.namprd21.prod.outlook.com> (raw)
In-Reply-To: <1566265863-21252-11-git-send-email-decui@microsoft.com>

From: Dexuan Cui <decui@microsoft.com> Sent: Monday, August 19, 2019 6:52 PM
> 
> Fake RESCIND_CHANNEL messages to clean up hv_sock channels by force for
> hibernation. There is no better method to clean up the channels since
> some of the channels may still be referenced by the userspace apps when
> hiberantin is triggered: in this case, the "rescind" fields of the

s/hiberantin/hibernation/

> channels are set, and the apps will thoroughly destroy the channels
> after hibernation.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 55
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index ce9974b..2bea669 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -24,6 +24,7 @@
>  #include <linux/sched/task_stack.h>
> 
>  #include <asm/mshyperv.h>
> +#include <linux/delay.h>
>  #include <linux/notifier.h>
>  #include <linux/ptrace.h>
>  #include <linux/screen_info.h>
> @@ -1069,6 +1070,41 @@ void vmbus_on_msg_dpc(unsigned long data)
>  	vmbus_signal_eom(msg, message_type);
>  }
> 
> +/*
> + * Fake RESCIND_CHANNEL messages to clean up hv_sock channels by force for
> + * hibernation, because hv_sock connections can not persist across hibernation.
> + */
> +static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
> +{
> +	struct onmessage_work_context *ctx;
> +	struct vmbus_channel_rescind_offer *rescind;
> +
> +	WARN_ON(!is_hvsock_channel(channel));
> +
> +	/*
> +	 * sizeof(*ctx) is small and the allocation should really not fail,
> +	 * otherwise the state of the hv_sock connections ends up in limbo.
> +	 */
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL | __GFP_NOFAIL);
> +
> +	/*
> +	 * So far, these are not really used by Linux. Just set them to the
> +	 * reasonable values conforming to the definitions of the fields.
> +	 */
> +	ctx->msg.header.message_type = 1;
> +	ctx->msg.header.payload_size = sizeof(*rescind);
> +
> +	/* These values are actually used by Linux. */
> +	rescind = (struct vmbus_channel_rescind_offer *)ctx->msg.u.payload;
> +	rescind->header.msgtype = CHANNELMSG_RESCIND_CHANNELOFFER;
> +	rescind->child_relid = channel->offermsg.child_relid;
> +
> +	INIT_WORK(&ctx->work, vmbus_onmessage_work);
> +
> +	queue_work_on(vmbus_connection.connect_cpu,
> +		      vmbus_connection.work_queue,
> +		      &ctx->work);
> +}
> 
>  /*
>   * Direct callback for channels using other deferred processing
> @@ -2091,6 +2127,25 @@ static int vmbus_acpi_add(struct acpi_device *device)
> 
>  static int vmbus_bus_suspend(struct device *dev)
>  {
> +	struct vmbus_channel *channel;
> +
> +	while (atomic_read(&vmbus_connection.offer_in_progress) != 0) {
> +		/*
> +		 * We wait here until any channel offer is currently
> +		 * being processed.
> +		 */

The wording of the comment is a bit off.  Maybe

		/*
		 * We wait here until the completion of any channel
		 * offers that are currently in progress.
		 */

> +		msleep(1);
> +	}
> +
> +	mutex_lock(&vmbus_connection.channel_mutex);
> +	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
> +		if (!is_hvsock_channel(channel))
> +			continue;
> +
> +		vmbus_force_channel_rescinded(channel);
> +	}
> +	mutex_unlock(&vmbus_connection.channel_mutex);
> +
>  	vmbus_initiate_unload(false);
> 
>  	vmbus_connection.conn_state = DISCONNECTED;
> --
> 1.8.3.1

Modulo the nits:

Reviewed-by: Michael Kelley <mikelley@microsoft.com>