From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler Date: Tue, 28 Jul 2015 11:15:10 +0200 Message-ID: <1438074910.2889.26.camel@citrix.com> References: <1436590356-3706-1-git-send-email-chong.li@wustl.edu> <1436590356-3706-4-git-send-email-chong.li@wustl.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6260449073162509123==" Return-path: In-Reply-To: <1436590356-3706-4-git-send-email-chong.li@wustl.edu> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chong Li Cc: Chong Li , wei.liu2@citrix.com, Sisu Xi , george.dunlap@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, ian.campbell@eu.citrix.com, Meng Xu , dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org --===============6260449073162509123== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-euKeUqA2wndjPNJh88XQ" --=-euKeUqA2wndjPNJh88XQ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. >=20 This patch looks mostly fine. A few comments. So, as for the other patches, I'd mention here in the changelog, that only RTDS supports this for now. > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index e9a2d26..9f7f859 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > =20 > +static int sched_rtds_validate_params(libxl__gc *gc, int period, > + int budget, uint32_t *sdom_period, > + uint32_t *sdom_budget) > +{ > + if (period !=3D LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { > + if (period < 1) { > + LOG(ERROR, "VCPU period is not set or out of range, " > + "valid values are larger than or equal to 1"); > That's probably a nit, but, if period is not set, as the error message says, it means it stays LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT? In which case you do not enter this branch, and you do not print this message, do you? What I mean is that, it looks to me that it's not accurate for the message to say "period not set", or am I overlooking something? > + return 1; > So, 1 is error, 0 is ok. That's fine, as this is an internal function, but please, add a quick doc comment above it. > + } > + *sdom_period =3D period; > + } > + > + if (budget !=3D LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { > + if (budget < 1) { > + LOG(ERROR, "VCPU budget is not set or out of range, " > + "valid values are larger than or equal to 1"); > + return 1; > + } > + *sdom_budget =3D budget; > + } > + > + if (budget > period) { > + LOG(ERROR, "VCPU budget is larger than VCPU period"); > I'd be more explicit: "VCPU budget must be smaller than VCPU period". > + return 1; > + } > + > + return 0; > +} > + > +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ > + uint16_t num_vcpus; > + int rc, i; > + xc_dominfo_t info; > + xen_domctl_schedparam_vcpu_t *vcpus; > + > + rc =3D xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; > + } > + > + if (scinfo->num_vcpus =3D=3D 0) > + num_vcpus =3D info.max_vcpu_id + 1; > + else > + num_vcpus =3D scinfo->num_vcpus; > + num_vcpus =3D scinfo->num_vcpus ? scinfo->num_vcpus : info->max_vcpu_id + 1; (And I think there is a 'contracted' form of this, but I keep forgetting it, and I'm not sure whether it's a GCC extension...) > + GCNEW_ARRAY(vcpus, num_vcpus); > + > + if (scinfo->num_vcpus > 0) > + for (i=3D0; i < num_vcpus; i++) { > + if (scinfo->vcpus[i].vcpuid < 0 || > + scinfo->vcpus[i].vcpuid > info.max_vcpu_id) { > + LOG(ERROR, "VCPU index is out of range, " > + "valid values are within range from 0 to %d", > + info.max_vcpu_id); > + return ERROR_INVAL; > + } > + vcpus[i].vcpuid =3D scinfo->vcpus[i].vcpuid; > + } else > + for (i=3D0; i < num_vcpus; i++) > + vcpus[i].vcpuid =3D i; > + > + rc =3D xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); > + if (rc !=3D 0) { > + LOGE(ERROR, "getting vcpu sched rtds"); > + return ERROR_FAIL; > + } > + > + scinfo->sched =3D LIBXL_SCHEDULER_RTDS; > + if (scinfo->num_vcpus =3D=3D 0) { > + scinfo->num_vcpus =3D num_vcpus; > + scinfo->vcpus =3D libxl__calloc(NOGC, num_vcpus, sizeof(libxl_sc= hed_params)); > This line looks a long... > + } > + for(i =3D 0; i < num_vcpus; i++) { > + scinfo->vcpus[i].period =3D vcpus[i].s.rtds.period; > + scinfo->vcpus[i].budget =3D vcpus[i].s.rtds.budget; > + scinfo->vcpus[i].vcpuid =3D vcpus[i].vcpuid; > + } > + > + return 0; > +} > + > +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) > +{ > + int rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + xen_domctl_schedparam_vcpu_t *vcpus; > + int num_vcpus; > + > + rc =3D xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; > + } > + max_vcpuid =3D info.max_vcpu_id; > + > + if (scinfo->num_vcpus > 0) { > + num_vcpus =3D scinfo->num_vcpus; > + GCNEW_ARRAY(vcpus, num_vcpus); > + for (i =3D 0; i < num_vcpus; i++) { > + if (scinfo->vcpus[i].vcpuid < 0 || > + scinfo->vcpus[i].vcpuid > max_vcpuid) { > + LOG(ERROR, "VCPU index is out of range, " > + "valid values are within range from 0 to %d", > + max_vcpuid); > + return ERROR_INVAL; > + } > + vcpus[i].vcpuid =3D scinfo->vcpus[i].vcpuid; > + > + rc =3D sched_rtds_validate_params(gc, > + scinfo->vcpus[i].period, scinfo->vcpus[i].budget, > + &vcpus[i].s.rtds.period, &vcpus[i].s.rtds.budget); > + if (rc) > + return ERROR_INVAL; > if (sched_rtds_validate_params(gc, scinfo->vcpus[i].period, scinfo->vcpus[i].budget, &vcpus[i].s.rtds.period, &vcpus[i].s.rtds.budget)) return ERROR_INVAL; I.e., I don't think you need to save rc. > + } > + } else { > + num_vcpus =3D max_vcpuid + 1; > + GCNEW_ARRAY(vcpus, num_vcpus); > + rc =3D sched_rtds_validate_params(gc, > + scinfo->vcpus[0].period, scinfo->vcpus[0].budget, > + &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget); > + if (rc) > + return ERROR_INVAL; > + for (i =3D 0; i < num_vcpus; i++) { > + vcpus[i].vcpuid =3D i; > + vcpus[i].s.rtds.period =3D scinfo->vcpus[0].period; > + vcpus[i].s.rtds.budget =3D scinfo->vcpus[0].budget; > + } > + } > + > + rc =3D xc_sched_rtds_vcpu_set(CTX->xch, domid, > + vcpus, num_vcpus); > + if (rc =3D=3D 1) { > + printf("WARN: too small period or budget may " > + "result in large scheduling overhead\n"); > + rc =3D 0; > As said, do the logging in Xen directly, and ditch this. > + } else if (rc !=3D 0) { > + LOGE(ERROR, "setting vcpu sched rtds"); > + return ERROR_FAIL; > + } > + > + return rc; > +} > @@ -5887,29 +6037,10 @@ static int sched_rtds_domain_set(libxl__gc *gc, u= int32_t domid, > return ERROR_FAIL; > } > =20 > - if (scinfo->period !=3D LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { > - if (scinfo->period < 1) { > - LOG(ERROR, "VCPU period is not set or out of range, " > - "valid values are larger than 1"); > - return ERROR_INVAL; > - } > - sdom.period =3D scinfo->period; > - } > - > - if (scinfo->budget !=3D LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { > - if (scinfo->budget < 1) { > - LOG(ERROR, "VCPU budget is not set or out of range, " > - "valid values are larger than 1"); > - return ERROR_INVAL; > - } > - sdom.budget =3D scinfo->budget; > - } > - > - if (sdom.budget > sdom.period) { > - LOG(ERROR, "VCPU budget is larger than VCPU period, " > - "VCPU budget should be no larger than VCPU period"); > + rc =3D sched_rtds_validate_params(gc, scinfo->period, scinfo->budget= , > + &sdom.period, &sdom.budget); > + if (rc) > return ERROR_INVAL; > Ditto. > - } > =20 > @@ -5920,6 +6051,11 @@ static int sched_rtds_domain_set(libxl__gc *gc, ui= nt32_t domid, > return 0; > } > =20 > +/* Set the per-domain scheduling parameters. > +* For schedulers that support per-vcpu settings (e.g., RTDS), > +* calling *_domain_set functions will set all vcpus with the same > +* scheduling parameters. > +*/ > int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, > const libxl_domain_sched_params *scinf= o) > { > @@ -5956,6 +6092,52 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, = uint32_t domid, > return ret; > } > =20 > +/* Set the per-vcpu scheduling parameters */ > +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) > +{ > + GC_INIT(ctx); > + libxl_scheduler sched =3D scinfo->sched; > + int ret; > + > + if (sched =3D=3D LIBXL_SCHEDULER_UNKNOWN) > + sched =3D libxl__domain_scheduler(gc, domid); > + > + switch (sched) { > + case LIBXL_SCHEDULER_SEDF: > + LOG(ERROR, "No per-vcpu set function provided"); > I'd say: "per-VCPU parameter setting not supported for this scheduler". > + ret =3D ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_CREDIT: > + LOG(ERROR, "No per-vcpu set function provided"); > + ret =3D ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_CREDIT2: > + LOG(ERROR, "No per-vcpu set function provided"); > + ret =3D ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_ARINC653: > + LOG(ERROR, "No per-vcpu set function provided"); > + ret =3D ERROR_INVAL; > + break; BTW, Can't you unify all the !supported cases? > + case LIBXL_SCHEDULER_RTDS: > + ret =3D sched_rtds_vcpu_set(gc, domid, scinfo); > + break; > + default: > + LOG(ERROR, "Unknown scheduler"); > + ret =3D ERROR_INVAL; > + break; > + } > + > + GC_FREE; > + return ret; > +} > @@ -5989,6 +6171,45 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, = uint32_t domid, > return ret; > } > =20 > +/* Get the per-vcpu scheduling parameters */ > +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ > + GC_INIT(ctx); > + int ret; > + > + scinfo->sched =3D libxl__domain_scheduler(gc, domid); > + > + switch (scinfo->sched) { > + case LIBXL_SCHEDULER_SEDF: > + LOG(ERROR, "No per-vcpu get function provided"); > Same here. > + ret =3D ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_CREDIT: > + LOG(ERROR, "No per-vcpu get function provided"); > + ret =3D ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_CREDIT2: > + LOG(ERROR, "No per-vcpu get function provided"); > + ret =3D ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_ARINC653: > + LOG(ERROR, "No per-vcpu get function provided"); > + ret =3D ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_RTDS: > + ret =3D sched_rtds_vcpu_get(gc, domid, scinfo); > + break; And here. > + default: > + LOG(ERROR, "Unknown scheduler"); > + ret =3D ERROR_INVAL; > + break; > + } > + > + GC_FREE; > + return ret; > +} > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index a1c5d15..040a248 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -200,6 +200,16 @@ > #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1 > =20 > /* > + * libxl_vcpu_sched_params is used to store per-vcpu params > +*/ > +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1 > + > +/* > + * libxl_sched_params is used to store the array of per-vcpu params > +*/ > +#define LIBXL_HAVE_SCHED_PARAMS 1 > + > I may be misremembering, but did not we say that one of these LIBXL_HAVE_* was enough? If we want to have two, I'd much rather have a generic one (LIBXL_HAVE_VCPU_SCHED_PARAMS) and one announcing that the feature is supported by RTDS (e.g., LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS). This way, if/when we'll add support for per-VCPU parameters in Credit, we'd add LIBXL_HAVE_SCHED_CREDIT_VCPU_PARAMS. But that's just an idea, it's best to see what tools maintainers think of this. BTW, be a little more verbose in commenting these macros. Just have a look and take inspiration from the already existing ones. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-euKeUqA2wndjPNJh88XQ 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 iEYEABECAAYFAlW3SB4ACgkQk4XaBE3IOsQqQwCeI3YeL/56PShc3iG2ZzlTg3mv oWUAn3RUSK6YJ/XgTZJ6tWQxXKmUSI6J =OZr+ -----END PGP SIGNATURE----- --=-euKeUqA2wndjPNJh88XQ-- --===============6260449073162509123== 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 --===============6260449073162509123==--