From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 08 of 10 v2] libxl: enable automatic placement of guests on NUMA nodes Date: Thu, 21 Jun 2012 18:43:52 +0200 Message-ID: <1340297032.4856.93.camel@Solace> References: <81f18379bb3d4d9397d1.1339779876@Solace> <4FE348C1.5030407@eu.citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1449598143496131169==" Return-path: In-Reply-To: <4FE348C1.5030407@eu.citrix.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: George Dunlap Cc: Andre Przywara , Ian Campbell , Stefano Stabellini , Juergen Gross , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============1449598143496131169== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-/gQHvu2D2Ezi9V16qqmU" --=-/gQHvu2D2Ezi9V16qqmU Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2012-06-21 at 17:16 +0100, George Dunlap wrote: > > Signed-off-by: Dario Faggioli > Overall I think this approach is much better. =20 > Cool, Thanks. > > + /* > > + * Round up and down some of the constraints. For instance, the mi= nimum > > + * number of cpus a candidate should have must at least be non-neg= ative. > > + * Regarding the minimum number of NUMA nodes, if not explicitly s= pecified > > + * (i.e., min_nodes<=3D 0), we try to figure out a sensible number= of nodes > > + * from where to start generating candidates, if possible (or just= start > > + * from 1 otherwise). The maximum number of nodes should not excee= d the > > + * number of existent NUMA nodes on the host, or the candidate gen= aration > > + * won't work properly. > > + */ > > + min_cpus =3D min_cpus<=3D 0 ? 0 : min_cpus; > Wouldn't it just make more sense to specify that "min_cpus" (and other= =20 > parameters) had to be >=3D0? >=20 Yes, I think that make sense, will do. > > +/* > > + * The NUMA placement candidates are reordered according to the follow= ing > > + * heuristics: > > + * - candidates involving fewer nodes come first. In case two (or > > + * more) candidates span the same number of nodes, > > + * - candidates with greater amount of free memory come first. In > > + * case two (or more) candidates differ in their amount of free > > + * memory by less than 10%, > Interesting idea -- sounds pretty reasonable. > Time will tell... :-O > > +static int numa_cmpf(const void *v1, const void *v2) > > +{ > > + const libxl__numa_candidate *c1 =3D (const libxl__numa_candidate*)= v1; > > + const libxl__numa_candidate *c2 =3D (const libxl__numa_candidate*)= v2; > > + double mem_diff =3D labs(c1->free_memkb - c2->free_memkb); > > + double mem_avg =3D (c1->free_memkb + c2->free_memkb) / 2.0; > > + > > + if (c1->nr_nodes !=3D c2->nr_nodes) > > + return c1->nr_nodes - c2->nr_nodes; > > + > > + if ((mem_diff / mem_avg) * 100.0< 10.0&& > > + c1->nr_domains !=3D c2->nr_domains) > > + return c1->nr_domains - c2->nr_domains; > I realize this isn't a hot path, but it seems like moving into FP is=20 > really unnecessary. You can just do this: >=20 Yeah, IanC pointed out that too. I'll convert everything toward integer arith. > One more thing: Is there a reason why you put get_numa_candidates() in= =20 > libxl_internal.h, but not sort_numa_candidates()? It seems like both or= =20 > neither should go. :-) >=20 diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -2021,6 +2021,134 @@ static inline void libxl__ctx_unlock(lib ... +_hidden int libxl__get_numa_candidates(libxl__gc *gc, + uint32_t min_free_memkb, int min_cpus, + int min_nodes, int max_nodes, + libxl__numa_candidate *cndts[], int *nr_cn= dts); + ... +/* signature for the comparison function between two candidates c1 and c2 + * (the thid parameter is provided to enable thread safety). */ +typedef int (*libxl__numa_candidate_cmpf)(const void *v1, const void *v2); +/* sort the list of candidates in cndts (an array with nr_cndts elements i= n + * it) using cmpf for comparing two candidates. Uses libc's qsort(). */ +_hidden void libxl__sort_numa_candidates(libxl__numa_candidate cndts[], + int nr_cndts, + libxl__numa_candidate_cmpf cmpf); But I'm not entirely sure I understood what you meant... > That's all I have for now. I'm OK with the general approach, so here's= =20 > a "weak ack", so if a maintainer is happy with the code, he can check it = in: >=20 > Acked-by: George Dunlap >=20 Ok, that's very nice. I'll have to respin the series, so I'll definitely address your comments and add your ack. :-) Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-/gQHvu2D2Ezi9V16qqmU 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 v1.4.12 (GNU/Linux) iEYEABECAAYFAk/jT0gACgkQk4XaBE3IOsSkYQCePx8xo5yBpyL4gmoI3tJmGNfF k4UAn0kaftnp/vdCcA6XemJ6eOpRGu5k =vTnK -----END PGP SIGNATURE----- --=-/gQHvu2D2Ezi9V16qqmU-- --===============1449598143496131169== 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 --===============1449598143496131169==--