From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH v2 1/2] xen: sched: reorganize cpu_disable_scheduler() Date: Mon, 20 Jul 2015 13:41:49 +0200 Message-ID: <55ACDE7D.50108@suse.com> References: <20150717133013.29612.53960.stgit@Solace.station> <20150717133554.29612.14835.stgit@Solace.station> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZH9SC-0001GT-H0 for xen-devel@lists.xenproject.org; Mon, 20 Jul 2015 11:41:52 +0000 In-Reply-To: <20150717133554.29612.14835.stgit@Solace.station> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli , xen-devel@lists.xenproject.org Cc: George Dunlap List-Id: xen-devel@lists.xenproject.org On 07/17/2015 03:35 PM, Dario Faggioli wrote: > The function is called both when we want to remove a cpu > from a cpupool, and during cpu teardown, for suspend or > shutdown. If, however, the boot cpu (cpu 0, most of the > times) is not present in the default cpupool, during > suspend or shutdown, Xen crashes like this: > > root@Zhaman:~# xl cpupool-cpu-remove Pool-0 0 > root@Zhaman:~# shutdown -h now > (XEN) ----[ Xen-4.6-unstable x86_64 debug=y Tainted: C ]---- > ... > (XEN) Xen call trace: > (XEN) [] _csched_cpu_pick+0x156/0x61f > (XEN) [] csched_cpu_pick+0xe/0x10 > (XEN) [] vcpu_migrate+0x18e/0x321 > (XEN) [] cpu_disable_scheduler+0x1cf/0x2ac > (XEN) [] __cpu_disable+0x313/0x36e > (XEN) [] take_cpu_down+0x34/0x3b > (XEN) [] stopmachine_action+0x70/0x99 > (XEN) [] do_tasklet_work+0x78/0xab > (XEN) [] do_tasklet+0x5e/0x8a > (XEN) [] idle_loop+0x56/0x6b > (XEN) > (XEN) > (XEN) **************************************** > (XEN) Panic on CPU 15: > (XEN) Assertion 'cpu < nr_cpu_ids' failed at ...URCES/xen/xen/xen.git/xen/include/xen/cpumask.h:97 > (XEN) **************************************** > > There also are problems when we try to suspend or shutdown > with a cpupool configured with just one cpu (no matter, in > this case, whether that is the boot cpu or not): > > root@Zhaman:~# xl create /etc/xen/test.cfg > root@Zhaman:~# xl cpupool-migrate test Pool-1 > root@Zhaman:~# xl cpupool-list -c > Name CPU list > Pool-0 0,1,2,3,4,5,6,7,8,9,10,11,13,14,15 > Pool-1 12 > root@Zhaman:~# shutdown -h now > (XEN) ----[ Xen-4.6-unstable x86_64 debug=y Tainted: C ]---- > (XEN) CPU: 12 > ... > (XEN) Xen call trace: > (XEN) [] __cpu_disable+0x317/0x36e > (XEN) [] take_cpu_down+0x34/0x3b > (XEN) [] stopmachine_action+0x70/0x99 > (XEN) [] do_tasklet_work+0x78/0xab > (XEN) [] do_tasklet+0x5e/0x8a > (XEN) [] idle_loop+0x56/0x6b > (XEN) > (XEN) > (XEN) **************************************** > (XEN) Panic on CPU 12: > (XEN) Xen BUG at smpboot.c:895 > (XEN) **************************************** > > In both cases, the problem is the scheduler not being able > to: > - move all the vcpus to the boot cpu (as the boot cpu is > not in the cpupool), in the former; > - move the vcpus away from a cpu at all (as that is the > only one cpu in the cpupool), in the latter. > > Solution is to distinguish, inside cpu_disable_scheduler(), > the two cases of cpupool manipulation and teardown. For > cpupool manipulation, it is correct to ask the scheduler to > take an action, as pathological situation (like there not > being any cpu in the pool where to send vcpus) are taken > care of (i.e., forbidden!) already. For suspend and shutdown, > we don't want the scheduler to be involved at all, as the > final goal is pretty simple: "send all the vcpus to the > boot cpu ASAP", so we just go for it. > > Signed-off-by: Dario Faggioli > --- > Changes from v1: > * BUG_ON() if, in the suspend/shutdown case, the mask of > online pCPUs will ever get empty, as suggested during > review; > * reorganize and improve comments inside cpu_disable_scheduler() > as suggested during review; > * make it more clear that vcpu_move_nosched() (name > changed, as suggested during review), should only be > called from "quite contextes", such us, during suspend s/quite/quiet/ > or shutdown. Do that via both comments and asserts, > as requested during review; > * reorganize cpu_disable_scheduler() and vcpu_move_nosched() > so that calling to sleep and wakeup functions are only > called when necessary (i.e., *not* in case we are > suspending/shutting down, as requested during review. > --- > Cc: George Dunlap > Cc: Juergen Gross > --- > xen/common/schedule.c | 102 +++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 86 insertions(+), 16 deletions(-) > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index df8c1d0..ed0f356 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c ... > @@ -644,25 +673,66 @@ int cpu_disable_scheduler(unsigned int cpu) > cpumask_setall(v->cpu_hard_affinity); > } > > - if ( v->processor == cpu ) > + if ( v->processor != cpu ) > { > - set_bit(_VPF_migrating, &v->pause_flags); > + /* This vcpu is not on cpu, so we can move on. */ > vcpu_schedule_unlock_irqrestore(lock, flags, v); > - vcpu_sleep_nosync(v); > - vcpu_migrate(v); > + continue; > + } > + > + /* If it is on cpu, we must send it away. */ > + if ( unlikely(system_state == SYS_STATE_suspend) ) > + { > + /* > + * If we are doing a shutdown/suspend, it is not necessary to > + * ask the scheduler to chime in. In fact: > + * * there is no reason for it: the end result we are after > + * is just 'all the vcpus on the boot pcpu, and no vcpu > + * anywhere else', so let's just go for it; > + * * it's wrong, for cpupools with only non-boot pcpus, as > + * the scheduler would always fail to send the vcpus away > + * from the last online (non boot) pcpu! > + * > + * Therefore, in the shutdown/suspend case, we just pick up > + * one (still) online pcpu. Note that, at this stage, all > + * domains (including dom0) have been paused already, so we > + * do not expect any vcpu activity at all. > + */ > + cpumask_andnot(&online_affinity, &cpu_online_map, > + cpumask_of(cpu)); > + BUG_ON(cpumask_empty(&online_affinity)); > + /* > + * As boot cpu is, usually, pcpu #0, using cpumask_first() > + * will make us converge quicker. > + */ > + new_cpu = cpumask_first(&online_affinity); > + vcpu_move_nosched(v, new_cpu); Shouldn't there be a vcpu_schedule_unlock_irqrestore() ? Juergen