|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |