[PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Frank Pan]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

Recent pvops kernel fails on suspending some of devices on the
following configuration:

CONFIG_SUSPEND=n
CONFIG_PM_SLEEP=y

Command suspend on control/shutdown is recognized when
CONFIG_PM_SLEEP=y. This indicates the device suspend logic should also
be enabled on this configuration.
The following patch works.
---
 linux-2.6-xen/drivers/base/power/main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-2.6-xen/drivers/base/power/main.c
b/linux-2.6-xen/drivers/base/power/main.c
index 8aa2443..4d40fc4 100644
--- a/linux-2.6-xen/drivers/base/power/main.c
+++ b/linux-2.6-xen/drivers/base/power/main.c
@@ -174,7 +174,7 @@ static int pm_op(struct device *dev,
 	int error = 0;

 	switch (state.event) {
-#ifdef CONFIG_SUSPEND
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_XEN_SAVE_RESTORE)
 	case PM_EVENT_SUSPEND:
 		if (ops->suspend) {
 			error = ops->suspend(dev);
@@ -238,7 +238,7 @@ static int pm_noirq_op(struct device *dev,
 	int error = 0;

 	switch (state.event) {
-#ifdef CONFIG_SUSPEND
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_XEN_SAVE_RESTORE)
 	case PM_EVENT_SUSPEND:
 		if (ops->suspend_noirq) {
 			error = ops->suspend_noirq(dev);
-- 
1.7.0.4


-- 
潘震皓, Frank Pan

Computer Science and Technology
Tsinghua University

[-- Attachment #2: 0001-Make-suspend-work-when-CONFIG_SUSPEND-n.patch --]
[-- Type: text/x-patch, Size: 1407 bytes --]

From 941b7222efe8be20ea285f5325fbe46706c55464 Mon Sep 17 00:00:00 2001
From: Frank Pan <frankpzh@gmail.com>
Date: Fri, 4 Mar 2011 18:49:38 +0800
Subject: [PATCH] Make suspend work when CONFIG_SUSPEND=n

Recent pvops kernel fails on suspending some of devices on the following configuration:

CONFIG_SUSPEND=n
CONFIG_PM_SLEEP=y

Command suspend on control/shutdown is recognized when CONFIG_PM_SLEEP=y. This indicates the device suspend logic should also be enabled on this configuration.
The following patch works.
---
 linux-2.6-xen/drivers/base/power/main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-2.6-xen/drivers/base/power/main.c b/linux-2.6-xen/drivers/base/power/main.c
index 8aa2443..4d40fc4 100644
--- a/linux-2.6-xen/drivers/base/power/main.c
+++ b/linux-2.6-xen/drivers/base/power/main.c
@@ -174,7 +174,7 @@ static int pm_op(struct device *dev,
 	int error = 0;
 
 	switch (state.event) {
-#ifdef CONFIG_SUSPEND
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_XEN_SAVE_RESTORE)
 	case PM_EVENT_SUSPEND:
 		if (ops->suspend) {
 			error = ops->suspend(dev);
@@ -238,7 +238,7 @@ static int pm_noirq_op(struct device *dev,
 	int error = 0;
 
 	switch (state.event) {
-#ifdef CONFIG_SUSPEND
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_XEN_SAVE_RESTORE)
 	case PM_EVENT_SUSPEND:
 		if (ops->suspend_noirq) {
 			error = ops->suspend_noirq(dev);
-- 
1.7.0.4


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Ian Campbell]

Thanks for this.

There has already been recent work on this, please see the threads "xen:
Use PM/Hibernate events for save/restore/chkpt" and "xen: fix
XEN_SAVE_RESTORE Kconfig dependencies" on both xen-devel and linux-pm.
AFAIK the conclusion is that an approach which ensures both
XEN_SAVE_RESTORE and SUSPEND (actually HIBERNATE after the above
discussion) are enable when necessary (by making the former depend on
the later) is what is going to be taken.

Ian.

On Fri, 2011-03-04 at 11:20 +0000, Frank Pan wrote:
> Recent pvops kernel fails on suspending some of devices on the
> following configuration:
> 
> CONFIG_SUSPEND=n
> CONFIG_PM_SLEEP=y
> 
> Command suspend on control/shutdown is recognized when
> CONFIG_PM_SLEEP=y. This indicates the device suspend logic should also
> be enabled on this configuration.
> The following patch works.
> ---
>  linux-2.6-xen/drivers/base/power/main.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-2.6-xen/drivers/base/power/main.c
> b/linux-2.6-xen/drivers/base/power/main.c
> index 8aa2443..4d40fc4 100644
> --- a/linux-2.6-xen/drivers/base/power/main.c
> +++ b/linux-2.6-xen/drivers/base/power/main.c
> @@ -174,7 +174,7 @@ static int pm_op(struct device *dev,
>  	int error = 0;
> 
>  	switch (state.event) {
> -#ifdef CONFIG_SUSPEND
> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_XEN_SAVE_RESTORE)
>  	case PM_EVENT_SUSPEND:
>  		if (ops->suspend) {
>  			error = ops->suspend(dev);
> @@ -238,7 +238,7 @@ static int pm_noirq_op(struct device *dev,
>  	int error = 0;
> 
>  	switch (state.event) {
> -#ifdef CONFIG_SUSPEND
> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_XEN_SAVE_RESTORE)
>  	case PM_EVENT_SUSPEND:
>  		if (ops->suspend_noirq) {
>  			error = ops->suspend_noirq(dev);
> -- 
> 1.7.0.4
> 
> 

Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Frank Pan]

> AFAIK the conclusion is that an approach which ensures both
> XEN_SAVE_RESTORE and SUSPEND (actually HIBERNATE after the above
> discussion) are enable when necessary (by making the former depend on
> the later) is what is going to be taken.

That's good. How long will it be committed on pvops trees?

-- 
潘震皓, Frank Pan

Computer Science and Technology
Tsinghua University

Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Ian Campbell]

On Fri, 2011-03-04 at 11:45 +0000, Frank Pan wrote:
> > AFAIK the conclusion is that an approach which ensures both
> > XEN_SAVE_RESTORE and SUSPEND (actually HIBERNATE after the above
> > discussion) are enable when necessary (by making the former depend on
> > the later) is what is going to be taken.
> 
> That's good. How long will it be committed on pvops trees?

I don't know. Stefano, Rafael and Shriram are taking care of it AFAIK.

Ian.

Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Shriram Rajagopalan]

On Fri, Mar 4, 2011 at 3:52 AM, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> On Fri, 2011-03-04 at 11:45 +0000, Frank Pan wrote:
>> > AFAIK the conclusion is that an approach which ensures both
>> > XEN_SAVE_RESTORE and SUSPEND (actually HIBERNATE after the above
>> > discussion) are enable when necessary (by making the former depend on
>> > the later) is what is going to be taken.
>>
>> That's good. How long will it be committed on pvops trees?
>
> I don't know. Stefano, Rafael and Shriram are taking care of it AFAIK.
>
Well, from my side, I am waiting on Stefano & Rafael. The discussion seemed
to have ended with "lets wait for the code to settle", though there seemed to be
no consensus regarding the need to enable HIBERNATE for XEN_SAVE_RESTORE
to work.
 Someone suggested creating a new user visible hibernate symbol that would
solve this issue and make the main hibernate logic depend on this symbol rather
than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
seemed to have reached a conclusion.

As far as pulling the patches is concerned, IIRC, Rafael & Stefano
were talking "git"
jargon, as to who is going to take which patch.
Meanwhile, recently, Stefano said his branch went into linux-next
(my patches are not in that tree last I checked. :( ).

shriram
> Ian.
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Konrad Rzeszutek Wilk]

On Fri, Mar 04, 2011 at 07:42:00AM -0800, Shriram Rajagopalan wrote:
> On Fri, Mar 4, 2011 at 3:52 AM, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> > On Fri, 2011-03-04 at 11:45 +0000, Frank Pan wrote:
> >> > AFAIK the conclusion is that an approach which ensures both
> >> > XEN_SAVE_RESTORE and SUSPEND (actually HIBERNATE after the above
> >> > discussion) are enable when necessary (by making the former depend on
> >> > the later) is what is going to be taken.
> >>
> >> That's good. How long will it be committed on pvops trees?
> >
> > I don't know. Stefano, Rafael and Shriram are taking care of it AFAIK.
> >
> Well, from my side, I am waiting on Stefano & Rafael. The discussion seemed
> to have ended with "lets wait for the code to settle", though there seemed to be
> no consensus regarding the need to enable HIBERNATE for XEN_SAVE_RESTORE
> to work.
>  Someone suggested creating a new user visible hibernate symbol that would
> solve this issue and make the main hibernate logic depend on this symbol rather
> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
> seemed to have reached a conclusion.

Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
to implement that.

That was the only thing gatting your patchset going in.

Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Shriram Rajagopalan]

On Fri, Mar 4, 2011 at 7:56 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Fri, Mar 04, 2011 at 07:42:00AM -0800, Shriram Rajagopalan wrote:
>> On Fri, Mar 4, 2011 at 3:52 AM, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
>> > On Fri, 2011-03-04 at 11:45 +0000, Frank Pan wrote:
>> >> > AFAIK the conclusion is that an approach which ensures both
>> >> > XEN_SAVE_RESTORE and SUSPEND (actually HIBERNATE after the above
>> >> > discussion) are enable when necessary (by making the former depend on
>> >> > the later) is what is going to be taken.
>> >>
>> >> That's good. How long will it be committed on pvops trees?
>> >
>> > I don't know. Stefano, Rafael and Shriram are taking care of it AFAIK.
>> >
>> Well, from my side, I am waiting on Stefano & Rafael. The discussion seemed
>> to have ended with "lets wait for the code to settle", though there seemed to be
>> no consensus regarding the need to enable HIBERNATE for XEN_SAVE_RESTORE
>> to work.
>>  Someone suggested creating a new user visible hibernate symbol that would
>> solve this issue and make the main hibernate logic depend on this symbol rather
>> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
>> seemed to have reached a conclusion.
>
> Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
> to implement that.
>
> That was the only thing gatting your patchset going in.
>
>

I certainly would have long time ago but for this comment in the thread
"xen: fix XEN_SAVE_RESTORE Kconfig dependencies"

Rafael:
 I think we can introduce CONFIG_HIBERNATE_INTERFACE that will be user-visible
option instead of CONFIG_HIBERNATION and will select the latter.  Then,
CONFIG_XEN_SAVE_RESTORE will also be able to select CONFIG_HIBERNATION without
building the hibernate interface in, which will prevent user space from being
confused, but that will cause too much code to be built anyway.

If by "too much code to be built", he meant the increase in kernel
image size, then its not much of a deal :P.
But if he meant, "too much code rework", then it is an issue.

But IMO, the CONFIG_HIBERNATE_INTERFACE needs to go in,
only in the main hibernation initiator logic, as we still need the
CONFIG_HIBERNATE
pieces of every driver anyway (their freeze/thaw routines).

shriram

Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Konrad Rzeszutek Wilk]

.. snip..
> >>  Someone suggested creating a new user visible hibernate symbol that would
> >> solve this issue and make the main hibernate logic depend on this symbol rather
> >> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
> >> seemed to have reached a conclusion.
> >
> > Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
> > to implement that.
> >
> > That was the only thing gatting your patchset going in.
> >
> >
> 
> I certainly would have long time ago but for this comment in the thread
> "xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
> 
> Rafael:
>  I think we can introduce CONFIG_HIBERNATE_INTERFACE that will be user-visible
> option instead of CONFIG_HIBERNATION and will select the latter.  Then,
> CONFIG_XEN_SAVE_RESTORE will also be able to select CONFIG_HIBERNATION without
> building the hibernate interface in, which will prevent user space from being
> confused, but that will cause too much code to be built anyway.
> 
> If by "too much code to be built", he meant the increase in kernel
> image size, then its not much of a deal :P.
> But if he meant, "too much code rework", then it is an issue.

The idea here is that the /sys/power/state won't be exposed with the "disk"
option.

> 
> But IMO, the CONFIG_HIBERNATE_INTERFACE needs to go in,
> only in the main hibernation initiator logic, as we still need the
> CONFIG_HIBERNATE
> pieces of every driver anyway (their freeze/thaw routines).

Right. The idea here is to seperate the sysfs interface to be behind
another config option. So you can still enable the hibernate kernel code
but without exposing it to the userland.

Rafael,

That is the general idea, right?

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Shriram Rajagopalan]

On Fri, Mar 4, 2011 at 10:26 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> .. snip..
>> >>  Someone suggested creating a new user visible hibernate symbol that would
>> >> solve this issue and make the main hibernate logic depend on this symbol rather
>> >> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
>> >> seemed to have reached a conclusion.
>> >
>> > Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
>> > to implement that.
>> >
>> > That was the only thing gatting your patchset going in.
>> >
>> >
>>
>> I certainly would have long time ago but for this comment in the thread
>> "xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
>>
>> Rafael:
>>  I think we can introduce CONFIG_HIBERNATE_INTERFACE that will be user-visible
>> option instead of CONFIG_HIBERNATION and will select the latter.  Then,
>> CONFIG_XEN_SAVE_RESTORE will also be able to select CONFIG_HIBERNATION without
>> building the hibernate interface in, which will prevent user space from being
>> confused, but that will cause too much code to be built anyway.
>>
>> If by "too much code to be built", he meant the increase in kernel
>> image size, then its not much of a deal :P.
>> But if he meant, "too much code rework", then it is an issue.
>
> The idea here is that the /sys/power/state won't be exposed with the "disk"
> option.
>
>>
>> But IMO, the CONFIG_HIBERNATE_INTERFACE needs to go in,
>> only in the main hibernation initiator logic, as we still need the
>> CONFIG_HIBERNATE
>> pieces of every driver anyway (their freeze/thaw routines).
>
> Right. The idea here is to seperate the sysfs interface to be behind
> another config option. So you can still enable the hibernate kernel code
> but without exposing it to the userland.
>
> Rafael,
>
> That is the general idea, right?
>
>

I was thinking along the lines of
config HIBERNATION
 def_bool n

config HIBERNATION_INTERFACE
 select HIBERNATE

config XEN_SAVE_RESTORE
 select HIBERNATION

in kernel/power/Makefile
obj-$(CONFIG_HIBERNATION_INTERFACE)  += hibernate.o snapshot.o swap.o \

  user.o block_io.o

Will this be sufficient to prevent unnecessary code from being built?
Or, is this oversimplified file exclusion totally wrong and I have to
dig deeper?

>From a cursory glance, these files seem to be dealing solely with SWSUSP which
roughly does the following:
 1. freezing devices (using pm_op functions in main.c)
 2. saving memory to swap
 3. thawing on resume (using pm_op functions in main.c)

XEN_SAVE_RESTORE only needs steps 1 & 3.


shriram

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Rafael J. Wysocki]

On Friday, March 04, 2011, Shriram Rajagopalan wrote:
> On Fri, Mar 4, 2011 at 10:26 AM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > .. snip..
> >> >>  Someone suggested creating a new user visible hibernate symbol that would
> >> >> solve this issue and make the main hibernate logic depend on this symbol rather
> >> >> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
> >> >> seemed to have reached a conclusion.
> >> >
> >> > Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
> >> > to implement that.
> >> >
> >> > That was the only thing gatting your patchset going in.
> >> >
> >> >
> >>
> >> I certainly would have long time ago but for this comment in the thread
> >> "xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
> >>
> >> Rafael:
> >>  I think we can introduce CONFIG_HIBERNATE_INTERFACE that will be user-visible
> >> option instead of CONFIG_HIBERNATION and will select the latter.  Then,
> >> CONFIG_XEN_SAVE_RESTORE will also be able to select CONFIG_HIBERNATION without
> >> building the hibernate interface in, which will prevent user space from being
> >> confused, but that will cause too much code to be built anyway.
> >>
> >> If by "too much code to be built", he meant the increase in kernel
> >> image size, then its not much of a deal :P.
> >> But if he meant, "too much code rework", then it is an issue.
> >
> > The idea here is that the /sys/power/state won't be exposed with the "disk"
> > option.
> >
> >>
> >> But IMO, the CONFIG_HIBERNATE_INTERFACE needs to go in,
> >> only in the main hibernation initiator logic, as we still need the
> >> CONFIG_HIBERNATE
> >> pieces of every driver anyway (their freeze/thaw routines).
> >
> > Right. The idea here is to seperate the sysfs interface to be behind
> > another config option. So you can still enable the hibernate kernel code
> > but without exposing it to the userland.
> >
> > Rafael,
> >
> > That is the general idea, right?
> >
> >
> 
> I was thinking along the lines of
> config HIBERNATION
>  def_bool n
> 
> config HIBERNATION_INTERFACE
>  select HIBERNATE

select HIBERNATION

> config XEN_SAVE_RESTORE
>  select HIBERNATION
> 
> in kernel/power/Makefile
> obj-$(CONFIG_HIBERNATION_INTERFACE)  += hibernate.o snapshot.o swap.o \
> 
>   user.o block_io.o
> 
> Will this be sufficient to prevent unnecessary code from being built?

Not all of it, but the majority.

> Or, is this oversimplified file exclusion totally wrong and I have to
> dig deeper?

That can be done in the future over time.

> From a cursory glance, these files seem to be dealing solely with SWSUSP which
> roughly does the following:
>  1. freezing devices (using pm_op functions in main.c)
>  2. saving memory to swap
>  3. thawing on resume (using pm_op functions in main.c)
> 
> XEN_SAVE_RESTORE only needs steps 1 & 3.

That's correct.

Thanks,
Rafael

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Shriram Rajagopalan]

On Fri, Mar 4, 2011 at 12:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, March 04, 2011, Shriram Rajagopalan wrote:
>> On Fri, Mar 4, 2011 at 10:26 AM, Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com> wrote:
>> > .. snip..
>> >> >>  Someone suggested creating a new user visible hibernate symbol that would
>> >> >> solve this issue and make the main hibernate logic depend on this symbol rather
>> >> >> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
>> >> >> seemed to have reached a conclusion.
>> >> >
>> >> > Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
>> >> > to implement that.
>> >> >
>> >> > That was the only thing gatting your patchset going in.
>> >> >
>> >> >
>> >>
>> >> I certainly would have long time ago but for this comment in the thread
>> >> "xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
>> >>
>> >> Rafael:
>> >>  I think we can introduce CONFIG_HIBERNATE_INTERFACE that will be user-visible
>> >> option instead of CONFIG_HIBERNATION and will select the latter.  Then,
>> >> CONFIG_XEN_SAVE_RESTORE will also be able to select CONFIG_HIBERNATION without
>> >> building the hibernate interface in, which will prevent user space from being
>> >> confused, but that will cause too much code to be built anyway.
>> >>
>> >> If by "too much code to be built", he meant the increase in kernel
>> >> image size, then its not much of a deal :P.
>> >> But if he meant, "too much code rework", then it is an issue.
>> >
>> > The idea here is that the /sys/power/state won't be exposed with the "disk"
>> > option.
>> >
>> >>
>> >> But IMO, the CONFIG_HIBERNATE_INTERFACE needs to go in,
>> >> only in the main hibernation initiator logic, as we still need the
>> >> CONFIG_HIBERNATE
>> >> pieces of every driver anyway (their freeze/thaw routines).
>> >
>> > Right. The idea here is to seperate the sysfs interface to be behind
>> > another config option. So you can still enable the hibernate kernel code
>> > but without exposing it to the userland.
>> >
>> > Rafael,
>> >
>> > That is the general idea, right?
>> >
>> >
>>
>> I was thinking along the lines of
>> config HIBERNATION
>>  def_bool n
>>
>> config HIBERNATION_INTERFACE
>>  select HIBERNATE
>
> select HIBERNATION
>
>> config XEN_SAVE_RESTORE
>>  select HIBERNATION
>>
>> in kernel/power/Makefile
>> obj-$(CONFIG_HIBERNATION_INTERFACE)  += hibernate.o snapshot.o swap.o \
>>
>>   user.o block_io.o
>>
>> Will this be sufficient to prevent unnecessary code from being built?
>
> Not all of it, but the majority.
>
>> Or, is this oversimplified file exclusion totally wrong and I have to
>> dig deeper?
>
> That can be done in the future over time.
>
>> From a cursory glance, these files seem to be dealing solely with SWSUSP which
>> roughly does the following:
>>  1. freezing devices (using pm_op functions in main.c)
>>  2. saving memory to swap
>>  3. thawing on resume (using pm_op functions in main.c)
>>
>> XEN_SAVE_RESTORE only needs steps 1 & 3.
>
> That's correct.
>
> Thanks,
> Rafael
>
>

Is it okay if I send out both the HIBERNATION_INTERFACE patch and
the XEN_SAVE_RESTORE kconfig fixes against Rafael's tree?

Rafael's tree on git.kernel.org and Stefano tree on
http://xenbits.xen.org/gitweb/ are out of sync (on linux-next branch,
2.6.38-rc6).
I am referring to files arch/x86/xen/Kconfig and kernel/power/Kconfig that
would be touched by these two patches.

Rafael's commit 5b56bff0f50bc1ed643c2ae85e803d17fc0a110e
"PM: Make CONFIG_PM depend on (CONFIG_PM_SLEEP || CONFIG_PM_RUNTIME)"
touches both these files and this commit is not present in stefano's tree.

The only issue is that I cannot completely "test" these two patches
against Rafael's tree
 - I have verified that the kernel config file generated is as expected.
 - I cannot verify any other xen save/restore functionality as my xen
   suspend freeze-thaw patches dont apply cleanly on Rafael's tree
  (it does not have xen suspend refactoring patches
  ceb180294790c8a6a437533488616f6b591b49d0, that my patches depend on.
  They are present only in Stefano's tree).

shriram

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Rafael J. Wysocki]

On Sunday, March 06, 2011, Shriram Rajagopalan wrote:
> On Fri, Mar 4, 2011 at 12:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, March 04, 2011, Shriram Rajagopalan wrote:
> >> On Fri, Mar 4, 2011 at 10:26 AM, Konrad Rzeszutek Wilk
> >> <konrad.wilk@oracle.com> wrote:
> >> > .. snip..
> >> >> >>  Someone suggested creating a new user visible hibernate symbol that would
> >> >> >> solve this issue and make the main hibernate logic depend on this symbol rather
> >> >> >> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
> >> >> >> seemed to have reached a conclusion.
> >> >> >
> >> >> > Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
> >> >> > to implement that.
> >> >> >
> >> >> > That was the only thing gatting your patchset going in.
> >> >> >
> >> >> >
> >> >>
> >> >> I certainly would have long time ago but for this comment in the thread
> >> >> "xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
> >> >>
> >> >> Rafael:
> >> >>  I think we can introduce CONFIG_HIBERNATE_INTERFACE that will be user-visible
> >> >> option instead of CONFIG_HIBERNATION and will select the latter.  Then,
> >> >> CONFIG_XEN_SAVE_RESTORE will also be able to select CONFIG_HIBERNATION without
> >> >> building the hibernate interface in, which will prevent user space from being
> >> >> confused, but that will cause too much code to be built anyway.
> >> >>
> >> >> If by "too much code to be built", he meant the increase in kernel
> >> >> image size, then its not much of a deal :P.
> >> >> But if he meant, "too much code rework", then it is an issue.
> >> >
> >> > The idea here is that the /sys/power/state won't be exposed with the "disk"
> >> > option.
> >> >
> >> >>
> >> >> But IMO, the CONFIG_HIBERNATE_INTERFACE needs to go in,
> >> >> only in the main hibernation initiator logic, as we still need the
> >> >> CONFIG_HIBERNATE
> >> >> pieces of every driver anyway (their freeze/thaw routines).
> >> >
> >> > Right. The idea here is to seperate the sysfs interface to be behind
> >> > another config option. So you can still enable the hibernate kernel code
> >> > but without exposing it to the userland.
> >> >
> >> > Rafael,
> >> >
> >> > That is the general idea, right?
> >> >
> >> >
> >>
> >> I was thinking along the lines of
> >> config HIBERNATION
> >>  def_bool n
> >>
> >> config HIBERNATION_INTERFACE
> >>  select HIBERNATE
> >
> > select HIBERNATION
> >
> >> config XEN_SAVE_RESTORE
> >>  select HIBERNATION
> >>
> >> in kernel/power/Makefile
> >> obj-$(CONFIG_HIBERNATION_INTERFACE)  += hibernate.o snapshot.o swap.o \
> >>
> >>   user.o block_io.o
> >>
> >> Will this be sufficient to prevent unnecessary code from being built?
> >
> > Not all of it, but the majority.
> >
> >> Or, is this oversimplified file exclusion totally wrong and I have to
> >> dig deeper?
> >
> > That can be done in the future over time.
> >
> >> From a cursory glance, these files seem to be dealing solely with SWSUSP which
> >> roughly does the following:
> >>  1. freezing devices (using pm_op functions in main.c)
> >>  2. saving memory to swap
> >>  3. thawing on resume (using pm_op functions in main.c)
> >>
> >> XEN_SAVE_RESTORE only needs steps 1 & 3.
> >
> > That's correct.
> >
> > Thanks,
> > Rafael
> >
> >
> 
> Is it okay if I send out both the HIBERNATION_INTERFACE patch and
> the XEN_SAVE_RESTORE kconfig fixes against Rafael's tree?
> 
> Rafael's tree on git.kernel.org and Stefano tree on
> http://xenbits.xen.org/gitweb/ are out of sync (on linux-next branch,
> 2.6.38-rc6).
> I am referring to files arch/x86/xen/Kconfig and kernel/power/Kconfig that
> would be touched by these two patches.
> 
> Rafael's commit 5b56bff0f50bc1ed643c2ae85e803d17fc0a110e
> "PM: Make CONFIG_PM depend on (CONFIG_PM_SLEEP || CONFIG_PM_RUNTIME)"
> touches both these files and this commit is not present in stefano's tree.
> 
> The only issue is that I cannot completely "test" these two patches
> against Rafael's tree
>  - I have verified that the kernel config file generated is as expected.
>  - I cannot verify any other xen save/restore functionality as my xen
>    suspend freeze-thaw patches dont apply cleanly on Rafael's tree
>   (it does not have xen suspend refactoring patches
>   ceb180294790c8a6a437533488616f6b591b49d0, that my patches depend on.
>   They are present only in Stefano's tree).

In that case, I'm afraid, it's better to wait until both trees are merged
and push your patches at that time.

Thanks,
Rafael

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Stefano Stabellini]

On Sun, 6 Mar 2011, Rafael J. Wysocki wrote:
> > The only issue is that I cannot completely "test" these two patches
> > against Rafael's tree
> >  - I have verified that the kernel config file generated is as expected.
> >  - I cannot verify any other xen save/restore functionality as my xen
> >    suspend freeze-thaw patches dont apply cleanly on Rafael's tree
> >   (it does not have xen suspend refactoring patches
> >   ceb180294790c8a6a437533488616f6b591b49d0, that my patches depend on.
> >   They are present only in Stefano's tree).
> 
> In that case, I'm afraid, it's better to wait until both trees are merged
> and push your patches at that time.
 
I agree even if it probably means loosing the next merge window.

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 1108 bytes --]

On Mon, Mar 7, 2011 at 4:44 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:

> On Sun, 6 Mar 2011, Rafael J. Wysocki wrote:
> > > The only issue is that I cannot completely "test" these two patches
> > > against Rafael's tree
> > >  - I have verified that the kernel config file generated is as
> expected.
> > >  - I cannot verify any other xen save/restore functionality as my xen
> > >    suspend freeze-thaw patches dont apply cleanly on Rafael's tree
> > >   (it does not have xen suspend refactoring patches
> > >   ceb180294790c8a6a437533488616f6b591b49d0, that my patches depend on.
> > >   They are present only in Stefano's tree).
> >
> > In that case, I'm afraid, it's better to wait until both trees are merged
> > and push your patches at that time.
>
> I agree even if it probably means loosing the next merge window.
>
> I believe the original intention of the freeze-thaw patches was to fix the
bug that caused
Guest VM hangs, while taking a checkpoint.

Anyway, is there any chance of pushing these patches into the pvops tree
atleast?
I can rebase them to pvops.

shriram

[-- Attachment #1.2: Type: text/html, Size: 1554 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Konrad Rzeszutek Wilk]

On Mon, Mar 07, 2011 at 08:33:25AM -0800, Shriram Rajagopalan wrote:
> On Mon, Mar 7, 2011 at 4:44 AM, Stefano Stabellini <
> stefano.stabellini@eu.citrix.com> wrote:
> 
> > On Sun, 6 Mar 2011, Rafael J. Wysocki wrote:
> > > > The only issue is that I cannot completely "test" these two patches
> > > > against Rafael's tree
> > > >  - I have verified that the kernel config file generated is as
> > expected.
> > > >  - I cannot verify any other xen save/restore functionality as my xen
> > > >    suspend freeze-thaw patches dont apply cleanly on Rafael's tree
> > > >   (it does not have xen suspend refactoring patches
> > > >   ceb180294790c8a6a437533488616f6b591b49d0, that my patches depend on.
> > > >   They are present only in Stefano's tree).
> > >
> > > In that case, I'm afraid, it's better to wait until both trees are merged
> > > and push your patches at that time.

<nods> But we need to review/test/etc his patches before the merge window..
> >
> > I agree even if it probably means loosing the next merge window.
> >
> > I believe the original intention of the freeze-thaw patches was to fix the
> bug that caused
> Guest VM hangs, while taking a checkpoint.
> 
> Anyway, is there any chance of pushing these patches into the pvops tree
> atleast?
> I can rebase them to pvops.

so Shriram could create a branch that has both Stefano's (and also Ian's patches)
and Rafeal's tree and you can use that. And when the merge window happens
we would let Stefano's and Rafeal's tree go in and after that ask Linus to pull
this complicated tree.

That means you are going to have to create this hybrid tree. You can create a git branch
from Stefano's tree, merge in Rafael's tree, and test it (baremetal and also Xen).
Once you are satisfied that it works and it is bug-free:

 a). send the tar ball to me (or just a link).
 b). I am going to extract that branch in my tree
 c). You are going to post the patches that you had to develop so we can review
     them.
 d). If everybody is happy and testing on our test-beds is OK, I can push this tree
     later in the merge window cycle.

It is a bit of dance but it ought to work out?

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 2736 bytes --]

On Mon, Mar 7, 2011 at 9:53 AM, Konrad Rzeszutek Wilk <
konrad.wilk@oracle.com> wrote:

> On Mon, Mar 07, 2011 at 08:33:25AM -0800, Shriram Rajagopalan wrote:
> > On Mon, Mar 7, 2011 at 4:44 AM, Stefano Stabellini <
> > stefano.stabellini@eu.citrix.com> wrote:
> >
> > > On Sun, 6 Mar 2011, Rafael J. Wysocki wrote:
> > > > > The only issue is that I cannot completely "test" these two patches
> > > > > against Rafael's tree
> > > > >  - I have verified that the kernel config file generated is as
> > > expected.
> > > > >  - I cannot verify any other xen save/restore functionality as my
> xen
> > > > >    suspend freeze-thaw patches dont apply cleanly on Rafael's tree
> > > > >   (it does not have xen suspend refactoring patches
> > > > >   ceb180294790c8a6a437533488616f6b591b49d0, that my patches depend
> on.
> > > > >   They are present only in Stefano's tree).
> > > >
> > > > In that case, I'm afraid, it's better to wait until both trees are
> merged
> > > > and push your patches at that time.
>
> <nods> But we need to review/test/etc his patches before the merge window..
>
> >
> > > I agree even if it probably means loosing the next merge window.
> > >
> > > I believe the original intention of the freeze-thaw patches was to fix
> the
> > bug that caused
> > Guest VM hangs, while taking a checkpoint.
> >
> > Anyway, is there any chance of pushing these patches into the pvops tree
> > atleast?
> > I can rebase them to pvops.
>
> so Shriram could create a branch that has both Stefano's (and also Ian's
> patches)
> and Rafeal's tree and you can use that. And when the merge window happens
> we would let Stefano's and Rafeal's tree go in and after that ask Linus to
> pull
> this complicated tree.
>
> That means you are going to have to create this hybrid tree. You can create
> a git branch
> from Stefano's tree, merge in Rafael's tree, and test it (baremetal and
> also Xen).
> Once you are satisfied that it works and it is bug-free:
>
>  a). send the tar ball to me (or just a link).
>
I can do either. I can also post a link to my git tree, if that would make
your life easier (in terms
of merging, etc).

>  b). I am going to extract that branch in my tree
>  c). You are going to post the patches that you had to develop so we can
> review
>     them.
>
Just to confirm, I should post these patches against my *hybrid* tree
because neither
Stefano's nor Rafael's tree are suitable. :(.

>  d). If everybody is happy and testing on our test-beds is OK, I can push
> this tree
>     later in the merge window cycle.
>
> so, when people test these patches, they would test it against your tree ?
(which would then
have incorporated my changes).

> It is a bit of dance but it ought to work out?
>
> shriram

[-- Attachment #1.2: Type: text/html, Size: 4112 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Konrad Rzeszutek Wilk]

> I can do either. I can also post a link to my git tree, if that would make
> your life easier (in terms
> of merging, etc).

Sure.
> 
> >  b). I am going to extract that branch in my tree
> >  c). You are going to post the patches that you had to develop so we can
> > review
> >     them.
> >
> Just to confirm, I should post these patches against my *hybrid* tree
> because neither
> Stefano's nor Rafael's tree are suitable. :(.

<nods>
> 
> >  d). If everybody is happy and testing on our test-beds is OK, I can push
> > this tree
> >     later in the merge window cycle.
> >
> > so, when people test these patches, they would test it against your tree ?
> (which would then
> have incorporated my changes).

Right. The idea is that I pull my tree, say #devel/next-2.6.38 and can
just do 'git pull <url>  pv-on-hvm-fixes' and it applies cleanly and I can
test it.

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Konrad Rzeszutek Wilk]

> > Is it okay if I send out both the HIBERNATION_INTERFACE patch and
> > the XEN_SAVE_RESTORE kconfig fixes against Rafael's tree?

Do they apply cleanly against's Rafael's tree?
> > 
> > Rafael's tree on git.kernel.org and Stefano tree on
> > http://xenbits.xen.org/gitweb/ are out of sync (on linux-next branch,
> > 2.6.38-rc6).
> > I am referring to files arch/x86/xen/Kconfig and kernel/power/Kconfig that
> > would be touched by these two patches.
> > 
> > Rafael's commit 5b56bff0f50bc1ed643c2ae85e803d17fc0a110e
> > "PM: Make CONFIG_PM depend on (CONFIG_PM_SLEEP || CONFIG_PM_RUNTIME)"
> > touches both these files and this commit is not present in stefano's tree.
> > 
> > The only issue is that I cannot completely "test" these two patches
> > against Rafael's tree
> >  - I have verified that the kernel config file generated is as expected.
> >  - I cannot verify any other xen save/restore functionality as my xen
> >    suspend freeze-thaw patches dont apply cleanly on Rafael's tree
> >   (it does not have xen suspend refactoring patches
> >   ceb180294790c8a6a437533488616f6b591b49d0, that my patches depend on.
> >   They are present only in Stefano's tree).

You could create a git tree where you merge Stefano's and Rafael's tree and
test on top of that.

Re: Q: Clarification about extra option ..Re: Re: [PATCH] pvops: Make suspend work when CONFIG_SUSPEND=n

[Rafael J. Wysocki]

On Friday, March 04, 2011, Konrad Rzeszutek Wilk wrote:
> .. snip..
> > >>  Someone suggested creating a new user visible hibernate symbol that would
> > >> solve this issue and make the main hibernate logic depend on this symbol rather
> > >> than the HIBERNATE symbol. I could certainly spin up a patch for that but nobody
> > >> seemed to have reached a conclusion.
> > >
> > > Please do. I was under the understanding that we were waiting for a victi^H^H^Hvolunteer
> > > to implement that.
> > >
> > > That was the only thing gatting your patchset going in.
> > >
> > >
> > 
> > I certainly would have long time ago but for this comment in the thread
> > "xen: fix XEN_SAVE_RESTORE Kconfig dependencies"
> > 
> > Rafael:
> >  I think we can introduce CONFIG_HIBERNATE_INTERFACE that will be user-visible
> > option instead of CONFIG_HIBERNATION and will select the latter.  Then,
> > CONFIG_XEN_SAVE_RESTORE will also be able to select CONFIG_HIBERNATION without
> > building the hibernate interface in, which will prevent user space from being
> > confused, but that will cause too much code to be built anyway.
> > 
> > If by "too much code to be built", he meant the increase in kernel
> > image size, then its not much of a deal :P.
> > But if he meant, "too much code rework", then it is an issue.
> 
> The idea here is that the /sys/power/state won't be exposed with the "disk"
> option.
> 
> > 
> > But IMO, the CONFIG_HIBERNATE_INTERFACE needs to go in,
> > only in the main hibernation initiator logic, as we still need the
> > CONFIG_HIBERNATE
> > pieces of every driver anyway (their freeze/thaw routines).
> 
> Right. The idea here is to seperate the sysfs interface to be behind
> another config option. So you can still enable the hibernate kernel code
> but without exposing it to the userland.
> 
> Rafael,
> 
> That is the general idea, right?

Right.  We can only make the code in kernel/power/snapshot.c depend on
CONFIG_HIBERNATE_INTERFACE and the ioctl interface too.

Thanks,
Rafael