From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning Date: Tue, 10 Sep 2013 19:42:03 +0200 Message-ID: <1378834923.2821.53.camel@Solace> References: <20130906154725.10218.45008.stgit@hit-nxdomain.opendns.com> <20130906155540.10218.3642.stgit@hit-nxdomain.opendns.com> <21034.1200.533286.543728@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2134099774721929171==" Return-path: In-Reply-To: <21034.1200.533286.543728@mariner.uk.xensource.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: Ian Jackson Cc: George Dunlap , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============2134099774721929171== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-vN5xz6M00wA7sExTphyD" --=-vN5xz6M00wA7sExTphyD Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On ven, 2013-09-06 at 17:37 +0100, Ian Jackson wrote: > Dario Faggioli writes ("[PATCH v2 3/5] xl: allow for node-wise specificat= ion of vcpu pinning"): > > Making it possible to use something like the following: > > * "nodes:0-3": all pCPUs of nodes 0,1,2,3; > > * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3; > > * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2 > > but not pCPU 6; > > * ... > ... > > * code rearranged in order to look more simple to follow > > and understand, as requested during review; >=20 > This is much better now. Thank you! >=20 Glad to hear that. :-) > > +static int parse_range(const char *str, unsigned long *a, unsigned lon= g *b) > > { > ... > > + if (endptr =3D=3D str) > > + return EINVAL; > > + if (*a =3D=3D ULONG_MAX) > > + return ERANGE; >=20 > So parse_range returns errno value or 0. This isn't mentioned > anywhere and is a bit unusual. >=20 > > +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap) > > +{ > ... > > + rc =3D libxl_node_bitmap_alloc(ctx, &node_cpumap, 0); > > + if (rc) { > > + fprintf(stderr, "libxl_node_bitmap_alloc failed.\n"); > > + return rc; >=20 > So update_cpumap_range returns a libxl error code. >=20 > > + rc =3D parse_range(str, &ida, &idb); > > + if (rc) { >=20 > But here you assign the errno value to an `rc' variable which holds a > libxl error code. I think it would be better to make parse_range > return a libxl rc value. >=20 You're right, I cleaned up the code, but messed up quite a bit with the error codes! Thanks for pointing this out, I will put thing in a more consistent state. > I think also that parse_range doesn't notice if the specified range > contains junk between the second number and the comma ? >=20 Mmm... I will double check, although I'm not sure I'm getting you 100%, since parse_range does not deal with comas... That are handled by "strtok_r(cpu, ",", &saveptr)", which now is in vcpupin_parse(). > > +static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap) > > +{ > ... > > + if (STR_HAS_PREFIX(ptr, "all") || > > + STR_HAS_PREFIX(ptr, "nodes:all")) { > > + libxl_bitmap_set_any(cpumap); >=20 > Why not deal with all in parse_range ? You'd avoid the second > special-case of "nodes:", and constructions like > all,^3 > would work. >=20 Having it here already makes that ("all,^3") work, but it is true that it'd simplify the if, and that it belongs more within parse_range() than here. I'll move it. > > -vcpp_out: > > - libxl_bitmap_dispose(&exclude_cpumap); > > + rc =3D update_cpumap_range(ptr, cpumap); > > + if (rc) { > > + /* If failing, reset the cpumap and exit */ > > + libxl_bitmap_set_none(cpumap); >=20 > Surely the caller who gets a error should expect the cpumap to contain > arbitrary contents ? >=20 Fair enough. BTW, any chance you could have a look at 4/5 and 5/5 as well, so that I can repost? 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) --=-vN5xz6M00wA7sExTphyD 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.14 (GNU/Linux) iEYEABECAAYFAlIvWesACgkQk4XaBE3IOsTJyQCcDAmPgaxWVm9UqnFh4UDqz0VC 6AkAnRLJn6FFWNkC+gyFa2hqQs8ENwrC =TbST -----END PGP SIGNATURE----- --=-vN5xz6M00wA7sExTphyD-- --===============2134099774721929171== 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 --===============2134099774721929171==--