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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
> ---
>
For v3, don't forget the summary of what changed between v2 and v3 that
Jan also asked, please. :-)

>  tools/libxl/libxl.c         | 189 
> ++++++++++++++++++++++++++++++++++++++------
>  tools/libxl/libxl.h         |  19 +++++
>  tools/libxl/libxl_types.idl |  11 +++
>  3 files changed, 196 insertions(+), 23 deletions(-)
>
The subject says 'XL', but then you are only touching libxl. I probably
see what you meant, and in fact, the 'libxl:' prefix is correct. Just
don't mention xl at all... there could be users of libxl different than
xl, and you're enabling all of them to use per-vcpu parameters, not just
xl. :-)
 
> +static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period,
> +                                 uint32_t budget, uint32_t *sdom_period,
> +                                 uint32_t *sdom_budget)
> +{
>
This is better, thanks. One thing I'm not sure about...

> +    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 1");
> +            return ERROR_INVAL;
> +        }
> +        *sdom_period = period;
> +    }
> +
... is whether I like it or not for this to have side effects. It's
rather easy to spot that from the prototype, I know, but probably, I'd
prefer if a function called *_validate_* would actually just do the
validation.

> +    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 1");
> +            return ERROR_INVAL;
>
It's a utility function, and you never return its return value directly,
so you can just go with 0==error, 1==ok.

> +        }
> +        *sdom_budget = budget;
> +    }
> +
> +    if (budget > period) {
> +        LOG(ERROR, "VCPU budget is larger than VCPU period, "
> +                   "VCPU budget should be no larger than VCPU period");
> +        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;
> +
>
And as in the hypervisor, get and set are asymmetrical: get reads
everything, set operates on a well defined subset.

Symmetry is a value here too, IMO. So, please, make *both* _get and _set
able to deal with (sparse?) clusters of VCPU.

> +    struct xen_domctl_sched_rtds_params  *sdom = libxl__malloc(NOGC,
> +                sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus);
>
As I said already, please, don't mix variable definition and code. This
belongs above, next to all the others, perhaps initialized to NULL, if
you need it.

BTW, do we really need this? I still haven't looked at the libxc patch,
but can't we make things such as this can be done 'in place'?

> +    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);
> +    for(i = 0; i < num_vcpus; i++) {
> +        scinfo->vcpus[i].period = sdom[i].period;
> +        scinfo->vcpus[i].budget = sdom[i].budget;
> +    }
> +
>
You're using scinfo for reporting the parameters back, so what about
sdom? It looks to me that you're using it internally and leaking it,
while (provided it's really necessary), you should free it, or let the
automatic garbage collector do so.
 
> +    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;
> +
Why doing the +1 and translating this from vcpu ID to number-of, if then
all you do with the result is comparing it with IDs?

> +    struct xen_domctl_sched_rtds_params  *sdom =
> +            libxl__malloc(NOGC, scinfo->num_vcpus);
>
Definition mixed with code.

I also don't like sdom as a name for the variable here... 'vcpus',
perhaps? But most important, why NOGC? This is internal only, right? I
mean you're preparing the array for libxc and pass it there, but you
never hand it to the caller, AFAICT. Therefore, you should go with an
allocation which is automatically garbage collected (see GCNEW_ARRAY(),
as already pointed out), and use GC_INIT and GC_FREE to properly make
that happen.

> +    for (i = 0; i < scinfo->num_vcpus; i++) {
> +        vcpuid = scinfo->vcpus[i].vcpuid;
> +        budget = scinfo->vcpus[i].budget;
> +        period = scinfo->vcpus[i].period;
> +        if (vcpuid < 0 || vcpuid >= num_vcpus) {
>
vcpuid should probably be of unsigned type, when defining the struct.
This will save you the '< 0' part.

> +            LOG(ERROR, "VCPU index is out of range, "
> +                       "valid values are within range from 0 to %d",
> +                       num_vcpus);
> +            return ERROR_INVAL;
>
And in fact, you're leaking the memory allocated for sdom, here and on
every exit path. You really should use GCNEW_ARRAY, and then use the
'goto out:' pattern for exit/error handling (look around, there are
thousands of examples :-D ).

> +        }
> +        sdom[i].vcpuid = vcpuid;
> +
> +        rc = sched_rtds_validate_params(gc, period, budget,
> +                &sdom[i].period, &sdom[i].budget);
> +        if (rc == ERROR_INVAL)
>
You don't need to check for an exact match with ERROR_INVAL, especially
considering that the helper, right now in this patch, if returning
non-0, is _only_ returning ERROR_INVAL (or did I miss something?).

If you simplify the helper as I suggested, this will become just:

 if (!rc)

Or, even better:

 if (!sched_rtds_validate_params(...))

> +            return rc;
> +    }
> +
> +    rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
> +            sdom, scinfo->num_vcpus);
> +    if (rc != 0) {
> +        LOGE(ERROR, "setting vcpu sched rtds");
> +        return ERROR_FAIL;
> +    }
> +
> +    return rc;
> +}
> +
 
> +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);
>
Coding style.

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

> +        break;
> +    }
> +
> +    GC_FREE;
> +    return ret;
> +}

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 44bd8e2..e565814 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -192,6 +192,18 @@
>   * is not present, instead of ERROR_INVAL.
>   */
>  #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
> +
> +/*
> + * libxl_vcpu_sched_params is used to get/set per-vcpu params
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * libxl_rtds_vcpu is used to represent the rtds scheduling params
> + * of a vcpu
> +*/
> +#define LIBXL_HAVE_RTDS_VCPU 1
> +
>
I don't think you need 2. Also, I'd make the whole thing more generic
(see below), with RTDS being the only scheduler implementing this new
interface.

>  /*
>   * libxl ABI compatibility
>   *

> 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")),
> +    ])
> +
>  libxl_domain_sched_params = Struct("domain_sched_params",[
>      ("sched",        libxl_scheduler),
>      ("weight",       integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>
I'd do something similar to what I suggested for Xen interface already.
I.e, I'd put together something that can accommodate per-vcpu parameters
for all schedulers and use it.

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)

Attachment: signature.asc
Description: This is a digitally signed message part

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