[PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Ben Guthro]

This review is another S3 scheduler problem with the system_state variable introduced with the following changeset:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e819e5d5ce58eda5c5

Specifically, the cpu_callback function that takes the CPU down during suspend, and back up during resume.
We were seeing situations where, after S3, only CPU0 was in cpupool0. Guest performance suffered greatly, since all vcpus were only on a single pcpu. Guests under high CPU load showed the problem much more quickly than an idle guest.

Removing this if condition forces the CPUs to go through the expected online/offline state, and be properly scheduled after S3.

This also includes a necessary partial change proposed earlier by Tomasz Wroblewski here:
http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html

It should also resolve the issues discussed in this thread:
http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html

Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
---
 xen/common/cpu.c     |    3 +++
 xen/common/cpupool.c |    5 -----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 630881e..e20868c 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -5,6 +5,7 @@
 #include <xen/init.h>
 #include <xen/sched.h>
 #include <xen/stop_machine.h>
+#include <xen/sched-if.h>
 
 unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
 #ifndef nr_cpumask_bits
@@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
             BUG_ON(error == -EBUSY);
             printk("Error taking CPU%d up: %d\n", cpu, error);
         }
+        if (system_state == SYS_STATE_resume)
+            cpumask_set_cpu(cpu, cpupool0->cpu_valid);
     }
 
     cpumask_clear(&frozen_cpus);
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 10b10f8..a9653a8 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -633,10 +633,6 @@ static int cpu_callback(
     unsigned int cpu = (unsigned long)hcpu;
     int rc = 0;
 
-    if ( (system_state == SYS_STATE_suspend) ||
-         (system_state == SYS_STATE_resume) )
-        goto out;
-
     switch ( action )
     {
     case CPU_DOWN_FAILED:
@@ -650,7 +646,6 @@ static int cpu_callback(
         break;
     }
 
-out:
     return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
 }
 
-- 
1.7.9.5

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Juergen Gross]

On 09.04.2013 14:46, Ben Guthro wrote:
> This review is another S3 scheduler problem with the system_state variable introduced with the following changeset:
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e819e5d5ce58eda5c5
>
> Specifically, the cpu_callback function that takes the CPU down during suspend, and back up during resume.
> We were seeing situations where, after S3, only CPU0 was in cpupool0. Guest performance suffered greatly, since all vcpus were only on a single pcpu. Guests under high CPU load showed the problem much more quickly than an idle guest.
>
> Removing this if condition forces the CPUs to go through the expected online/offline state, and be properly scheduled after S3.
>
> This also includes a necessary partial change proposed earlier by Tomasz Wroblewski here:
> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html
>
> It should also resolve the issues discussed in this thread:
> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html
>
> Signed-off-by: Ben Guthro<benjamin.guthro@citrix.com>
> ---
>   xen/common/cpu.c     |    3 +++
>   xen/common/cpupool.c |    5 -----
>   2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index 630881e..e20868c 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -5,6 +5,7 @@
>   #include<xen/init.h>
>   #include<xen/sched.h>
>   #include<xen/stop_machine.h>
> +#include<xen/sched-if.h>
>
>   unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>   #ifndef nr_cpumask_bits
> @@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
>               BUG_ON(error == -EBUSY);
>               printk("Error taking CPU%d up: %d\n", cpu, error);
>           }
> +        if (system_state == SYS_STATE_resume)
> +            cpumask_set_cpu(cpu, cpupool0->cpu_valid);

This might solve YOUR problem, but reintroduces the problem why the original
change was done: ALL cpus will be in cpupool0 after resume!

So: NAK


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Ben Guthro]

How would you suggest this be solved then - because we are seeing that
all non-boot cpus are not being put in a pool at all - which is
clearly worse than being put into pool0



On Tue, Apr 9, 2013 at 8:57 AM, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
> On 09.04.2013 14:46, Ben Guthro wrote:
>>
>> This review is another S3 scheduler problem with the system_state variable
>> introduced with the following changeset:
>>
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e819e5d5ce58eda5c5
>>
>> Specifically, the cpu_callback function that takes the CPU down during
>> suspend, and back up during resume.
>> We were seeing situations where, after S3, only CPU0 was in cpupool0.
>> Guest performance suffered greatly, since all vcpus were only on a single
>> pcpu. Guests under high CPU load showed the problem much more quickly than
>> an idle guest.
>>
>> Removing this if condition forces the CPUs to go through the expected
>> online/offline state, and be properly scheduled after S3.
>>
>> This also includes a necessary partial change proposed earlier by Tomasz
>> Wroblewski here:
>> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html
>>
>> It should also resolve the issues discussed in this thread:
>> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html
>>
>> Signed-off-by: Ben Guthro<benjamin.guthro@citrix.com>
>> ---
>>   xen/common/cpu.c     |    3 +++
>>   xen/common/cpupool.c |    5 -----
>>   2 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
>> index 630881e..e20868c 100644
>> --- a/xen/common/cpu.c
>> +++ b/xen/common/cpu.c
>> @@ -5,6 +5,7 @@
>>   #include<xen/init.h>
>>   #include<xen/sched.h>
>>   #include<xen/stop_machine.h>
>> +#include<xen/sched-if.h>
>>
>>   unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>>   #ifndef nr_cpumask_bits
>> @@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
>>               BUG_ON(error == -EBUSY);
>>               printk("Error taking CPU%d up: %d\n", cpu, error);
>>           }
>> +        if (system_state == SYS_STATE_resume)
>> +            cpumask_set_cpu(cpu, cpupool0->cpu_valid);
>
>
> This might solve YOUR problem, but reintroduces the problem why the original
> change was done: ALL cpus will be in cpupool0 after resume!
>
> So: NAK
>
>
> Juergen
>
> --
> Juergen Gross                 Principal Developer Operating Systems
> PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 3222 2967
> Fujitsu Technology Solutions              e-mail:
> juergen.gross@ts.fujitsu.com
> Domagkstr. 28                           Internet: ts.fujitsu.com
> D-80807 Muenchen                 Company details:
> ts.fujitsu.com/imprint.html
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Juergen Gross]

On 09.04.2013 15:04, Ben Guthro wrote:
> How would you suggest this be solved then - because we are seeing that
> all non-boot cpus are not being put in a pool at all - which is
> clearly worse than being put into pool0

I proposed a patch before, which should have worked:

http://lists.xen.org/archives/html/xen-devel/2012-03/msg01843.html

Keir didn't like it and came up with the state variable...


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Jan Beulich]

>>> On 09.04.13 at 15:04, Ben Guthro <benjamin.guthro@citrix.com> wrote:
> How would you suggest this be solved then - because we are seeing that
> all non-boot cpus are not being put in a pool at all - which is
> clearly worse than being put into pool0

There ought to be a way for them to be put back where they
belong?

Plus so far I understood that this mishandling happens sometimes,
not always. Yet the change you propose would imply this didn't
ever work.

Jan

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[George Dunlap]

On Tue, Apr 9, 2013 at 2:04 PM, Ben Guthro <benjamin.guthro@citrix.com> wrote:
> How would you suggest this be solved then - because we are seeing that
> all non-boot cpus are not being put in a pool at all - which is
> clearly worse than being put into pool0

I don't think this is what you're actually seeing -- or if you are,
then there's a bug somewhere else that this is just triggering.

For the benefit of others: What they're seeing sometimes is that after
a suspend/resume cycle, certain pcpus don't generate any trace
records.  However, it is clear from the traces that *are* generated,
that it's not the case that nothing is happening on those invisible
pcpus; there is clear evidence of the vcpus running on visible pcpus
communicating with vcpus on the invisible pcpus.

But they are also sometimes seeing performance degradation that would
be consistent with pcpus not coming back up; but it's hard to be sure
because sometimes it's just tracing not working.

In any case, the "system state" patch doesn't work for dom0 affinity,
and the recent patch to generically save/restore vcpu affinity is one
step towards making it not needed anymore.  We should either:
* Revert the patch and save/restore stuff properly
* Special-case dom0 to save and re-pin on suspend/resume, since it's
still running
* Make sure that dom0 is handled some other way (e.g., by not
migrating vcpus which are paused)?

Looking at this patch -- it looks like the sched-state patch makes it
so that the scheduler cpu initialization stuff is not called on resume
-- e.g., tick_resume.  Am I reading that correctly?  That could be
part of the problem...

 -George




>
>
>
> On Tue, Apr 9, 2013 at 8:57 AM, Juergen Gross
> <juergen.gross@ts.fujitsu.com> wrote:
>> On 09.04.2013 14:46, Ben Guthro wrote:
>>>
>>> This review is another S3 scheduler problem with the system_state variable
>>> introduced with the following changeset:
>>>
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e819e5d5ce58eda5c5
>>>
>>> Specifically, the cpu_callback function that takes the CPU down during
>>> suspend, and back up during resume.
>>> We were seeing situations where, after S3, only CPU0 was in cpupool0.
>>> Guest performance suffered greatly, since all vcpus were only on a single
>>> pcpu. Guests under high CPU load showed the problem much more quickly than
>>> an idle guest.
>>>
>>> Removing this if condition forces the CPUs to go through the expected
>>> online/offline state, and be properly scheduled after S3.
>>>
>>> This also includes a necessary partial change proposed earlier by Tomasz
>>> Wroblewski here:
>>> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html
>>>
>>> It should also resolve the issues discussed in this thread:
>>> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html
>>>
>>> Signed-off-by: Ben Guthro<benjamin.guthro@citrix.com>
>>> ---
>>>   xen/common/cpu.c     |    3 +++
>>>   xen/common/cpupool.c |    5 -----
>>>   2 files changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
>>> index 630881e..e20868c 100644
>>> --- a/xen/common/cpu.c
>>> +++ b/xen/common/cpu.c
>>> @@ -5,6 +5,7 @@
>>>   #include<xen/init.h>
>>>   #include<xen/sched.h>
>>>   #include<xen/stop_machine.h>
>>> +#include<xen/sched-if.h>
>>>
>>>   unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>>>   #ifndef nr_cpumask_bits
>>> @@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
>>>               BUG_ON(error == -EBUSY);
>>>               printk("Error taking CPU%d up: %d\n", cpu, error);
>>>           }
>>> +        if (system_state == SYS_STATE_resume)
>>> +            cpumask_set_cpu(cpu, cpupool0->cpu_valid);
>>
>>
>> This might solve YOUR problem, but reintroduces the problem why the original
>> change was done: ALL cpus will be in cpupool0 after resume!
>>
>> So: NAK
>>
>>
>> Juergen
>>
>> --
>> Juergen Gross                 Principal Developer Operating Systems
>> PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 3222 2967
>> Fujitsu Technology Solutions              e-mail:
>> juergen.gross@ts.fujitsu.com
>> Domagkstr. 28                           Internet: ts.fujitsu.com
>> D-80807 Muenchen                 Company details:
>> ts.fujitsu.com/imprint.html
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Jan Beulich]

>>> On 09.04.13 at 17:30, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> Looking at this patch -- it looks like the sched-state patch makes it
> so that the scheduler cpu initialization stuff is not called on resume
> -- e.g., tick_resume.  Am I reading that correctly?  That could be
> part of the problem...

ISTR having mentioned this issue as a possible part of the resume
problems. The only missing piece for me is that tick_suspend doesn't
get called during suspend either (i.e. the only problem ought to be
that the tick may get delivered prematurely, which is what led me
to suggest to suppress full softirq handling from rcu_barrier_action()
when resuming, or perhaps more generically for all callers, i.e. in
process_pending_softirqs() itself). And we need to not forget that
tick_suspend()/tick_resume() are meant to be used by the idle
management code only at present, so using it for another purpose
could have other issues.

Jan

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Jan Beulich]

>>> On 09.04.13 at 14:46, Ben Guthro <benjamin.guthro@citrix.com> wrote:
> This review is another S3 scheduler problem with the system_state variable 
> introduced with the following changeset:
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e8 
> 19e5d5ce58eda5c5
> 
> Specifically, the cpu_callback function that takes the CPU down during 
> suspend, and back up during resume.
> We were seeing situations where, after S3, only CPU0 was in cpupool0. Guest 
> performance suffered greatly, since all vcpus were only on a single pcpu. 
> Guests under high CPU load showed the problem much more quickly than an idle 
> guest.
> 
> Removing this if condition forces the CPUs to go through the expected 
> online/offline state, and be properly scheduled after S3.

But this doesn't explain _why_ the code block you remove was
wrong. And that would be vital to understand, so we can be
reasonably sure this change won't lead to regressions elsewhere
again.

Jan

> This also includes a necessary partial change proposed earlier by Tomasz 
> Wroblewski here:
> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html 
> 
> It should also resolve the issues discussed in this thread:
> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html 
> 
> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
> ---
>  xen/common/cpu.c     |    3 +++
>  xen/common/cpupool.c |    5 -----
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index 630881e..e20868c 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -5,6 +5,7 @@
>  #include <xen/init.h>
>  #include <xen/sched.h>
>  #include <xen/stop_machine.h>
> +#include <xen/sched-if.h>
>  
>  unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>  #ifndef nr_cpumask_bits
> @@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
>              BUG_ON(error == -EBUSY);
>              printk("Error taking CPU%d up: %d\n", cpu, error);
>          }
> +        if (system_state == SYS_STATE_resume)
> +            cpumask_set_cpu(cpu, cpupool0->cpu_valid);
>      }
>  
>      cpumask_clear(&frozen_cpus);
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 10b10f8..a9653a8 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -633,10 +633,6 @@ static int cpu_callback(
>      unsigned int cpu = (unsigned long)hcpu;
>      int rc = 0;
>  
> -    if ( (system_state == SYS_STATE_suspend) ||
> -         (system_state == SYS_STATE_resume) )
> -        goto out;
> -
>      switch ( action )
>      {
>      case CPU_DOWN_FAILED:
> @@ -650,7 +646,6 @@ static int cpu_callback(
>          break;
>      }
>  
> -out:
>      return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
>  }
>  
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Ben Guthro]

On Tue, Apr 9, 2013 at 9:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 09.04.13 at 14:46, Ben Guthro <benjamin.guthro@citrix.com> wrote:
>> This review is another S3 scheduler problem with the system_state variable
>> introduced with the following changeset:
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e8
>> 19e5d5ce58eda5c5
>>
>> Specifically, the cpu_callback function that takes the CPU down during
>> suspend, and back up during resume.
>> We were seeing situations where, after S3, only CPU0 was in cpupool0. Guest
>> performance suffered greatly, since all vcpus were only on a single pcpu.
>> Guests under high CPU load showed the problem much more quickly than an idle
>> guest.
>>
>> Removing this if condition forces the CPUs to go through the expected
>> online/offline state, and be properly scheduled after S3.
>
> But this doesn't explain _why_ the code block you remove was
> wrong. And that would be vital to understand, so we can be
> reasonably sure this change won't lead to regressions elsewhere
> again.

I would argue that there has been so many problems with the original
changeset, that the argument should be in the other direction - since
this changeset that introduced the system_state variable, nobody has
been able to successfully suspend, as has been discussed in multiple
threads over the past year.

What is the reason that this particular callback gets bailed out of,
but not others?
Previously, the code worked, and went through this code path.
Why this one, in particular?

We have been systematically removing parts of the system_state
changeset, in regard to the S3 path. This is just another one that
puts it back to the way it was prior to that changeset (at least the
second hunk of it)

I'm open to other suggestions, but this was the only path that
explained the fact that all of the vcpus would end up on cpu0.

Ben

>
> Jan
>
>> This also includes a necessary partial change proposed earlier by Tomasz
>> Wroblewski here:
>> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html
>>
>> It should also resolve the issues discussed in this thread:
>> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html
>>
>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
>> ---
>>  xen/common/cpu.c     |    3 +++
>>  xen/common/cpupool.c |    5 -----
>>  2 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
>> index 630881e..e20868c 100644
>> --- a/xen/common/cpu.c
>> +++ b/xen/common/cpu.c
>> @@ -5,6 +5,7 @@
>>  #include <xen/init.h>
>>  #include <xen/sched.h>
>>  #include <xen/stop_machine.h>
>> +#include <xen/sched-if.h>
>>
>>  unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>>  #ifndef nr_cpumask_bits
>> @@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
>>              BUG_ON(error == -EBUSY);
>>              printk("Error taking CPU%d up: %d\n", cpu, error);
>>          }
>> +        if (system_state == SYS_STATE_resume)
>> +            cpumask_set_cpu(cpu, cpupool0->cpu_valid);
>>      }
>>
>>      cpumask_clear(&frozen_cpus);
>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>> index 10b10f8..a9653a8 100644
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -633,10 +633,6 @@ static int cpu_callback(
>>      unsigned int cpu = (unsigned long)hcpu;
>>      int rc = 0;
>>
>> -    if ( (system_state == SYS_STATE_suspend) ||
>> -         (system_state == SYS_STATE_resume) )
>> -        goto out;
>> -
>>      switch ( action )
>>      {
>>      case CPU_DOWN_FAILED:
>> @@ -650,7 +646,6 @@ static int cpu_callback(
>>          break;
>>      }
>>
>> -out:
>>      return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
>>  }
>>
>> --
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Jan Beulich]

>>> On 09.04.13 at 15:17, Ben Guthro <benjamin.guthro@citrix.com> wrote:
> On Tue, Apr 9, 2013 at 9:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 09.04.13 at 14:46, Ben Guthro <benjamin.guthro@citrix.com> wrote:
>>> Removing this if condition forces the CPUs to go through the expected
>>> online/offline state, and be properly scheduled after S3.
>>
>> But this doesn't explain _why_ the code block you remove was
>> wrong. And that would be vital to understand, so we can be
>> reasonably sure this change won't lead to regressions elsewhere
>> again.
> 
> I would argue that there has been so many problems with the original
> changeset, that the argument should be in the other direction - since
> this changeset that introduced the system_state variable, nobody has
> been able to successfully suspend, as has been discussed in multiple
> threads over the past year.
> 
> What is the reason that this particular callback gets bailed out of,
> but not others?
> Previously, the code worked, and went through this code path.
> Why this one, in particular?

You'd need to work this out with Juergen and Keir. I don't
really recall the details of the discussion they had back then.

> We have been systematically removing parts of the system_state
> changeset, in regard to the S3 path. This is just another one that
> puts it back to the way it was prior to that changeset (at least the
> second hunk of it)

But as said by Juergen - at the expense of breaking cpupools.

> I'm open to other suggestions, but this was the only path that
> explained the fact that all of the vcpus would end up on cpu0.

Again - one of my main concerns here is that so far I was
under the impression that this "all CPUs on pCPU0" was occurring
sometime, not always. And with that, I would expect the problem
to be a race of some sort, and the fix be dealing with a race.
Which isn't the case.

So the question that first needs answering is - was that impression
of mine wrong? If not, the next question would be whether you
spotted the race...

Jan

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Keir Fraser]

On 09/04/2013 14:17, "Ben Guthro" <benjamin.guthro@citrix.com> wrote:

> On Tue, Apr 9, 2013 at 9:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 09.04.13 at 14:46, Ben Guthro <benjamin.guthro@citrix.com> wrote:
>>> This review is another S3 scheduler problem with the system_state variable
>>> introduced with the following changeset:
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e8
>>> 19e5d5ce58eda5c5
>>> 
>>> Specifically, the cpu_callback function that takes the CPU down during
>>> suspend, and back up during resume.
>>> We were seeing situations where, after S3, only CPU0 was in cpupool0. Guest
>>> performance suffered greatly, since all vcpus were only on a single pcpu.
>>> Guests under high CPU load showed the problem much more quickly than an idle
>>> guest.
>>> 
>>> Removing this if condition forces the CPUs to go through the expected
>>> online/offline state, and be properly scheduled after S3.
>> 
>> But this doesn't explain _why_ the code block you remove was
>> wrong. And that would be vital to understand, so we can be
>> reasonably sure this change won't lead to regressions elsewhere
>> again.
> 
> I would argue that there has been so many problems with the original
> changeset, that the argument should be in the other direction - since
> this changeset that introduced the system_state variable, nobody has
> been able to successfully suspend, as has been discussed in multiple
> threads over the past year.

We could revert all the system_state patches and try Juergen's original
patch?

 -- Keir

> What is the reason that this particular callback gets bailed out of,
> but not others?
> Previously, the code worked, and went through this code path.
> Why this one, in particular?
> 
> We have been systematically removing parts of the system_state
> changeset, in regard to the S3 path. This is just another one that
> puts it back to the way it was prior to that changeset (at least the
> second hunk of it)
> 
> I'm open to other suggestions, but this was the only path that
> explained the fact that all of the vcpus would end up on cpu0.
> 
> Ben
> 
>> 
>> Jan
>> 
>>> This also includes a necessary partial change proposed earlier by Tomasz
>>> Wroblewski here:
>>> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html
>>> 
>>> It should also resolve the issues discussed in this thread:
>>> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html
>>> 
>>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
>>> ---
>>>  xen/common/cpu.c     |    3 +++
>>>  xen/common/cpupool.c |    5 -----
>>>  2 files changed, 3 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
>>> index 630881e..e20868c 100644
>>> --- a/xen/common/cpu.c
>>> +++ b/xen/common/cpu.c
>>> @@ -5,6 +5,7 @@
>>>  #include <xen/init.h>
>>>  #include <xen/sched.h>
>>>  #include <xen/stop_machine.h>
>>> +#include <xen/sched-if.h>
>>> 
>>>  unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>>>  #ifndef nr_cpumask_bits
>>> @@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
>>>              BUG_ON(error == -EBUSY);
>>>              printk("Error taking CPU%d up: %d\n", cpu, error);
>>>          }
>>> +        if (system_state == SYS_STATE_resume)
>>> +            cpumask_set_cpu(cpu, cpupool0->cpu_valid);
>>>      }
>>> 
>>>      cpumask_clear(&frozen_cpus);
>>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>>> index 10b10f8..a9653a8 100644
>>> --- a/xen/common/cpupool.c
>>> +++ b/xen/common/cpupool.c
>>> @@ -633,10 +633,6 @@ static int cpu_callback(
>>>      unsigned int cpu = (unsigned long)hcpu;
>>>      int rc = 0;
>>> 
>>> -    if ( (system_state == SYS_STATE_suspend) ||
>>> -         (system_state == SYS_STATE_resume) )
>>> -        goto out;
>>> -
>>>      switch ( action )
>>>      {
>>>      case CPU_DOWN_FAILED:
>>> @@ -650,7 +646,6 @@ static int cpu_callback(
>>>          break;
>>>      }
>>> 
>>> -out:
>>>      return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
>>>  }
>>> 
>>> --
>>> 1.7.9.5
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>> 
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume

[Ben Guthro]

On Tue, Apr 9, 2013 at 10:52 AM, Keir Fraser <keir.xen@gmail.com> wrote:
> On 09/04/2013 14:17, "Ben Guthro" <benjamin.guthro@citrix.com> wrote:
>
>> On Tue, Apr 9, 2013 at 9:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 09.04.13 at 14:46, Ben Guthro <benjamin.guthro@citrix.com> wrote:
>>>> This review is another S3 scheduler problem with the system_state variable
>>>> introduced with the following changeset:
>>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e8
>>>> 19e5d5ce58eda5c5
>>>>
>>>> Specifically, the cpu_callback function that takes the CPU down during
>>>> suspend, and back up during resume.
>>>> We were seeing situations where, after S3, only CPU0 was in cpupool0. Guest
>>>> performance suffered greatly, since all vcpus were only on a single pcpu.
>>>> Guests under high CPU load showed the problem much more quickly than an idle
>>>> guest.
>>>>
>>>> Removing this if condition forces the CPUs to go through the expected
>>>> online/offline state, and be properly scheduled after S3.
>>>
>>> But this doesn't explain _why_ the code block you remove was
>>> wrong. And that would be vital to understand, so we can be
>>> reasonably sure this change won't lead to regressions elsewhere
>>> again.
>>
>> I would argue that there has been so many problems with the original
>> changeset, that the argument should be in the other direction - since
>> this changeset that introduced the system_state variable, nobody has
>> been able to successfully suspend, as has been discussed in multiple
>> threads over the past year.
>
> We could revert all the system_state patches and try Juergen's original
> patch?

Apologies for my delay on responding to this - I have been traveling.



I think that reversion of the original patch should be considered, if
we want to have S3 work.

IMO - Severe regression of a long standing platform (S3) feature for
the introduction of a new one (cpupools) seems like a problem that
should be addressed by the author of the new feature, or that new
feature should be reverted.

Unfortunately, since Konrad's acpi-s3 patches are not upstream, it
makes it difficult to add to the automated tests to catch these early.


Ben


>
>  -- Keir
>
>> What is the reason that this particular callback gets bailed out of,
>> but not others?
>> Previously, the code worked, and went through this code path.
>> Why this one, in particular?
>>
>> We have been systematically removing parts of the system_state
>> changeset, in regard to the S3 path. This is just another one that
>> puts it back to the way it was prior to that changeset (at least the
>> second hunk of it)
>>
>> I'm open to other suggestions, but this was the only path that
>> explained the fact that all of the vcpus would end up on cpu0.
>>
>> Ben
>>
>>>
>>> Jan
>>>
>>>> This also includes a necessary partial change proposed earlier by Tomasz
>>>> Wroblewski here:
>>>> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html
>>>>
>>>> It should also resolve the issues discussed in this thread:
>>>> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html
>>>>
>>>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
>>>> ---
>>>>  xen/common/cpu.c     |    3 +++
>>>>  xen/common/cpupool.c |    5 -----
>>>>  2 files changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
>>>> index 630881e..e20868c 100644
>>>> --- a/xen/common/cpu.c
>>>> +++ b/xen/common/cpu.c
>>>> @@ -5,6 +5,7 @@
>>>>  #include <xen/init.h>
>>>>  #include <xen/sched.h>
>>>>  #include <xen/stop_machine.h>
>>>> +#include <xen/sched-if.h>
>>>>
>>>>  unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>>>>  #ifndef nr_cpumask_bits
>>>> @@ -212,6 +213,8 @@ void enable_nonboot_cpus(void)
>>>>              BUG_ON(error == -EBUSY);
>>>>              printk("Error taking CPU%d up: %d\n", cpu, error);
>>>>          }
>>>> +        if (system_state == SYS_STATE_resume)
>>>> +            cpumask_set_cpu(cpu, cpupool0->cpu_valid);
>>>>      }
>>>>
>>>>      cpumask_clear(&frozen_cpus);
>>>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>>>> index 10b10f8..a9653a8 100644
>>>> --- a/xen/common/cpupool.c
>>>> +++ b/xen/common/cpupool.c
>>>> @@ -633,10 +633,6 @@ static int cpu_callback(
>>>>      unsigned int cpu = (unsigned long)hcpu;
>>>>      int rc = 0;
>>>>
>>>> -    if ( (system_state == SYS_STATE_suspend) ||
>>>> -         (system_state == SYS_STATE_resume) )
>>>> -        goto out;
>>>> -
>>>>      switch ( action )
>>>>      {
>>>>      case CPU_DOWN_FAILED:
>>>> @@ -650,7 +646,6 @@ static int cpu_callback(
>>>>          break;
>>>>      }
>>>>
>>>> -out:
>>>>      return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
>>>>  }
>>>>
>>>> --
>>>> 1.7.9.5
>>>>
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xen.org
>>>> http://lists.xen.org/xen-devel
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel