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

[Ben Guthro <benjamin.guthro@xxxxxxxxxx>]

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@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>   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@xxxxxxxxxxxxxx
> Domagkstr. 28                           Internet: ts.fujitsu.com
> D-80807 Muenchen                 Company details:
> ts.fujitsu.com/imprint.html
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel