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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Sun, Aug 24, 2014 at 11:58 PM, Meng Xu <mengxu@xxxxxxxxxxxxx> 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 );

There are two ways to do allocation; one which will be garbage
collected automatically (so you don't need to call free on it), and
one which returns something which the caller has to remember to free.

You can call libxl__calloc(gc, num_vcpus, sizeof(struct
xen_domctl_sched_rt_params)) with the garbage collector for this one;
then you won't need to free it below, as it will be freed by the
GC_FREE at the end of libxl_domain_params_get().  Then...

> +    if ( !sdom ){
> +        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);
> +
> +    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 );

...for this one, call libxl__calloc(NOGC, scinfo->rt.num_vcpus,
sizeof(libxl_vcpu)).  NOGC will cause the values not to be freed by
GC_FREE (so they should be freed by the caller).

And again, as Wei said, you don't need to check the return value for
either one, as libxl will handle memory allocation errors.

However, it looks like libxl_list_domain() managed to make the libxl
struct look exactly like the libxc struct, so that you don't need to
do this allocate-and-copy thing.  Could you try to arrange for that to
be the case for libxl_rt_vcpu?

> +/*
> + * 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
> + */

These should be checked in the hypervisor, not the toolstack.  The
hypervisor can return -EINVAL, and libxl can pass that error message
on to the caller.

So this function should go away; but in general, just as a style note,
you should avoid "magic constants" like this.  Ideally you'd re-use
some of the LIBXL error codes; but if that won't work, you should make
#define's with a suitable descriptive name.

> +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)
> +    {
> +        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);
> +        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);
> +        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;

Er, one of the parameters is the default value, and so you don't set
any of them?

> @@ -303,6 +304,19 @@ libxl_domain_restore_params = 
> Struct("domain_restore_params", [
>      ("checkpointed_stream", integer),
>      ])
>
> +libxl_rt_vcpu = Struct("vcpu",[
> +    ("period",       uint64, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> +    ("budget",       uint64, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +    ("index",        integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> +    ])
> +
> +libxl_domain_sched_rt_params = Struct("domain_sched_rt_params",[
> +    ("vcpus",        Array(libxl_rt_vcpu, "num_vcpus")),
> +    ("period",       uint64, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> +    ("budget",       uint64, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +    ("vcpu_index",   integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> +    ])
> +
>  libxl_domain_sched_params = Struct("domain_sched_params",[
>      ("sched",        libxl_scheduler),
>      ("weight",       integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
> @@ -311,6 +325,7 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
>      ("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'}),
> +    ("rt",           libxl_domain_sched_rt_params),
>      ])

While the domctl interface is not stable, the libxl interface *is*
stable, so we definitely need to think carefully about what we want
this to look like.

Let me give that a think. :-)

 -George

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