From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] xen: add hypercall option to temporarily pin a vcpu Date: Fri, 26 Feb 2016 12:20:32 +0100 Message-ID: <1456485632.2959.154.camel@citrix.com> References: <1456419000-390-1-git-send-email-jgross@suse.com> <56D0395702000078000D69A6@suse.com> <56D033A8.7020009@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8946028772194526359==" Return-path: In-Reply-To: <56D033A8.7020009@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Juergen Gross , Jan Beulich Cc: george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, dgdegra@tycho.nsa.gov, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============8946028772194526359== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-jSUsRsB0G+3TZeohjWGE" --=-jSUsRsB0G+3TZeohjWGE Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-02-26 at 12:14 +0100, Juergen Gross wrote: > On 26/02/16 11:39, Jan Beulich wrote: > >=C2=A0 > > > @@ -670,7 +676,13 @@ int cpu_disable_scheduler(unsigned int cpu) > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0if ( cpumask_empty(&online_affinity) && > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_test_cpu(cpu, v->cpu_hard_af= finity) ) > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0{ > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0printk(XENLOG_DEBUG "Breaking affinity for > > > %pv\n", v); > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( v->affinity_broken ) > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* The vcpu is tempor= arily pinned, can't > > > move it. */ > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vcpu_schedule_unlock_= irqrestore(lock, flags, > > > v); > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D -EBUSY; > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0continue; > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > So far the function can only return 0 or -EAGAIN. By using > > "continue" > > here you will make it impossible for the caller to reliably > > determine > > whether possibly both things failed. Despite -EBUSY being a logical > > choice here, I think you'd better use -EAGAIN here too. And it > > needs > > to be determined whether continuing the loop in this as well as the > > pre-existing cases is actually the right thing to do. > EBUSY vs. EAGAIN: by returning EAGAIN I would signal to Xen tools > that > the hypervisor is currently not able to do the desired operation > (especially removing a cpu from a cpupool), but the situation will > change automatically via scheduling. EBUSY will stop retries in Xen > tools and this is want I want here: I can't be sure the situation > will change soon. >=20 I agree with this. > Regarding continuation of the loop: I think you are right in the > EBUSY case: I should break out of the loop. I should not do so in the > EAGAIN case as I want to remove as many vcpus from the physical cpu > as > possible without returning to the Xen tools in between. >=20 And with this too. And I think that, if we indeed break out of the loop on EBUSY, that will also make it possible to figure out properly what actually went wrong, so it should be fine from that point of view as well. > > > @@ -679,6 +691,8 @@ int cpu_disable_scheduler(unsigned int cpu) > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0v->affinity_broken= =3D 1; > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > =C2=A0 > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0printk(XENLOG_DEBUG "Breaking affinity for > > > %pv\n", v); > > Wouldn't it be even better to make this the "else" to the > > preceding if(), since in the suspend case this is otherwise going > > to be printed for every vCPU not currently running on pCPU0? > Yes, I'll change it. >=20 On this, can (either of) you elaborate a bit more? I don't think I'm following... Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-jSUsRsB0G+3TZeohjWGE Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlbQNQEACgkQk4XaBE3IOsSKYgCdEdFMqy5NIvmx8ZdwJzSE0DDp w00An17wePadYKanDDxXa5CoNvBPPj4r =bdEO -----END PGP SIGNATURE----- --=-jSUsRsB0G+3TZeohjWGE-- --===============8946028772194526359== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============8946028772194526359==--