Re: [Xen-devel] [PATCH v1 3/4] libxl: add rt scheduler

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Sun, Aug 24, 2014 at 06:58:44PM -0400, Meng Xu wrote:
> Add libxl functions to set/get domain's parameters for rt scheduler
> 
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl.c         |  139 
> +++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl.h         |    7 +++
>  tools/libxl/libxl_types.idl |   15 +++++
>  3 files changed, 161 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 3526539..440e8df31 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5154,6 +5154,139 @@ 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_params* sdom;
> +    uint16_t num_vcpus;
> +    int rc, i;
> +
> +    rc = xc_sched_rt_domain_get_num_vcpus(CTX->xch, domid, &num_vcpus);
> +    if (rc != 0) {
> +        LOGE(ERROR, "getting num_vcpus of domain sched rt");
> +        return ERROR_FAIL;
> +    }
> +    
> +    /* FIXME: can malloc be used in libxl? seems it was used in the file */
> +    sdom = (struct xen_domctl_sched_rt_params *)
> +            malloc( sizeof(struct xen_domctl_sched_rt_params) * num_vcpus );

It's better to use libxl__malloc here. With that change you can also
omit the test of sdom.

> +    if ( !sdom ){

No need to add spaces inside brackets, on the other hand you need
one space before {.

I can see this issues appears repeatly in this patch. Please check and
fix them.

> +        LOGE(ERROR, "Allocate sdom array fails\n");
> +        return ERROR_INVAL;
> +    }
> +
> +    rc = xc_sched_rt_domain_get(CTX->xch, domid, sdom, num_vcpus);
> +    if (rc != 0) {
> +        LOGE(ERROR, "getting domain sched rt");
> +        return ERROR_FAIL;
> +    }
> +
> +    /* FIXME: how to guarantee libxl_*_dispose be called exactly once? */
> +    libxl_domain_sched_params_init(scinfo);
> +    

libxl_*_dispose should be idempotent (as least we intent to make it so).
And it's caller's responsibility to ensure it's called once if it wishes
to.

> +    scinfo->rt.num_vcpus = num_vcpus;
> +    scinfo->sched = LIBXL_SCHEDULER_RT;
> +    /* FIXME: can malloc be used in libxl? seems it was used in the file */
> +    scinfo->rt.vcpus = (libxl_vcpu *)
> +                       malloc( sizeof(libxl_vcpu) * scinfo->rt.num_vcpus );

libxl__malloc.

If you don't want this allocation to be automatically freed, use NOGC
instead of gc.

> +    if ( !scinfo->rt.vcpus ){
> +        LOGE(ERROR, "Allocate lib_vcpu array fails\n");
> +        return ERROR_INVAL;
> +    }
> +    for( i = 0; i < num_vcpus; ++i)
> +    {
> +        scinfo->rt.vcpus[i].period = sdom[i].period;
> +        scinfo->rt.vcpus[i].budget = sdom[i].budget;
> +        scinfo->rt.vcpus[i].index = sdom[i].index;
> +    }
> +    
> +    free(sdom);

Remove this if you use libxl__malloc.

> +    return 0;
> +}
> +
> +#define SCHED_RT_VCPU_PERIOD_MAX    31536000000000 /* one year in 
> microsecond*/
> +#define SCHED_RT_VCPU_BUDGET_MAX    SCHED_RT_VCPU_PERIOD_MAX
> +
> +/*
> + * Sanity check of the scinfo parameters
> + * return 0 if all values are valid
> + * return 1 if one param is default value
> + * return 2 if the target vcpu's index, period or budget is out of range
> + */
> +static int sched_rt_domain_set_validate_params(libxl__gc *gc,
> +                                               const 
> libxl_domain_sched_params *scinfo,
> +                                               const uint16_t num_vcpus)
> +{
> +    int vcpu_index = scinfo->rt.vcpu_index;
> +
> +    if (vcpu_index == LIBXL_DOMAIN_SCHED_PARAM_VCPU_DEFAULT ||
> +        scinfo->rt.period == LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT ||
> +        scinfo->rt.budget == LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
> +    {
> +        return 1;
> +    }
> +
> +    if (vcpu_index < 0 || vcpu_index > num_vcpus)
> +    {

Libxl coding style normally has { in the same line of "if" "for" etc. Please
check and fix other occurences.

> +        LOG(ERROR, "VCPU index is not set or out of range, "
> +                    "valid values are within range from 0 to %d", num_vcpus);
> +        return 2;
> +    }
> +
> +    if (scinfo->rt.period < 1 ||
> +        scinfo->rt.period > SCHED_RT_VCPU_PERIOD_MAX)
> +    {
> +        LOG(ERROR, "VCPU period is not set or out of range, "
> +                    "valid values are within range from 0 to %lu", 
> SCHED_RT_VCPU_PERIOD_MAX);

Line too long. Move SCHED_RT_VCPU_PERIOD_MAX to a new line.

> +        return 2;
> +    }
> +
> +    if (scinfo->rt.budget < 1 ||
> +        scinfo->rt.budget > SCHED_RT_VCPU_BUDGET_MAX)
> +    {
> +        LOG(ERROR, "VCPU budget is not set or out of range, "
> +                    "valid values are within range from 0 to %lu", 
> SCHED_RT_VCPU_BUDGET_MAX);

Ditto.

> +        return 2;
> +    }
> +
> +    return 0;
> +
> +}
> +
> +static int sched_rt_domain_set(libxl__gc *gc, uint32_t domid,
> +                               const libxl_domain_sched_params *scinfo)
> +{
> +    struct xen_domctl_sched_rt_params sdom;
> +    uint16_t num_vcpus;
> +    int rc;
> + 
> +    rc = xc_sched_rt_domain_get_num_vcpus(CTX->xch, domid, &num_vcpus);
> +    if (rc != 0) {
> +        LOGE(ERROR, "getting domain sched rt");
> +        return ERROR_FAIL;
> +    }
> +    
> +    rc = sched_rt_domain_set_validate_params(gc, scinfo, num_vcpus);
> +    if (rc == 2)
> +        return ERROR_INVAL;
> +    if (rc == 1)
> +        return 0;
> +    if (rc == 0)
> +    {
> +        sdom.index = scinfo->rt.vcpu_index;
> +        sdom.period = scinfo->rt.period;
> +        sdom.budget = scinfo->rt.budget;
> +    }
> +

No need to test for 0 here as there can't be other value at this point.

> +    rc = xc_sched_rt_domain_set(CTX->xch, domid, &sdom);
> +    if ( rc < 0 ) {
> +        LOGE(ERROR, "setting domain sched rt");
> +        return ERROR_FAIL;
> +    }
> +
> +    return 0;

The code structure can be rearranged to use "goto out" style so that
there's only one "return rc" at the end.

> +}
> +
>  int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>                                    const libxl_domain_sched_params *scinfo)
>  {
> @@ -5177,6 +5310,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:
> +        ret=sched_rt_domain_set(gc, domid, scinfo);
> +        break;
>      default:
>          LOG(ERROR, "Unknown scheduler");
>          ret=ERROR_INVAL;
> @@ -5207,6 +5343,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:
> +        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 bfeb3bc..4657056 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1240,6 +1240,13 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, 
> uint32_t poolid,
>  #define LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT   -1
>  #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
>  
> +#define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT     -1
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_DEFAULT       -1
> +#define LIBXL_DOMAIN_SCHED_PARAM_NUM_VCPUS_DEFAULT  -1
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1
> +/* Consistent with XEN_LEGACY_MAX_VCPUS xen/arch-x86/xen.h*/
> +#define LIBXL_XEN_LEGACY_MAX_VCPUS                  32 
> +

Where is this macro used? I cannot find it in this one and following xl
patch.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel