From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume Date: Tue, 09 Apr 2013 14:57:02 +0200 Message-ID: <5164101E.6010907@ts.fujitsu.com> References: <1365511601-31522-1-git-send-email-benjamin.guthro@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1365511601-31522-1-git-send-email-benjamin.guthro@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ben Guthro Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > --- > 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 > #include > #include > +#include > > 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