Re: [Xen-devel] [PATCH v3 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Sun, 2015-06-28 at 21:44 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to 
> support
> per-VCPU settings.
> 
> Changes on PATCH v2:
> 
> 1) New data structure (libxl_vcpu_sched_params and libxl_sched_params) to 
> help per-VCPU settings.
> 
> 2) sched_rtds_vcpu_get now can return a random subset of the parameters of 
> the VCPUs of a specific domain.
> 
> Signed-off-by: Chong Li <chong.li@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
> 
So, apart from what IanC said already, I think this patch is mostly
fine, apart from:
 - the changelog needing to improve, as explained already while
   reviewing one other patch in the series (that, in fact, applies to
   all of them)
 - comments/documentation about the new behavior of per domain
   scheduling API calls looks to be missing to me...
 - some coding style issues (see below).

> +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;
> +
> +    if (scinfo->num_vcpus == 0) {
> +        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;
> +    } else
> +        num_vcpus = scinfo->num_vcpus;
> +
> +    GCNEW_ARRAY(vcpus, num_vcpus);
> +
> +    if (scinfo->num_vcpus > 0)
> +        for(i=0; i < num_vcpus; i++)
spaces ("for (...")

> +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;
> +        }
> +    } else {
> +        num_vcpus = max_vcpuid + 1;
> +     GCNEW_ARRAY(vcpus, num_vcpus);
>
Indentation.

> --- 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 get/set per-vcpu params
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * libxl_sched_params is used to store per-vcpu params
> +*/
> +#define LIBXL_SCHED_PARAMS 1
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> @@ -1554,10 +1564,17 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, 
> uint32_t poolid,
>  #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
>  #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT    -1
>  
> +/* Per-VCPU parameters*/
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1
> +
>  int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>                                    libxl_domain_sched_params *params);
>  int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>                                    const libxl_domain_sched_params *params);
> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> +                                  libxl_vcpu_sched_params *params);
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> +                                  const libxl_vcpu_sched_params *params);
>
Didn't we say that we wanted the fact that now, at least for RTDS,
libxl_domain_sched_params_*() deals with default parameters to be
documented in a comment somewhere? Have I missed it?

I appreciate that this is happening in lower levels, and that libxl is
just reporting it, but, still, it is something that I would like to find
in the headers of the library, in order to be able to tell the
differences between  libxl_vcpu_sched_params_* and
libxl_domain_sched_params_* ... Ian?

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