| Message ID | 1457286958-5427-4-git-send-email-lichong659@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Mar 06, 2016 at 11:55:57AM -0600, Chong Li wrote: [...] > tools/libxl/libxl.c | 326 ++++++++++++++++++++++++++++++++++++++++---- > tools/libxl/libxl.h | 37 +++++ > tools/libxl/libxl_types.idl | 14 ++ > 3 files changed, 354 insertions(+), 23 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index bd3aac8..4532e86 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5770,6 +5770,207 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid, > return 0; > } > > +static int sched_rtds_validate_params(libxl__gc *gc, int period, > + int budget, uint32_t *sdom_period, > + uint32_t *sdom_budget) Indentation. > +{ > + int rc = 0; > + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { > + if (period < 1) { > + LOG(ERROR, "VCPU period is out of range, " > + "valid values are larger than or equal to 1"); > + rc = ERROR_INVAL; /* error scheduling parameter */ > + goto out; > + } > + *sdom_period = period; > + } > + > + if (budget != 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"); > + rc = ERROR_INVAL; > + goto out; > + } > + *sdom_budget = budget; > + } > + > + if (*sdom_budget > *sdom_period) { > + LOG(ERROR, "VCPU budget must be smaller than " > + "or equal to VCPU period"); > + rc = ERROR_INVAL; > + } This is a strange pattern. It does more than what it's name suggests. It seems this function just returns the vanilla values in period and budget. It can be simplified by removing sdom_period and sdom_budget all together. Do you agree? Then at callsites you set those values with two direct assignment: if (validate(period_value, budget_value) != 0) { error; } period = period_value; budget = budget_value; > +out: > + return rc; > +} > + > +/* Get the RTDS scheduling parameters of vcpu(s) */ > +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ > + uint32_t num_vcpus; > + int i, r, rc; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + > + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (r < 0) { > + LOGE(ERROR, "getting domain info"); > + rc = ERROR_FAIL; > + goto out; > + } > + > + num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus : > + info.max_vcpu_id + 1; > + > + GCNEW_ARRAY(vcpus, num_vcpus); > + > + if (scinfo->num_vcpus > 0) { > + for (i = 0; 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); > + rc = ERROR_INVAL; > + goto out; > + } > + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; > + } > + } else > + for (i = 0; i < num_vcpus; i++) > + vcpus[i].vcpuid = i; > + > + r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); > + if (r != 0) { > + LOGE(ERROR, "getting vcpu sched rtds"); > + rc = ERROR_FAIL; > + goto out; > + } > + scinfo->sched = LIBXL_SCHEDULER_RTDS; > + if (scinfo->num_vcpus == 0) { > + scinfo->num_vcpus = num_vcpus; > + scinfo->vcpus = libxl__calloc(NOGC, num_vcpus, > + sizeof(libxl_sched_params)); > + } > + for(i = 0; i < num_vcpus; i++) { > + scinfo->vcpus[i].period = vcpus[i].s.rtds.period; > + scinfo->vcpus[i].budget = vcpus[i].s.rtds.budget; > + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; > + } > +return r; Just set rc = 0 here? > +out: > + return rc; > +} > + > +/* Set the RTDS scheduling parameters of vcpu(s) */ > +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) If this is indentation that you're not sure about. Just leave it like this. > +{ > + int r, rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + uint32_t num_vcpus; > + > + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (r < 0) { > + LOGE(ERROR, "getting domain info"); > + rc = ERROR_FAIL; > + goto out; > + } > + max_vcpuid = info.max_vcpu_id; > + > + if (scinfo->num_vcpus > 0) { > + num_vcpus = scinfo->num_vcpus; > + GCNEW_ARRAY(vcpus, num_vcpus); > + for (i = 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); > + rc = ERROR_INVAL; > + goto out; > + } > + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; > + I would suggest you use a separate loop to validate the input, otherwise you risk getting input partial success. > + rc = 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) { > + rc = ERROR_INVAL; > + goto out; > + } > + } > + } else { > + rc = ERROR_INVAL; > + goto out; > + } The above hunk would be better if you write it like: if (scinfo->num_vcpus <= 0) { rc = ERROR_INVAL; goto out; } /* ... The other branch follows. No indentation needed. */ num_vcpus = ... > + > + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, > + vcpus, num_vcpus); Indentation. > + if (r != 0) { > + LOGE(ERROR, "setting vcpu sched rtds"); > + rc = ERROR_FAIL; > + goto out; > + } > +return r; Use rc = 0 is better. > +out: > + return rc; > +} > + > +/* Set the RTDS scheduling parameters of all vcpus of a domain */ > +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) > +{ > + int r, rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + uint32_t num_vcpus; > + > + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (r < 0) { > + LOGE(ERROR, "getting domain info"); > + rc = ERROR_FAIL; > + goto out; > + } > + max_vcpuid = info.max_vcpu_id; > + > + if (scinfo->num_vcpus == 1) { > + num_vcpus = max_vcpuid + 1; > + GCNEW_ARRAY(vcpus, num_vcpus); > + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, > + scinfo->vcpus[0].budget, &vcpus[0].s.rtds.period, > + &vcpus[0].s.rtds.budget)) { > + rc = ERROR_INVAL; > + goto out; > + } > + for (i = 0; i < num_vcpus; i++) { > + vcpus[i].vcpuid = i; > + vcpus[i].s.rtds.period = scinfo->vcpus[0].period; > + vcpus[i].s.rtds.budget = scinfo->vcpus[0].budget; > + } > + } else { > + rc = ERROR_INVAL; > + goto out; > + } > + Same here, just move the else branch up. > + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, > + vcpus, num_vcpus); Indentation. > + if (r != 0) { > + LOGE(ERROR, "setting vcpu sched rtds"); > + rc = ERROR_FAIL; > + goto out; > + } > +return r; rc = 0; > +out: > + return rc; > +} > + > static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, > libxl_domain_sched_params *scinfo) > { > @@ -5802,30 +6003,9 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, > LOGE(ERROR, "getting domain sched rtds"); > return ERROR_FAIL; > } > - > - if (scinfo->period != 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 = scinfo->period; > - } > - > - if (scinfo->budget != 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 = 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"); > + if (sched_rtds_validate_params(gc, scinfo->period, scinfo->budget, > + &sdom.period, &sdom.budget)) > return ERROR_INVAL; > - } > > rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom); > if (rc < 0) { > @@ -5873,6 +6053,74 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, > return ret; > } > > +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) Indentation. > +{ > + GC_INIT(ctx); > + libxl_scheduler sched = scinfo->sched; > + int rc; > + > + if (sched == LIBXL_SCHEDULER_UNKNOWN) > + sched = libxl__domain_scheduler(gc, domid); > + > + switch (sched) { > + case LIBXL_SCHEDULER_SEDF: > + LOG(ERROR, "SEDF scheduler no longer available"); > + rc = ERROR_FEATURE_REMOVED; > + break; > + case LIBXL_SCHEDULER_CREDIT: > + case LIBXL_SCHEDULER_CREDIT2: > + case LIBXL_SCHEDULER_ARINC653: > + LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler"); > + rc = ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_RTDS: > + rc = sched_rtds_vcpus_params_set(gc, domid, scinfo); > + break; > + default: > + LOG(ERROR, "Unknown scheduler"); > + rc = ERROR_INVAL; > + break; > + } > + > + GC_FREE; > + return rc; > +} [...] > static int libxl__domain_s3_resume(libxl__gc *gc, int domid) > { > int rc = 0; > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 6b73848..f1d53d8 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -206,6 +206,17 @@ > #define LIBXL_HAVE_DEVICE_MODEL_USER 1 > > /* > + * libxl_vcpu_sched_params is used to store per-vcpu params. > + */ > +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1 > + > +/* > + * LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS indicates RTDS scheduler > + * now supports per-vcpu settings. > + */ > +#define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1 > + > +/* > * libxl_domain_build_info has the arm.gic_version field. > */ > #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 > @@ -1647,11 +1658,37 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid, > #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1 > #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1 > > +/* Per-VCPU parameters*/ > +#define LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT -1 > + > +/* Get the per-domain scheduling parameters. > + * For schedulers that support per-vcpu settings (e.g., RTDS), > + * calling *_domain_get functions will get default scheduling > + * parameters. > + */ > int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, > libxl_domain_sched_params *params); > + > +/* 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 *params); > > +/* Get the per-vcpu scheduling parameters */ > +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, > + libxl_vcpu_sched_params *params); > + > +/* Set the per-vcpu scheduling parameters */ > +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, > + const libxl_vcpu_sched_params *params); > + > +/* Set the per-vcpu scheduling parameters of all vcpus of a domain*/ > +int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, > + const libxl_vcpu_sched_params *params); > + > int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid, > libxl_trigger trigger, uint32_t vcpuid); > int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index cf3730f..7487fc9 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -378,6 +378,20 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ > ("stream_version", uint32, {'init_val': '1'}), > ]) > > +libxl_sched_params = Struct("sched_params",[ > + ("vcpuid", integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}), > + ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}), > + ("cap", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}), > + ("period", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}), > + ("extratime", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}), > + ("budget", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}), > + ]) > + > +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[ > + ("sched", libxl_scheduler), > + ("vcpus", Array(libxl_sched_params, "num_vcpus")), > + ]) > + IIRC Dario and you have come to agreement on the data structure so I skim this. Overall I think the patch is moving towards the right direction. Just that there are too many places where indentation need fixing. Please fix them in the next iteration. I really don't like holding back patches just because of indentation issues. Depending on the editor you use, there might be some common runes that we can give you. If you feel unclear about my comments, just ask for clarification. Wei. > libxl_domain_sched_params = Struct("domain_sched_params",[ > ("sched", libxl_scheduler), > ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}), > -- > 1.9.1 >
On Tue, Mar 8, 2016 at 1:12 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Sun, Mar 06, 2016 at 11:55:57AM -0600, Chong Li wrote: > [...] >> tools/libxl/libxl.c | 326 ++++++++++++++++++++++++++++++++++++++++---- >> tools/libxl/libxl.h | 37 +++++ >> tools/libxl/libxl_types.idl | 14 ++ >> 3 files changed, 354 insertions(+), 23 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index bd3aac8..4532e86 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -5770,6 +5770,207 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid, >> return 0; >> } >> >> +static int sched_rtds_validate_params(libxl__gc *gc, int period, >> + int budget, uint32_t *sdom_period, >> + uint32_t *sdom_budget) > > > This is a strange pattern. It does more than what it's name suggests. > > It seems this function just returns the vanilla values in period and > budget. It can be simplified by removing sdom_period and sdom_budget all > together. Do you agree? Yes. >> + >> +/* Get the RTDS scheduling parameters of vcpu(s) */ >> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, >> + libxl_vcpu_sched_params *scinfo) >> +{ >> + uint32_t num_vcpus; >> + int i, r, rc; >> + xc_dominfo_t info; >> + struct xen_domctl_schedparam_vcpu *vcpus; >> + >> + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); >> + if (r < 0) { >> + LOGE(ERROR, "getting domain info"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus : >> + info.max_vcpu_id + 1; >> + >> + GCNEW_ARRAY(vcpus, num_vcpus); >> + >> + if (scinfo->num_vcpus > 0) { >> + for (i = 0; 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); >> + rc = ERROR_INVAL; >> + goto out; >> + } >> + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; >> + } >> + } else >> + for (i = 0; i < num_vcpus; i++) >> + vcpus[i].vcpuid = i; >> + >> + r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); >> + if (r != 0) { >> + LOGE(ERROR, "getting vcpu sched rtds"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + scinfo->sched = LIBXL_SCHEDULER_RTDS; >> + if (scinfo->num_vcpus == 0) { >> + scinfo->num_vcpus = num_vcpus; >> + scinfo->vcpus = libxl__calloc(NOGC, num_vcpus, >> + sizeof(libxl_sched_params)); >> + } >> + for(i = 0; i < num_vcpus; i++) { >> + scinfo->vcpus[i].period = vcpus[i].s.rtds.period; >> + scinfo->vcpus[i].budget = vcpus[i].s.rtds.budget; >> + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; >> + } >> +return r; > > Just set rc = 0 here? Ok. > >> +out: >> + return rc; >> +} >> + >> +/* Set the RTDS scheduling parameters of vcpu(s) */ >> +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid, >> + const libxl_vcpu_sched_params *scinfo) >> +{ >> + int r, rc; >> + int i; >> + uint16_t max_vcpuid; >> + xc_dominfo_t info; >> + struct xen_domctl_schedparam_vcpu *vcpus; >> + uint32_t num_vcpus; >> + >> + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); >> + if (r < 0) { >> + LOGE(ERROR, "getting domain info"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + max_vcpuid = info.max_vcpu_id; >> + >> + if (scinfo->num_vcpus > 0) { >> + num_vcpus = scinfo->num_vcpus; >> + GCNEW_ARRAY(vcpus, num_vcpus); >> + for (i = 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); >> + rc = ERROR_INVAL; >> + goto out; >> + } >> + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; >> + > > I would suggest you use a separate loop to validate the input, otherwise > you risk getting input partial success. What do you mean by "partial success"? Do you suggest to validate the entire input first, and then create && set the vcpus array? > >> + rc = 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) { >> + rc = ERROR_INVAL; >> + goto out; >> + } >> + } >> + } else { >> + rc = ERROR_INVAL; >> + goto out; >> + } > Chong
On Tue, Mar 08, 2016 at 06:38:54PM -0600, Chong Li wrote: > >> + if (scinfo->num_vcpus > 0) { > >> + num_vcpus = scinfo->num_vcpus; > >> + GCNEW_ARRAY(vcpus, num_vcpus); > >> + for (i = 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); > >> + rc = ERROR_INVAL; > >> + goto out; > >> + } > >> + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; > >> + > > > > I would suggest you use a separate loop to validate the input, otherwise > > you risk getting input partial success. > > What do you mean by "partial success"? You discover error half way through the array. All vcpus before the error occurs would have been set to new parameters. > Do you suggest to validate the entire input first, and then create && > set the vcpus array? > Yes. Wei.
On the whole series, you are Cc-ing ian.campbell@eu.citrix.com, which is no longer a tools maintainer. Please, double check by looking at MAINTAINERS and/or with ./scripts/get_maintainer.pl. While fiddling with the Cc-list, can you please try to arrange things in such a way that each person is Cc-ed only to the subset of the series that he would reasonably care about. For instance, Jan is likely not going to look at libxl and xl stuff, so no need bothering him with that. Similarly, IanJ wouldn't probably look at hypervisor stuff. If you are in doubt, stick to MAINTAINERS, with the exception that, if some random person commented on vX, he should be Cc-ed to vX+1, where the comments are addressed, no matter what he/she maintains. There are other exceptions, actually. For example, it's ok to Cc me to the whole series. This is hard to "set in stone", so again, if in doubt, follow the rule above... If people are interested in a patch that they happen not to be Cc-ed to, they'll find it via the mailing list. On Sun, 2016-03-06 at 11:55 -0600, Chong Li wrote: > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index bd3aac8..4532e86 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5770,6 +5770,207 @@ static int sched_credit2_domain_set(libxl__gc > *gc, uint32_t domid, > return 0; > } > > +static int sched_rtds_validate_params(libxl__gc *gc, int period, > + int budget, uint32_t > *sdom_period, > + uint32_t *sdom_budget) > +{ > + int rc = 0; > Empty line here (between local variables and code). Also, libxl coding style says: * If the function is to return a libxl error value, `rc' is used to contain the error code, but it is NOT initialised: int rc; This, in addition to all what Wei pointed out, to mean that, although things are certainly improving, some non negligible amount of coding style issues needs fixing. (I'll elaborate more, and more in general, in another email.) Regards, Dario
On Wed, 2016-03-09 at 18:09 +0100, Dario Faggioli wrote: > While fiddling with the Cc-list, can you please try to arrange things > in such a way that each person is Cc-ed only to the subset of the > series that he would reasonably care about. > > For instance, Jan is likely not going to look at libxl and xl stuff, > so > no need bothering him with that. Similarly, IanJ wouldn't probably > look > at hypervisor stuff. > > If you are in doubt, stick to MAINTAINERS, with the exception that, > if > some random person commented on vX, he should be Cc-ed to vX+1, where > the comments are addressed, no matter what he/she maintains. > > There are other exceptions, actually. For example, it's ok to Cc me > to > the whole series. This is hard to "set in stone", so again, if in > doubt, follow the rule above... If people are interested in a patch > that they happen not to be Cc-ed to, they'll find it via the mailing > list. > To be fair, you are actually doing this already, and it was me that had the wrong impression that you weren't. So, good work with that, and sorry (you and everyone else) for the noise on this. :-) Dario
On Tue, 2016-03-08 at 19:12 +0000, Wei Liu wrote: > Overall I think the patch is moving towards the right direction. > I also think so. Thanks a lot Wei for the review, BTW. > Just > that there are too many places where indentation need fixing. Please > fix them in the next iteration. I really don't like holding back > patches > just because of indentation issues. > Exactly. Trying to elaborate a bit more, this series does not include any complex algorithm or similar, so one may thing that it is not that hard to review. But it is indeed complex and hard to review, because the patches are rather big and because the API and the command line syntax we want to support is complex. If there are too many style issue, any review will likely end up focusing mostly, if not only, on them, for various reasons. E.g., when one finds a stile issue, avoiding commenting on it (e.g., because one wants to focus on "more important" aspects) means risking forgetting about it and, in the end, letting it hit the repository (if others also miss it or does the same). Also, we're all used to look at code that (well, mostly :-D) conforms to coding style, so it's harder to focus on code that does not. And more. Add to this that the most difficult part of the tools side of this series (like API and data structures) is actually ok. So, Chong, for us to be able to quickly and effectively help you forward, we need to ask you to do your best and get rid of (ideally) all coding style problems. Once that's done, we're not far from calling this all a done deal. :-) Thanks and Regards, Dario
On Wed, Mar 9, 2016 at 11:28 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Tue, 2016-03-08 at 19:12 +0000, Wei Liu wrote: > >> Overall I think the patch is moving towards the right direction. >> > I also think so. Thanks a lot Wei for the review, BTW. > >> Just >> that there are too many places where indentation need fixing. Please >> fix them in the next iteration. I really don't like holding back >> patches >> just because of indentation issues. >> > Exactly. Trying to elaborate a bit more, this series does not include > any complex algorithm or similar, so one may thing that it is not that > hard to review. But it is indeed complex and hard to review, because > the patches are rather big and because the API and the command line > syntax we want to support is complex. > > If there are too many style issue, any review will likely end up > focusing mostly, if not only, on them, for various reasons. E.g., when > one finds a stile issue, avoiding commenting on it (e.g., because one > wants to focus on "more important" aspects) means risking forgetting > about it and, in the end, letting it hit the repository (if others also > miss it or does the same). Also, we're all used to look at code that > (well, mostly :-D) conforms to coding style, so it's harder to focus on > code that does not. And more. > > Add to this that the most difficult part of the tools side of this > series (like API and data structures) is actually ok. > > So, Chong, for us to be able to quickly and effectively help you > forward, we need to ask you to do your best and get rid of (ideally) > all coding style problems. > > Once that's done, we're not far from calling this all a done deal. :-) > > Thanks and Regards, > Dario Wei and Dario, thanks for your help reviewing this post. I'm now working on the coding style issues and will send out the next version when all problems are figured out. Chong > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) >
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index bd3aac8..4532e86 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5770,6 +5770,207 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid, return 0; } +static int sched_rtds_validate_params(libxl__gc *gc, int period, + int budget, uint32_t *sdom_period, + uint32_t *sdom_budget) +{ + int rc = 0; + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { + if (period < 1) { + LOG(ERROR, "VCPU period is out of range, " + "valid values are larger than or equal to 1"); + rc = ERROR_INVAL; /* error scheduling parameter */ + goto out; + } + *sdom_period = period; + } + + if (budget != 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"); + rc = ERROR_INVAL; + goto out; + } + *sdom_budget = budget; + } + + if (*sdom_budget > *sdom_period) { + LOG(ERROR, "VCPU budget must be smaller than " + "or equal to VCPU period"); + rc = ERROR_INVAL; + } +out: + return rc; +} + +/* Get the RTDS scheduling parameters of vcpu(s) */ +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + uint32_t num_vcpus; + int i, r, rc; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + + num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus : + info.max_vcpu_id + 1; + + GCNEW_ARRAY(vcpus, num_vcpus); + + if (scinfo->num_vcpus > 0) { + for (i = 0; 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); + rc = ERROR_INVAL; + goto out; + } + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + } + } else + for (i = 0; i < num_vcpus; i++) + vcpus[i].vcpuid = i; + + r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "getting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + scinfo->sched = LIBXL_SCHEDULER_RTDS; + if (scinfo->num_vcpus == 0) { + scinfo->num_vcpus = num_vcpus; + scinfo->vcpus = libxl__calloc(NOGC, num_vcpus, + sizeof(libxl_sched_params)); + } + for(i = 0; i < num_vcpus; i++) { + scinfo->vcpus[i].period = vcpus[i].s.rtds.period; + scinfo->vcpus[i].budget = vcpus[i].s.rtds.budget; + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; + } +return r; +out: + return rc; +} + +/* Set the RTDS scheduling parameters of vcpu(s) */ +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + int r, rc; + int i; + uint16_t max_vcpuid; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + uint32_t num_vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + max_vcpuid = info.max_vcpu_id; + + if (scinfo->num_vcpus > 0) { + num_vcpus = scinfo->num_vcpus; + GCNEW_ARRAY(vcpus, num_vcpus); + for (i = 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); + rc = ERROR_INVAL; + goto out; + } + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + + rc = 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) { + rc = ERROR_INVAL; + goto out; + } + } + } else { + rc = ERROR_INVAL; + goto out; + } + + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } +return r; +out: + return rc; +} + +/* Set the RTDS scheduling parameters of all vcpus of a domain */ +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + int r, rc; + int i; + uint16_t max_vcpuid; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + uint32_t num_vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + max_vcpuid = info.max_vcpu_id; + + if (scinfo->num_vcpus == 1) { + num_vcpus = max_vcpuid + 1; + GCNEW_ARRAY(vcpus, num_vcpus); + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, + scinfo->vcpus[0].budget, &vcpus[0].s.rtds.period, + &vcpus[0].s.rtds.budget)) { + rc = ERROR_INVAL; + goto out; + } + for (i = 0; i < num_vcpus; i++) { + vcpus[i].vcpuid = i; + vcpus[i].s.rtds.period = scinfo->vcpus[0].period; + vcpus[i].s.rtds.budget = scinfo->vcpus[0].budget; + } + } else { + rc = ERROR_INVAL; + goto out; + } + + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } +return r; +out: + return rc; +} + static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5802,30 +6003,9 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, LOGE(ERROR, "getting domain sched rtds"); return ERROR_FAIL; } - - if (scinfo->period != 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 = scinfo->period; - } - - if (scinfo->budget != 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 = 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"); + if (sched_rtds_validate_params(gc, scinfo->period, scinfo->budget, + &sdom.period, &sdom.budget)) return ERROR_INVAL; - } rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom); if (rc < 0) { @@ -5873,6 +6053,74 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, return ret; } +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + libxl_scheduler sched = scinfo->sched; + int rc; + + if (sched == LIBXL_SCHEDULER_UNKNOWN) + sched = libxl__domain_scheduler(gc, domid); + + switch (sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpus_params_set(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + +int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + libxl_scheduler sched = scinfo->sched; + int rc; + + if (sched == LIBXL_SCHEDULER_UNKNOWN) + sched = libxl__domain_scheduler(gc, domid); + + switch (sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpus_params_set_all(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5907,6 +6155,38 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, return ret; } +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + int rc; + + scinfo->sched = libxl__domain_scheduler(gc, domid); + + switch (scinfo->sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler is no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpu_get(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + static int libxl__domain_s3_resume(libxl__gc *gc, int domid) { int rc = 0; diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 6b73848..f1d53d8 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -206,6 +206,17 @@ #define LIBXL_HAVE_DEVICE_MODEL_USER 1 /* + * libxl_vcpu_sched_params is used to store per-vcpu params. + */ +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1 + +/* + * LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS indicates RTDS scheduler + * now supports per-vcpu settings. + */ +#define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1 + +/* * libxl_domain_build_info has the arm.gic_version field. */ #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 @@ -1647,11 +1658,37 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid, #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1 #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1 +/* Per-VCPU parameters*/ +#define LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT -1 + +/* Get the per-domain scheduling parameters. + * For schedulers that support per-vcpu settings (e.g., RTDS), + * calling *_domain_get functions will get default scheduling + * parameters. + */ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, libxl_domain_sched_params *params); + +/* 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 *params); +/* Get the per-vcpu scheduling parameters */ +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *params); + +/* Set the per-vcpu scheduling parameters */ +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *params); + +/* Set the per-vcpu scheduling parameters of all vcpus of a domain*/ +int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *params); + int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid, libxl_trigger trigger, uint32_t vcpuid); int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index cf3730f..7487fc9 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -378,6 +378,20 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ ("stream_version", uint32, {'init_val': '1'}), ]) +libxl_sched_params = Struct("sched_params",[ + ("vcpuid", integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}), + ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}), + ("cap", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}), + ("period", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}), + ("extratime", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}), + ("budget", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}), + ]) + +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[ + ("sched", libxl_scheduler), + ("vcpus", Array(libxl_sched_params, "num_vcpus")), + ]) + libxl_domain_sched_params = Struct("domain_sched_params",[ ("sched", libxl_scheduler), ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),