Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler

[Wei Liu <wei.liu2@citrix.com>]

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
>