[PATCH] hv_balloon: Add the support of hibernation

[PATCH] hv_balloon: Add the support of hibernation

[Dexuan Cui]

When hibernation is enabled, we must ignore the balloon up/down and
hot-add requests from the host, if any.

Fow now, if people want to test hibernation, please blacklist hv_balloon
or do not enable Dynamic Memory and Memory Resizing. See the comment in
balloon_probe() for more info.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---

This patch is basically a pure Hyper-V specific change and it has a
build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus: Implement
suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
Hyper-V tree's hyperv-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next

I request this patch should go through Sasha's tree rather than the
other tree(s).

 drivers/hv/hv_balloon.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 34bd735..7df0f67 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -24,6 +24,8 @@
 
 #include <linux/hyperv.h>
 
+#include <asm/mshyperv.h>
+
 #define CREATE_TRACE_POINTS
 #include "hv_trace_balloon.h"
 
@@ -457,6 +459,7 @@ struct hot_add_wrk {
 	struct work_struct wrk;
 };
 
+static bool allow_hibernation;
 static bool hot_add = true;
 static bool do_hot_add;
 /*
@@ -1053,8 +1056,12 @@ static void hot_add_req(struct work_struct *dummy)
 	else
 		resp.result = 0;
 
-	if (!do_hot_add || (resp.page_count == 0))
-		pr_err("Memory hot add failed\n");
+	if (!do_hot_add || resp.page_count == 0) {
+		if (!allow_hibernation)
+			pr_err("Memory hot add failed\n");
+		else
+			pr_info("Ignore hot-add request!\n");
+	}
 
 	dm->state = DM_INITIALIZED;
 	resp.hdr.trans_id = atomic_inc_return(&trans_id);
@@ -1509,6 +1516,11 @@ static void balloon_onchannelcallback(void *context)
 			break;
 
 		case DM_BALLOON_REQUEST:
+			if (allow_hibernation) {
+				pr_info("Ignore balloon-up request!\n");
+				break;
+			}
+
 			if (dm->state == DM_BALLOON_UP)
 				pr_warn("Currently ballooning\n");
 			bal_msg = (struct dm_balloon *)recv_buffer;
@@ -1518,6 +1530,11 @@ static void balloon_onchannelcallback(void *context)
 			break;
 
 		case DM_UNBALLOON_REQUEST:
+			if (allow_hibernation) {
+				pr_info("Ignore balloon-down request!\n");
+				break;
+			}
+
 			dm->state = DM_BALLOON_DOWN;
 			balloon_down(dm,
 				 (struct dm_unballoon_request *)recv_buffer);
@@ -1623,6 +1640,11 @@ static int balloon_connect_vsp(struct hv_device *dev)
 	cap_msg.hdr.size = sizeof(struct dm_capabilities);
 	cap_msg.hdr.trans_id = atomic_inc_return(&trans_id);
 
+	/*
+	 * When hibernation (i.e. virtual ACPI S4 state) is enabled, the host
+	 * currently still requires the bits to be set, so we have to add code
+	 * to fail the host's hot-add and balloon up/down requests, if any.
+	 */
 	cap_msg.caps.cap_bits.balloon = 1;
 	cap_msg.caps.cap_bits.hot_add = 1;
 
@@ -1672,6 +1694,24 @@ static int balloon_probe(struct hv_device *dev,
 {
 	int ret;
 
+#if 0
+	/*
+	 * The patch to implement hv_is_hibernation_supported() is going
+	 * through the tip tree. For now, let's hardcode allow_hibernation
+	 * to false to keep the current behavior of hv_balloon. If people
+	 * want to test hibernation, please blacklist hv_balloon fow now
+	 * or do not enable Dynamid Memory and Memory Resizing.
+	 *
+	 * We'll remove the conditional compilation as soon as
+	 * hv_is_hibernation_supported() is available in the mainline tree.
+	 */
+	allow_hibernation = hv_is_hibernation_supported();
+#else
+	allow_hibernation = false;
+#endif
+	if (allow_hibernation)
+		hot_add = false;
+
 #ifdef CONFIG_MEMORY_HOTPLUG
 	do_hot_add = hot_add;
 #else
@@ -1711,6 +1751,8 @@ static int balloon_probe(struct hv_device *dev,
 	return 0;
 
 probe_error:
+	dm_device.state = DM_INIT_ERROR;
+	dm_device.thread  = NULL;
 	vmbus_close(dev->channel);
 #ifdef CONFIG_MEMORY_HOTPLUG
 	unregister_memory_notifier(&hv_memory_nb);
@@ -1752,6 +1794,59 @@ static int balloon_remove(struct hv_device *dev)
 	return 0;
 }
 
+static int balloon_suspend(struct hv_device *hv_dev)
+{
+	struct hv_dynmem_device *dm = hv_get_drvdata(hv_dev);
+
+	tasklet_disable(&hv_dev->channel->callback_event);
+
+	cancel_work_sync(&dm->balloon_wrk.wrk);
+	cancel_work_sync(&dm->ha_wrk.wrk);
+
+	if (dm->thread) {
+		kthread_stop(dm->thread);
+		dm->thread = NULL;
+		vmbus_close(hv_dev->channel);
+	}
+
+	tasklet_enable(&hv_dev->channel->callback_event);
+
+	return 0;
+
+}
+
+static int balloon_resume(struct hv_device *dev)
+{
+	int ret;
+
+	dm_device.state = DM_INITIALIZING;
+
+	ret = balloon_connect_vsp(dev);
+
+	if (ret != 0)
+		goto out;
+
+	dm_device.thread =
+		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
+	if (IS_ERR(dm_device.thread)) {
+		ret = PTR_ERR(dm_device.thread);
+		dm_device.thread = NULL;
+		goto close_channel;
+	}
+
+	dm_device.state = DM_INITIALIZED;
+	return 0;
+close_channel:
+	vmbus_close(dev->channel);
+out:
+	dm_device.state = DM_INIT_ERROR;
+#ifdef CONFIG_MEMORY_HOTPLUG
+	unregister_memory_notifier(&hv_memory_nb);
+	restore_online_page_callback(&hv_online_page);
+#endif
+	return ret;
+}
+
 static const struct hv_vmbus_device_id id_table[] = {
 	/* Dynamic Memory Class ID */
 	/* 525074DC-8985-46e2-8057-A307DC18A502 */
@@ -1766,6 +1861,8 @@ static int balloon_remove(struct hv_device *dev)
 	.id_table = id_table,
 	.probe =  balloon_probe,
 	.remove =  balloon_remove,
+	.suspend = balloon_suspend,
+	.resume = balloon_resume,
 	.driver = {
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 	},
-- 
1.8.3.1

Re: [PATCH] hv_balloon: Add the support of hibernation

[David Hildenbrand]

On 12.09.19 01:36, Dexuan Cui wrote:
> When hibernation is enabled, we must ignore the balloon up/down and
> hot-add requests from the host, if any.
> 
> Fow now, if people want to test hibernation, please blacklist hv_balloon
> or do not enable Dynamic Memory and Memory Resizing. See the comment in
> balloon_probe() for more info.
> 

Why do you even care about supporting hibernation? Can't you just pause
the VM in the hypervisor and continue to live a happy life? :)

> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> 
> This patch is basically a pure Hyper-V specific change and it has a
> build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus: Implement
> suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
> Hyper-V tree's hyperv-next branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
> 
> I request this patch should go through Sasha's tree rather than the
> other tree(s).
> 
>  drivers/hv/hv_balloon.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 34bd735..7df0f67 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -24,6 +24,8 @@
>  
>  #include <linux/hyperv.h>
>  
> +#include <asm/mshyperv.h>
> +
>  #define CREATE_TRACE_POINTS
>  #include "hv_trace_balloon.h"
>  
> @@ -457,6 +459,7 @@ struct hot_add_wrk {
>  	struct work_struct wrk;
>  };
>  
> +static bool allow_hibernation;
>  static bool hot_add = true;
>  static bool do_hot_add;
>  /*
> @@ -1053,8 +1056,12 @@ static void hot_add_req(struct work_struct *dummy)
>  	else
>  		resp.result = 0;
>  
> -	if (!do_hot_add || (resp.page_count == 0))
> -		pr_err("Memory hot add failed\n");
> +	if (!do_hot_add || resp.page_count == 0) {
> +		if (!allow_hibernation)
> +			pr_err("Memory hot add failed\n");
> +		else
> +			pr_info("Ignore hot-add request!\n");
> +	}
>  
>  	dm->state = DM_INITIALIZED;
>  	resp.hdr.trans_id = atomic_inc_return(&trans_id);
> @@ -1509,6 +1516,11 @@ static void balloon_onchannelcallback(void *context)
>  			break;
>  
>  		case DM_BALLOON_REQUEST:
> +			if (allow_hibernation) {
> +				pr_info("Ignore balloon-up request!\n");
> +				break;
> +			}
> +
>  			if (dm->state == DM_BALLOON_UP)
>  				pr_warn("Currently ballooning\n");
>  			bal_msg = (struct dm_balloon *)recv_buffer;
> @@ -1518,6 +1530,11 @@ static void balloon_onchannelcallback(void *context)
>  			break;
>  
>  		case DM_UNBALLOON_REQUEST:
> +			if (allow_hibernation) {
> +				pr_info("Ignore balloon-down request!\n");
> +				break;
> +			}
> +
>  			dm->state = DM_BALLOON_DOWN;
>  			balloon_down(dm,
>  				 (struct dm_unballoon_request *)recv_buffer);
> @@ -1623,6 +1640,11 @@ static int balloon_connect_vsp(struct hv_device *dev)
>  	cap_msg.hdr.size = sizeof(struct dm_capabilities);
>  	cap_msg.hdr.trans_id = atomic_inc_return(&trans_id);
>  
> +	/*
> +	 * When hibernation (i.e. virtual ACPI S4 state) is enabled, the host
> +	 * currently still requires the bits to be set, so we have to add code
> +	 * to fail the host's hot-add and balloon up/down requests, if any.
> +	 */
>  	cap_msg.caps.cap_bits.balloon = 1;
>  	cap_msg.caps.cap_bits.hot_add = 1;
>  
> @@ -1672,6 +1694,24 @@ static int balloon_probe(struct hv_device *dev,
>  {
>  	int ret;
>  
> +#if 0

I am not sure if that's a good idea. Can't you base this series on
hv_is_hibernation_supported() ?

> +	/*
> +	 * The patch to implement hv_is_hibernation_supported() is going
> +	 * through the tip tree. For now, let's hardcode allow_hibernation
> +	 * to false to keep the current behavior of hv_balloon. If people
> +	 * want to test hibernation, please blacklist hv_balloon fow now
> +	 * or do not enable Dynamid Memory and Memory Resizing.
> +	 *
> +	 * We'll remove the conditional compilation as soon as
> +	 * hv_is_hibernation_supported() is available in the mainline tree.
> +	 */
> +	allow_hibernation = hv_is_hibernation_supported();
> +#else
> +	allow_hibernation = false;
> +#endif
> +	if (allow_hibernation)
> +		hot_add = false;
> +
>  #ifdef CONFIG_MEMORY_HOTPLUG
>  	do_hot_add = hot_add;
>  #else
> @@ -1711,6 +1751,8 @@ static int balloon_probe(struct hv_device *dev,
>  	return 0;
>  
>  probe_error:
> +	dm_device.state = DM_INIT_ERROR;
> +	dm_device.thread  = NULL;
>  	vmbus_close(dev->channel);
>  #ifdef CONFIG_MEMORY_HOTPLUG
>  	unregister_memory_notifier(&hv_memory_nb);
> @@ -1752,6 +1794,59 @@ static int balloon_remove(struct hv_device *dev)
>  	return 0;
>  }
>  
> +static int balloon_suspend(struct hv_device *hv_dev)
> +{
> +	struct hv_dynmem_device *dm = hv_get_drvdata(hv_dev);
> +
> +	tasklet_disable(&hv_dev->channel->callback_event);
> +
> +	cancel_work_sync(&dm->balloon_wrk.wrk);
> +	cancel_work_sync(&dm->ha_wrk.wrk);
> +
> +	if (dm->thread) {
> +		kthread_stop(dm->thread);
> +		dm->thread = NULL;
> +		vmbus_close(hv_dev->channel);
> +	}
> +
> +	tasklet_enable(&hv_dev->channel->callback_event);
> +
> +	return 0;
> +
> +}
> +
> +static int balloon_resume(struct hv_device *dev)
> +{
> +	int ret;
> +
> +	dm_device.state = DM_INITIALIZING;
> +
> +	ret = balloon_connect_vsp(dev);
> +
> +	if (ret != 0)
> +		goto out;
> +
> +	dm_device.thread =
> +		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> +	if (IS_ERR(dm_device.thread)) {
> +		ret = PTR_ERR(dm_device.thread);
> +		dm_device.thread = NULL;
> +		goto close_channel;
> +	}
> +
> +	dm_device.state = DM_INITIALIZED;
> +	return 0;
> +close_channel:
> +	vmbus_close(dev->channel);
> +out:
> +	dm_device.state = DM_INIT_ERROR;
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	unregister_memory_notifier(&hv_memory_nb);
> +	restore_online_page_callback(&hv_online_page);
> +#endif
> +	return ret;
> +}
> +
>  static const struct hv_vmbus_device_id id_table[] = {
>  	/* Dynamic Memory Class ID */
>  	/* 525074DC-8985-46e2-8057-A307DC18A502 */
> @@ -1766,6 +1861,8 @@ static int balloon_remove(struct hv_device *dev)
>  	.id_table = id_table,
>  	.probe =  balloon_probe,
>  	.remove =  balloon_remove,
> +	.suspend = balloon_suspend,
> +	.resume = balloon_resume,
>  	.driver = {
>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>  	},
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH] hv_balloon: Add the support of hibernation

[David Hildenbrand]

On 12.09.19 12:08, David Hildenbrand wrote:
> On 12.09.19 01:36, Dexuan Cui wrote:
>> When hibernation is enabled, we must ignore the balloon up/down and
>> hot-add requests from the host, if any.
>>
>> Fow now, if people want to test hibernation, please blacklist hv_balloon
>> or do not enable Dynamic Memory and Memory Resizing. See the comment in
>> balloon_probe() for more info.
>>
> 
> Why do you even care about supporting hibernation? Can't you just pause
> the VM in the hypervisor and continue to live a happy life? :)

(to be more precise, most QEMU/KVM distributions I am aware of don't
support suspend/hibernation of guests for said reason, so I wonder why
Hyper-V needs it)


-- 

Thanks,

David / dhildenb

RE: [PATCH] hv_balloon: Add the support of hibernation

[Dexuan Cui]

> From: David Hildenbrand <david@redhat.com>
> Sent: Thursday, September 12, 2019 3:09 AM
> On 12.09.19 01:36, Dexuan Cui wrote:
> > When hibernation is enabled, we must ignore the balloon up/down and
> > hot-add requests from the host, if any.
> 
> Why do you even care about supporting hibernation? Can't you just pause
> the VM in the hypervisor and continue to live a happy life? :)
> 
> (to be more precise, most QEMU/KVM distributions I am aware of don't
> support suspend/hibernation of guests for said reason, so I wonder why
> Hyper-V needs it)

In some scenarios, hibernation can be better than pause/unpause,
save/restore and live migration:

1. Compared to pause/unpause, the VM can power off completely with
hibernation, and all the states are saved inside the VM image, then the
image can be copied to another host to start the VM again, as long as
the new host uses exactly the same configuration for the VM.

2. Compared to pause/unpause, hibernation may be more reliable, since it's
performed by the VM kernel rather than the host, so the VM kernel may
better tackle some clock-source/event-sensitive issues.

3. Hibernation can be especially useful when we pass through a PCIe device,
e.g. a NIC, a NVMe controller or a GPU, to the VM, as usually save/restore
and live migration can not work with this kind of configuration, because
usually the host doesn't know how to save/restore the state of the PCIe
device.

> > This patch is basically a pure Hyper-V specific change and it has a
> > build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus:
> Implement
> > suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
> > Hyper-V tree's hyperv-next branch
> > @@ -1672,6 +1694,24 @@ static int balloon_probe(struct hv_device *dev,
> >  {
> >  	int ret;
> >
> > +#if 0
> 
> I am not sure if that's a good idea. Can't you base this series on
> hv_is_hibernation_supported() ?

Unluckily, I can not. :-(

My hv_is_hibernation_supported() patch is still in review, and has not been
in any tree yet (it's supposed to go through the tip.git tree's timers/core
branch since otherwise the branch contains some patches that would 
cause conflicts): 
https://lkml.org/lkml/2019/9/5/1158
https://lkml.org/lkml/2019/9/5/1160
 
> > +	/*
> > +	 * The patch to implement hv_is_hibernation_supported() is going
> > +	 * through the tip tree. For now, let's hardcode allow_hibernation
> > +	 * to false to keep the current behavior of hv_balloon. If people
> > +	 * want to test hibernation, please blacklist hv_balloon for now
> > +	 * or do not enable Dynamid Memory and Memory Resizing.
> > +	 *
> > +	 * We'll remove the conditional compilation as soon as
> > +	 * hv_is_hibernation_supported() is available in the mainline tree.
> > +	 */
> > +	allow_hibernation = hv_is_hibernation_supported();
> > +#else
> > +	allow_hibernation = false;
> > +#endif

Thanks,
-- Dexuan

Re: [PATCH] hv_balloon: Add the support of hibernation

[David Hildenbrand]

On 12.09.19 21:18, Dexuan Cui wrote:
>> From: David Hildenbrand <david@redhat.com>
>> Sent: Thursday, September 12, 2019 3:09 AM
>> On 12.09.19 01:36, Dexuan Cui wrote:
>>> When hibernation is enabled, we must ignore the balloon up/down and
>>> hot-add requests from the host, if any.
>>
>> Why do you even care about supporting hibernation? Can't you just pause
>> the VM in the hypervisor and continue to live a happy life? :)
>>
>> (to be more precise, most QEMU/KVM distributions I am aware of don't
>> support suspend/hibernation of guests for said reason, so I wonder why
>> Hyper-V needs it)
> 
> In some scenarios, hibernation can be better than pause/unpause,
> save/restore and live migration:
> 
> 1. Compared to pause/unpause, the VM can power off completely with
> hibernation, and all the states are saved inside the VM image, then the
> image can be copied to another host to start the VM again, as long as
> the new host uses exactly the same configuration for the VM.

Okay, under QEMU that also works just fine via pause/unpause (e.g.,
simply migration).

> 
> 2. Compared to pause/unpause, hibernation may be more reliable, since it's
> performed by the VM kernel rather than the host, so the VM kernel may
> better tackle some clock-source/event-sensitive issues.

Not sure I agree, but maybe that's a Hyper-V special issue.

> 
> 3. Hibernation can be especially useful when we pass through a PCIe device,
> e.g. a NIC, a NVMe controller or a GPU, to the VM, as usually save/restore
> and live migration can not work with this kind of configuration, because
> usually the host doesn't know how to save/restore the state of the PCIe
> device.

Interesting. Under QEMU/KVM (especially for migration), the discussed
solutions I am aware of rather wanted to temporarily unplug the PCI
devices or replace them with some kind of "standby" device temporarily.


Anyhow, would it also be an option for you instead of making the balloon
basically useless in case the virtual ACPI S4 state is enabled to

a) Remember if there was a harmful requests that was processed (memory
add, balloon up, balloon down) - or if the device is *currently* in an
un-hibernatable state. E.g., if somebody inflated the balloon, you can't
hibernate. But if the balloon was deflated again, you can again hibernate.

b) Block hibernation in balloon_suspend() in case the device is in such
an un-hibernatable state.


Then you don't need hv_is_hibernation_supported(). The VM is able to
hibernate as long as Dynamic Memory and Memory Resizing was not used.
This is something that can be documented perfectly well.

-- 

Thanks,

David / dhildenb

RE: [PATCH] hv_balloon: Add the support of hibernation

[Dexuan Cui]

> From: David Hildenbrand <david@redhat.com>
> Sent: Friday, September 13, 2019 12:46 AM
> 
> On 12.09.19 21:18, Dexuan Cui wrote:
> > 3. Hibernation can be especially useful when we pass through a PCIe device,
> > e.g. a NIC, a NVMe controller or a GPU, to the VM, as usually save/restore
> > and live migration can not work with this kind of configuration, because
> > usually the host doesn't know how to save/restore the state of the PCIe
> > device.
> 
> Interesting. Under QEMU/KVM (especially for migration), the discussed
> solutions I am aware of rather wanted to temporarily unplug the PCI
> devices or replace them with some kind of "standby" device temporarily.

For the complex devices like a modern GPU, there may not be an 
equivalent "standby" software-emulated device for it, and unplugging the
PCI device temporarily is not good, as it may not be transparent to the
userspace applications. Hibernation here is especially useful, e.g. to Virtual
Desktop Infrastructure users whose VMs can own physical GPUs, because
all the userspace applications are frozen when the VM is hibernated, and
when the VM resumes back, the applications are automatically resumed 
and continue to run seamlessly, at least in theory. A hibernated VM saves
compute resources and cost for the users.
 
> Anyhow, would it also be an option for you instead of making the balloon
> basically useless in case the virtual ACPI S4 state is enabled to
> 
> a) Remember if there was a harmful requests that was processed (memory
> add, balloon up, balloon down) - or if the device is *currently* in an
> un-hibernatable state. E.g., if somebody inflated the balloon, you can't
> hibernate. But if the balloon was deflated again, you can again hibernate.
> 
> b) Block hibernation in balloon_suspend() in case the device is in such
> an un-hibernatable state.
> 
> 
> Then you don't need hv_is_hibernation_supported(). The VM is able to
> hibernate as long as Dynamic Memory and Memory Resizing was not used.
> This is something that can be documented perfectly well.
> 
> David / dhildenb

On recent Windows Server 2019+ hosts, the toolstacks on the hosts
guarantees that Dynamic Memory and Memory Resizing can not be enabled
if the virtual ACPI S4 state is enabled, and vice versa. Please refer to the
long write-up I made here: https://lkml.org/lkml/2019/9/5/1160 .

And, to make the hibernation functionality automated, the host is able to
send a "please hibernate" message to the VM via the Hyper-V shutdown
device upon the user's request (e.g. via GUI or scripting): see 
https://lkml.org/lkml/2019/9/13/811 . When the host sends the message,
it checks if the virtual ACPI S4 state is enabled for the VM: if not, the host
refuses to send the message. This means that the user does want to make
sure the virtual ACPI S4 state is enabled for the VM, if the user of the VM
wants to use the hibernation feature, and this means Dynamic Memory
and Memory Resizing can not be active due to the restrictions from the 
host toolstack.

And the hibernation functionality won't be officially supported on old
Windows Server hosts.

So, IMHO we can't be bother to implement the idea you described in
detail. Sorry. :-)

And, while I agree your idea is good, technically speaking I suspect it may
not be really useful, because once hv_balloon allows balloon-up/down,
hv_balloon effectively loses control of memory pages: after the host
takes some memory away, the VM never knows when exactly the
host will give it back -- actually the host never guarantees how soon
it will give the memory back. Consequently, the VM almost immediately
ends up in an un-hibernatable state...

Thanks,
-- Dexuan

Re: [PATCH] hv_balloon: Add the support of hibernation

[David Hildenbrand]

On 13.09.19 22:54, Dexuan Cui wrote:
>> From: David Hildenbrand <david@redhat.com>
>> Sent: Friday, September 13, 2019 12:46 AM
>>
>> On 12.09.19 21:18, Dexuan Cui wrote:
>>> 3. Hibernation can be especially useful when we pass through a PCIe device,
>>> e.g. a NIC, a NVMe controller or a GPU, to the VM, as usually save/restore
>>> and live migration can not work with this kind of configuration, because
>>> usually the host doesn't know how to save/restore the state of the PCIe
>>> device.
>>
>> Interesting. Under QEMU/KVM (especially for migration), the discussed
>> solutions I am aware of rather wanted to temporarily unplug the PCI
>> devices or replace them with some kind of "standby" device temporarily.
> 
> For the complex devices like a modern GPU, there may not be an 
> equivalent "standby" software-emulated device for it, and unplugging the
> PCI device temporarily is not good, as it may not be transparent to the
> userspace applications. Hibernation here is especially useful, e.g. to Virtual
> Desktop Infrastructure users whose VMs can own physical GPUs, because
> all the userspace applications are frozen when the VM is hibernated, and
> when the VM resumes back, the applications are automatically resumed 
> and continue to run seamlessly, at least in theory. A hibernated VM saves
> compute resources and cost for the users.

Yes, I can see how GPUs might be problematic, especially for desktop
infrastructures (and maybe especially when running specific guest
operating systems :) ). Thanks for the explanation.

[...]

> On recent Windows Server 2019+ hosts, the toolstacks on the hosts
> guarantees that Dynamic Memory and Memory Resizing can not be enabled
> if the virtual ACPI S4 state is enabled, and vice versa. Please refer to the
> long write-up I made here: https://lkml.org/lkml/2019/9/5/1160 .

Hah, so the patch here is not actually relevant for modern Hyper-V
installations. (I would have loved to read that in the patch description
- but maybe I missed that)

> 
> And, to make the hibernation functionality automated, the host is able to
> send a "please hibernate" message to the VM via the Hyper-V shutdown
> device upon the user's request (e.g. via GUI or scripting): see 
> https://lkml.org/lkml/2019/9/13/811 . When the host sends the message,
> it checks if the virtual ACPI S4 state is enabled for the VM: if not, the host
> refuses to send the message. This means that the user does want to make
> sure the virtual ACPI S4 state is enabled for the VM, if the user of the VM
> wants to use the hibernation feature, and this means Dynamic Memory
> and Memory Resizing can not be active due to the restrictions from the 
> host toolstack.

Okay, *but* this is a current limitation. Just saying. If you could at
least support balloon inflate/deflate, that would be a clear win for
users. And less configuration knobs.

> 
> And the hibernation functionality won't be officially supported on old
> Windows Server hosts.
> 
> So, IMHO we can't be bother to implement the idea you described in
> detail. Sorry. :-)

No worries, I neither develop for, use or work with Hyper-V. I was just
reading along and wondering why you basically make the hv_balloon
unusable in these environments. (initially I thought, "why don't you
just disallow probing the device completely")

I am aware of the (hypervisor) issues of hibernation/suspend when it
comes to balloon drivers / memory hot(un)plug. (currently working on
virtio-mem myself and initially decided to block any
hibernation/suspension attempts in case the driver is loaded and memory
was plugged/unplugged)

> 
> And, while I agree your idea is good, technically speaking I suspect it may
> not be really useful, because once hv_balloon allows balloon-up/down,
> hv_balloon effectively loses control of memory pages: after the host
> takes some memory away, the VM never knows when exactly the
> host will give it back -- actually the host never guarantees how soon
> it will give the memory back. Consequently, the VM almost immediately
> ends up in an un-hibernatable state...
If you go via the host, you might be able to make sure to request to
deflate the balloon before you try to hibernate, and inflate again when
back up. You might even ask the user for permissions. Of course, once
you deflated the balloon, it might not be guaranteed to inflate the
balloon to the original size. But after all, it's "dynamic memory", so
it might even be what the name suggests. It could be very well
controlled from the host.

If you go via the guest, you would first have to tell your hypervisor
"please allow me to deflate so I can hibernate", or something like that.
After hibernation (or some time X), the host might then decide to
inflate again.

E.g., take a look at virtio-balloon. When suspending, it simply deflates
(without asking ...), to inflate again when resuming. Not saying that's
the best approach (it's not :) ), but one approach to at least make it work.

Anyhow, just some comments from my side :) I can see how Windows Server
worked around that issue right now by just XOR'ing both features.

> 
> Thanks,
> -- Dexuan
> 


-- 

Thanks,

David / dhildenb

RE: [PATCH] hv_balloon: Add the support of hibernation

[Dexuan Cui]

> From: David Hildenbrand <david@redhat.com>
> Sent: Friday, September 13, 2019 2:44 PM
>
> > On recent Windows Server 2019+ hosts, the toolstacks on the hosts
> > guarantees that Dynamic Memory and Memory Resizing can not be enabled
> > if the virtual ACPI S4 state is enabled, and vice versa. Please refer to the
> > long write-up I made here.
>
> Hah, so the patch here is not actually relevant for modern Hyper-V

Correct.

> installations. (I would have loved to read that in the patch description
> - but maybe I missed that)

I'll add the related description into the changelog of v2 of this patch.

> > And, to make the hibernation functionality automated, the host is able to
> > send a "please hibernate" message to the VM via the Hyper-V shutdown
> > device upon the user's request (e.g. via GUI or scripting): see [...]
> > When the host sends the message,
> > it checks if the virtual ACPI S4 state is enabled for the VM: if not, the host
> > refuses to send the message. This means that the user does want to make
> > sure the virtual ACPI S4 state is enabled for the VM, if the user of the VM
> > wants to use the hibernation feature, and this means Dynamic Memory
> > and Memory Resizing can not be active due to the restrictions from the
> > host toolstack.
>
> Okay, *but* this is a current limitation. Just saying. If you could at
> least support balloon inflate/deflate, that would be a clear win for
> users. And less configuration knobs.

For Hyper-V (on recently hosts), Dynamic Memory (and Memory Resizing)
and hibernation are mutually exclusive and as I mentioned the host toolstack
guarantees they can not be both enabled. This is a host limitation and the VM
(i.e. we the Linux team) can do nothing about this. Note: here "enable
hibernation for a VM" means "enable the virtual ACPI S4 state for the VM".

By default a VM running on Hyper-V doesn't have the S4 state enabled, and
balloon inflate/deflate are indeed supported.

The knob (I think you mean the virtual ACPI S4 state) is introduced in the
host side design of the VM hibernation feature, and is enforced in the
host toolstack (as I described about the host-to-VM "please hibernate"
message). No knob or module parameter is introduced by the VM here.

> > And the hibernation functionality won't be officially supported on old
> > Windows Server hosts.
> >
> > So, IMHO we can't be bother to implement the idea you described in
> > detail. Sorry. :-)
>
> No worries, I neither develop for, use or work with Hyper-V. I was just
> reading along and wondering why you basically make the hv_balloon
> unusable in these environments. (initially I thought, "why don't you
> just disallow probing the device completely")

The Hyper-V team told me that: when hibernation is enabled & used for
a VM the only purpose of loading hv_balloon is that the driver can
still report the VM's memory pressure to the host, and it looks due to
some (non-technical?) reason the Hyper-V team thinks this info can be
useful.

> I am aware of the (hypervisor) issues of hibernation/suspend when it
> comes to balloon drivers / memory hot(un)plug. (currently working on
> virtio-mem myself and initially decided to block any
> hibernation/suspension attempts in case the driver is loaded and memory
> was plugged/unplugged)
>
> >
> > And, while I agree your idea is good, technically speaking I suspect it may
> > not be really useful, because once hv_balloon allows balloon-up/down,
> > hv_balloon effectively loses control of memory pages: after the host
> > takes some memory away, the VM never knows when exactly the
> > host will give it back -- actually the host never guarantees how soon
> > it will give the memory back. Consequently, the VM almost immediately
> > ends up in an un-hibernatable state...
> If you go via the host, you might be able to make sure to request to
> deflate the balloon before you try to hibernate, and inflate again when
> back up. You might even ask the user for permissions. Of course, once
> you deflated the balloon, it might not be guaranteed to inflate the
> balloon to the original size. But after all, it's "dynamic memory", so
> it might even be what the name suggests. It could be very well
> controlled from the host.
>
> If you go via the guest, you would first have to tell your hypervisor
> "please allow me to deflate so I can hibernate", or something like that.
> After hibernation (or some time X), the host might then decide to
> inflate again.
>
> E.g., take a look at virtio-balloon. When suspending, it simply deflates
> (without asking ...), to inflate again when resuming. Not saying that's
> the best approach (it's not :) ), but one approach to at least make it work.

Yes, I noticed this a few months ago. I think a major difference in Hyper-V
ballooning mechanism is that: all the deflate/inflate requests are from
the host and the VM can never proactively ask the host to deflate/inflate
the VM's memory. All that the VM can do is report its memory pressure
to the host and hope the host will soon give back the memory that was
taken away by the host.

I personally like the approach used in virtio-balloon. :-)

> Anyhow, just some comments from my side :) I can see how Windows Server
> worked around that issue right now by just XOR'ing both features.
>
> David / dhildenb

Thanks for sharing your thoughts!

Thanks,
-- Dexuan

RE: [PATCH] hv_balloon: Add the support of hibernation

[Dexuan Cui]

> From: linux-hyperv-owner@vger.kernel.org
>  [... snipped ...]
> > Anyhow, just some comments from my side :) I can see how Windows Server
> > worked around that issue right now by just XOR'ing both features.
> >
> > David / dhildenb
> 
> Thanks for sharing your thoughts!
> 
> -- Dexuan

Hi David,
If my explanation sounds good to you, can I have an Acked-by from you? 

Thanks,
-- Dexuan

Re: [PATCH] hv_balloon: Add the support of hibernation

[David Hildenbrand]

On 25.09.19 22:03, Dexuan Cui wrote:
>> From: linux-hyperv-owner@vger.kernel.org
>>  [... snipped ...]
>>> Anyhow, just some comments from my side :) I can see how Windows Server
>>> worked around that issue right now by just XOR'ing both features.
>>>
>>> David / dhildenb
>>
>> Thanks for sharing your thoughts!
>>
>> -- Dexuan
> 
> Hi David,
> If my explanation sounds good to you, can I have an Acked-by from you? 
> 

I do ACK the approach but not the patch in it's current state. I don't
like the ifdefs - once you can get rid of the ifdefery - e.g., after the
prerequisite is upstream - you can add my

Acked-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

RE: [PATCH] hv_balloon: Add the support of hibernation

[Dexuan Cui]

> From: David Hildenbrand <david@redhat.com>
> Sent: Thursday, September 26, 2019 12:20 AM
> To: Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; sashal@kernel.org;
> linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Michael Kelley
> <mikelley@microsoft.com>
> Subject: Re: [PATCH] hv_balloon: Add the support of hibernation
> 
> On 25.09.19 22:03, Dexuan Cui wrote:
> >> From: linux-hyperv-owner@vger.kernel.org
> >>  [... snipped ...]
> >>> Anyhow, just some comments from my side :) I can see how Windows
> Server
> >>> worked around that issue right now by just XOR'ing both features.
> >>>
> >>> David / dhildenb
> >>
> >> Thanks for sharing your thoughts!
> >>
> >> -- Dexuan
> >
> > Hi David,
> > If my explanation sounds good to you, can I have an Acked-by from you?
> >
> 
> I do ACK the approach but not the patch in it's current state. I don't
> like the ifdefs - once you can get rid of the ifdefery - e.g., after the
> prerequisite is upstream - you can add my
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 
> David / dhildenb

Makes sense. I'll wait for the prerequisite patch (i.e. the patch that implements
hv_is_hibernation_supported(), https://lkml.org/lkml/2019/9/5/1160 ) to be 
in upstream first, then I'll be able to get rid of the below "if 0" and post a v2
with your Acked-by. Thanks, David!

+#if 0
+	/*
+	 * The patch to implement hv_is_hibernation_supported() is going
+	 * through the tip tree. For now, let's hardcode allow_hibernation
+	 * to false to keep the current behavior of hv_balloon. If people
+	 * want to test hibernation, please blacklist hv_balloon fow now
+	 * or do not enable Dynamid Memory and Memory Resizing.
+	 *
+	 * We'll remove the conditional compilation as soon as
+	 * hv_is_hibernation_supported() is available in the mainline tree.
+	 */
+	allow_hibernation = hv_is_hibernation_supported();
+#else
+	allow_hibernation = false;

Thanks,
-- Dexuan