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 14:49:55 +0200 Message-ID: <53BA9773.6090004@canonical.com> References: <53BA857A.8070608@canonical.com> <53BA8BD1.4020506@citrix.com> <53BA94D0.80201@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7734455041388110224==" Return-path: In-Reply-To: <53BA94D0.80201@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) --===============7734455041388110224== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="pcnbDbhN6LXxWwja4Kup4IP3WtjnxK6UQ" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --pcnbDbhN6LXxWwja4Kup4IP3WtjnxK6UQ Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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 shut= down in >>> some >> > cases. This happened only on my AMD system and also not all the tim= e. >> Finally >> > realized that it is related to the use of using cpupool-numa-split >> > (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 fo= r >> the case of >> > cpu_down returning -EBUSY. This happens in my case as soon as the >> first cpu that >> > has been moved to pool-1 by cpupool-numa-split is attempted. The er= ror is >> > returned by running the notifier_call_chain and I suspect that ends= >> up calling >> > cpupool_cpu_remove which always returns EBUSY for cpus not in pool0= =2E >> > >> > I am not sure which end needs to be fixed but looping over all onli= ne >> cpus in >> > disable_nonboot_cpus sounds plausible. So maybe the check for pool-= 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 comment= >> 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 nast= ly >> but there appears to be no cpu_to_cpupool mapping available. >=20 > Your patch has the disadvantage to support hot-unplug of the last cpu i= n > 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. >=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; >=20 > 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 that= mean unplugging the last cpu of pool-0 is ok?) But why testing for suspended h= ere to succeed (and not valid)? > + { > + ret =3D 0; > + break; > + } > + } > + } > + if ( !ret ) > cpumask_set_cpu(cpu, &cpupool_locked_cpus); > spin_unlock(&cpupool_lock); >=20 >=20 >=20 > Juergen --pcnbDbhN6LXxWwja4Kup4IP3WtjnxK6UQ 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/ iQIcBAEBCgAGBQJTupdzAAoJEOhnXe7L7s6js24P/Rt06howGLpcPzwZP5sqb+vX lf6TPGnpToBrGO4W+WurjrvH6ikLDNi2vJZB+4pjamUjshJuQrycK4Yx0s3VA3Q6 +swdmN3MKhCJzxV/VzDeiCL6JDjIBz5e19AI1PIzLbp0BpDlJg02qDiLrnY6/6Tc 4iW4e6idq8Ob0L7hB4R+QxTWJTI3RTEGL231Gzv2uKYVsanyHXl15UZcOjPIwB1f Y9LXJ0OpV4uLq0wAJ3KZ8Rwd+DIhQVhP6L532vcmrKLlZBbtsw2BlWsabHHqEUIX 1/j9zkgG+LJ8gtQAc8rIV35Bmr7clUnIp4dt2O3r93TbeV0zmMhVGIOJVuTojmZC Yd0PFP0anqqfiuW7Qgx10ApzXsPwHZ2zJT49k8zEIi/PKxrY6LDd6KlHkJ1wllze hpkVjOt5wyyf6lxZZyh4YD2onXoV4icIIwU7JUMrst84cYHQ9qlbNvCWHyy5rU1z Q+nKqQwgFJ4S2a+vi7IuYeQn+AkZO7hMNvb6eA9VIJxy5hGFu9tIFCRdblxWiHNt QRRZTRFq7VRYHv16YF1tXsm3M/RhT8UWN2e8vwDwudOx011wYhbo9LF03ZGq4rnn SWHKCre8q6zxgd+pFTBS3mJSfxKyb+2LZmfh+MSkhdHI7JRH61WVj3j8Wap51pMX koSuGmBIrtHWfh7sylTI =Rzpx -----END PGP SIGNATURE----- --pcnbDbhN6LXxWwja4Kup4IP3WtjnxK6UQ-- --===============7734455041388110224== 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 --===============7734455041388110224==--