| Message ID | 1454626244-5511-4-git-send-email-lichong659@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > I will need Dario or George to review the logic of the code. If some of the comments below don't make sense, just ask. I'm sure I make stupid comments at times. > 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> > > --- > Changes on PATCH v4: > 1) Coding style changes > > Changes on PATCH v3: > 1) Add sanity check on vcpuid > > 2) Add comments on per-domain and per-vcpu functions for libxl > users > > Changes on PATCH v2: > 1) New data structure (libxl_vcpu_sched_params and libxl_sched_params) > to help per-VCPU settings. > > 2) sched_rtds_vcpu_get now can return a random subset of the parameters > of the VCPUs of a specific domain. > > CC: <dario.faggioli@citrix.com> > CC: <george.dunlap@eu.citrix.com> > CC: <dgolomb@seas.upenn.edu> > CC: <mengxu@cis.upenn.edu> > CC: <wei.liu2@citrix.com> > CC: <lichong659@gmail.com> > CC: <ian.jackson@eu.citrix.com> > CC: <ian.campbell@eu.citrix.com> > --- > tools/libxl/libxl.c | 249 ++++++++++++++++++++++++++++++++++++++++---- > tools/libxl/libxl.h | 19 ++++ > tools/libxl/libxl_types.idl | 16 +++ > 3 files changed, 262 insertions(+), 22 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index bd3aac8..ac4a103 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5770,6 +5770,151 @@ 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. > +{ > + 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"); > + return 1; /* error scheduling parameter */ Though this is internal function I would very like it to stick to CODING_STYLE in libxl. In this particular case, the error handling should be using goto and the return value should be a ERROR_* value. BTW there is no upper bound check for this value? Just asking -- I don't know enough to judge. > + } > + *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"); > + return 1; Same here. > + } > + *sdom_budget = budget; > + } > + > + if (budget > period) { > + LOG(ERROR, "VCPU budget must be smaller than " > + "or equal to VCPU period"); > + return 1; > + } > + > + return 0; /* period and budget are valid */ > +} > + > +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ > + uint32_t num_vcpus; > + int rc, i; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + > + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); According to CODING_STYLE, the return value of a system call or libxc call should be called "r"; > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; Same here, please use goto style error handling. > + } > + > + num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus : > + info.max_vcpu_id + 1; > + Please document the semantics of this function. > + 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) { Indentation. > + 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 = scinfo->vcpus[i].vcpuid; > + } else The "}" doesn't seem to match the preceding "if". Either this doesn't compile or the indentation is confusing. > + for (i=0; i < num_vcpus; i++) > + vcpus[i].vcpuid = i; > + > + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); > + if (rc != 0) { > + LOGE(ERROR, "getting vcpu sched rtds"); > + return ERROR_FAIL; > + } > + 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 0; > +} > + > +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) > +{ Again, please document the semantics of this function. > + int rc; int r; And please use it to store return value from xc_ functions. > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + uint32_t num_vcpus; > + > + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; Please use goto style error handling. > + } > + 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); > + return ERROR_INVAL; > + } > + 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) > + return ERROR_INVAL; > + } > + } else { > + num_vcpus = max_vcpuid + 1; > + GCNEW_ARRAY(vcpus, num_vcpus); > + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, > + scinfo->vcpus[0].budget, This doesn't make sense. You take this path because scinfo->num_vcpus is 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? > + &vcpus[0].s.rtds.period, > + &vcpus[0].s.rtds.budget)) Indentation. > + return ERROR_INVAL; > + 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; > + } > + } > + > + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid, > + vcpus, num_vcpus); Indentation. > + if (rc != 0) { > + LOGE(ERROR, "setting vcpu sched rtds"); > + return ERROR_FAIL; > + } > + > + return rc; > +} > + > static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, > libxl_domain_sched_params *scinfo) > { > @@ -5803,29 +5948,9 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, > 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) { > @@ -5836,6 +5961,11 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, > return 0; > } > > +/* 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 *scinfo) > { > @@ -5873,6 +6003,47 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, > return ret; > } > > +/* Set the per-vcpu scheduling parameters */ > +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 ret; ret => rc please. > + > + if (sched == LIBXL_SCHEDULER_UNKNOWN) > + sched = libxl__domain_scheduler(gc, domid); > + > + switch (sched) { > + case LIBXL_SCHEDULER_SEDF: > + LOG(ERROR, "SEDF scheduler no longer available"); > + ret=ERROR_FEATURE_REMOVED; Space before and after "=". > + break; > + case LIBXL_SCHEDULER_CREDIT: > + case LIBXL_SCHEDULER_CREDIT2: > + case LIBXL_SCHEDULER_ARINC653: > + LOG(ERROR, "per-VCPU parameter setting " > + "not supported for this scheduler"); Join these two lines please. > + ret = ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_RTDS: > + ret = sched_rtds_vcpu_set(gc, domid, scinfo); > + break; > + default: > + LOG(ERROR, "Unknown scheduler"); > + ret = ERROR_INVAL; > + break; > + } > + > + GC_FREE; > + return ret; > +} > + > +/* 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. > +*/ Indentation. > int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, > libxl_domain_sched_params *scinfo) > { > @@ -5907,6 +6078,40 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, > return ret; > } > > +/* Get the per-vcpu scheduling parameters */ > +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) Indentation. > +{ > + GC_INIT(ctx); > + int ret; According to CODING_STYLE, the return value should be called rc. > + > + scinfo->sched = libxl__domain_scheduler(gc, domid); > + > + switch (scinfo->sched) { > + case LIBXL_SCHEDULER_SEDF: > + LOG(ERROR, "SEDF scheduler no longer available"); is no longer available > + ret=ERROR_FEATURE_REMOVED; Space before and after "=" please. > + break; > + case LIBXL_SCHEDULER_CREDIT: > + case LIBXL_SCHEDULER_CREDIT2: > + case LIBXL_SCHEDULER_ARINC653: > + LOG(ERROR, "per-VCPU parameter getting " > + "not supported for this scheduler"); Please join the two string into one. It would be easier for grepping. > + ret = ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_RTDS: > + ret = sched_rtds_vcpu_get(gc, domid, scinfo); > + break; > + default: > + LOG(ERROR, "Unknown scheduler"); > + ret = ERROR_INVAL; > + break; > + } > + > + GC_FREE; > + return ret; > +} > + > 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..4ba30d3 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -206,6 +206,18 @@ > #define LIBXL_HAVE_DEVICE_MODEL_USER 1 > > /* > + * libxl_vcpu_sched_params is used to store per-vcpu params. > + * The 'vcpuid' field specifies the vcpu to be set or read. The second sentence doesn't seem to be particularly useful. I think deleting it would be fine. The semantics of the function is better documented in the comment preceding the function prototype or the implementation. > +*/ > +#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,10 +1659,17 @@ 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 > + > int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, > libxl_domain_sched_params *params); > int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, > const libxl_domain_sched_params *params); > +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, > + libxl_vcpu_sched_params *params); Indentation. > +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, > + const libxl_vcpu_sched_params *params); > Indentation. > int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid, > libxl_trigger trigger, uint32_t vcpuid); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index cf3730f..4e7210e 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -378,6 +378,22 @@ 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'}), > + ("slice", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}), > + ("latency", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_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'}), > -- > 1.9.1 >
On Fri, 2016-02-05 at 14:44 +0000, Wei Liu wrote: > On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote: > > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > > functions to support per-VCPU settings. > > > > I will need Dario or George to review the logic of the code. > Sure, it's on my short TODO list. It's either going to be today or Monday. > If some of the comments below don't make sense, just ask. I'm sure I > make stupid comments at times. > Yeah, I'm sure you've said plenty of stupid things! ;-P ;-P > > +{ > > + 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"); > > + return 1; /* error scheduling parameter */ > > Though this is internal function I would very like it to stick to > CODING_STYLE in libxl. In this particular case, the error handling > should be using goto and the return value should be a ERROR_* value. > > BTW there is no upper bound check for this value? Just asking -- I > don't > know enough to judge. > It's checked in the hypervisor. As usual, in these cases, checking in tools as well would make things more robust, allow better error reporting, etc, _BUT_ it would require to keep the limits in sync, which is undesirable. So, as long as type-related confusion is not a possibility, I would be ok with no checks here in libxl. And just to be sure that we are on the safe side wrt that: in Xen these values are uint32, should we use uint32 here as well (in the idl, instead of 'integer')? > > + } > > + 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); > > + return ERROR_INVAL; > > + } > > + 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) > > + return ERROR_INVAL; > > + } > > + } else { > > + num_vcpus = max_vcpuid + 1; > > + GCNEW_ARRAY(vcpus, num_vcpus); > > + if (sched_rtds_validate_params(gc, scinfo- > > >vcpus[0].period, > > + scinfo->vcpus[0].budget, > > This doesn't make sense. You take this path because scinfo->num_vcpus > is > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? > IIRC, the idea here may be that this is how we set all the vcpus parameters to the same values... But I'll get back to this when properly reviewing the series. Thanks and Regards, Dario
On Fri, Feb 05, 2016 at 04:59:43PM +0100, Dario Faggioli wrote: > On Fri, 2016-02-05 at 14:44 +0000, Wei Liu wrote: > > On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote: > > > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > > > functions to support per-VCPU settings. > > > > > > > I will need Dario or George to review the logic of the code. > > > Sure, it's on my short TODO list. It's either going to be today or > Monday. > > > If some of the comments below don't make sense, just ask. I'm sure I > > make stupid comments at times. > > > Yeah, I'm sure you've said plenty of stupid things! ;-P ;-P > Yeah. My trick is that when I say too many stupid things people don't know which one to remember so I'm safe. :-) > > > +{ > > > + 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"); > > > + return 1; /* error scheduling parameter */ > > > > Though this is internal function I would very like it to stick to > > CODING_STYLE in libxl. In this particular case, the error handling > > should be using goto and the return value should be a ERROR_* value. > > > > BTW there is no upper bound check for this value? Just asking -- I > > don't > > know enough to judge. > > > It's checked in the hypervisor. As usual, in these cases, checking in > tools as well would make things more robust, allow better error > reporting, etc, _BUT_ it would require to keep the limits in sync, > which is undesirable. > > So, as long as type-related confusion is not a possibility, I would be > ok with no checks here in libxl. > > And just to be sure that we are on the safe side wrt that: in Xen these > values are uint32, should we use uint32 here as well (in the idl, > instead of 'integer')? > > > > + } > > > + 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); > > > + return ERROR_INVAL; > > > + } > > > + 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) > > > + return ERROR_INVAL; > > > + } > > > + } else { > > > + num_vcpus = max_vcpuid + 1; > > > + GCNEW_ARRAY(vcpus, num_vcpus); > > > + if (sched_rtds_validate_params(gc, scinfo- > > > >vcpus[0].period, > > > + scinfo->vcpus[0].budget, > > > > This doesn't make sense. You take this path because scinfo->num_vcpus > > is > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? > > > IIRC, the idea here may be that this is how we set all the vcpus > parameters to the same values... But I'll get back to this when > properly reviewing the series. > It's one thing that when ->num_vcpus == 0 you allocate array, it's another when the array is non-NULL but num_vcpus == 0. Such usage is bad. What if I need to iterate through the array at some point? How do you know if it is really a NULL array? Wei. > Thanks and 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) >
I'll fix these coding style issues. On Fri, Feb 5, 2016 at 8:44 AM, Wei Liu <wei.liu2@citrix.com> wrote: > On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote: >> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set >> functions to support per-VCPU settings. >> > > I will need Dario or George to review the logic of the code. > > If some of the comments below don't make sense, just ask. I'm sure I > make stupid comments at times. > >> 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> >> >> --- >> Changes on PATCH v4: >> 1) Coding style changes >> >> Changes on PATCH v3: >> 1) Add sanity check on vcpuid >> >> 2) Add comments on per-domain and per-vcpu functions for libxl >> users >> >> Changes on PATCH v2: >> 1) New data structure (libxl_vcpu_sched_params and libxl_sched_params) >> to help per-VCPU settings. >> >> 2) sched_rtds_vcpu_get now can return a random subset of the parameters >> of the VCPUs of a specific domain. >> >> CC: <dario.faggioli@citrix.com> >> CC: <george.dunlap@eu.citrix.com> >> CC: <dgolomb@seas.upenn.edu> >> CC: <mengxu@cis.upenn.edu> >> CC: <wei.liu2@citrix.com> >> CC: <lichong659@gmail.com> >> CC: <ian.jackson@eu.citrix.com> >> CC: <ian.campbell@eu.citrix.com> >> --- >> tools/libxl/libxl.c | 249 ++++++++++++++++++++++++++++++++++++++++---- >> tools/libxl/libxl.h | 19 ++++ >> tools/libxl/libxl_types.idl | 16 +++ >> 3 files changed, 262 insertions(+), 22 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index bd3aac8..ac4a103 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> + >> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid, >> + const libxl_vcpu_sched_params *scinfo) >> +{ > > Again, please document the semantics of this function. > >> + int rc; > > int r; > > And please use it to store return value from xc_ functions. > >> + int i; >> + uint16_t max_vcpuid; >> + xc_dominfo_t info; >> + struct xen_domctl_schedparam_vcpu *vcpus; >> + uint32_t num_vcpus; >> + >> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); >> + if (rc < 0) { >> + LOGE(ERROR, "getting domain info"); >> + return ERROR_FAIL; > > Please use goto style error handling. > >> + } >> + 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); >> + return ERROR_INVAL; >> + } >> + 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) >> + return ERROR_INVAL; >> + } >> + } else { >> + num_vcpus = max_vcpuid + 1; >> + GCNEW_ARRAY(vcpus, num_vcpus); >> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, >> + scinfo->vcpus[0].budget, > > This doesn't make sense. You take this path because scinfo->num_vcpus is > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000" (which sets all vcpus with the same scheduling parameters), we pass the budget and period via scinfo->vcpus[0]. I'll add more explanation here. > >> + &vcpus[0].s.rtds.period, >> + &vcpus[0].s.rtds.budget)) >
On Fri, Feb 05, 2016 at 06:10:33PM -0600, Chong Li wrote: > I'll fix these coding style issues. > Thank you. :-) [...] > >> + num_vcpus = max_vcpuid + 1; > >> + GCNEW_ARRAY(vcpus, num_vcpus); > >> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, > >> + scinfo->vcpus[0].budget, > > > > This doesn't make sense. You take this path because scinfo->num_vcpus is > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? > For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000" > (which sets all vcpus with > the same scheduling parameters), we pass the budget and period via > scinfo->vcpus[0]. > > I'll add more explanation here. No, adding more explanation won't help. Let me explain a bit. Libxl is the library that can be used by multiple applications. Xl is just one of the applications. The other application that I know of is libvirt. So, the incarnation of a particular xl command is of no concern how we define the semantics of a libxl API. That is, you can come up with an unambiguous API but still support the same xl command. Currently the semantics of this (new?) libxl API seems to be broken, because you're (ab)using num_vcpus to represent a special case. In effect you can't really whether the array is empty. When num_vcpus is 0, you shouldn't dereference vcpus array, at all, because the semantics of num_vcpus == 0 is that the array is empty. Wei. > > > >> + &vcpus[0].s.rtds.period, > >> + &vcpus[0].s.rtds.budget)) > > > > -- > Chong Li > Department of Computer Science and Engineering > Washington University in St.louis
On Mon, Feb 8, 2016 at 5:07 AM, Wei Liu <wei.liu2@citrix.com> wrote: > > > [...] > > >> + num_vcpus = max_vcpuid + 1; > > >> + GCNEW_ARRAY(vcpus, num_vcpus); > > >> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, > > >> + scinfo->vcpus[0].budget, > > > > > > This doesn't make sense. You take this path because scinfo->num_vcpus is > > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? > > For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000" > > (which sets all vcpus with > > the same scheduling parameters), we pass the budget and period via > > scinfo->vcpus[0]. > > > > I'll add more explanation here. > > No, adding more explanation won't help. > > Let me explain a bit. Libxl is the library that can be used by multiple > applications. Xl is just one of the applications. The other application > that I know of is libvirt. > > So, the incarnation of a particular xl command is of no concern how we > define the semantics of a libxl API. That is, you can come up with an > unambiguous API but still support the same xl command. > > Currently the semantics of this (new?) libxl API seems to be broken, > because you're (ab)using num_vcpus to represent a special case. In > effect you can't really whether the array is empty. When num_vcpus is 0, > you shouldn't dereference vcpus array, at all, because the semantics of > num_vcpus == 0 is that the array is empty. > > Wei. I see. I'll think about re-designing the data structure of libxl_vcpu_sched_params. Chong > > > > > > >> + &vcpus[0].s.rtds.period, > > >> + &vcpus[0].s.rtds.budget)) > > > > > > > -- > > Chong Li > > Department of Computer Science and Engineering > > Washington University in St.louis
On Mon, Feb 08, 2016 at 04:59:46PM -0600, Chong Li wrote: > On Mon, Feb 8, 2016 at 5:07 AM, Wei Liu <wei.liu2@citrix.com> wrote: > > > > > > [...] > > > >> + num_vcpus = max_vcpuid + 1; > > > >> + GCNEW_ARRAY(vcpus, num_vcpus); > > > >> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, > > > >> + scinfo->vcpus[0].budget, > > > > > > > > This doesn't make sense. You take this path because scinfo->num_vcpus is > > > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything? > > > For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000" > > > (which sets all vcpus with > > > the same scheduling parameters), we pass the budget and period via > > > scinfo->vcpus[0]. > > > > > > I'll add more explanation here. > > > > No, adding more explanation won't help. > > > > Let me explain a bit. Libxl is the library that can be used by multiple > > applications. Xl is just one of the applications. The other application > > that I know of is libvirt. > > > > So, the incarnation of a particular xl command is of no concern how we > > define the semantics of a libxl API. That is, you can come up with an > > unambiguous API but still support the same xl command. > > > > Currently the semantics of this (new?) libxl API seems to be broken, > > because you're (ab)using num_vcpus to represent a special case. In > > effect you can't really whether the array is empty. When num_vcpus is 0, > > you shouldn't dereference vcpus array, at all, because the semantics of > > num_vcpus == 0 is that the array is empty. > > > > Wei. > > I see. I'll think about re-designing the data structure of > libxl_vcpu_sched_params. > Or you can come up with a new API (function) that sets parameter for all vcpus at once? Just a random thought. You can post your proposed data structure and / or the API here. We can discuss this a bit before you actually start writing code, so that you avoid wasting effort. Wei. > Chong > > > > > > > > > >> + &vcpus[0].s.rtds.period, > > > >> + &vcpus[0].s.rtds.budget)) > > > > > > > > > > -- > > > Chong Li > > > Department of Computer Science and Engineering > > > Washington University in St.louis > > > > > -- > Chong Li > Department of Computer Science and Engineering > Washington University in St.louis
On Tue, 2016-02-09 at 10:19 +0000, Wei Liu wrote: > On Mon, Feb 08, 2016 at 04:59:46PM -0600, Chong Li wrote: > > > > I see. I'll think about re-designing the data structure of > > libxl_vcpu_sched_params. > > > > Or you can come up with a new API (function) that sets parameter for > all > vcpus at once? Just a random thought. > Exactly! > You can post your proposed data structure and / or the API here. We > can discuss this a bit before you actually start writing code, so > that > you avoid wasting effort. > I'm looking at the patches right now, and I don't think there is the need to redesign the data structures! I'll send in my comments as soon as I'm done... Dario
On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote: > > +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params > *scinfo) > I'd call this sched_rtds_vcpus_params_set(). I know, it's longer, which is bad, but it's a lot more evident what it does. > +{ > + int rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + uint32_t num_vcpus; > + > + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; > + } > + 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); > + return ERROR_INVAL; > + } > + 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) > + return ERROR_INVAL; > + } > + } else { > So, it looks to me that this function can be split in two. One would be the actual sched_rtds_vcpus_params_set(), and it will do what is being done above here. The other one would be something like sched_rtds_vcpus_params_set_all(), and it will do what is being done below here. About scinfo->num_vcpus, I think it would be fine for sched_rtds_vcpus_params_set() to enforce it being > 0, and erroring out if not. On the other hand, in sched_rtds_vcpus_params_set_all(), since the semantic is "use this set of params for all vcpus", I think it would be fine to enforce scinfo->num_vcpus == 1 (and maybe even scinfo.vcpus[0].vcpuid == LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT). Now, for external callers (like xl, but also like any other toolstack wanting to build on top of libxl). If you think a 'set all vcpus' function would be useufl (as it is probably the case), you can define a libxl API function called libxl_vcpus_params_set_all(), doing exactly the same thing that libxl_vcpus_params_set() is doing, but calling the sched_rtds_vcpus_params_set_all() internal function. Chong, do you think this could work? Wei, what do you think of the resulting API? > + 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)) > + return ERROR_INVAL; > + 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; > + } > + } > + > + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid, > + vcpus, num_vcpus); > + if (rc != 0) { > + LOGE(ERROR, "setting vcpu sched rtds"); > + return ERROR_FAIL; > + } > + > + return rc; > +} > @@ -5836,6 +5961,11 @@ static int sched_rtds_domain_set(libxl__gc > *gc, uint32_t domid, > return 0; > } > > +/* 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 > *scinfo) > I think this comment would be better put in libxl.h. > @@ -5873,6 +6003,47 @@ int libxl_domain_sched_params_set(libxl_ctx > *ctx, uint32_t domid, > > +/* 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 *scinfo) > (same as above: move in libxl.h) > diff --git a/tools/libxl/libxl_types.idl > b/tools/libxl/libxl_types.idl > index cf3730f..4e7210e 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -378,6 +378,22 @@ 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'}), > + ("slice", integer, {'init_val': > 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}), > + ("latency", integer, {'init_val': > 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}), > + ("extratime", integer, {'init_val': > 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}), > So, 'slice', 'latency' and 'extratime' are not in use by any scheduler. They were for SEDF, which is now removed from all the places where we could remove it, and deprecated elsewhere (e.g., in the definition of libxl_domain_sched_params, here in this file). So, unless we want to keep this struct sort-of in sync with libxl_domain_sched_params, I think we don't need to have these fields here. I defer this to the tools maintainers, in case there is something I'm missing, but I'd say we can get rid of them from here. > + ("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'}), > (Apart from the nit above) This looks ok to me as a set of data structures. Regards, Dario
On Tue, Feb 9, 2016 at 6:00 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote: >> >> +{ >> + int rc; >> + int i; >> + uint16_t max_vcpuid; >> + xc_dominfo_t info; >> + struct xen_domctl_schedparam_vcpu *vcpus; >> + uint32_t num_vcpus; >> + >> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); >> + if (rc < 0) { >> + LOGE(ERROR, "getting domain info"); >> + return ERROR_FAIL; >> + } >> + 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); >> + return ERROR_INVAL; >> + } >> + 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) >> + return ERROR_INVAL; >> + } >> + } else { >> > So, it looks to me that this function can be split in two. One would be > the actual sched_rtds_vcpus_params_set(), and it will do what is being > done above here. > > The other one would be something like > sched_rtds_vcpus_params_set_all(), and it will do what is being done > below here. > > About scinfo->num_vcpus, I think it would be fine for > sched_rtds_vcpus_params_set() to enforce it being > 0, and erroring out > if not. > > On the other hand, in sched_rtds_vcpus_params_set_all(), since the > semantic is "use this set of params for all vcpus", I think it would be > fine to enforce scinfo->num_vcpus == 1 (and maybe even > scinfo.vcpus[0].vcpuid == LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT). > > > Now, for external callers (like xl, but also like any other toolstack > wanting to build on top of libxl). > > If you think a 'set all vcpus' function would be useufl (as it is > probably the case), you can define a libxl API function called > libxl_vcpus_params_set_all(), doing exactly the same thing that > libxl_vcpus_params_set() is doing, but calling the > sched_rtds_vcpus_params_set_all() internal function. > > Chong, do you think this could work? I think it would work. Thanks for this suggestion. Chong
On Tue, Feb 09, 2016 at 01:00:37PM +0100, Dario Faggioli wrote: [...] > So, it looks to me that this function can be split in two. One would be > the actual sched_rtds_vcpus_params_set(), and it will do what is being > done above here. > > The other one would be something like > sched_rtds_vcpus_params_set_all(), and it will do what is being done > below here. > > About scinfo->num_vcpus, I think it would be fine for > sched_rtds_vcpus_params_set() to enforce it being > 0, and erroring out > if not. > > On the other hand, in sched_rtds_vcpus_params_set_all(), since the > semantic is "use this set of params for all vcpus", I think it would be > fine to enforce scinfo->num_vcpus == 1 (and maybe even > scinfo.vcpus[0].vcpuid == LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT). > > > Now, for external callers (like xl, but also like any other toolstack > wanting to build on top of libxl). > > If you think a 'set all vcpus' function would be useufl (as it is > probably the case), you can define a libxl API function called > libxl_vcpus_params_set_all(), doing exactly the same thing that > libxl_vcpus_params_set() is doing, but calling the > sched_rtds_vcpus_params_set_all() internal function. > > Chong, do you think this could work? > Wei, what do you think of the resulting API? Introducing a _all function sounds reasonable. Wei.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index bd3aac8..ac4a103 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5770,6 +5770,151 @@ 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) +{ + 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"); + return 1; /* error scheduling parameter */ + } + *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"); + return 1; + } + *sdom_budget = budget; + } + + if (budget > period) { + LOG(ERROR, "VCPU budget must be smaller than " + "or equal to VCPU period"); + return 1; + } + + return 0; /* period and budget are valid */ +} + +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + uint32_t num_vcpus; + int rc, i; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (rc < 0) { + LOGE(ERROR, "getting domain info"); + return ERROR_FAIL; + } + + 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); + return ERROR_INVAL; + } + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + } else + for (i=0; i < num_vcpus; i++) + vcpus[i].vcpuid = i; + + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); + if (rc != 0) { + LOGE(ERROR, "getting vcpu sched rtds"); + return ERROR_FAIL; + } + 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 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; + struct xen_domctl_schedparam_vcpu *vcpus; + uint32_t num_vcpus; + + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (rc < 0) { + LOGE(ERROR, "getting domain info"); + return ERROR_FAIL; + } + 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); + return ERROR_INVAL; + } + 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) + return ERROR_INVAL; + } + } else { + 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)) + return ERROR_INVAL; + 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; + } + } + + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, num_vcpus); + if (rc != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + return ERROR_FAIL; + } + + return rc; +} + static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5803,29 +5948,9 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, 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) { @@ -5836,6 +5961,11 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, return 0; } +/* 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 *scinfo) { @@ -5873,6 +6003,47 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, return ret; } +/* 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 = scinfo->sched; + int ret; + + if (sched == LIBXL_SCHEDULER_UNKNOWN) + sched = libxl__domain_scheduler(gc, domid); + + switch (sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + ret=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"); + ret = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + ret = sched_rtds_vcpu_set(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + ret = ERROR_INVAL; + break; + } + + GC_FREE; + return ret; +} + +/* 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 *scinfo) { @@ -5907,6 +6078,40 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, return ret; } +/* 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 = libxl__domain_scheduler(gc, domid); + + switch (scinfo->sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + ret=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"); + ret = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + ret = sched_rtds_vcpu_get(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + ret = ERROR_INVAL; + break; + } + + GC_FREE; + return ret; +} + 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..4ba30d3 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -206,6 +206,18 @@ #define LIBXL_HAVE_DEVICE_MODEL_USER 1 /* + * libxl_vcpu_sched_params is used to store per-vcpu params. + * The 'vcpuid' field specifies the vcpu to be set or read. +*/ +#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,10 +1659,17 @@ 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 + int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, libxl_domain_sched_params *params); int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, const libxl_domain_sched_params *params); +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *params); +int libxl_vcpu_sched_params_set(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); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index cf3730f..4e7210e 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -378,6 +378,22 @@ 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'}), + ("slice", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}), + ("latency", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_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'}),