Re: [Xen-devel] [PATCH] Use freeze/thaw/restore PM events for Guest suspend/resume/checkpoint

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2011-02-14 at 21:47 +0000, Shriram Rajagopalan wrote:
> On Mon, Feb 14, 2011 at 11:06 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 
> wrote:
> > On Mon, 2011-02-14 at 16:21 +0000, Shriram Rajagopalan wrote:
> >> Use PM_FREEZE, PM_THAW and PM_RESTORE power events for
> >> suspend/resume functionality, instead of PM_SUSPEND and
> >> PM_RESUME. Use of these pm events also fixes the Xen Guest
> >> hangup when taking checkpoints. When a suspend event is cancelled
> >> (i.e. 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);
> >
> > Thanks.
> >
> > Which tree/branch is this against?
> >
> Oh I thought that info be present in the git indices
> "index 5413248..ff32ffb 100644" .

I think those are the indices of the actual versions of the file, rather
than the tree->commit->branch which contains them. I'm sure you could
map up that chain but suspect it'd be a rather brute force affair.

> Its against my local git branch (which tracks xen/stable-2.6.32.x and
> is uptodate).

In general we'd prefer to get this sort of thing fixed in the mainline
kernel (e.g. Linus' tree) first and then consider backports to the
stable branch as necessary.

> Sorry, I am still a bit of git newbie.

No problem.

> > Can you please at least do the dev_pm_ops as a separate patch to 
> > allow bisectability etc. (generally each patch should be a single 
> > logical change so if the remaineder can be sensibly split too it is
> > worth doing so).

> ok

You can probably take Kazuhiro's first patch verbatim for this bit? If
you save the mail to a file you can apply it with "git am < foo.mail"
and git will preserve authorship attribution etc and this will persist
through "git format-patch" and "git send-email" etc.

> > Did you test regular save/restore as well as cancelled migrations? What
> > about PVHMV guests?
> >
> >> +static struct dev_pm_ops xenbus_pm_ops = {
> >> +     .suspend = xenbus_dev_suspend,
> >> +     .resume  = xenbus_dev_resume,
> >> +     .freeze  = xenbus_dev_suspend,
> >> +     .thaw    = xenbus_dev_cancel,
> >> +     .restore = xenbus_dev_resume,
> >> +};
> >
> > Perhaps xenbus_dev_thaw?
> >
> > Are suspend/freeze and resume/restore really the same?
> >
> Semantically they are not. From the documentation in pm.h,
[...]
> So, in our case, suspend/freeze and resume/restore are basically same.
> suspend-cancel is a thaw event.

OK.

> > Once we've transitioned to the PMSG_FREEZE way of doing things do we
> > still need to keep the other hooks around? If not then the other ones
> > could be renamed as well?
> >
> If your question is whether we can change
> static struct dev_pm_ops xenbus_pm_ops = {
>      .suspend = xenbus_dev_suspend,
>      .resume  = xenbus_dev_resume,
>      .freeze  = xenbus_dev_suspend,
>      .thaw    = xenbus_dev_cancel,
>      .restore = xenbus_dev_resume,
> };
> to just
> static struct dev_pm_ops xenbus_pm_ops = {
>      .freeze  = xenbus_dev_freeze,
>      .thaw    = xenbus_dev_thaw,
>      .restore = xenbus_dev_restore,
> };
> 
> then the answer is no, AFAICS, from the code in drivers/base/power/main.c
> (pm_op function).

Looking at pm_op it doesn't appear to be an error to not implement one
or more hooks. For example we currently don't implement freeze/thaw and
that is currently ok because we don't initiate that sort of suspend.

Currently drivers/xen/manage.c hangs the suspend code off
CONFIG_PM_SLEEP, I wonder if it shouldn't be CONFIG_SUSPEND already and
become CONFIG_HIBERNATE with your change?

I also wonder if shutdown_handler shouldn't check it is actually going
to be able to perform the suspend _before_ acknowledging the suspend
request, but that is outside the scope of this patch.

Ian.



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