Re: [Xen-devel] [PATCH] xen: use freeze/restore/thaw PM events for suspend/resume/chkpt

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2011-02-16 at 06:51 +0000, 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);

Thank you.

This applies without fuzz on top of my pvhvm suspend cleanup series from
yesterday but with the side effect that this change now impacts PVHVM
suspend too (since xen_hvm_suspend, which you did not patch here, was
merged into xen_suspend by that series) -- did you test that
configuration or did you deliberately avoid changing xen_hvm_suspend?

Tiny fixlet I needed to compile on top of that series

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 1c855e3..8f62fec 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -147,7 +147,7 @@ static void do_suspend(void)
 
        err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
-       dpm_resume_noirq(cancelled ? PMSG_THAW : PMSG_RESTORE);
+       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(cancelled ? PMSG_THAW : PMSG_RESTORE);
+       dpm_resume_end(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
        /* Make sure timer events get retriggered on all CPUs */
        clock_was_set();


>  
> -     printk(KERN_DEBUG "suspending xenstore...\n");
> +     /* printk(KERN_DEBUG "suspending xenstore...\n"); */
>       xs_suspend();
> [...] 
>  int xenbus_dev_cancel(struct device *dev)
>  {
>       /* Do nothing */
> -     DPRINTK("cancel");
> +     /* DPRINTK("cancel"); */
>       return 0;

Please don't make unrelated changes in patches.

Also please don't just comment out code, either leave these lines alone
or remove them entirely.

Otherwise the patch appears to be fine to me.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel