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, 28 Jul 2014 10:36:13 +0200 Message-ID: <53D60B7D.1020104@canonical.com> References: <53BA857A.8070608@canonical.com> <53BA8BD1.4020506@citrix.com> <53BA94D0.80201@suse.com> <53BA9773.6090004@canonical.com> <53BA9AB7.70105@suse.com> <53BAA9DD.4040403@canonical.com> <53BAAE82.2090200@suse.com> <53BAB20C.1020409@canonical.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0130623126567728960==" Return-path: In-Reply-To: <53BAB20C.1020409@canonical.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: Juergen Gross , 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) --===============0130623126567728960== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="GssXv47n72h8kDGIWJRq7DRMUd1N6d1ip" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --GssXv47n72h8kDGIWJRq7DRMUd1N6d1ip Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 07.07.2014 16:43, Stefan Bader wrote: > On 07.07.2014 16:28, Juergen Gross wrote: >> On 07/07/2014 04:08 PM, Stefan Bader wrote: >>> 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= shutdown 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= -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 for >>>>>>> the case of >>>>>>> > cpu_down returning -EBUSY. This happens in my case as soon a= s the >>>>>>> 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 th= at ends >>>>>>> up calling >>>>>>> > cpupool_cpu_remove which always returns EBUSY for cpus not i= n pool0. >>>>>>> > >>>>>>> > I am not sure which end needs to be fixed but looping over a= ll online >>>>>>> cpus in >>>>>>> > disable_nonboot_cpus sounds plausible. So maybe the check fo= r 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_d= own(), >>>>>>> and will unconditionally fail for cpus outside of the default poo= l. >>>>>>> >>>>>>> It is not obvious at all how this is supposed to work, and the co= mment >>>>>>> beside cpupool_cpu_remove() doesn't help. >>>>>>> >>>>>>> Can you try the following (only compile tested) patch, which look= s >>>>>>> plausibly like it might DTRT. The for_each_cpupool() is a little= nastly >>>>>>> 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 me= ans >>>>> hot-unplugging the last cpu of a pool is bad and should not be work= ing. >>>> >>>> Correct. >>>> >>>>> >>>>>> >>>>>> 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 doe= s that mean >>>>> unplugging the last cpu of pool-0 is ok?) But why testing for suspe= nded here to >>>>> succeed (and not valid)? >>>> >>>> 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 remo= ved >>>> due to shutdown, suspend to ram/disk, ... >>> >>> Ah, ok. Thanks for the detail explanation. So I was trying this chang= e in >>> parallel and can confirm that it gets rid of the panic on shutdown. B= ut when I >>> try to offline any cpu in pool1 (if echoing 0 into /sys/devices/xen_c= pu/xen_cpu? >>> is the correct method) I always get EBUSY. IOW I cannot hot-unplug an= y cpu that >>> is in a pool other than 0. It does only work after removing it from p= ool1, then >>> add it to pool0 and then echo 0 into online. >> >> That's how it was designed some years ago. I don't want to change the >> behavior in the hypervisor. Adding some tool support could make sense,= >> however. >=20 > Ok, so in that case everything works as expected and the change fixes t= he > currently broken shutdown and could be properly submitted for inclusion= (with my > tested-by). >=20 Is this needing something from my side to do? I could re-submit the whole= patch but it since it is Juergen's work it felt a little rude to do so. -Stefan --GssXv47n72h8kDGIWJRq7DRMUd1N6d1ip 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 iQIcBAEBCgAGBQJT1gt9AAoJEOhnXe7L7s6jK8UP/0Af5Uiw5VbZ1mXhYS4Mfekk cnbw6jaHakgCBNiuphJLb7PN96tHGwBvfjXkGrxEosE4YXZ1cX8S8d00VQywfMfa hWpzNX3MyObF7tcvQPU6CbyDCRhkTydtsH6Zv+PQXQ4qLOXIEIGi3gXAmTQ2001V +CcsadeYVnoM+OxPxhN0sBB4xQPom5pwwA7M1gJVDzydeMKPMtIAB944q9KD3YcQ 7Hpt7mAtrPkS0OYQBJZslYBHbhcl9fBxspnpbVqOlmubKH0LvarDi9ruNzgmcaP8 ojnG67yvVjUbsIMyQ6zOPPVvcbqmO/Bf8BoazswJkMatrGjtbJnmOAbUZAI/XN7M irjSiYyrizuuioeEdTqXaCWHsQQLl4GY1/wmQtwWJ0mg8LUPWM7YsfyQLJukmxuK B+kcJ++9ZTpacIiyJgtPPTHlwaHGNc6swQk+Ivk3+cXQIBfLh3sFcqDu2gZgZ7Q9 ykOWtFohtu0pzy5cFhuZtimXbht6jjFCHRL8zC0xNVLC4J5qX9enGpKBoKHZxivz AuEE8HU53hJOXbUGRxwokEGqgAugdsCERhHT/p1a+xG5KLRNniRe2HeTIdKa1Qm1 YQYFMhyxtcGw2LkSK8rq5AZS20TpEpk+69P2miuDR1ZUGnRMXzeQY8LrtvogZU+m 94TxD7kpgyhJxc6MR7Ap =CDuU -----END PGP SIGNATURE----- --GssXv47n72h8kDGIWJRq7DRMUd1N6d1ip-- --===============0130623126567728960== 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 --===============0130623126567728960==--