| Message ID | 1458146871-2813-4-git-send-email-lichong659@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 16, 2016 at 11:47:50AM -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > > Signed-off-by: Chong Li <chong.li@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> > Good work fixing all issues. Acked-by: Wei Liu <wei.liu2@citrix.com> And I have some nit-picking below. > + > +/* 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; > + > + 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) { > + rc = ERROR_INVAL; > + goto out; > + } else { > + for (i = 0; i < scinfo->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; > + } > + rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period, > + scinfo->vcpus[i].budget); > + if (rc) { > + rc = ERROR_INVAL; > + goto out; > + } > + } > + GCNEW_ARRAY(vcpus, scinfo->num_vcpus); > + for (i = 0; i < scinfo->num_vcpus; i++) { > + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; > + vcpus[i].u.rtds.period = scinfo->vcpus[i].period; > + vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget; > + } > + } > + You could have written this hunk like this: if (scinfo->num_vcpus <= 0) { rc = ERROR_INVAL; goto out; } for (i = 0; i < scinfo->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; } rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period, scinfo->vcpus[i].budget); if (rc) { rc = ERROR_INVAL; goto out; } } GCNEW_ARRAY(vcpus, scinfo->num_vcpus); for (i = 0; i < scinfo->num_vcpus; i++) { vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; vcpus[i].u.rtds.period = scinfo->vcpus[i].period; vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget; } But, you original code is still OK. This is just FYI. No need to resend just because of this. > + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, > + vcpus, scinfo->num_vcpus); > + if (r != 0) { > + LOGE(ERROR, "setting vcpu sched rtds"); > + rc = ERROR_FAIL; > + goto out; > + } > + rc = 0; > +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) { > + rc = ERROR_INVAL; > + goto out; > + } else { > + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, > + scinfo->vcpus[0].budget)) { > + rc = ERROR_INVAL; > + goto out; > + } > + num_vcpus = max_vcpuid + 1; > + GCNEW_ARRAY(vcpus, num_vcpus); > + for (i = 0; i < num_vcpus; i++) { > + vcpus[i].vcpuid = i; > + vcpus[i].u.rtds.period = scinfo->vcpus[0].period; > + vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget; > + } > + } Same here, the else branch can be simplified. Again, it's just FYI. No need to resend.
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > Hey, Good job indeed, Chong, this is much better. Now, I appreciate that Wei already Acked this, but nevertheless, I have some comments. I'll put them down here, and then it will be up to maintainers and committers to figure out whether they need to be addressed or not (Wei? Ian?) > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5770,6 +5770,201 @@ 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) > +{ > + int rc; > + > + if (period < 1) { > + LOG(ERROR, "VCPU period is out of range, " > + "valid values are larger than or equal to 1"); > These strings are really better if kept on one line, and it does not look impossible in this case: LOG(ERROR, "Invalid VCPU period of %d (it should be >= 1)", period); > + rc = ERROR_INVAL; /* error scheduling parameter */ > The comment should go away. > + goto out; > + } > + > + if (budget < 1) { > + LOG(ERROR, "VCPU budget is not set or out of range, " > + "valid values are larger than or equal to 1"); > LOG(ERROR, "Invalid VCPU budget of %d (it should be >= 1)", budget); > + rc = ERROR_INVAL; > + goto out; > + } > + > + if (budget > period) { > + LOG(ERROR, "VCPU budget must be smaller than " > + "or equal to VCPU period"); > LOG(ERROR, "VCPU budget must be smaller than period, but %d > %d", budget, period); > +/* 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; > + > + 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) { > + rc = ERROR_INVAL; > + goto out; > + } else { > Personally, I consider what Wei suggested for avoiding, at once, one more level of indentation, as well as this really really ugly if { goto } else { } rather important to do. But then again, I'm not the maintainer of this code, so if it's fine for them, then fine. :-) > + for (i = 0; i < scinfo->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); > LOG(ERROR, "Invalid VCPU %d: valid range is [0, %d]", scinfo->vcpus[i].vcpuid, info.max_vcpu_id); > +/* 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) > Indentation? It seems to me that it just fits, even if done properly: 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 != 1) { > + rc = ERROR_INVAL; > + goto out; > + } else { > Same as above, of course. > @@ -5802,30 +5997,10 @@ 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"); > + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && > + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) > + if (sched_rtds_validate_params(gc, scinfo->period, scinfo- > >budget)) > return ERROR_INVAL; > I'm not sure I understand. What's happening in this function? As it stands after this patch, it looks to me that: - we read the default scheduling parameter from Xen, via xc_sched_rtds_domain_get() - we (possibly, if both are non-default) validate a new period and budget couple of values - we don't use such values for anything, and set back what we got from Xen, via xc_sched_rtds_domain_set() Either I'm missing something very basic, or this is not what Wei said when reviewing v6: "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;" Is it? Thanks and Regards, Dario
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > And a couple more things that I forgot. I'd shorten the subject line, as already suggested for libxc. > +/* 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) > +{ I think this should be called static int sched_rtds_vcpu_params_get(libxl__gc *gc, uint32_t domid, libxl_vcpu_sched_params *scinfo) for homogeneity with the _set counterpart here below: > @@ -5873,6 +6048,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) > +{ So, this function takes a libxl_vcpu_sched_params*, which is basically an array of vcpus' parameters, one element per vcpu (and it can be sparse). Are there any restrictions on how such array should be constructed, for calling this function? Are they any special meaning of particular configurations of the array? I think at least the latter is true, in fact: - this calls sched_rtds_vcpus_params_set(); - in there, if the array is empty, we fail; - if the array has one or more elements, we deal with the vcpus (and just them) specified in the elements of the array itself. Then there is libxl_vcpu_sched_params_set_all(). That one: - calls sched_rtds_vcpus_params_set_all(); - in there, if the array has more than just one element, we fail. And then the get side, with libxl_vcpu_sched_params_get(). That one: - calls sched_rtds_vcpu_get(); - in there, if the array is empty, we create one, and return info for all the vcpus; - if the array has one or more elements, we deal with them (and just them). I think this should be documented some (unless it's already there and I missed it). Thanks and Regards again, Dario
On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: >> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set >> functions to support per-VCPU settings. >> >> +/* 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) >> > Indentation? If I follow the indentation rule, the second line would be longer than 80 characters. The function name is just too long. > >> @@ -5802,30 +5997,10 @@ 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"); >> + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && >> + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) >> + if (sched_rtds_validate_params(gc, scinfo->period, scinfo- >> >budget)) >> return ERROR_INVAL; >> > I'm not sure I understand. What's happening in this function? > > As it stands after this patch, it looks to me that: > - we read the default scheduling parameter from Xen, > via xc_sched_rtds_domain_get() > - we (possibly, if both are non-default) validate a new period and > budget couple of values > - we don't use such values for anything, and set back what we got > from Xen, via xc_sched_rtds_domain_set() > > Either I'm missing something very basic, or this is not what Wei said > when reviewing v6: > > "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;" > > Is it? The current RTDS (xen 4.6) is: 1) Read the (per-domain) scheduling params from Xen, and store them to sdom 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is -1), use sdom.period; else sdom.period = new period (if new period is valid); If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is -1), use sdom.budget; else sdom.budget = new budget; 3) xc_sched_rtds_domain_set (sdom); In our patch, my plan is: 1) Read the default params from Xen, and store them to sdom 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is -1), use sdom.period; else sdom.period = new period (if new period is valid); If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is -1), use sdom.budget; else sdom.budget = new budget; 3) xc_sched_rtds_domain_set (sdom); Even though I made some mistakes in this post (forgot the two "else"s in my plan), is this plan ok? Chong
On Thu, 2016-03-17 at 14:50 -0500, Chong Li wrote: > On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > > > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > > > > > > +/* 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) > > > > > Indentation? > If I follow the indentation rule, the second line would be longer > than > 80 characters. > The function name is just too long. > static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, const libxl_vcpu_sched_params *scinfo) or static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, const libxl_vcpu_sched_params *scinfo) or shorten the name. From quickly looking at libxl, neither of the first two proposed solutions seems to happen much, so I recommend shortening the name a bit. It's an internal function, so we can do that pretty freely. > > > 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"); > > > + if (scinfo->period != > > > LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && > > > + scinfo->budget != > > > LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) > > > + if (sched_rtds_validate_params(gc, scinfo->period, > > > scinfo- > > > > > > > > budget)) > > > return ERROR_INVAL; > > > > > I'm not sure I understand. What's happening in this function? > > > > As it stands after this patch, it looks to me that: > > - we read the default scheduling parameter from Xen, > > via xc_sched_rtds_domain_get() > > - we (possibly, if both are non-default) validate a new period and > > budget couple of values > > - we don't use such values for anything, and set back what we got > > from Xen, via xc_sched_rtds_domain_set() > > > > Either I'm missing something very basic, or this is not what Wei > > said > > when reviewing v6: > > > > "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;" > > > > Is it? > The current RTDS (xen 4.6) is: > > 1) Read the (per-domain) scheduling params from Xen, and store them > to sdom > 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is > -1), use sdom.period; > else sdom.period = new period (if new period is valid); > If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which > is > -1), use sdom.budget; > else sdom.budget = new budget; > 3) xc_sched_rtds_domain_set (sdom); > Sort of, let me restate it more generally: 1) get currently set period and budget; 2) if scinfo->period is not PERIOD_DEFAULT and is valid, update the period; 3) if scinfo->budget is not BUDGET_DEFAULT and is valid, update the budget However that happens in terms of where the values of budget and period are stashed, what call to xc_sched_rtds_domain_{get,set}() are made, etc. So, basically, when you say "if period equals PERIOD_DEFAULT", you mean scinfo->period, i.e., the period being set is just the default libxl value for it, which in turns means (most likely) one want to only alter the budget. > In our patch, my plan is: > 1) Read the default params from Xen, and store them to sdom > 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is > -1), use sdom.period; > else sdom.period = new period (if new period is valid); > If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which > is > -1), use sdom.budget; > else sdom.budget = new budget; > 3) xc_sched_rtds_domain_set (sdom); > > Even though I made some mistakes in this post (forgot the two "else"s > in my plan), is this plan ok? > Your plan should be to leve things exactly as they are, from a functional/logical perspective. The difference between "(per-domain) scheduling params from Xen" of right now, and "the default params from Xen" of after the patch, is of no concern here, as it's all done by Xen. So, really, this should be _all_ about taking the chance of refactoring the code such as the validating function does not have weird side effects, not about changing the logic, which looks fine to me. Regards, Dario -- <<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..0f9fb7e 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5770,6 +5770,201 @@ 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) +{ + int rc; + + 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; + } + + 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; + } + + if (budget > period) { + LOG(ERROR, "VCPU budget must be smaller than " + "or equal to VCPU period"); + rc = ERROR_INVAL; + goto out; + } + rc = 0; +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].u.rtds.period; + scinfo->vcpus[i].budget = vcpus[i].u.rtds.budget; + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; + } + rc = 0; +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; + + 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) { + rc = ERROR_INVAL; + goto out; + } else { + for (i = 0; i < scinfo->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; + } + rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period, + scinfo->vcpus[i].budget); + if (rc) { + rc = ERROR_INVAL; + goto out; + } + } + GCNEW_ARRAY(vcpus, scinfo->num_vcpus); + for (i = 0; i < scinfo->num_vcpus; i++) { + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + vcpus[i].u.rtds.period = scinfo->vcpus[i].period; + vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget; + } + } + + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, scinfo->num_vcpus); + if (r != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + rc = 0; +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) { + rc = ERROR_INVAL; + goto out; + } else { + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, + scinfo->vcpus[0].budget)) { + rc = ERROR_INVAL; + goto out; + } + num_vcpus = max_vcpuid + 1; + GCNEW_ARRAY(vcpus, num_vcpus); + for (i = 0; i < num_vcpus; i++) { + vcpus[i].vcpuid = i; + vcpus[i].u.rtds.period = scinfo->vcpus[0].period; + vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget; + } + } + + 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; + } + rc = 0; +out: + return rc; +} + static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5802,30 +5997,10 @@ 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"); + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) + if (sched_rtds_validate_params(gc, scinfo->period, scinfo->budget)) 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"); - return ERROR_INVAL; - } rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom); if (rc < 0) { @@ -5873,6 +6048,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 +6150,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..df35d09 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'}),