Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Dexuan Cui <decui@microsoft.com>
Cc: KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"sashal\@kernel.org" <sashal@kernel.org>,
	"linux-hyperv\@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Michael Kelley <mikelley@microsoft.com>
Subject: Re: [PATCH 1/3] hv_utils: Add the support of hibernation
Date: Thu, 12 Sep 2019 18:36:32 +0200
Message-ID: <877e6dcvzj.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <1568245130-70712-2-git-send-email-decui@microsoft.com>

Dexuan Cui <decui@microsoft.com> writes:

> On hibernation, Linux can not guarantee the host side utils operations
> still succeed without any issue, so let's simply cancel the work items.
> The host is supposed to retry the operations, if necessary.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/hv_fcopy.c     |  9 ++++++++-
>  drivers/hv/hv_kvp.c       | 11 +++++++++--
>  drivers/hv/hv_snapshot.c  | 11 +++++++++--
>  drivers/hv/hv_util.c      | 37 ++++++++++++++++++++++++++++++++++++-
>  drivers/hv/hyperv_vmbus.h |  3 +++
>  include/linux/hyperv.h    |  1 +
>  6 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index 7e30ae0..f44df3d 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -345,9 +345,16 @@ int hv_fcopy_init(struct hv_util_service *srv)
>  	return 0;
>  }
>  
> +void hv_fcopy_cancel_work(void)
> +{
> +	cancel_delayed_work_sync(&fcopy_timeout_work);
> +}
> +
>  void hv_fcopy_deinit(void)
>  {
>  	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
> -	cancel_delayed_work_sync(&fcopy_timeout_work);
> +
> +	hv_fcopy_cancel_work();
> +
>  	hvutil_transport_destroy(hvt);
>  }
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index 5054d11..064c384 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -757,11 +757,18 @@ static void kvp_on_reset(void)
>  	return 0;
>  }
>  
> -void hv_kvp_deinit(void)
> +void hv_kvp_cancel_work(void)
>  {
> -	kvp_transaction.state = HVUTIL_DEVICE_DYING;
>  	cancel_delayed_work_sync(&kvp_host_handshake_work);
>  	cancel_delayed_work_sync(&kvp_timeout_work);
>  	cancel_work_sync(&kvp_sendkey_work);
> +}
> +
> +void hv_kvp_deinit(void)
> +{
> +	kvp_transaction.state = HVUTIL_DEVICE_DYING;
> +
> +	hv_kvp_cancel_work();
> +
>  	hvutil_transport_destroy(hvt);
>  }
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index 20ba95b..0eb718a 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -378,10 +378,17 @@ static void vss_on_reset(void)
>  	return 0;
>  }
>  
> -void hv_vss_deinit(void)
> +void hv_vss_cancel_work(void)
>  {
> -	vss_transaction.state = HVUTIL_DEVICE_DYING;
>  	cancel_delayed_work_sync(&vss_timeout_work);
>  	cancel_work_sync(&vss_handle_request_work);
> +}
> +
> +void hv_vss_deinit(void)
> +{
> +	vss_transaction.state = HVUTIL_DEVICE_DYING;
> +
> +	hv_vss_cancel_work();
> +
>  	hvutil_transport_destroy(hvt);
>  }
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index e32681e..039c752 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -81,12 +81,14 @@
>  };
>  
>  static int hv_timesync_init(struct hv_util_service *srv);
> +static void hv_timesync_cancel_work(void);
>  static void hv_timesync_deinit(void);
>  
>  static void timesync_onchannelcallback(void *context);
>  static struct hv_util_service util_timesynch = {
>  	.util_cb = timesync_onchannelcallback,
>  	.util_init = hv_timesync_init,
> +	.util_cancel_work = hv_timesync_cancel_work,
>  	.util_deinit = hv_timesync_deinit,
>  };
>  
> @@ -98,18 +100,21 @@
>  static struct hv_util_service util_kvp = {
>  	.util_cb = hv_kvp_onchannelcallback,
>  	.util_init = hv_kvp_init,
> +	.util_cancel_work = hv_kvp_cancel_work,
>  	.util_deinit = hv_kvp_deinit,
>  };
>  
>  static struct hv_util_service util_vss = {
>  	.util_cb = hv_vss_onchannelcallback,
>  	.util_init = hv_vss_init,
> +	.util_cancel_work = hv_vss_cancel_work,
>  	.util_deinit = hv_vss_deinit,
>  };
>  
>  static struct hv_util_service util_fcopy = {
>  	.util_cb = hv_fcopy_onchannelcallback,
>  	.util_init = hv_fcopy_init,
> +	.util_cancel_work = hv_fcopy_cancel_work,
>  	.util_deinit = hv_fcopy_deinit,
>  };
>  
> @@ -440,6 +445,28 @@ static int util_remove(struct hv_device *dev)
>  	return 0;
>  }
>  
> +static int util_suspend(struct hv_device *dev)
> +{
> +	struct hv_util_service *srv = hv_get_drvdata(dev);
> +
> +	if (srv->util_cancel_work)
> +		srv->util_cancel_work();
> +
> +	vmbus_close(dev->channel);

And what happens if you receive a real reply from userspace after you
close the channel? You've only cancelled timeout work so the driver
will not try to reply by itself but this doesn't mean we won't try to
write to the channel on legitimate replies from daemons.

I think you need to block all userspace requests (hang in kernel until
util_resume()).

While I'm not sure we can't get away without it but I'd suggest we add a
new HVUTIL_SUSPENDED state to the hvutil state machine.

> +
> +	return 0;
> +}
> +
> +static int util_resume(struct hv_device *dev)
> +{
> +	struct hv_util_service *srv = hv_get_drvdata(dev);
> +	int ret;
> +
> +	ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE,
> +			 NULL, 0, srv->util_cb, dev->channel);
> +	return ret;
> +}
> +
>  static const struct hv_vmbus_device_id id_table[] = {
>  	/* Shutdown guid */
>  	{ HV_SHUTDOWN_GUID,
> @@ -476,6 +503,8 @@ static int util_remove(struct hv_device *dev)
>  	.id_table = id_table,
>  	.probe =  util_probe,
>  	.remove =  util_remove,
> +	.suspend = util_suspend,
> +	.resume =  util_resume,
>  	.driver = {
>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>  	},
> @@ -545,11 +574,17 @@ static int hv_timesync_init(struct hv_util_service *srv)
>  	return 0;
>  }
>  
> +static void hv_timesync_cancel_work(void)
> +{
> +	cancel_work_sync(&adj_time_work);
> +}
> +
>  static void hv_timesync_deinit(void)
>  {
>  	if (hv_ptp_clock)
>  		ptp_clock_unregister(hv_ptp_clock);
> -	cancel_work_sync(&adj_time_work);
> +
> +	hv_timesync_cancel_work();
>  }
>  
>  static int __init init_hyperv_utils(void)
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index f7a5f56..dc280fa 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -353,14 +353,17 @@ int vmbus_add_channel_kobj(struct hv_device *device_obj,
>  void vmbus_on_msg_dpc(unsigned long data);
>  
>  int hv_kvp_init(struct hv_util_service *srv);
> +void hv_kvp_cancel_work(void);
>  void hv_kvp_deinit(void);
>  void hv_kvp_onchannelcallback(void *context);
>  
>  int hv_vss_init(struct hv_util_service *srv);
> +void hv_vss_cancel_work(void);
>  void hv_vss_deinit(void);
>  void hv_vss_onchannelcallback(void *context);
>  
>  int hv_fcopy_init(struct hv_util_service *srv);
> +void hv_fcopy_cancel_work(void);
>  void hv_fcopy_deinit(void);
>  void hv_fcopy_onchannelcallback(void *context);
>  void vmbus_initiate_unload(bool crash);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index a3aa9e9..b4e2768 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1412,6 +1412,7 @@ struct hv_util_service {
>  	void (*util_cb)(void *);
>  	int (*util_init)(struct hv_util_service *);
>  	void (*util_deinit)(void);
> +	void (*util_cancel_work)(void);
>  };
>  
>  struct vmbuspipe_hdr {

-- 
Vitaly

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-11 23:38 [PATCH 0/3] Enhance hv_utils to support hibernation Dexuan Cui
2019-09-11 23:38 ` [PATCH 1/3] hv_utils: Add the support of hibernation Dexuan Cui
2019-09-12 16:36   ` Vitaly Kuznetsov [this message]
2019-09-13 19:15     ` Dexuan Cui
2019-09-16  8:45       ` Vitaly Kuznetsov
2019-09-11 23:38 ` [PATCH 2/3] hv_utils: Support host-initiated hibernation request Dexuan Cui
2019-09-12 16:26   ` Vitaly Kuznetsov
2019-09-13 16:42     ` Dexuan Cui
2019-09-11 23:39 ` [PATCH 3/3] hv_utils: Support host-initiated restart request Dexuan Cui

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877e6dcvzj.fsf@vitty.brq.redhat.com \
    --to=vkuznets@redhat.com \
    --cc=decui@microsoft.com \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=sashal@kernel.org \
    --cc=sthemmin@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org linux-hyperv@archiver.kernel.org
	public-inbox-index linux-hyperv


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/ public-inbox