2019-08-20 01:55:01

by Dexuan Cui

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

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
channels are set, and the apps will thoroughly destroy the channels
after hibernation.

Signed-off-by: Dexuan Cui <[email protected]>
---
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.
+ */
+ 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


2019-08-23 23:37:59

by Michael Kelley (LINUX)

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

From: Dexuan Cui <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>

2019-08-31 02:04:04

by Dexuan Cui

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

> From: Michael Kelley <[email protected]>
> Sent: Friday, August 23, 2019 1:02 PM
>
> From: Dexuan Cui 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/

Thanks! Will fix this in v4.

> > @@ -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.
> */

Will use the better version this in v4.

Thanks,
-- Dexuan