From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v9 6/9] libxl/xl: deprecate the build_info->cpumap field Date: Wed, 18 Jun 2014 18:26:57 +0200 Message-ID: <1403108817.21681.18.camel@Solace> References: <20140618141449.21586.55528.stgit@Solace> <20140618142825.21586.54202.stgit@Solace> <1403106794.6568.102.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2372727355567563220==" Return-path: In-Reply-To: <1403106794.6568.102.camel@kazak.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 Campbell Cc: Ian.Jackson@citrix.com, Andrew.Cooper3@citrix.com, Wei Liu , George.Dunlap@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============2372727355567563220== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-aYfFx7tp4uauc/knYw7V" --=-aYfFx7tp4uauc/knYw7V Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On mer, 2014-06-18 at 16:53 +0100, Ian Campbell wrote: > On Wed, 2014-06-18 at 16:28 +0200, Dario Faggioli wrote: > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > > index c087cbc..af48622 100644 > > --- a/docs/man/xl.cfg.pod.5 > > +++ b/docs/man/xl.cfg.pod.5 > > @@ -143,11 +143,11 @@ Combining this with "all" is also possible, meani= ng "all,^nodes:1" > > results in all the vcpus of the guest running on all the cpus on the > > host, except for the cpus belonging to the host NUMA node 1. > > =20 > > -=3Ditem ["2", "3"] (or [2, 3]) > > +=3Ditem ["2", "3-8,^5"] > > =20 > > -To ask for specific vcpu mapping. That means (in this example), vcpu #= 0 > > -of the guest will run on cpu #2 of the host and vcpu #1 of the guest w= ill > > -run on cpu #3 of the host. > > +To ask for specific vcpu mapping. That means (in this example), vcpu 0 > > +of the guest will run on cpu 2 of the host and vcpu 1 of the guest wil= l > > +run on cpus 3,4,6,7,8 of the host. >=20 > Why is deprecating a field in the libxl API changing the xl > configuration file syntax? >=20 Because what happens as a consequence of, in xl, filling the array instead of just setting b_info->cpumap, is that we get to use the same parsing function (vcpupin_parse()) for all the elements of the array itself, in both the following cases: cpus=3D"1-4,7" (which sort of becomes equivalent to ["1-4,7", "1-4,7"]) and: cpus=3D["3", "4"] Up to the previous patch, the latter case was handled asking the elements of the list to be integers. Here we start using vcpupin_parse() on them, and that is more powerful, and allows the elements to be ranges. Anyway, I'll see if I can split this patch in two, as you suggest below, to make things more evident. > > @@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid= , > > return rc; > > } > > libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap); > > + /* > > + * info->cpumap is DEPRECATED, but we still want old applications > > + * that may be using it to continue working. > > + */ > > + if (!libxl_bitmap_is_full(&info->cpumap)) >=20 > The caller is expected to initialise this unused field to a non-default > state? That doesn't sound right. Did you mean !is_empty? >=20 Nope. The default for this is to be full, so what I'm checking is really that it stayed default. See libxl__domain_build_info_setdefault(): ... if (!b_info->cpumap.size) { if (libxl_cpu_bitmap_alloc(CTX, &b_info->cpumap, 0)) return ERROR_FAIL; libxl_bitmap_set_any(&b_info->cpumap); } ... Can I change this? If I can, I think the best would be to remove the allocation from libxl__domain_build_info_setdefault(), so that all the checks could become something like `if(cpumap.size)'. But does stop allocating the bitmap qualifies as an incompatible API change? If yes, I think we're stuck with checking whether or not it's full, to see if the user is doing something with it or not. > TBH I think you'd be better off just silently ignoring cpumap if the new > thing is set. >=20 This, I can try. > Or maybe converting the cpumap into the new array so the rest of the > libxl internals only needs to deal with one. >=20 Converting it to the new array where? AFAICT, this is the only place where cpumap is used, so I don't think it's worth converting it. Ignoring if the array is specified is a better solution, I think > > + LOG(WARN, "cpumap field of libxl_domain_build_info is DEPRECAT= ED. " > > + "Please, use the vcpu_hard_affinity array instead"); > > libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus, > > &info->cpumap, NULL); > > =20 > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > index 05978d7..0b3e4e9 100644 > > --- a/tools/libxl/libxl_types.idl > > +++ b/tools/libxl/libxl_types.idl > > @@ -297,7 +297,12 @@ libxl_domain_sched_params =3D Struct("domain_sched= _params",[ > > libxl_domain_build_info =3D Struct("domain_build_info",[ > > ("max_vcpus", integer), > > ("avail_vcpus", libxl_bitmap), > > - ("cpumap", libxl_bitmap), > > + ("cpumap", libxl_bitmap), # DEPRECATED! > > + # The cpumap field above has been deprecated by the introduction o= f the > > + # vcpu_hard_affinity array. It is not removed and it is still hono= ured, for > > + # API stability and backward compatibility reasons, but should not= be used > > + # any longer. The vcpu_hard_affinity array is what should be used = instead, > > + # to set the hard affinity of the various vCPUs. >=20 > This comment needs to talk about the precedence between the two fields > in the event that both are present. >=20 Right. I'll add a word about it. > WRT the structure of the series: All of the libxl deprecation stuff here > could be squashed into the previous patch which added the new field. > That would make more sense since otherwise you have a middle state where > both fields are present and valid and it is ill defined what is what. >=20 > All the xl stuff could then come next as a "move away from deprecated > interface" patch. >=20 > As it is each patch seems to do half of each thing. I'm not entirely > sure what the intermediate state is supposed to be. >=20 > > @@ -840,42 +839,30 @@ static void parse_config_data(const char *config_= source, > > fprintf(stderr, "Unable to allocate cpumap for vcpu %d= \n", i); > > exit(1); > > } > > - libxl_bitmap_set_any(&b_info->vcpu_hard_affinity[i]); > > + libxl_bitmap_set_none(&b_info->vcpu_hard_affinity[i]); >=20 > What do these sorts of changes have to do with the deprecation of > another field? >=20 > It looks to me like the previous patch has just done something wrong and > you are fixing it here for some reason. >=20 It's not like that, but I think I see what you mean. I'll try to restructure the series so it does not gives this impression. 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) --=-aYfFx7tp4uauc/knYw7V 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.0.22 (GNU/Linux) iEYEABECAAYFAlOhvdEACgkQk4XaBE3IOsS5HgCfacuugOzUoTLINeUQjkGbjwcp 8LUAni7hc7LeK6xqn9ZP3MBKi4sg28fL =ZSzA -----END PGP SIGNATURE----- --=-aYfFx7tp4uauc/knYw7V-- --===============2372727355567563220== 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 --===============2372727355567563220==--