Re: [PATCH v2 3/4] libxl: add rt scheduler

[George Dunlap <george.dunlap@eu.citrix.com>]

On 09/07/2014 08:41 PM, Meng Xu wrote:
> Add libxl functions to set/get domain's parameters for rt scheduler
> Note: VCPU's information (period, budget) is in microsecond (us).
>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> ---
>   tools/libxl/libxl.c         |   75 +++++++++++++++++++++++++++++++++++++++++++
>   tools/libxl/libxl.h         |    1 +
>   tools/libxl/libxl_types.idl |    2 ++
>   3 files changed, 78 insertions(+)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2ae5fca..6840c92 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5155,6 +5155,75 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
>       return 0;
>   }
>   
> +static int sched_rt_domain_get(libxl__gc *gc, uint32_t domid,
> +                               libxl_domain_sched_params *scinfo)
> +{
> +    struct xen_domctl_sched_rt sdom;
> +    int rc;
> +
> +    rc = xc_sched_rt_domain_get(CTX->xch, domid, &sdom);
> +    if (rc != 0) {
> +        LOGE(ERROR, "getting domain sched rt");
> +        return ERROR_FAIL;
> +    }
> +
> +    libxl_domain_sched_params_init(scinfo);
> +
> +    scinfo->sched = LIBXL_SCHEDULER_RT_DS;
> +    scinfo->period = sdom.period;
> +    scinfo->budget = sdom.budget;
> +
> +    return 0;
> +}
> +
> +#define SCHED_RT_DS_VCPU_PERIOD_UINT_MAX    4294967295U /* 2^32 - 1 us */
> +#define SCHED_RT_DS_VCPU_BUDGET_UINT_MAX    SCHED_RT_DS_VCPU_PERIOD_UINT_MAX

I think what Dario was looking for was this:

#define SCHED_RT_DS_VCPU_PERIOD_MAX UINT_MAX

I.e., use the already-defined #defines with meaningful names (line 
UINT_MAX), and avoid open-coding (i.e., typing out a "magic" number, 
like 429....U).

> +
> +static int sched_rt_domain_set(libxl__gc *gc, uint32_t domid,
> +                               const libxl_domain_sched_params *scinfo)
> +{
> +    struct xen_domctl_sched_rt sdom;
> +    int rc;
> +
> +    rc = xc_sched_rt_domain_get(CTX->xch, domid, &sdom);

You need to check the return value here and bail out on an error.

> +
> +    if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> +        if (scinfo->period < 1 ||
> +            scinfo->period > SCHED_RT_DS_VCPU_PERIOD_UINT_MAX) {

...but this isn't right anyway, right?  scinfo->period is a signed 
integer.  You shouldn't be comparing it to an unsigned int; and this can 
never be false anyway, because even if it's automatically cast to be 
unsigned, the type isn't big enough to be bigger than UINT_MAX anyway.

If period is allowed to be anything up to INT_MAX, then there's no need 
to check the upper bound.  Checking to make sure it's >= 1 should be 
sufficient.  Then you can just get rid of the #defines above.

> +            LOG(ERROR, "VCPU period is not set or out of range, "
> +                       "valid values are within range from 0 to %u",
> +                       SCHED_RT_DS_VCPU_PERIOD_UINT_MAX);
> +            return ERROR_INVAL;
> +        }
> +        sdom.period = scinfo->period;
> +    }
> +
> +    if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> +        if (scinfo->budget < 1 ||
> +            scinfo->budget > SCHED_RT_DS_VCPU_BUDGET_UINT_MAX) {

Same here.

> +            LOG(ERROR, "VCPU budget is not set or out of range, "
> +                       "valid values are within range from 0 to %u",
> +                       SCHED_RT_DS_VCPU_BUDGET_UINT_MAX);
> +            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");
> +        return ERROR_INVAL;
> +    }
> +
> +    rc = xc_sched_rt_domain_set(CTX->xch, domid, &sdom);
> +    if (rc < 0) {
> +        LOGE(ERROR, "setting domain sched rt");
> +        return ERROR_FAIL;
> +    }
> +
> +    return 0;
> +}
> +
>   int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>                                     const libxl_domain_sched_params *scinfo)
>   {
> @@ -5178,6 +5247,9 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>       case LIBXL_SCHEDULER_ARINC653:
>           ret=sched_arinc653_domain_set(gc, domid, scinfo);
>           break;
> +    case LIBXL_SCHEDULER_RT_DS:
> +        ret=sched_rt_domain_set(gc, domid, scinfo);
> +        break;
>       default:
>           LOG(ERROR, "Unknown scheduler");
>           ret=ERROR_INVAL;
> @@ -5208,6 +5280,9 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>       case LIBXL_SCHEDULER_CREDIT2:
>           ret=sched_credit2_domain_get(gc, domid, scinfo);
>           break;
> +    case LIBXL_SCHEDULER_RT_DS:
> +        ret=sched_rt_domain_get(gc, domid, scinfo);
> +        break;
>       default:
>           LOG(ERROR, "Unknown scheduler");
>           ret=ERROR_INVAL;
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 460207b..dbe736c 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1280,6 +1280,7 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
>   #define LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT     -1
>   #define LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT   -1
>   #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
> +#define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT     -1
>   
>   int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>                                     libxl_domain_sched_params *params);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 931c9e9..72f24fe 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -153,6 +153,7 @@ libxl_scheduler = Enumeration("scheduler", [
>       (5, "credit"),
>       (6, "credit2"),
>       (7, "arinc653"),
> +    (8, "rt_ds"),

rtds

Other than that, looks good.

  -George