From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu Date: Wed, 2 Mar 2016 10:27:33 +0100 Message-ID: <1456910853.2959.286.camel@citrix.com> References: <1456822933-25041-1-git-send-email-jgross@suse.com> <1456822933-25041-3-git-send-email-jgross@suse.com> <56D5BACD.1030502@citrix.com> <56D692C3.4010509@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2136903544224182344==" Return-path: In-Reply-To: <56D692C3.4010509@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Juergen Gross , George Dunlap , xen-devel@lists.xen.org Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com, george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, david.vrabel@citrix.com, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============2136903544224182344== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-vMYVAkiv+cP8gg2xV3Hk" --=-vMYVAkiv+cP8gg2xV3Hk Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2016-03-02 at 08:14 +0100, Juergen Gross wrote: > On 01/03/16 16:52, George Dunlap wrote: > >=C2=A0 > >=20 > > Also -- have you actually tested the "cpupool move while pinned" > > functionality to make sure it actually works?=C2=A0=C2=A0There's a weir= d bit > > in > > cpupool_unassign_cpu_helper() where after calling > > cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in > > the > > cpupool_free_cpus mask, even if it returns an error.=C2=A0=C2=A0That ca= n't be > > right, even for the existing -EAGAIN case, can it? > That should be no problem. Such a failure can be repaired easily by > adding the cpu to the cpupool again.=20 > And there's not much else one can do, I would say. When we are in cpu_disable_scheduler(), coming from cpupool_unassign_cpu()-->cpupool_unassign_cpu() we're already halfway through removing the cpu from the pool (e.g., we already cleared the relevant bit from the cpupool's cpu_valid mask). And we don't actually want to revert that, as doing so would allow the scheduler to start again moving vcpus to that cpu (and the following attempts will risk failing with EAGAIN again :-D). FWIW, I've also found that part rather weird for quite some time... But it does indeed makes sense, IMO. > Adding a comment seems to be a > good idea. :-) >=20 Yep. Should we also add an error message for the user to be able to see it, even if she can't read the comment in the source code? (Not necessarily right there, if that would make it trigger too much... just in a place where it can be seen in the case the user actually need to do something). > What is wrong and even worse, schedule_cpu_switch() returning an > error > will leak domlist_read_lock.=20 > Indeed, good catch. :-) > > And, in general, what happens if the device driver gets mixed up > > and > > forgets to unpin the vcpu?=C2=A0=C2=A0Is the only recourse to reboot yo= ur > > host (or > > deal with the fact that you can't reconfigure your cpupools)? > Unless we add a "forced" option to "xl vcpu-pin", yes. >=20 Which would be fine to have, IMO. I'm not sure if it would better be an `xl vcpu-pin' flag, or a separate utility (as Jan is also saying). A separate utility would fit better the "emergency nature" of the thing, avoiding having to clobber xl for that (as this will be the only, pretty uncommon, case where such flag would be needed). However, an xl flag is easier to add, easier to document and easier and more natural to find, from the point of view of an user that really needs it. And perhaps it could turn out useful for other situations in future. So, I guess I'd say: =C2=A0- yes, let's add that =C2=A0- let's do it as a "force flag" of `xl vcpu-pin'. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-vMYVAkiv+cP8gg2xV3Hk 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 iEYEABECAAYFAlbWsgUACgkQk4XaBE3IOsSJiwCgm+NhqzN6a+u6MOsN1Lthdctj kLsAoJoAGAX9MjDNSJZfd8yopSH7VytM =1oql -----END PGP SIGNATURE----- --=-vMYVAkiv+cP8gg2xV3Hk-- --===============2136903544224182344== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============2136903544224182344==--