Re: [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6987 bytes --]

On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
> 
Hey,

Good job indeed, Chong, this is much better.

Now, I appreciate that Wei already Acked this, but nevertheless, I have
some comments.

I'll put them down here, and then it will be up to maintainers and
committers to figure out whether they need to be addressed or not (Wei?
Ian?)

> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5770,6 +5770,201 @@ static int sched_credit2_domain_set(libxl__gc
> *gc, uint32_t domid,
>      return 0;
>  }
>  
> +static int sched_rtds_validate_params(libxl__gc *gc, int period, int
> budget)
> +{
> +    int rc;
> +
> +    if (period < 1) {
> +        LOG(ERROR, "VCPU period is out of range, "
> +                   "valid values are larger than or equal to 1");
>
These strings are really better if kept on one line, and it does not
look impossible in this case:

        LOG(ERROR, "Invalid VCPU period of %d (it should be >= 1)", period);

> +        rc = ERROR_INVAL; /* error scheduling parameter */
>
The comment should go away.

> +        goto out;
> +    }
> +
> +    if (budget < 1) {
> +        LOG(ERROR, "VCPU budget is not set or out of range, "
> +                   "valid values are larger than or equal to 1");
>
        LOG(ERROR, "Invalid VCPU budget of %d (it should be >= 1)", budget);

> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    if (budget > period) {
> +        LOG(ERROR, "VCPU budget must be smaller than "
> +                   "or equal to VCPU period");
>
        LOG(ERROR, "VCPU budget must be smaller than period, but %d > %d",
            budget, period);

> +/* Set the RTDS scheduling parameters of vcpu(s) */
> +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t
> domid,
> +                                       const libxl_vcpu_sched_params
> *scinfo)
> +{
> +    int r, rc;
> +    int i;
> +    uint16_t max_vcpuid;
> +    xc_dominfo_t info;
> +    struct xen_domctl_schedparam_vcpu *vcpus;
> +
> +    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> +    if (r < 0) {
> +        LOGE(ERROR, "getting domain info");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    max_vcpuid = info.max_vcpu_id;
> +
> +    if (scinfo->num_vcpus <= 0) {
> +        rc = ERROR_INVAL;
> +        goto out;
> +    } else {
>
Personally, I consider what Wei suggested for avoiding, at once, one
more level of indentation, as well as this really really ugly

 if {
   goto
 } else {
 }

rather important to do.

But then again, I'm not the maintainer of this code, so if it's fine
for them, then fine. :-)

> +        for (i = 0; i < scinfo->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);
>
                LOG(ERROR, "Invalid VCPU %d: valid range is [0, %d]",
                    scinfo->vcpus[i].vcpuid, info.max_vcpu_id);


> +/* Set the RTDS scheduling parameters of all vcpus of a domain */
> +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t
> domid,
> +                               const libxl_vcpu_sched_params
> *scinfo)
>
Indentation?

It seems to me that it just fits, even if done properly:

static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid,
                                       const libxl_vcpu_sched_params *scinfo)

> +{
> +    int r, rc;
> +    int i;
> +    uint16_t max_vcpuid;
> +    xc_dominfo_t info;
> +    struct xen_domctl_schedparam_vcpu *vcpus;
> +    uint32_t num_vcpus;
> +
> +    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> +    if (r < 0) {
> +        LOGE(ERROR, "getting domain info");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    max_vcpuid = info.max_vcpu_id;
> +
> +    if (scinfo->num_vcpus != 1) {
> +        rc = ERROR_INVAL;
> +        goto out;
> +    } else {
>
Same as above, of course.

> @@ -5802,30 +5997,10 @@ static int sched_rtds_domain_set(libxl__gc
> *gc, uint32_t domid,
>          LOGE(ERROR, "getting domain sched rtds");
>          return ERROR_FAIL;
>      }
> -
> -    if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> -        if (scinfo->period < 1) {
> -            LOG(ERROR, "VCPU period is not set or out of range, "
> -                       "valid values are larger than 1");
> -            return ERROR_INVAL;
> -        }
> -        sdom.period = scinfo->period;
> -    }
> -
> -    if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> -        if (scinfo->budget < 1) {
> -            LOG(ERROR, "VCPU budget is not set or out of range, "
> -                       "valid values are larger than 1");
> +    if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT &&
> +        scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
> +        if (sched_rtds_validate_params(gc, scinfo->period, scinfo-
> >budget))
>              return ERROR_INVAL;
>
I'm not sure I understand. What's happening in this function?

As it stands after this patch, it looks to me that:
 - we read the default scheduling parameter from Xen,
   via xc_sched_rtds_domain_get()
 - we (possibly, if both are non-default) validate a new period and 
   budget couple of values
 - we don't use such values for anything, and set back what we got 
   from Xen, via xc_sched_rtds_domain_set()

Either I'm missing something very basic, or this is not what Wei said
when reviewing v6:

"Then at callsites you set those values with two direct assignment:

   if (validate(period_value, budget_value) != 0) {
       error;
   }
   period = period_value;
   budget = budget_value;"

Is it?

Thanks and 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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