On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
> 
This patch looks mostly fine. A few comments.

So, as for the other patches, I'd mention here in the changelog, that
only RTDS supports this for now.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e9a2d26..9f7f859 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
>  
> +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 not set or out of range, "
> +                       "valid values are larger than or equal to 1");
>
That's probably a nit, but, if period is not set, as the error message
says, it means it stays LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT? In
which case you do not enter this branch, and you do not print this
message, do you?

What I mean is that, it looks to me that it's not accurate for the
message to say "period not set", or am I overlooking something?

> +            return 1;
>
So, 1 is error, 0 is ok. That's fine, as this is an internal function,
but please, add a quick doc comment above it.

> +        }
> +        *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 is larger than VCPU period");
>
I'd be more explicit: "VCPU budget must be smaller than VCPU period".

> +        return 1;
> +    }
> +
> +    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;
> +    xen_domctl_schedparam_vcpu_t *vcpus;
> +
> +    rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> +    if (rc < 0) {
> +        LOGE(ERROR, "getting domain info");
> +        return ERROR_FAIL;
> +    }
> +
> +    if (scinfo->num_vcpus == 0)
> +        num_vcpus = info.max_vcpu_id + 1;
> +    else
> +        num_vcpus = scinfo->num_vcpus;
> +
num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus :
                                info->max_vcpu_id + 1;

(And I think there is a 'contracted' form of this, but I keep forgetting
it, and I'm not sure whether it's a GCC extension...)

> +    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));
>
This line looks a long...

> +    }
> +    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;
> +    xen_domctl_schedparam_vcpu_t *vcpus;
> +    int 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;
>
  if (sched_rtds_validate_params(gc, scinfo->vcpus[i].period,
                                 scinfo->vcpus[i].budget,
                                 &vcpus[i].s.rtds.period,
                                 &vcpus[i].s.rtds.budget))
      return ERROR_INVAL;

I.e., I don't think you need to save rc.

> +        }
> +    } else {
> +        num_vcpus = max_vcpuid + 1;
> +        GCNEW_ARRAY(vcpus, num_vcpus);
> +        rc = sched_rtds_validate_params(gc,
> +                    scinfo->vcpus[0].period, scinfo->vcpus[0].budget,
> +                    &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget);
> +        if (rc)
> +            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 == 1) {
> +        printf("WARN: too small period or budget may "
> +                   "result in large scheduling overhead\n");
> +        rc = 0;
>
As said, do the logging in Xen directly, and ditch this.

> +    } else if (rc != 0) {
> +        LOGE(ERROR, "setting vcpu sched rtds");
> +        return ERROR_FAIL;
> +    }
> +
> +    return rc;
> +}

> @@ -5887,29 +6037,10 @@ 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");
> +    rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
> +            &sdom.period, &sdom.budget);
> +    if (rc)
>          return ERROR_INVAL;
>
Ditto.

> -    }
>  

> @@ -5920,6 +6051,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)
>  {
> @@ -5956,6 +6092,52 @@ 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, "No per-vcpu set function provided");
>
I'd say: "per-VCPU parameter setting not supported for this scheduler".

> +        ret = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +        LOG(ERROR, "No per-vcpu set function provided");
> +        ret = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_CREDIT2:
> +        LOG(ERROR, "No per-vcpu set function provided");
> +        ret = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_ARINC653:
> +        LOG(ERROR, "No per-vcpu set function provided");
> +        ret = ERROR_INVAL;
> +        break;
BTW, Can't you unify all the !supported cases?

> +    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;
> +}

> @@ -5989,6 +6171,45 @@ 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, "No per-vcpu get function provided");
>
Same here.

> +        ret = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +        LOG(ERROR, "No per-vcpu get function provided");
> +        ret = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_CREDIT2:
> +        LOG(ERROR, "No per-vcpu get function provided");
> +        ret = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_ARINC653:
> +        LOG(ERROR, "No per-vcpu get function provided");
> +        ret = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_RTDS:
> +        ret = sched_rtds_vcpu_get(gc, domid, scinfo);
> +        break;
And here.

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

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index a1c5d15..040a248 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -200,6 +200,16 @@
>  #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>  
>  /*
> + * libxl_vcpu_sched_params is used to store per-vcpu params
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * libxl_sched_params is used to store the array of per-vcpu params
> +*/
> +#define LIBXL_HAVE_SCHED_PARAMS 1
> +
>
I may be misremembering, but did not we say that one of these
LIBXL_HAVE_* was enough?

If we want to have two, I'd much rather have a generic one
(LIBXL_HAVE_VCPU_SCHED_PARAMS) and one announcing that the feature is
supported by RTDS (e.g., LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS).

This way, if/when we'll add support for per-VCPU parameters in Credit,
we'd add LIBXL_HAVE_SCHED_CREDIT_VCPU_PARAMS.

But that's just an idea, it's best to see what tools maintainers think
of this.

BTW, be a little more verbose in commenting these macros. Just have a
look and take inspiration from the already existing ones.

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)