From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] libxl: avoid considering pCPUs outside of the cpupool during NUMA placement Date: Fri, 21 Oct 2016 14:52:05 +0200 Message-ID: <1477054325.24930.112.camel@citrix.com> References: <147704377421.10420.14327289650457148893.stgit@Solace.fritz.box> <20161021102923.GT2639@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1555427833655695363==" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bxZJ6-0000MR-Cp for xen-devel@lists.xenproject.org; Fri, 21 Oct 2016 12:52:20 +0000 In-Reply-To: <20161021102923.GT2639@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Wei Liu Cc: Juergen Gross , xen-devel@lists.xenproject.org, Anshul Makkar , Ian Jackson , George Dunlap List-Id: xen-devel@lists.xenproject.org --===============1555427833655695363== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-lGBQdsu0lkx0lE5aFXic" --=-lGBQdsu0lkx0lE5aFXic Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-10-21 at 11:29 +0100, Wei Liu wrote: > On Fri, Oct 21, 2016 at 11:56:14AM +0200, Dario Faggioli wrote: > > diff --git a/tools/libxl/libxl_numa.c b/tools/libxl/libxl_numa.c > > index 33289d5..f2a719d 100644 > > --- a/tools/libxl/libxl_numa.c > > +++ b/tools/libxl/libxl_numa.c > > @@ -186,9 +186,12 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, > > libxl_cputopology *tinfo, > > =C2=A0{ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0libxl_dominfo *dinfo =3D NULL; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0libxl_bitmap dom_nodemap, nodes_counted;= > > +=C2=A0=C2=A0=C2=A0=C2=A0libxl_cpupoolinfo cpupool_info; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int nr_doms, nr_cpus; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int i, j, k; > > =C2=A0 > > +=C2=A0=C2=A0=C2=A0=C2=A0libxl_cpupoolinfo_init(&cpupool_info); > > + >=20 > Please move this into the loop below, see (*). >=20 Seems unnecessary, but I certainly can do that. I guess it makes the code less dependent from the actual implementation of=C2=A0libxl_cpupoolinfo_dispose(), which is probably a good thing. > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dinfo =3D libxl_list_domain(CTX, &nr_dom= s); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (dinfo =3D=3D NULL) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ERROR_FAI= L; > > @@ -205,12 +208,18 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, > > libxl_cputopology *tinfo, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > =C2=A0 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for (i =3D 0; i < nr_doms; i++) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0libxl_vcpuinfo *vinfo= ; > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int nr_dom_vcpus; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0libxl_vcpuinfo *vinfo= =3D NULL; >=20 > This is not necessary because vinfo is written right away. >=20 Is it? It was before this patch, but with it, if this [*] fails... > >=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int cpupool, nr_dom_v= cpus; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpupool =3D libxl__do= main_cpupool(gc, dinfo[i].domid); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (cpupool < 0) [*] > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0goto next; ... we go to next which does=C2=A0libxl_vcpuinfo_list_free() on a non- initialised pointer > > @@ -236,7 +252,10 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, > > libxl_cputopology *tinfo, > > =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 > > + next: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0libxl_cpupoolinfo_dis= pose(&cpupool_info); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0libxl_vcpuinfo_l= ist_free(vinfo, nr_dom_vcpus); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vinfo =3D NULL; >=20 > This is not necessary as vinfo is rewritten at the beginning of every > loop. Actually, I do agree that this is not necessary, iff I keep the assignment there, at the beginning of the loop... or am I missing something? 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) --=-lGBQdsu0lkx0lE5aFXic 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 iQIcBAABCAAGBQJYCg92AAoJEBZCeImluHPuc84QALlFY762bsqb1x7iLvdqe0KT rfV1V3SK/os3UxyECDp4jD7QvByEqlTOPZDZ5Xa/zVcriiFe46g28MTsgEeKV/VW EA+RQW1oOKWpDsIUJpLpOmBKS3QmUe6SlYr9Ocuce/Pj7qNFQ1NEDGDDPVl9Mmr0 GFGDABl8O1gTYoKGVlI08TJu6cqjIp5vCOKZIarCL/+xWdESj+FXW18wj8J2sDN3 Lf8Fac/aWVTC1VTW62q28hF7yupZGXfJgfDYuqIF9eJISWWkhVVhiF16HVQbjTCA 51qp2wrWsZtXd9hbJf0wUflXbOhLdAQYphKcz7RBuSKQqcBKWOLaetNTZ4HRZhpa lV4WrVRsConuZoSc2vP6BJSFf/MTFynSNnRA9VlGfoH84g2y3B8yRUtPTyYzA7Kl 6V+FCkFsbFUWnCfj/uBXwwSxkjutEO7fav9KGnU/LWxcfLMVH+TORtkqEmHX7sXH O9BvgosrdCe+8wY2k9NXuT55KW4WDJxvP02R40Kdsf+Xw2IaggnbQbWNqjqGN/8w l1G5z/15+dNNNoOV7EPET/dS4wFr2Sc/6F27nmmuJlKLbwmEnzghh1TqkNagoHQw 8+hETaFD+6NV54KKWDsO81dkSihplcst6ewq+z9Ev+738EnLinCj9KpwyI5d4N6m /KzVogZD0bqZiV3xTwjI =AUOc -----END PGP SIGNATURE----- --=-lGBQdsu0lkx0lE5aFXic-- --===============1555427833655695363== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============1555427833655695363==--