[PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Shriram Rajagopalan]

The current implementation of xen guest save/restore/checkpoint functionality
uses PM_SUSPEND and PM_RESUME events. This is not optimal when taking
checkpoints of a virtual machine (where the suspend hypercall returns
non-zero, requiring the devices and xenbus to just pickup from where they left
off instead of a complete teardown/reconnect to backend). 

The following set of patches modify this implementation to use Hibernate style
control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
which is merely a cancelled save akin to failed swsusp() ).

These patches are against Ian Campbell's PVHVM tree at
git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm

at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
kernel version: 2.6.38-rc4

Save/Restore and Checkpoint functionality have been tested on PV and PV-HVM
Guest Domains. Patches have been run through checkpatch.pl.

Changes since last resend(s):
  - xenbus_pm_ops has static const qualifier..
  - The conditional macro CONFIG_HIBERNATION around the freeze/thaw/restore
     cases in pm_op and pm_noirq_op functions in drivers/base/power/main.c has
     been expanded to CONFIG_HIBERNATION || CONFIG_XEN_SAVE_RESTORE.
  - CONFIG_PM_SLEEP in drivers/xen/manage.c has been changed to
    CONFIG_XEN_SAVE_RESTORE


Kazuhiro SUZUKI (1):
  xen: xenbus PM events support

Shriram Rajagopalan (2):
  xen: use freeze/restore/thaw PM events for suspend/resume/chkpt
  PM: pm.h - Add comments about Xen save/restore/chkpt use case

 drivers/base/power/main.c                  |    8 ++++----
 drivers/xen/manage.c                       |   16 ++++++++--------
 drivers/xen/xenbus/xenbus_probe.c          |   12 ++++++++++--
 drivers/xen/xenbus/xenbus_probe.h          |    3 ++-
 drivers/xen/xenbus/xenbus_probe_frontend.c |   11 +++++++++--
 include/linux/pm.h                         |   19 +++++++++++++++++++
 include/xen/xenbus.h                       |    2 +-
 7 files changed, 53 insertions(+), 18 deletions(-)

[PATCH 1/3] xen: xenbus PM events support

[Shriram Rajagopalan]

From: Kazuhiro SUZUKI <kaz@jp.fujitsu.com>

Make xenbus frontend device subscribe to PM events
to receive suspend/resume/freeze/thaw/restore
notifications.

Signed-off-by: Kenji Wakamiya <wkenji@jp.fujitsu.com>
Signed-off-by: Kazuhiro Suzuki <kaz@jp.fujitsu.com>
Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
[shriram--minor mods and improved commit message]
---
 drivers/xen/xenbus/xenbus_probe.c          |   12 ++++++++++--
 drivers/xen/xenbus/xenbus_probe.h          |    3 ++-
 drivers/xen/xenbus/xenbus_probe_frontend.c |    9 +++++++--
 include/xen/xenbus.h                       |    2 +-
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index baa65e7..7397695 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -577,7 +577,7 @@ void xenbus_dev_changed(const char *node, struct xen_bus_type *bus)
 }
 EXPORT_SYMBOL_GPL(xenbus_dev_changed);
 
-int xenbus_dev_suspend(struct device *dev, pm_message_t state)
+int xenbus_dev_suspend(struct device *dev)
 {
 	int err = 0;
 	struct xenbus_driver *drv;
@@ -590,7 +590,7 @@ int xenbus_dev_suspend(struct device *dev, pm_message_t state)
 		return 0;
 	drv = to_xenbus_driver(dev->driver);
 	if (drv->suspend)
-		err = drv->suspend(xdev, state);
+		err = drv->suspend(xdev);
 	if (err)
 		printk(KERN_WARNING
 		       "xenbus: suspend %s failed: %i\n", dev_name(dev), err);
@@ -642,6 +642,14 @@ int xenbus_dev_resume(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(xenbus_dev_resume);
 
+int xenbus_dev_cancel(struct device *dev)
+{
+	/* Do nothing */
+	DPRINTK("cancel");
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xenbus_dev_cancel);
+
 /* A flag to determine if xenstored is 'ready' (i.e. has started) */
 int xenstored_ready = 0;
 
diff --git a/drivers/xen/xenbus/xenbus_probe.h b/drivers/xen/xenbus/xenbus_probe.h
index 2466581..888b990 100644
--- a/drivers/xen/xenbus/xenbus_probe.h
+++ b/drivers/xen/xenbus/xenbus_probe.h
@@ -64,8 +64,9 @@ extern void xenbus_dev_changed(const char *node, struct xen_bus_type *bus);
 
 extern void xenbus_dev_shutdown(struct device *_dev);
 
-extern int xenbus_dev_suspend(struct device *dev, pm_message_t state);
+extern int xenbus_dev_suspend(struct device *dev);
 extern int xenbus_dev_resume(struct device *dev);
+extern int xenbus_dev_cancel(struct device *dev);
 
 extern void xenbus_otherend_changed(struct xenbus_watch *watch,
 				    const char **vec, unsigned int len,
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 5bcc2d6..ea83999 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -85,6 +85,12 @@ static struct device_attribute xenbus_frontend_dev_attrs[] = {
 	__ATTR_NULL
 };
 
+static const struct dev_pm_ops xenbus_pm_ops = {
+	.suspend = xenbus_dev_suspend,
+	.resume  = xenbus_dev_resume,
+	.thaw  = xenbus_dev_cancel,
+};
+
 static struct xen_bus_type xenbus_frontend = {
 	.root = "device",
 	.levels = 2,		/* device/type/<id> */
@@ -100,8 +106,7 @@ static struct xen_bus_type xenbus_frontend = {
 		.shutdown	= xenbus_dev_shutdown,
 		.dev_attrs	= xenbus_frontend_dev_attrs,
 
-		.suspend	= xenbus_dev_suspend,
-		.resume		= xenbus_dev_resume,
+		.pm		= &xenbus_pm_ops,
 	},
 };
 
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 7a1d15f..5467369 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -92,7 +92,7 @@ struct xenbus_driver {
 	void (*otherend_changed)(struct xenbus_device *dev,
 				 enum xenbus_state backend_state);
 	int (*remove)(struct xenbus_device *dev);
-	int (*suspend)(struct xenbus_device *dev, pm_message_t state);
+	int (*suspend)(struct xenbus_device *dev);
 	int (*resume)(struct xenbus_device *dev);
 	int (*uevent)(struct xenbus_device *, struct kobj_uevent_env *);
 	struct device_driver driver;
-- 
1.7.0.4

[PATCH 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Shriram Rajagopalan]

Use PM_FREEZE, PM_THAW and PM_RESTORE power events for
suspend/resume/checkpoint functionality, instead of PM_SUSPEND
and PM_RESUME. Use of these pm events fixes the Xen Guest hangup
when taking checkpoints. When a suspend event is cancelled
(while taking checkpoints once/continuously), we use PM_THAW
instead of PM_RESUME. PM_RESTORE is used when suspend is not
cancelled. See Documentation/power/devices.txt and linux/pm.h
for more info about freeze, thaw and restore. The sequence of
pm events in a suspend-resume scenario is shown below.

        dpm_suspend_start(PMSG_FREEZE);

                dpm_suspend_noirq(PMSG_FREEZE);

                       sysdev_suspend(PMSG_FREEZE);
                       cancelled = suspend_hypercall()
                       sysdev_resume();

               dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);

       dpm_resume_end(cancelled ? PMSG_THAW : PMSG_RESTORE);

Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 drivers/base/power/main.c                  |    8 ++++----
 drivers/xen/manage.c                       |   16 ++++++++--------
 drivers/xen/xenbus/xenbus_probe_frontend.c |    8 +++++---
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 8340497..aab4f60 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -233,7 +233,7 @@ static int pm_op(struct device *dev,
 		}
 		break;
 #endif /* CONFIG_SUSPEND */
-#ifdef CONFIG_HIBERNATION
+#if defined(CONFIG_HIBERNATION) || defined(CONFIG_XEN_SAVE_RESTORE)
 	case PM_EVENT_FREEZE:
 	case PM_EVENT_QUIESCE:
 		if (ops->freeze) {
@@ -260,7 +260,7 @@ static int pm_op(struct device *dev,
 			suspend_report_result(ops->restore, error);
 		}
 		break;
-#endif /* CONFIG_HIBERNATION */
+#endif /* CONFIG_HIBERNATION || CONFIG_XEN_SAVE_RESTORE */
 	default:
 		error = -EINVAL;
 	}
@@ -308,7 +308,7 @@ static int pm_noirq_op(struct device *dev,
 		}
 		break;
 #endif /* CONFIG_SUSPEND */
-#ifdef CONFIG_HIBERNATION
+#if defined(CONFIG_HIBERNATION) || defined(CONFIG_XEN_SAVE_RESTORE)
 	case PM_EVENT_FREEZE:
 	case PM_EVENT_QUIESCE:
 		if (ops->freeze_noirq) {
@@ -335,7 +335,7 @@ static int pm_noirq_op(struct device *dev,
 			suspend_report_result(ops->restore_noirq, error);
 		}
 		break;
-#endif /* CONFIG_HIBERNATION */
+#endif /* CONFIG_HIBERNATION || CONFIG_XEN_SAVE_RESTORE */
 	default:
 		error = -EINVAL;
 	}
diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index c255ed4..c24d0e7 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -61,7 +61,7 @@ static void xen_post_suspend(int cancelled)
 	xen_mm_unpin_all();
 }
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_XEN_SAVE_RESTORE
 static int xen_suspend(void *data)
 {
 	struct suspend_info *si = data;
@@ -69,7 +69,7 @@ static int xen_suspend(void *data)
 
 	BUG_ON(!irqs_disabled());
 
-	err = sysdev_suspend(PMSG_SUSPEND);
+	err = sysdev_suspend(PMSG_FREEZE);
 	if (err) {
 		printk(KERN_ERR "xen_suspend: sysdev_suspend failed: %d\n",
 			err);
@@ -118,7 +118,7 @@ static void do_suspend(void)
 	}
 #endif
 
-	err = dpm_suspend_start(PMSG_SUSPEND);
+	err = dpm_suspend_start(PMSG_FREEZE);
 	if (err) {
 		printk(KERN_ERR "xen suspend: dpm_suspend_start %d\n", err);
 		goto out_thaw;
@@ -127,7 +127,7 @@ static void do_suspend(void)
 	printk(KERN_DEBUG "suspending xenstore...\n");
 	xs_suspend();
 
-	err = dpm_suspend_noirq(PMSG_SUSPEND);
+	err = dpm_suspend_noirq(PMSG_FREEZE);
 	if (err) {
 		printk(KERN_ERR "dpm_suspend_noirq failed: %d\n", err);
 		goto out_resume;
@@ -147,7 +147,7 @@ static void do_suspend(void)
 
 	err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
-	dpm_resume_noirq(PMSG_RESUME);
+	dpm_resume_noirq(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
 	if (err) {
 		printk(KERN_ERR "failed to start xen_suspend: %d\n", err);
@@ -161,7 +161,7 @@ out_resume:
 	} else
 		xs_suspend_cancel();
 
-	dpm_resume_end(PMSG_RESUME);
+	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
 	/* Make sure timer events get retriggered on all CPUs */
 	clock_was_set();
@@ -173,7 +173,7 @@ out:
 #endif
 	shutting_down = SHUTDOWN_INVALID;
 }
-#endif	/* CONFIG_PM_SLEEP */
+#endif	/* CONFIG_XEN_SAVE_RESTORE */
 
 struct shutdown_handler {
 	const char *command;
@@ -202,7 +202,7 @@ static void shutdown_handler(struct xenbus_watch *watch,
 		{ "poweroff",	do_poweroff },
 		{ "halt",	do_poweroff },
 		{ "reboot",	do_reboot   },
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_XEN_SAVE_RESTORE
 		{ "suspend",	do_suspend  },
 #endif
 		{NULL, NULL},
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
index ea83999..b6a2690 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -86,9 +86,11 @@ static struct device_attribute xenbus_frontend_dev_attrs[] = {
 };
 
 static const struct dev_pm_ops xenbus_pm_ops = {
-	.suspend = xenbus_dev_suspend,
-	.resume  = xenbus_dev_resume,
-	.thaw  = xenbus_dev_cancel,
+	.suspend	= xenbus_dev_suspend,
+	.resume		= xenbus_dev_resume,
+	.freeze		= xenbus_dev_suspend,
+	.thaw		= xenbus_dev_cancel,
+	.restore	= xenbus_dev_resume,
 };
 
 static struct xen_bus_type xenbus_frontend = {
-- 
1.7.0.4

[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case

[Shriram Rajagopalan]

Add documentation to pm.h on how xen uses PM events (freeze,
thaw, restore) to implement Guest VM save/checkpoint/restore
functionality.

Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 include/linux/pm.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index dd9c7ab..2ddd9d3 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -516,6 +516,25 @@ extern void update_pm_runtime_accounting(struct device *dev);
  * well as during system sleep states like PM_SUSPEND_STANDBY.  They may
  * be able to use wakeup events to exit from runtime low-power states,
  * or from system low-power states such as standby or suspend-to-RAM.
+ *
+ * Xen Guest Kernels use PM_FREEZE, PM_RESTORE and PM_THAW to implement
+ * VM save/restore/checkpoint functionality. Save and Restore are somewhat
+ * similar to hibernate functionality. The sequence of events is shown below:
+ *        dpm_suspend_start(PMSG_FREEZE);
+ *
+ *              dpm_suspend_noirq(PMSG_FREEZE);
+ *
+ *                     sysdev_suspend(PMSG_FREEZE);
+ *                     cancelled = suspend_hypercall()
+ *                     sysdev_resume();
+ *
+ *             dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);
+ *
+ *     dpm_resume_end(cancelled ? PMSG_THAW : PMSG_RESTORE);
+ *
+ * If the syspend_hypercall returns 1, it means that the VM was merely
+ * checkpointed (akin to THAW). If it returns 0, it means the system has been
+ * fully restored from its on-disk snapshot (akin to RESTORE).
  */
 
 #ifdef CONFIG_PM_SLEEP
-- 
1.7.0.4

Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Pavel Machek]

On Sat 2011-02-19 15:12:35, Shriram Rajagopalan wrote:
> The current implementation of xen guest save/restore/checkpoint functionality
> uses PM_SUSPEND and PM_RESUME events. This is not optimal when taking
> checkpoints of a virtual machine (where the suspend hypercall returns
> non-zero, requiring the devices and xenbus to just pickup from where they left
> off instead of a complete teardown/reconnect to backend). 
> 
> The following set of patches modify this implementation to use Hibernate style
> control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> which is merely a cancelled save akin to failed swsusp() ).
> 
> These patches are against Ian Campbell's PVHVM tree at
> git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> 
> at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> kernel version: 2.6.38-rc4

Series looks ok to me...
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Ian Campbell]

On Sun, 2011-02-20 at 07:49 +0000, Pavel Machek wrote:
> On Sat 2011-02-19 15:12:35, Shriram Rajagopalan wrote:
> > The current implementation of xen guest save/restore/checkpoint functionality
> > uses PM_SUSPEND and PM_RESUME events. This is not optimal when taking
> > checkpoints of a virtual machine (where the suspend hypercall returns
> > non-zero, requiring the devices and xenbus to just pickup from where they left
> > off instead of a complete teardown/reconnect to backend). 
> > 
> > The following set of patches modify this implementation to use Hibernate style
> > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > which is merely a cancelled save akin to failed swsusp() ).
> > 
> > These patches are against Ian Campbell's PVHVM tree at
> > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > 
> > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > kernel version: 2.6.38-rc4
> 
> Series looks ok to me...

Thanks Pavel, may we take that as an Acked-by?

For my part the Xen side is:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

The changes to drivers/xen/manage.c depend on some other cleanups made
on the Xen side (in linux-next soon if not already via Stefano's tree)
so how would you like to handle them? I could point you to a suitable to
apply the pm.h bits to or we could carry them in the Xen tree if you are
happy with them.

Ian.

Re: [linux-pm] [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Alan Stern]

On Mon, 21 Feb 2011, Ian Campbell wrote:

> On Sun, 2011-02-20 at 07:49 +0000, Pavel Machek wrote:
> > On Sat 2011-02-19 15:12:35, Shriram Rajagopalan wrote:
> > > The current implementation of xen guest save/restore/checkpoint functionality
> > > uses PM_SUSPEND and PM_RESUME events. This is not optimal when taking
> > > checkpoints of a virtual machine (where the suspend hypercall returns
> > > non-zero, requiring the devices and xenbus to just pickup from where they left
> > > off instead of a complete teardown/reconnect to backend). 
> > > 
> > > The following set of patches modify this implementation to use Hibernate style
> > > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > > which is merely a cancelled save akin to failed swsusp() ).
> > > 
> > > These patches are against Ian Campbell's PVHVM tree at
> > > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > > 
> > > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > > kernel version: 2.6.38-rc4
> > 
> > Series looks ok to me...
> 
> Thanks Pavel, may we take that as an Acked-by?
> 
> For my part the Xen side is:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

There's one part of this which could be troublesome.  The new code 
generates FREEZE, THAW, and RESTORE events even in kernels where 
CONFIG_HIBERNATION isn't set.  In such kernels, drivers are not 
obliged to handle these events correctly.

Shouldn't the CONFIG_XEN_SAVE_RESTORE option select CONFIG_HIBERNATION?
In which case the #ifdef lines in pm_op() wouldn't need to be changed.

Alan Stern

Re: [linux-pm] [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Ian Campbell]

On Mon, 2011-02-21 at 16:40 +0000, Alan Stern wrote:
> On Mon, 21 Feb 2011, Ian Campbell wrote:
> 
> > On Sun, 2011-02-20 at 07:49 +0000, Pavel Machek wrote:
> > > On Sat 2011-02-19 15:12:35, Shriram Rajagopalan wrote:
> > > > The current implementation of xen guest save/restore/checkpoint functionality
> > > > uses PM_SUSPEND and PM_RESUME events. This is not optimal when taking
> > > > checkpoints of a virtual machine (where the suspend hypercall returns
> > > > non-zero, requiring the devices and xenbus to just pickup from where they left
> > > > off instead of a complete teardown/reconnect to backend). 
> > > > 
> > > > The following set of patches modify this implementation to use Hibernate style
> > > > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > > > which is merely a cancelled save akin to failed swsusp() ).
> > > > 
> > > > These patches are against Ian Campbell's PVHVM tree at
> > > > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > > > 
> > > > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > > > kernel version: 2.6.38-rc4
> > > 
> > > Series looks ok to me...
> > 
> > Thanks Pavel, may we take that as an Acked-by?
> > 
> > For my part the Xen side is:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> There's one part of this which could be troublesome.  The new code 
> generates FREEZE, THAW, and RESTORE events even in kernels where 
> CONFIG_HIBERNATION isn't set.  In such kernels, drivers are not 
> obliged to handle these events correctly.

The dependencies on CONFIG_HIBERNATION which I can see appear to be more
often at the bus level (e.g. in drivers/acpi drivers/pci/pci-driver.c
etc) is that right?

For a PV guest only the Xen PV drivers really matter.

But for a PVHVM guest you are right since there are the emulated "PC"
devices though which could be problematic. There's nothing especially
thrilling in that set of devices although I don't think that invalidates
your point.

> Shouldn't the CONFIG_XEN_SAVE_RESTORE option select CONFIG_HIBERNATION?
> In which case the #ifdef lines in pm_op() wouldn't need to be changed.

I think selecting user-visible symbols is generally frowned upon.

But apart from that I was concerned that tying the Xen functionality
into the hibernation option was a bit odd/artificial. Perhaps it's the
only solution though.

Ian.

Re: [linux-pm] [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Rafael J. Wysocki]

On Monday, February 21, 2011, Ian Campbell wrote:
> On Mon, 2011-02-21 at 16:40 +0000, Alan Stern wrote:
> > On Mon, 21 Feb 2011, Ian Campbell wrote:
> > 
> > > On Sun, 2011-02-20 at 07:49 +0000, Pavel Machek wrote:
> > > > On Sat 2011-02-19 15:12:35, Shriram Rajagopalan wrote:
> > > > > The current implementation of xen guest save/restore/checkpoint functionality
> > > > > uses PM_SUSPEND and PM_RESUME events. This is not optimal when taking
> > > > > checkpoints of a virtual machine (where the suspend hypercall returns
> > > > > non-zero, requiring the devices and xenbus to just pickup from where they left
> > > > > off instead of a complete teardown/reconnect to backend). 
> > > > > 
> > > > > The following set of patches modify this implementation to use Hibernate style
> > > > > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > > > > which is merely a cancelled save akin to failed swsusp() ).
> > > > > 
> > > > > These patches are against Ian Campbell's PVHVM tree at
> > > > > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > > > > 
> > > > > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > > > > kernel version: 2.6.38-rc4
> > > > 
> > > > Series looks ok to me...
> > > 
> > > Thanks Pavel, may we take that as an Acked-by?
> > > 
> > > For my part the Xen side is:
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > There's one part of this which could be troublesome.  The new code 
> > generates FREEZE, THAW, and RESTORE events even in kernels where 
> > CONFIG_HIBERNATION isn't set.  In such kernels, drivers are not 
> > obliged to handle these events correctly.
> 
> The dependencies on CONFIG_HIBERNATION which I can see appear to be more
> often at the bus level (e.g. in drivers/acpi drivers/pci/pci-driver.c
> etc) is that right?
> 
> For a PV guest only the Xen PV drivers really matter.
> 
> But for a PVHVM guest you are right since there are the emulated "PC"
> devices though which could be problematic. There's nothing especially
> thrilling in that set of devices although I don't think that invalidates
> your point.
> 
> > Shouldn't the CONFIG_XEN_SAVE_RESTORE option select CONFIG_HIBERNATION?
> > In which case the #ifdef lines in pm_op() wouldn't need to be changed.
> 
> I think selecting user-visible symbols is generally frowned upon.
> 
> But apart from that I was concerned that tying the Xen functionality
> into the hibernation option was a bit odd/artificial. Perhaps it's the
> only solution though.

I'd very much prefer it if the patchset didn't touch drivers/base/power/main.c.

However, if you want to select CONFIG_HIBERNATION from CONFIG_XEN_SAVE_RESTORE,
you should make sure that CONFIG_HIBERNATION is really selectable (ie.
CONFIG_SWAP is set and CONFIG_ARCH_HIBERNATION_POSSIBLE is set).

Thanks,
Rafael

Re: [PATCH 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Konrad Rzeszutek Wilk]

On Sat, Feb 19, 2011 at 03:12:37PM -0800, Shriram Rajagopalan wrote:
> Use PM_FREEZE, PM_THAW and PM_RESTORE power events for
> suspend/resume/checkpoint functionality, instead of PM_SUSPEND
> and PM_RESUME. Use of these pm events fixes the Xen Guest hangup
> when taking checkpoints. When a suspend event is cancelled
> (while taking checkpoints once/continuously), we use PM_THAW
> instead of PM_RESUME. PM_RESTORE is used when suspend is not
> cancelled. See Documentation/power/devices.txt and linux/pm.h
> for more info about freeze, thaw and restore. The sequence of
> pm events in a suspend-resume scenario is shown below.
> 
>         dpm_suspend_start(PMSG_FREEZE);
> 
>                 dpm_suspend_noirq(PMSG_FREEZE);
> 
>                        sysdev_suspend(PMSG_FREEZE);
>                        cancelled = suspend_hypercall()
>                        sysdev_resume();
> 
>                dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);
> 
>        dpm_resume_end(cancelled ? PMSG_THAW : PMSG_RESTORE);
> 
> Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  drivers/base/power/main.c                  |    8 ++++----
>  drivers/xen/manage.c                       |   16 ++++++++--------
>  drivers/xen/xenbus/xenbus_probe_frontend.c |    8 +++++---
>  3 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 8340497..aab4f60 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -233,7 +233,7 @@ static int pm_op(struct device *dev,
>  		}
>  		break;
>  #endif /* CONFIG_SUSPEND */
> -#ifdef CONFIG_HIBERNATION
> +#if defined(CONFIG_HIBERNATION) || defined(CONFIG_XEN_SAVE_RESTORE)

Could we just make CONFIG_XEN_SAVE_RESTORE depend on CONFIG_HIBERANTION?
Like this:


diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index 5b54892..838e20c 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -38,7 +38,7 @@ config XEN_MAX_DOMAIN_MEMORY
 
 config XEN_SAVE_RESTORE
        bool
-       depends on XEN && PM
+       depends on XEN && PM && HIBERNATION
        default y
 
 config XEN_DEBUG_FS

Re: [PATCH 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Rafael J. Wysocki]

On Tuesday, February 22, 2011, Konrad Rzeszutek Wilk wrote:
> On Sat, Feb 19, 2011 at 03:12:37PM -0800, Shriram Rajagopalan wrote:
> > Use PM_FREEZE, PM_THAW and PM_RESTORE power events for
> > suspend/resume/checkpoint functionality, instead of PM_SUSPEND
> > and PM_RESUME. Use of these pm events fixes the Xen Guest hangup
> > when taking checkpoints. When a suspend event is cancelled
> > (while taking checkpoints once/continuously), we use PM_THAW
> > instead of PM_RESUME. PM_RESTORE is used when suspend is not
> > cancelled. See Documentation/power/devices.txt and linux/pm.h
> > for more info about freeze, thaw and restore. The sequence of
> > pm events in a suspend-resume scenario is shown below.
> > 
> >         dpm_suspend_start(PMSG_FREEZE);
> > 
> >                 dpm_suspend_noirq(PMSG_FREEZE);
> > 
> >                        sysdev_suspend(PMSG_FREEZE);
> >                        cancelled = suspend_hypercall()
> >                        sysdev_resume();
> > 
> >                dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);
> > 
> >        dpm_resume_end(cancelled ? PMSG_THAW : PMSG_RESTORE);
> > 
> > Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> > ---
> >  drivers/base/power/main.c                  |    8 ++++----
> >  drivers/xen/manage.c                       |   16 ++++++++--------
> >  drivers/xen/xenbus/xenbus_probe_frontend.c |    8 +++++---
> >  3 files changed, 17 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 8340497..aab4f60 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -233,7 +233,7 @@ static int pm_op(struct device *dev,
> >  		}
> >  		break;
> >  #endif /* CONFIG_SUSPEND */
> > -#ifdef CONFIG_HIBERNATION
> > +#if defined(CONFIG_HIBERNATION) || defined(CONFIG_XEN_SAVE_RESTORE)
> 
> Could we just make CONFIG_XEN_SAVE_RESTORE depend on CONFIG_HIBERANTION?
> Like this:
> 
> 
> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> index 5b54892..838e20c 100644
> --- a/arch/x86/xen/Kconfig
> +++ b/arch/x86/xen/Kconfig
> @@ -38,7 +38,7 @@ config XEN_MAX_DOMAIN_MEMORY
>  
>  config XEN_SAVE_RESTORE
>         bool
> -       depends on XEN && PM
> +       depends on XEN && PM && HIBERNATION

PM is not necessary.  Apart from this it looks OK.

It might be better to use select from the user's point of view, but I'm
not sure we can always assume that HIBERNATION will be selectable
by XEN_SAVE_RESTORE (eg. SWAP has to be set for HIBERNATION too).

>         default y
>  
>  config XEN_DEBUG_FS

Thanks,
Rafael

Re: [PATCH 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Shriram Rajagopalan]

On Tue, Feb 22, 2011 at 12:43 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Sat, Feb 19, 2011 at 03:12:37PM -0800, Shriram Rajagopalan wrote:
>> Use PM_FREEZE, PM_THAW and PM_RESTORE power events for
>> suspend/resume/checkpoint functionality, instead of PM_SUSPEND
>> and PM_RESUME. Use of these pm events fixes the Xen Guest hangup
>> when taking checkpoints. When a suspend event is cancelled
>> (while taking checkpoints once/continuously), we use PM_THAW
>> instead of PM_RESUME. PM_RESTORE is used when suspend is not
>> cancelled. See Documentation/power/devices.txt and linux/pm.h
>> for more info about freeze, thaw and restore. The sequence of
>> pm events in a suspend-resume scenario is shown below.
>>
>>         dpm_suspend_start(PMSG_FREEZE);
>>
>>                 dpm_suspend_noirq(PMSG_FREEZE);
>>
>>                        sysdev_suspend(PMSG_FREEZE);
>>                        cancelled = suspend_hypercall()
>>                        sysdev_resume();
>>
>>                dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);
>>
>>        dpm_resume_end(cancelled ? PMSG_THAW : PMSG_RESTORE);
>>
>> Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>> ---
>>  drivers/base/power/main.c                  |    8 ++++----
>>  drivers/xen/manage.c                       |   16 ++++++++--------
>>  drivers/xen/xenbus/xenbus_probe_frontend.c |    8 +++++---
>>  3 files changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 8340497..aab4f60 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -233,7 +233,7 @@ static int pm_op(struct device *dev,
>>               }
>>               break;
>>  #endif /* CONFIG_SUSPEND */
>> -#ifdef CONFIG_HIBERNATION
>> +#if defined(CONFIG_HIBERNATION) || defined(CONFIG_XEN_SAVE_RESTORE)
>
> Could we just make CONFIG_XEN_SAVE_RESTORE depend on CONFIG_HIBERANTION?
> Like this:
>
>
> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> index 5b54892..838e20c 100644
> --- a/arch/x86/xen/Kconfig
> +++ b/arch/x86/xen/Kconfig
> @@ -38,7 +38,7 @@ config XEN_MAX_DOMAIN_MEMORY
>
>  config XEN_SAVE_RESTORE
>        bool
> -       depends on XEN && PM
> +       depends on XEN && PM && HIBERNATION
>        default y
>
>  config XEN_DEBUG_FS
>
>
On that aspect, just noticed
config PM_SLEEP
        bool
        depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
        default y
we could get rid of XEN_SAVE_RESTORE as the suspend/resume code doesnt
depend on PM_SLEEP
anymore.

Re: [PATCH 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Rafael J. Wysocki]

On Tuesday, February 22, 2011, Shriram Rajagopalan wrote:
> On Tue, Feb 22, 2011 at 12:43 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Sat, Feb 19, 2011 at 03:12:37PM -0800, Shriram Rajagopalan wrote:
> >> Use PM_FREEZE, PM_THAW and PM_RESTORE power events for
> >> suspend/resume/checkpoint functionality, instead of PM_SUSPEND
> >> and PM_RESUME. Use of these pm events fixes the Xen Guest hangup
> >> when taking checkpoints. When a suspend event is cancelled
> >> (while taking checkpoints once/continuously), we use PM_THAW
> >> instead of PM_RESUME. PM_RESTORE is used when suspend is not
> >> cancelled. See Documentation/power/devices.txt and linux/pm.h
> >> for more info about freeze, thaw and restore. The sequence of
> >> pm events in a suspend-resume scenario is shown below.
> >>
> >>         dpm_suspend_start(PMSG_FREEZE);
> >>
> >>                 dpm_suspend_noirq(PMSG_FREEZE);
> >>
> >>                        sysdev_suspend(PMSG_FREEZE);
> >>                        cancelled = suspend_hypercall()
> >>                        sysdev_resume();
> >>
> >>                dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);
> >>
> >>        dpm_resume_end(cancelled ? PMSG_THAW : PMSG_RESTORE);
> >>
> >> Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> >> ---
> >>  drivers/base/power/main.c                  |    8 ++++----
> >>  drivers/xen/manage.c                       |   16 ++++++++--------
> >>  drivers/xen/xenbus/xenbus_probe_frontend.c |    8 +++++---
> >>  3 files changed, 17 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> index 8340497..aab4f60 100644
> >> --- a/drivers/base/power/main.c
> >> +++ b/drivers/base/power/main.c
> >> @@ -233,7 +233,7 @@ static int pm_op(struct device *dev,
> >>               }
> >>               break;
> >>  #endif /* CONFIG_SUSPEND */
> >> -#ifdef CONFIG_HIBERNATION
> >> +#if defined(CONFIG_HIBERNATION) || defined(CONFIG_XEN_SAVE_RESTORE)
> >
> > Could we just make CONFIG_XEN_SAVE_RESTORE depend on CONFIG_HIBERANTION?
> > Like this:
> >
> >
> > diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> > index 5b54892..838e20c 100644
> > --- a/arch/x86/xen/Kconfig
> > +++ b/arch/x86/xen/Kconfig
> > @@ -38,7 +38,7 @@ config XEN_MAX_DOMAIN_MEMORY
> >
> >  config XEN_SAVE_RESTORE
> >        bool
> > -       depends on XEN && PM
> > +       depends on XEN && PM && HIBERNATION
> >        default y
> >
> >  config XEN_DEBUG_FS
> >
> >
> On that aspect, just noticed
> config PM_SLEEP
>         bool
>         depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
>         default y
> we could get rid of XEN_SAVE_RESTORE as the suspend/resume code doesnt
> depend on PM_SLEEP anymore.

Not really, it still depends on it.

However, if you make XEN_SAVE_RESTORE depend on HIBERNATION, then of course
PM_SLEEP won't need to dependon XEN_SAVE_RESTORE directly.

Thanks,
Rafael

Re: [PATCH 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Konrad Rzeszutek Wilk]

> > Could we just make CONFIG_XEN_SAVE_RESTORE depend on CONFIG_HIBERANTION?
> > Like this:
> >
> >
> > diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> > index 5b54892..838e20c 100644
> > --- a/arch/x86/xen/Kconfig
> > +++ b/arch/x86/xen/Kconfig
> > @@ -38,7 +38,7 @@ config XEN_MAX_DOMAIN_MEMORY
> >
> >  config XEN_SAVE_RESTORE
> >        bool
> > -       depends on XEN && PM
> > +       depends on XEN && PM && HIBERNATION
> >        default y
> >
> >  config XEN_DEBUG_FS
> >
> >
> On that aspect, just noticed
> config PM_SLEEP
>         bool
>         depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
>         default y
> we could get rid of XEN_SAVE_RESTORE as the suspend/resume code doesnt
> depend on PM_SLEEP

OK. Do you want to respin patch #2 without the #ifdefs in the drivers/base/...
and provide a new patch (#4?) which will alter the Kconfig file appropriately?

Re: [PATCH 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Shriram Rajagopalan]

On Tue, Feb 22, 2011 at 2:36 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> > Could we just make CONFIG_XEN_SAVE_RESTORE depend on CONFIG_HIBERANTION?
>> > Like this:
>> >
>> >
>> > diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
>> > index 5b54892..838e20c 100644
>> > --- a/arch/x86/xen/Kconfig
>> > +++ b/arch/x86/xen/Kconfig
>> > @@ -38,7 +38,7 @@ config XEN_MAX_DOMAIN_MEMORY
>> >
>> >  config XEN_SAVE_RESTORE
>> >        bool
>> > -       depends on XEN && PM
>> > +       depends on XEN && PM && HIBERNATION
>> >        default y
>> >
>> >  config XEN_DEBUG_FS
>> >
>> >
>> On that aspect, just noticed
>> config PM_SLEEP
>>         bool
>>         depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
>>         default y
>> we could get rid of XEN_SAVE_RESTORE as the suspend/resume code doesnt
>> depend on PM_SLEEP
>
> OK. Do you want to respin patch #2 without the #ifdefs in the drivers/base/...
> and provide a new patch (#4?) which will alter the Kconfig file appropriately?
>
>
yep. will do.
shriram

[PATCH v2 2/3] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Shriram Rajagopalan]

Use PM_FREEZE, PM_THAW and PM_RESTORE power events for
suspend/resume/checkpoint functionality, instead of PM_SUSPEND
and PM_RESUME. Use of these pm events fixes the Xen Guest hangup
when taking checkpoints. When a suspend event is cancelled
(while taking checkpoints once/continuously), we use PM_THAW
instead of PM_RESUME. PM_RESTORE is used when suspend is not
cancelled. See Documentation/power/devices.txt and linux/pm.h
for more info about freeze, thaw and restore. The sequence of
pm events in a suspend-resume scenario is shown below.

        dpm_suspend_start(PMSG_FREEZE);

                dpm_suspend_noirq(PMSG_FREEZE);

                       sysdev_suspend(PMSG_FREEZE);
                       cancelled = suspend_hypercall()
                       sysdev_resume();

               dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);

       dpm_resume_end(cancelled ? PMSG_THAW : PMSG_RESTORE);

Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 drivers/xen/manage.c                       |   16 ++++++++--------
 drivers/xen/xenbus/xenbus_probe_frontend.c |    8 +++++---
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index c255ed4..c24d0e7 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -61,7 +61,7 @@ static void xen_post_suspend(int cancelled)
 	xen_mm_unpin_all();
 }
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_XEN_SAVE_RESTORE
 static int xen_suspend(void *data)
 {
 	struct suspend_info *si = data;
@@ -69,7 +69,7 @@ static int xen_suspend(void *data)
 
 	BUG_ON(!irqs_disabled());
 
-	err = sysdev_suspend(PMSG_SUSPEND);
+	err = sysdev_suspend(PMSG_FREEZE);
 	if (err) {
 		printk(KERN_ERR "xen_suspend: sysdev_suspend failed: %d\n",
 			err);
@@ -118,7 +118,7 @@ static void do_suspend(void)
 	}
 #endif
 
-	err = dpm_suspend_start(PMSG_SUSPEND);
+	err = dpm_suspend_start(PMSG_FREEZE);
 	if (err) {
 		printk(KERN_ERR "xen suspend: dpm_suspend_start %d\n", err);
 		goto out_thaw;
@@ -127,7 +127,7 @@ static void do_suspend(void)
 	printk(KERN_DEBUG "suspending xenstore...\n");
 	xs_suspend();
 
-	err = dpm_suspend_noirq(PMSG_SUSPEND);
+	err = dpm_suspend_noirq(PMSG_FREEZE);
 	if (err) {
 		printk(KERN_ERR "dpm_suspend_noirq failed: %d\n", err);
 		goto out_resume;
@@ -147,7 +147,7 @@ static void do_suspend(void)
 
 	err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
-	dpm_resume_noirq(PMSG_RESUME);
+	dpm_resume_noirq(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
 	if (err) {
 		printk(KERN_ERR "failed to start xen_suspend: %d\n", err);
@@ -161,7 +161,7 @@ out_resume:
 	} else
 		xs_suspend_cancel();
 
-	dpm_resume_end(PMSG_RESUME);
+	dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
 	/* Make sure timer events get retriggered on all CPUs */
 	clock_was_set();
@@ -173,7 +173,7 @@ out:
 #endif
 	shutting_down = SHUTDOWN_INVALID;
 }
-#endif	/* CONFIG_PM_SLEEP */
+#endif	/* CONFIG_XEN_SAVE_RESTORE */
 
 struct shutdown_handler {
 	const char *command;
@@ -202,7 +202,7 @@ static void shutdown_handler(struct xenbus_watch *watch,
 		{ "poweroff",	do_poweroff },
 		{ "halt",	do_poweroff },
 		{ "reboot",	do_reboot   },
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_XEN_SAVE_RESTORE
 		{ "suspend",	do_suspend  },
 #endif
 		{NULL, NULL},
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
index ea83999..b6a2690 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -86,9 +86,11 @@ static struct device_attribute xenbus_frontend_dev_attrs[] = {
 };
 
 static const struct dev_pm_ops xenbus_pm_ops = {
-	.suspend = xenbus_dev_suspend,
-	.resume  = xenbus_dev_resume,
-	.thaw  = xenbus_dev_cancel,
+	.suspend	= xenbus_dev_suspend,
+	.resume		= xenbus_dev_resume,
+	.freeze		= xenbus_dev_suspend,
+	.thaw		= xenbus_dev_cancel,
+	.restore	= xenbus_dev_resume,
 };
 
 static struct xen_bus_type xenbus_frontend = {
-- 
1.7.0.4

Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Pavel Machek]

Hi!

> > > The following set of patches modify this implementation to use Hibernate style
> > > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > > which is merely a cancelled save akin to failed swsusp() ).
> > > 
> > > These patches are against Ian Campbell's PVHVM tree at
> > > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > > 
> > > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > > kernel version: 2.6.38-rc4
> > 
> > Series looks ok to me...
> 
> Thanks Pavel, may we take that as an Acked-by?

ok.

> For my part the Xen side is:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> The changes to drivers/xen/manage.c depend on some other cleanups made
> on the Xen side (in linux-next soon if not already via Stefano's tree)
> so how would you like to handle them? I could point you to a suitable to
> apply the pm.h bits to or we could carry them in the Xen tree if you are
> happy with them.

Rafael maintains pm tree these days... Rafael?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Rafael J. Wysocki]

On Wednesday, February 23, 2011, Pavel Machek wrote:
> Hi!
> 
> > > > The following set of patches modify this implementation to use Hibernate style
> > > > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > > > which is merely a cancelled save akin to failed swsusp() ).
> > > > 
> > > > These patches are against Ian Campbell's PVHVM tree at
> > > > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > > > 
> > > > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > > > kernel version: 2.6.38-rc4
> > > 
> > > Series looks ok to me...
> > 
> > Thanks Pavel, may we take that as an Acked-by?
> 
> ok.
> 
> > For my part the Xen side is:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > The changes to drivers/xen/manage.c depend on some other cleanups made
> > on the Xen side (in linux-next soon if not already via Stefano's tree)
> > so how would you like to handle them? I could point you to a suitable to
> > apply the pm.h bits to or we could carry them in the Xen tree if you are
> > happy with them.
> 
> Rafael maintains pm tree these days... Rafael?

It doesn't matter a lot.  The Xen people will need to wait for the previous
PM patches to be merged anyway, so I can take the PM bits, so long as I know
which patches are for me to take.

Thanks,
Rafael

Re: Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Konrad Rzeszutek Wilk]

On Wed, Feb 23, 2011 at 09:10:21PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 23, 2011, Pavel Machek wrote:
> > Hi!
> > 
> > > > > The following set of patches modify this implementation to use Hibernate style
> > > > > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > > > > which is merely a cancelled save akin to failed swsusp() ).
> > > > > 
> > > > > These patches are against Ian Campbell's PVHVM tree at
> > > > > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > > > > 
> > > > > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > > > > kernel version: 2.6.38-rc4
> > > > 
> > > > Series looks ok to me...
> > > 
> > > Thanks Pavel, may we take that as an Acked-by?
> > 
> > ok.
> > 
> > > For my part the Xen side is:
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > The changes to drivers/xen/manage.c depend on some other cleanups made
> > > on the Xen side (in linux-next soon if not already via Stefano's tree)
> > > so how would you like to handle them? I could point you to a suitable to
> > > apply the pm.h bits to or we could carry them in the Xen tree if you are
> > > happy with them.
> > 
> > Rafael maintains pm tree these days... Rafael?
> 
> It doesn't matter a lot.  The Xen people will need to wait for the previous
> PM patches to be merged anyway, so I can take the PM bits, so long as I know
> which patches are for me to take.

I believe it only "[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case"

(http://marc.info/?i=1298157158-5421-4-git-send-email-rshriram@cs.ubc.ca)

I or Stefano (these patches are against Ian's tree which is againsts Stefano's
tree) can take the other patches and stick Pavel's Ack, Rafeal's Ack, Ian's Ack
on them and also my Signed-off for the Xen bits.

I think that would work?

Re: Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Rafael J. Wysocki]

On Thursday, February 24, 2011, Konrad Rzeszutek Wilk wrote:
> On Wed, Feb 23, 2011 at 09:10:21PM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 23, 2011, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > The following set of patches modify this implementation to use Hibernate style
> > > > > > control flow (freeze/restore for save/restore and freeze/thaw for checkpoint,
> > > > > > which is merely a cancelled save akin to failed swsusp() ).
> > > > > > 
> > > > > > These patches are against Ian Campbell's PVHVM tree at
> > > > > > git://xenbits.xen.org/people/ianc/linux-2.6.git for-stefano/pvhvm
> > > > > > 
> > > > > > at commit 8a8d1bc753c4e2dda5f2890292d60c67d6ebb573
> > > > > > kernel version: 2.6.38-rc4
> > > > > 
> > > > > Series looks ok to me...
> > > > 
> > > > Thanks Pavel, may we take that as an Acked-by?
> > > 
> > > ok.
> > > 
> > > > For my part the Xen side is:
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > > 
> > > > The changes to drivers/xen/manage.c depend on some other cleanups made
> > > > on the Xen side (in linux-next soon if not already via Stefano's tree)
> > > > so how would you like to handle them? I could point you to a suitable to
> > > > apply the pm.h bits to or we could carry them in the Xen tree if you are
> > > > happy with them.
> > > 
> > > Rafael maintains pm tree these days... Rafael?
> > 
> > It doesn't matter a lot.  The Xen people will need to wait for the previous
> > PM patches to be merged anyway, so I can take the PM bits, so long as I know
> > which patches are for me to take.
> 
> I believe it only "[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case"
> 
> (http://marc.info/?i=1298157158-5421-4-git-send-email-rshriram@cs.ubc.ca)

This particular one should go in _after_ the functional patches.

> I or Stefano (these patches are against Ian's tree which is againsts Stefano's
> tree) can take the other patches and stick Pavel's Ack, Rafeal's Ack, Ian's Ack
> on them and also my Signed-off for the Xen bits.
> 
> I think that would work?

In fact, I think it's better if all patches go through the Xen tree.

Thanks,
Rafael

Re: [linux-pm] [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Stefano Stabellini]

On Mon, 21 Feb 2011, Rafael J. Wysocki wrote:
> > For a PV guest only the Xen PV drivers really matter.
> > 
> > But for a PVHVM guest you are right since there are the emulated "PC"
> > devices though which could be problematic. There's nothing especially
> > thrilling in that set of devices although I don't think that invalidates
> > your point.

This is a genuine concern because it currently breaks save/restore for
all PV on HVM guests (dummy_hcd being the offending driver).


> > > Shouldn't the CONFIG_XEN_SAVE_RESTORE option select CONFIG_HIBERNATION?
> > > In which case the #ifdef lines in pm_op() wouldn't need to be changed.
> > 
> > I think selecting user-visible symbols is generally frowned upon.
> > 
> > But apart from that I was concerned that tying the Xen functionality
> > into the hibernation option was a bit odd/artificial. Perhaps it's the
> > only solution though.
> 
> I'd very much prefer it if the patchset didn't touch drivers/base/power/main.c.
> 
> However, if you want to select CONFIG_HIBERNATION from CONFIG_XEN_SAVE_RESTORE,
> you should make sure that CONFIG_HIBERNATION is really selectable (ie.
> CONFIG_SWAP is set and CONFIG_ARCH_HIBERNATION_POSSIBLE is set).

I think we should follow this suggestion.

Re: [linux-pm] [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Ian Campbell]

On Fri, 2011-02-25 at 16:17 +0000, Stefano Stabellini wrote:
> On Mon, 21 Feb 2011, Rafael J. Wysocki wrote:
> > > For a PV guest only the Xen PV drivers really matter.
> > > 
> > > But for a PVHVM guest you are right since there are the emulated "PC"
> > > devices though which could be problematic. There's nothing especially
> > > thrilling in that set of devices although I don't think that invalidates
> > > your point.
> 
> This is a genuine concern because it currently breaks save/restore for
> all PV on HVM guests (dummy_hcd being the offending driver).

Only if !CONFIG_HIBERNATION, right?

> > > > Shouldn't the CONFIG_XEN_SAVE_RESTORE option select CONFIG_HIBERNATION?
> > > > In which case the #ifdef lines in pm_op() wouldn't need to be changed.
> > > 
> > > I think selecting user-visible symbols is generally frowned upon.
> > > 
> > > But apart from that I was concerned that tying the Xen functionality
> > > into the hibernation option was a bit odd/artificial. Perhaps it's the
> > > only solution though.
> > 
> > I'd very much prefer it if the patchset didn't touch drivers/base/power/main.c.
> > 
> > However, if you want to select CONFIG_HIBERNATION from CONFIG_XEN_SAVE_RESTORE,
> > you should make sure that CONFIG_HIBERNATION is really selectable (ie.
> > CONFIG_SWAP is set and CONFIG_ARCH_HIBERNATION_POSSIBLE is set).
> 
> I think we should follow this suggestion.

See the thread "[PATCH] xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
from Shriram <1298446066-11754-1-git-send-email-rshriram@cs.ubc.ca>

Ian.

Re: [linux-pm] [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Stefano Stabellini]

On Fri, 25 Feb 2011, Ian Campbell wrote:
> On Fri, 2011-02-25 at 16:17 +0000, Stefano Stabellini wrote:
> > On Mon, 21 Feb 2011, Rafael J. Wysocki wrote:
> > > > For a PV guest only the Xen PV drivers really matter.
> > > > 
> > > > But for a PVHVM guest you are right since there are the emulated "PC"
> > > > devices though which could be problematic. There's nothing especially
> > > > thrilling in that set of devices although I don't think that invalidates
> > > > your point.
> > 
> > This is a genuine concern because it currently breaks save/restore for
> > all PV on HVM guests (dummy_hcd being the offending driver).
> 
> Only if !CONFIG_HIBERNATION, right?

Yes

> > > > > Shouldn't the CONFIG_XEN_SAVE_RESTORE option select CONFIG_HIBERNATION?
> > > > > In which case the #ifdef lines in pm_op() wouldn't need to be changed.
> > > > 
> > > > I think selecting user-visible symbols is generally frowned upon.
> > > > 
> > > > But apart from that I was concerned that tying the Xen functionality
> > > > into the hibernation option was a bit odd/artificial. Perhaps it's the
> > > > only solution though.
> > > 
> > > I'd very much prefer it if the patchset didn't touch drivers/base/power/main.c.
> > > 
> > > However, if you want to select CONFIG_HIBERNATION from CONFIG_XEN_SAVE_RESTORE,
> > > you should make sure that CONFIG_HIBERNATION is really selectable (ie.
> > > CONFIG_SWAP is set and CONFIG_ARCH_HIBERNATION_POSSIBLE is set).
> > 
> > I think we should follow this suggestion.
> 
> See the thread "[PATCH] xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
> from Shriram <1298446066-11754-1-git-send-email-rshriram@cs.ubc.ca>

Thanks, I am a bit behind schedule with my unread emails...

Re: Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Stefano Stabellini]

On Thu, 24 Feb 2011, Rafael J. Wysocki wrote:
> > I believe it only "[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case"
> > 
> > (http://marc.info/?i=1298157158-5421-4-git-send-email-rshriram@cs.ubc.ca)
> 
> This particular one should go in _after_ the functional patches.
> 
> > I or Stefano (these patches are against Ian's tree which is againsts Stefano's
> > tree) can take the other patches and stick Pavel's Ack, Rafeal's Ack, Ian's Ack
> > on them and also my Signed-off for the Xen bits.
> > 
> > I think that would work?
> 
> In fact, I think it's better if all patches go through the Xen tree.
> 
 
I don't mind taking them but if they have to go after your
suspend-2.6/linux-next tree this would introduce a new dependency in the
branch I am preparing for linux-next myself.

Should I pull your suspend-2.6/linux-next tree into my linux-next branch?
Considering that this could create conflicts in linux-next if you
force-push your tree with some new changes and I don't update my version
of it, maybe it is better if I pull only a reduced version of it with
just the strict dependencies?

Re: Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Rafael J. Wysocki]

On Friday, February 25, 2011, Stefano Stabellini wrote:
> On Thu, 24 Feb 2011, Rafael J. Wysocki wrote:
> > > I believe it only "[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case"
> > > 
> > > (http://marc.info/?i=1298157158-5421-4-git-send-email-rshriram@cs.ubc.ca)
> > 
> > This particular one should go in _after_ the functional patches.
> > 
> > > I or Stefano (these patches are against Ian's tree which is againsts Stefano's
> > > tree) can take the other patches and stick Pavel's Ack, Rafeal's Ack, Ian's Ack
> > > on them and also my Signed-off for the Xen bits.
> > > 
> > > I think that would work?
> > 
> > In fact, I think it's better if all patches go through the Xen tree.
> > 
>  
> I don't mind taking them but if they have to go after your
> suspend-2.6/linux-next tree this would introduce a new dependency in the
> branch I am preparing for linux-next myself.
> 
> Should I pull your suspend-2.6/linux-next tree into my linux-next branch?
> Considering that this could create conflicts in linux-next if you
> force-push your tree with some new changes and I don't update my version
> of it, maybe it is better if I pull only a reduced version of it with
> just the strict dependencies?

It's not that simple, I think you'd need to pull my entire linux-next branch
because of the dependencies between commits in there.

Alternatively, I can take the entire $subject patchset.

Still, I'd like the discussion to settle before anyway.

Thanks,
Rafael

Re: [Xen-devel] Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Stefano Stabellini]

On Fri, 25 Feb 2011, Rafael J. Wysocki wrote:
> On Friday, February 25, 2011, Stefano Stabellini wrote:
> > On Thu, 24 Feb 2011, Rafael J. Wysocki wrote:
> > > > I believe it only "[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case"
> > > > 
> > > > (http://marc.info/?i=1298157158-5421-4-git-send-email-rshriram@cs.ubc.ca)
> > > 
> > > This particular one should go in _after_ the functional patches.
> > > 
> > > > I or Stefano (these patches are against Ian's tree which is againsts Stefano's
> > > > tree) can take the other patches and stick Pavel's Ack, Rafeal's Ack, Ian's Ack
> > > > on them and also my Signed-off for the Xen bits.
> > > > 
> > > > I think that would work?
> > > 
> > > In fact, I think it's better if all patches go through the Xen tree.
> > > 
> >  
> > I don't mind taking them but if they have to go after your
> > suspend-2.6/linux-next tree this would introduce a new dependency in the
> > branch I am preparing for linux-next myself.
> > 
> > Should I pull your suspend-2.6/linux-next tree into my linux-next branch?
> > Considering that this could create conflicts in linux-next if you
> > force-push your tree with some new changes and I don't update my version
> > of it, maybe it is better if I pull only a reduced version of it with
> > just the strict dependencies?
> 
> It's not that simple, I think you'd need to pull my entire linux-next branch
> because of the dependencies between commits in there.
> 
> Alternatively, I can take the entire $subject patchset.
> 
> Still, I'd like the discussion to settle before anyway.
 
Ok then, when the discussion (and the code) is completely settled I'll
ask you if it is alright for me to pull your branch and have both in
linux-next.

Re: Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Shriram Rajagopalan]

On Mon, Feb 28, 2011 at 3:06 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 25 Feb 2011, Rafael J. Wysocki wrote:
>> On Friday, February 25, 2011, Stefano Stabellini wrote:
>> > On Thu, 24 Feb 2011, Rafael J. Wysocki wrote:
>> > > > I believe it only "[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case"
>> > > >
>> > > > (http://marc.info/?i=1298157158-5421-4-git-send-email-rshriram@cs.ubc.ca)
>> > >
>> > > This particular one should go in _after_ the functional patches.
>> > >
>> > > > I or Stefano (these patches are against Ian's tree which is againsts Stefano's
>> > > > tree) can take the other patches and stick Pavel's Ack, Rafeal's Ack, Ian's Ack
>> > > > on them and also my Signed-off for the Xen bits.
>> > > >
>> > > > I think that would work?
>> > >
>> > > In fact, I think it's better if all patches go through the Xen tree.
>> > >
>> >
>> > I don't mind taking them but if they have to go after your
>> > suspend-2.6/linux-next tree this would introduce a new dependency in the
>> > branch I am preparing for linux-next myself.
>> >
>> > Should I pull your suspend-2.6/linux-next tree into my linux-next branch?
>> > Considering that this could create conflicts in linux-next if you
>> > force-push your tree with some new changes and I don't update my version
>> > of it, maybe it is better if I pull only a reduced version of it with
>> > just the strict dependencies?
>>
>> It's not that simple, I think you'd need to pull my entire linux-next branch
>> because of the dependencies between commits in there.
>>
>> Alternatively, I can take the entire $subject patchset.
>>
>> Still, I'd like the discussion to settle before anyway.
>
There has been no further discussion on this issue so far. Is there a
consensus ? To summarize:
 XEN_SAVE_RESTORE depends on HIBERNATE, and in order to
enable the save/restore functionality, the user has to enable HIBERNATE
explicitly.
 In thread "xen: fix XEN_SAVE_RESTORE Kconfig dependencies", Jan raised
an issue about "selecting" HIBERNATE & SWAP without the user's knowledge. That
was resolved by making XEN_SAVE_RESTORE "depend" on HIBERNATE and
making the user explicitly select it.
 Rafael suggested making an intermediate interface CONFIG_HIBERNATE_INTERFACE
as an alternative but at the cost of a lot of code rework possibly.

Either way, all patches in this series and the
"xen: fix XEN_SAVE_RESTORE Kconfig dependencies" were acked but havent
yet made it to either Stefano's or Rafael's tree.

Would somebody please pull it into their tree?

shriram
> Ok then, when the discussion (and the code) is completely settled I'll
> ask you if it is alright for me to pull your branch and have both in
> linux-next.
>
>

Re: Re: [PATCH 0/3] xen: Use PM/Hibernate events for save/restore/chkpt

[Rafael J. Wysocki]

On Friday, March 04, 2011, Shriram Rajagopalan wrote:
> On Mon, Feb 28, 2011 at 3:06 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 25 Feb 2011, Rafael J. Wysocki wrote:
> >> On Friday, February 25, 2011, Stefano Stabellini wrote:
> >> > On Thu, 24 Feb 2011, Rafael J. Wysocki wrote:
> >> > > > I believe it only "[PATCH 3/3] PM: pm.h - Add comments about Xen save/restore/chkpt use case"
> >> > > >
> >> > > > (http://marc.info/?i=1298157158-5421-4-git-send-email-rshriram@cs.ubc.ca)
> >> > >
> >> > > This particular one should go in _after_ the functional patches.
> >> > >
> >> > > > I or Stefano (these patches are against Ian's tree which is againsts Stefano's
> >> > > > tree) can take the other patches and stick Pavel's Ack, Rafeal's Ack, Ian's Ack
> >> > > > on them and also my Signed-off for the Xen bits.
> >> > > >
> >> > > > I think that would work?
> >> > >
> >> > > In fact, I think it's better if all patches go through the Xen tree.
> >> > >
> >> >
> >> > I don't mind taking them but if they have to go after your
> >> > suspend-2.6/linux-next tree this would introduce a new dependency in the
> >> > branch I am preparing for linux-next myself.
> >> >
> >> > Should I pull your suspend-2.6/linux-next tree into my linux-next branch?
> >> > Considering that this could create conflicts in linux-next if you
> >> > force-push your tree with some new changes and I don't update my version
> >> > of it, maybe it is better if I pull only a reduced version of it with
> >> > just the strict dependencies?
> >>
> >> It's not that simple, I think you'd need to pull my entire linux-next branch
> >> because of the dependencies between commits in there.
> >>
> >> Alternatively, I can take the entire $subject patchset.
> >>
> >> Still, I'd like the discussion to settle before anyway.
> >
> There has been no further discussion on this issue so far. Is there a
> consensus ? To summarize:
>  XEN_SAVE_RESTORE depends on HIBERNATE, and in order to
> enable the save/restore functionality, the user has to enable HIBERNATE
> explicitly.
>  In thread "xen: fix XEN_SAVE_RESTORE Kconfig dependencies", Jan raised
> an issue about "selecting" HIBERNATE & SWAP without the user's knowledge. That
> was resolved by making XEN_SAVE_RESTORE "depend" on HIBERNATE and
> making the user explicitly select it.
>  Rafael suggested making an intermediate interface CONFIG_HIBERNATE_INTERFACE
> as an alternative but at the cost of a lot of code rework possibly.

Not really.  There are a few files I'd like to depend on
CONFIG_HIBERNATE_INTERFACE initially (in kernel/power/ mostly) and the more
fine-grained differentiation can be done later over time.

So, my suggestion is to introduce CONFIG_HIBERNATE_INTERFACE as discussed
elsewhere and rework CONFIG_XEN_SAVE_RESTORE accordingly.

Thanks,
Rafael