|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
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@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
>
> ---
> 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@xxxxxxxxxx>
> CC: <george.dunlap@xxxxxxxxxxxxx>
> CC: <dgolomb@xxxxxxxxxxxxxx>
> CC: <mengxu@xxxxxxxxxxxxx>
> CC: <wei.liu2@xxxxxxxxxx>
> CC: <lichong659@xxxxxxxxx>
> CC: <ian.jackson@xxxxxxxxxxxxx>
> CC: <ian.campbell@xxxxxxxxxxxxx>
> ---
> 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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |