From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Wroblewski Subject: Re: [PATCH v2] Fix scheduler crash after s3 resume Date: Thu, 24 Jan 2013 17:25:09 +0100 Message-ID: <51016065.3080902@citrix.com> References: <5100070F.7010808@citrix.com> <5100D229.4030906@ts.fujitsu.com> <510144A3.9060302@citrix.com> <5101630D02000078000B93AD@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5101630D02000078000B93AD@nat28.tlf.novell.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: Jan Beulich Cc: George Dunlap , Juergen Gross , "Keir (Xen.org)" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 24/01/13 16:36, Jan Beulich wrote: >>>> On 24.01.13 at 15:26, Tomasz Wroblewski wrote: >>>> >> @@ -212,6 +213,8 @@ >> 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 can't be right: What tells you that all CPUs were in pool 0? > > You're right, in my simple tests this was the case, but generally speaking it might not be.. Would an approach based on storing cpupool0 mask in disable_nonboot_cpus() and restoring it in enable_nonboot_cpus() be more acceptable? > Also, for the future - generating patches with -p helps quite > a bit in reviewing them. > > Ok, thanks! >> --- a/xen/common/schedule.c Mon Jan 21 17:03:10 2013 +0000 >> +++ b/xen/common/schedule.c Thu Jan 24 13:40:31 2013 +0000 >> @@ -545,7 +545,7 @@ >> int ret = 0; >> >> c = per_cpu(cpupool, cpu); >> - if ( (c == NULL) || (system_state == SYS_STATE_suspend) ) >> + if ( c == NULL ) >> return ret; >> >> for_each_domain_in_cpupool ( d, c ) >> @@ -556,7 +556,8 @@ >> >> cpumask_and(&online_affinity, v->cpu_affinity, c->cpu_valid); >> if ( cpumask_empty(&online_affinity)&& >> - cpumask_test_cpu(cpu, v->cpu_affinity) ) >> + cpumask_test_cpu(cpu, v->cpu_affinity)&& >> + system_state != SYS_STATE_suspend ) >> { >> printk("Breaking vcpu affinity for domain %d vcpu %d\n", >> v->domain->domain_id, v->vcpu_id); >> > I doubt this is correct, as you don't restore any of the settings > during resume that you tear down here. > > Is the objection about the affinity part or also the (c == NULL) bit? The cpu_disable_scheduler() function is currently part of a regular cpu down process, and was also part of suspend process before the "system state variable" changeset which regressed it. So the (c==NULL) hunk mostly just returns to previous state where this was working alot better (by empirical testing). But I am no expert on this, so would be grateful for ideas how this could be fixed in a better way! Just to recap, the current problem boils down, I believe, to the fact that vcpu_wake (schedule.c) function keeps getting called occasionally during the S3 path for cpus which have the per_cpu data freed, causing a crash. Safest way of fixing it seemed to be just put the suspend cpu_disable_scheduler under regular path again - it probably isn't the best..