Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-05-25 at 19:09 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support
> per-VCPU settings for RTDS scheduler.
> 
> Add a new data structure (libxl_vcpu_sched_params) to help per-VCPU settings.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> ---
>  tools/libxl/libxl.c         | 189 ++++++++++++++++++++++++++++++++++++++------
>  tools/libxl/libxl.h         |  19 +++++
>  tools/libxl/libxl_types.idl |  11 +++
>  3 files changed, 196 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index feb3aa9..169901a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5797,6 +5797,120 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
>      return 0;
>  }
>  
> +static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period,
> +                                 uint32_t budget, uint32_t *sdom_period,
> +                                 uint32_t *sdom_budget)
> +{
> +    if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {

LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT is -1, while period appears to
be unsigned, so I think think this will ever be false.

This means you will be setting the period to 2^32 if the default is
requested.

> +        if (period < 1) {
> +            LOG(ERROR, "VCPU period is not set or out of range, "
> +                       "valid values are larger than 1");

According to the check they are 1 or greater i.e. 1 is considered valid
while the message suggests that 2 is the first valid value.

> +            return ERROR_INVAL;
> +        }
> +        *sdom_period = period;
> +    }
> +
> +    if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {

Likewise.

> +        if (budget < 1) {
> +            LOG(ERROR, "VCPU budget is not set or out of range, "
> +                       "valid values are larger than 1");

Likewise.

> +            return ERROR_INVAL;
> +        }
> +        *sdom_budget = budget;
> +    }
> +
> +    if (budget > period) {
> +        LOG(ERROR, "VCPU budget is larger than VCPU period, "
> +                   "VCPU budget should be no larger than VCPU period");

The second half of this statement is redundant I think.

> +        return ERROR_INVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
> +                               libxl_vcpu_sched_params *scinfo)
> +{
> +    uint16_t num_vcpus;
> +    int rc, i;
> +    xc_dominfo_t info;
> +
> +    rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> +    if (rc < 0) {
> +        LOGE(ERROR, "getting domain info");
> +        return ERROR_FAIL;
> +    }
> +    num_vcpus = info.max_vcpu_id + 1;
> +
> +    struct xen_domctl_sched_rtds_params  *sdom = libxl__malloc(NOGC,

                                           ^ one space only please.

> +                sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus);

GCNEW_ARRAY, I think.

Please define sdom at the top (but init it here)

Also, I think you leak sdom here, since it is not returned to the user
it should be gc'd.

> +    rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus);
> +    if (rc != 0) {
> +        LOGE(ERROR, "getting vcpu sched rtds");
> +        return ERROR_FAIL;
> +    }
> +
> +    libxl_vcpu_sched_params_init(scinfo);
> +
> +    scinfo->sched = LIBXL_SCHEDULER_RTDS;
> +    scinfo->num_vcpus = num_vcpus;
> +    scinfo->vcpus = (libxl_rtds_vcpu *)
> +            libxl__malloc(NOGC, sizeof(libxl_rtds_vcpu) * num_vcpus);

You don't need to cast the result of malloc (in C at least)

Also libxl__calloc is better for allocating arrays, but as above you
want GCNEW_ARRAY anyway.

NOGC is correct for this allocation.

> +    for(i = 0; i < num_vcpus; i++) {
> +        scinfo->vcpus[i].period = sdom[i].period;
> +        scinfo->vcpus[i].budget = sdom[i].budget;
> +    }
> +
> +    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 num_vcpus;
> +    int vcpuid;
> +    uint32_t budget, period;
> +    xc_dominfo_t info;
> +
> +    rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> +    if (rc < 0) {
> +        LOGE(ERROR, "getting domain info");
> +        return ERROR_FAIL;
> +    }
> +    num_vcpus = info.max_vcpu_id + 1;
> +
> +    struct xen_domctl_sched_rtds_params  *sdom =
> +            libxl__malloc(NOGC, scinfo->num_vcpus);

GCNEW_ARRAY, and not NOGC, define sdom at the top please and the same
double space issue in the type definition as before too.

> +    for (i = 0; i < scinfo->num_vcpus; i++) {
> +        vcpuid = scinfo->vcpus[i].vcpuid;

Isn't scinfo->vcpus[i].vcpuid unsigned, while the local var is an int.

> +    rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
> +            &sdom.period, &sdom.budget);

While this is ok, we normally align the parameters under each other,
i.e. align the second line with "gc" in this case. There are a few of
these here.

> +    if (rc == ERROR_INVAL)
> +        return rc;
>  
>      rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
>      if (rc < 0) {
> @@ -5899,6 +5994,30 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>      return ret;
>  }
>  
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> +                                  const libxl_vcpu_sched_params *scinfo)
> +{
> +    GC_INIT(ctx);
> +    libxl_scheduler sched = scinfo->sched;
> +    int ret;
> +
> +    if (sched == LIBXL_SCHEDULER_UNKNOWN)
> +        sched = libxl__domain_scheduler(gc, domid);
> +
> +    switch (sched) {
> +    case LIBXL_SCHEDULER_RTDS:
> +        ret=sched_rtds_vcpu_set(gc, domid, scinfo);

Spaces around = please (in several places)

> +        break;
> +    default:
> +        LOG(ERROR, "Unknown scheduler");
> +        ret=ERROR_INVAL;

I think unknown scheduler is distinct from a scheduler having no
per-vcpu parameters.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 117b61d..d28d274 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
>      ("checkpointed_stream", integer),
>      ])
>  
> +libxl_rtds_vcpu = Struct("rtds_vcpu",[
> +    ("period",       uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> +    ("budget",       uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +    ("vcpuid",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> +    ])
> +
> +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> +    ("sched",        libxl_scheduler),
> +    ("vcpus",        Array(libxl_rtds_vcpu, "num_vcpus")),

How will we handle a second scheduler with per-vcpu parameters being
added in the future without breaking the API?


> +    ])
> +
>  libxl_domain_sched_params = Struct("domain_sched_params",[
>      ("sched",        libxl_scheduler),
>      ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),