From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v3 4/6] libxc: do some retries in xc_cpupool_removecpu() for EBUSY case Date: Tue, 8 Mar 2016 14:16:32 +0100 Message-ID: <1457442992.3102.210.camel@citrix.com> References: <1457023730-10997-1-git-send-email-jgross@suse.com> <1457023730-10997-5-git-send-email-jgross@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8169831536335718135==" Return-path: In-Reply-To: <1457023730-10997-5-git-send-email-jgross@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Juergen Gross , xen-devel@lists.xen.org Cc: Ian Jackson , Wei Liu , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org --===============8169831536335718135== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-gC+lnkWiePyQIbG5fEG0" --=-gC+lnkWiePyQIbG5fEG0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote: > The hypervisor might return EBUSY when trying to remove a cpu from a > cpupool when a domain running in this cpupool has pinned a vcpu > temporarily. Do some retries in this case, perhaps the situation > cleans up. >=20 I now I'm at high risk of being called nitpicker (or, more likely, much worse names), but I think that: > --- a/tools/libxc/xc_cpupool.c > +++ b/tools/libxc/xc_cpupool.c > @@ -20,8 +20,11 @@ > =C2=A0 */ > =C2=A0 > =C2=A0#include > +#include > =C2=A0#include "xc_private.h" > =C2=A0 > +#define LIBXC_BUSY_RETRIES 5 > + This name makes me think about something which wants to be more generic than =C2=A0it is actually the case... Like some number of retries that libx= c does in general, while it's only applicable to a very specific cpupool operation. Just something like CPUPOOL_NUM_REMOVECPU_RETRIES (or, maybe, even without the CPUPOOL_ prefix, as we're already inside cpupool.c) would be more appropriate. I'd also define it closer to=C2=A0xc_cpupool_removecpu() (but that is a lot about personal taste, I guess) and would add a brief comment (basically, a summary of what's in the changelog already), if only to save people having to go through `git blame'. > @@ -141,13 +144,21 @@ int xc_cpupool_removecpu(xc_interface *xch, > =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=A0uint32_t poolid, > =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=A0int cpu) > =C2=A0{ > +=C2=A0=C2=A0=C2=A0=C2=A0unsigned retries; > +=C2=A0=C2=A0=C2=A0=C2=A0int err; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0DECLARE_SYSCTL; > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sysctl.cmd =3D XEN_SYSCTL_cpupool_op; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sysctl.u.cpupool_op.op =3D XEN_SYSCTL_CPUPO= OL_OP_RMCPU; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sysctl.u.cpupool_op.cpupool_id =3D poolid; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sysctl.u.cpupool_op.cpu =3D (cpu < 0) ? XEN= _SYSCTL_CPUPOOL_PAR_ANY > : cpu; > -=C2=A0=C2=A0=C2=A0=C2=A0return do_sysctl_save(xch, &sysctl); > +=C2=A0=C2=A0=C2=A0=C2=A0for ( retries =3D 0; retries < LIBXC_BUSY_RETRIE= S; retries++ ) { > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0err =3D do_sysctl_save(x= ch, &sysctl); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( err >=3D 0 || errno= !=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= break; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sleep(1); > +=C2=A0=C2=A0=C2=A0=C2=A0} > Doing this the other way round (basically, exactly as the same thing is done in do_sysctl_save() already), reads, IMHO, more natural: =C2=A0for (...) { =C2=A0 =C2=A0err =3D do_sysctl_save(..); =C2=A0 =C2=A0if ( err < 0 && errno =3D=3D EBUSY ) =C2=A0 =C2=A0 =C2=A0sleep(1); =C2=A0 =C2=A0else =C2=A0 =C2=A0 =C2=A0break; =C2=A0} But yeah, this really is nitpicking. :-) Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-gC+lnkWiePyQIbG5fEG0 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 iEYEABECAAYFAlbe0LAACgkQk4XaBE3IOsQi2gCgkSgTSLLJOHvGOpx8mLcofrlZ E6YAn34HdmkpoXqHi7+nVE/Y2Z/VpKev =umAK -----END PGP SIGNATURE----- --=-gC+lnkWiePyQIbG5fEG0-- --===============8169831536335718135== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============8169831536335718135==--