From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Bader Subject: Re: Shutdown panic in disable_nonboot_cpus after cpupool-numa-split Date: Mon, 07 Jul 2014 16:08:29 +0200 Message-ID: <53BAA9DD.4040403@canonical.com> References: <53BA857A.8070608@canonical.com> <53BA8BD1.4020506@citrix.com> <53BA94D0.80201@suse.com> <53BA9773.6090004@canonical.com> <53BA9AB7.70105@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2960624632957078613==" Return-path: In-Reply-To: <53BA9AB7.70105@suse.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: =?ISO-8859-1?Q?J=FCrgen_Gro=DF?= , Andrew Cooper , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============2960624632957078613== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="LTE706lpt4RONEOqJoM8qrQxuEunitsFM" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --LTE706lpt4RONEOqJoM8qrQxuEunitsFM Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 07.07.2014 15:03, J=FCrgen Gro=DF wrote: > On 07/07/2014 02:49 PM, Stefan Bader wrote: >> On 07.07.2014 14:38, J=FCrgen Gro=DF wrote: >>> On 07/07/2014 02:00 PM, Andrew Cooper wrote: >>>> On 07/07/14 12:33, Stefan Bader wrote: >>>>> I recently noticed that I get a panic (rebooting the system) on sh= utdown in >>>>> some >>>> > cases. This happened only on my AMD system and also not all the = time. >>>> Finally >>>> > realized that it is related to the use of using cpupool-numa-spl= it >>>> > (libxl with xen-4.4 maybe, but not 100% sure 4.3 as well). >>>> > >>>> > What happens is that on shutdown the hypervisor runs >>>> disable_nonboot_cpus which >>>> > call cpu_down for each online cpu. There is a BUG_ON in the code= for >>>> the case of >>>> > cpu_down returning -EBUSY. This happens in my case as soon as th= e >>>> first cpu that >>>> > has been moved to pool-1 by cpupool-numa-split is attempted. The= error is >>>> > returned by running the notifier_call_chain and I suspect that e= nds >>>> up calling >>>> > cpupool_cpu_remove which always returns EBUSY for cpus not in po= ol0. >>>> > >>>> > I am not sure which end needs to be fixed but looping over all o= nline >>>> cpus in >>>> > disable_nonboot_cpus sounds plausible. So maybe the check for po= ol-0 in >>>> > cpupool_cpu_remove is wrong...? >>>> > >>>> > -Stefan >>>> >>>> Hmm yes - this looks completely broken. >>>> >>>> cpupool_cpu_remove() only has a single caller which is from cpu_down= (), >>>> and will unconditionally fail for cpus outside of the default pool. >>>> >>>> It is not obvious at all how this is supposed to work, and the comme= nt >>>> beside cpupool_cpu_remove() doesn't help. >>>> >>>> Can you try the following (only compile tested) patch, which looks >>>> plausibly like it might DTRT. The for_each_cpupool() is a little na= stly >>>> but there appears to be no cpu_to_cpupool mapping available. >>> >>> Your patch has the disadvantage to support hot-unplug of the last cpu= in >>> a cpupool. The following should work, however: >> >> Disadvantage and support sounded a bit confusing. But I think it means= >> hot-unplugging the last cpu of a pool is bad and should not be working= =2E >=20 > Correct. >=20 >> >>> >>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c >>> index 4a0e569..73249d3 100644 >>> --- a/xen/common/cpupool.c >>> +++ b/xen/common/cpupool.c >>> @@ -471,12 +471,24 @@ static void cpupool_cpu_add(unsigned int cpu) >>> */ >>> static int cpupool_cpu_remove(unsigned int cpu) >>> { >>> - int ret =3D 0; >>> + int ret =3D -EBUSY; >>> + struct cpupool **c; >>> >>> spin_lock(&cpupool_lock); >>> - if ( !cpumask_test_cpu(cpu, cpupool0->cpu_valid)) >>> - ret =3D -EBUSY; >>> + if ( cpumask_test_cpu(cpu, cpupool0->cpu_valid) ) >>> + ret =3D 0; >>> else >>> + { >>> + for_each_cpupool(c) >>> + { >>> + if ( cpumask_test_cpu(cpu, (*c)->cpu_suspended ) ) >> >> The rest seems to keep the semantics the same as before (though does t= hat mean >> unplugging the last cpu of pool-0 is ok?) But why testing for suspende= d here to >> succeed (and not valid)? >=20 > Testing valid would again enable to remove the last cpu of a cpupool in= > case of hotplugging. cpu_suspended is set if all cpus are to be removed= > due to shutdown, suspend to ram/disk, ... Ah, ok. Thanks for the detail explanation. So I was trying this change in= parallel and can confirm that it gets rid of the panic on shutdown. But w= hen I try to offline any cpu in pool1 (if echoing 0 into /sys/devices/xen_cpu/x= en_cpu? is the correct method) I always get EBUSY. IOW I cannot hot-unplug any cp= u that is in a pool other than 0. It does only work after removing it from pool1= , then add it to pool0 and then echo 0 into online. -Stefan >=20 > Juergen >=20 >> >> >>> + { >>> + ret =3D 0; >>> + break; >>> + } >>> + } >>> + } >>> + if ( !ret ) >>> cpumask_set_cpu(cpu, &cpupool_locked_cpus); >>> spin_unlock(&cpupool_lock); >>> >>> >>> >>> Juergen >=20 --LTE706lpt4RONEOqJoM8qrQxuEunitsFM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBCgAGBQJTuqndAAoJEOhnXe7L7s6jIAwQAME7FAaKJW3wN+snpocJFuiR BxgqPycrxXkFrqxTpv5iItNY9SSVg8LkCx3lPfnotd8/aqedEA8RuJPvaTmRgMCF XFbwd1qbLzdFeqXUzjOGY2u2gC9Gs3UFYm69XYSVJqzO5zoLgbdPeSvFRxvElLZ+ c2vORSCvLoBNQFJ7GThC55f+duB2omL+BWGJCewUrF/I5vjuEKeFgKbpI0dvMIyj eTR8un7PxKAANfOnc/wjKeGouBNJM7p3iaOdMkSitr5/h6oMYbbaNC03nxPOMLGE U5y3R4krroOXHgzol6jZd5bMr1m0tHageUf8FAzbIaQB1QsMiM9rqcPGFy+P0vJx cMvQHIJaaXbh87oOz2Ep55lm4cO+XOUk7vn0bo9GglMYPlxOgKDH3almmGxuq20u hnAnJirRPCPk6nZILl5MTyOcmmoVYjmu0HuVsKZM+3pBjkLgzPaTQiqZ82MJ5nQc rAbz+A/n1WPlZiM2igi/ntz/AcKczLuUW+E+EnJGHP+WnmlurTwJ9ENjZU3W6hhG kfllcxjt6FjBMIF+RP99aVTuBePnSKHjSh9/+Myga/QQ9s50ThjMJ20Gjw/9xyXX gbjSRxUEyObUMycSgEYMuVNDu9YKNycn9QE+dw6O7Hdkrp2yeDm8HbEmzPT+JoE8 7gj3lq1m3icm6/ID3eBz =+HwR -----END PGP SIGNATURE----- --LTE706lpt4RONEOqJoM8qrQxuEunitsFM-- --===============2960624632957078613== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2960624632957078613==--