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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On dom, 2014-08-24 at 18:58 -0400, Meng Xu wrote:
> --- 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;
> +    }
>
As George pointed out already, you can get the num_vcpus via
xc_domain_getinfo().

I agree with George's review about the rest of this function
(appropriately updated to take what we decided about the interface into
account :-) ).

> +#define SCHED_RT_VCPU_PERIOD_MAX    31536000000000 /* one year in 
> microsecond*/
> +#define SCHED_RT_VCPU_BUDGET_MAX    SCHED_RT_VCPU_PERIOD_MAX
> +
This may be me not remembering correctly the outcome of a preceding
discussion... did we say we were going with _RT_ or with something like
_RTDS_ ?

ISTR the latter...

Also, macros like INT_MAX, UINT_MAX, etc., or << and ~ "tricks" are,
IMO, preferrable to the open coding of the value.

Finally, I wonder whether these would better live in some headers,
closer to the declaration of period and budget (where their type is also
visible) and, as a nice side effect of that, available to libxl callers
as well.

> +/*
> + * 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;
> +
As per the low level interface (Xen and libxc) there should be no need
for any vcpu_index anymore, right?

I'm just double checking, as the discussion was --as it should, on these
things-- long and involved :-D

> +    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;
> +    }
> +
I think some basics sanity checking are fine done here. E.g., you may
also add a (budget <= period) check. However, as George said already:
 - make sure you limit these to the kind of checks based on info the 
   toolstack should have, and defer the rest to the hypervisor
 - if something goes wrong, make sure to always return libxl error codes
   (typically, ERROR_INVAL), as sched_credit_domain_set() does.

Oh and, just double checking again, I think we decided to handle
(budget ==0 && period == 0) in a special way, so remember that! :-P

It's personal taste, I guess, but I think you don't really need an
helper function for this, and it can leave in sched_rt_domain_set.

> +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;
> +    }
> +
So, I see that you are actually returning libxl error codes, good. Well,
again, just put the code above directly here, instead of having to
define and then interpret an ad-hoc error handling logic. :-)

> @@ -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;
Again, I seriously think I remember we agreed upon _SCHEDULER_RTDS (or
_RT_DS) as thee name of this thing. Am I wrong?

> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -153,6 +153,7 @@ libxl_scheduler = Enumeration("scheduler", [
>      (5, "credit"),
>      (6, "credit2"),
>      (7, "arinc653"),
> +    (8, "rt"),
rtds? rt_ds?

> @@ -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),
>      ])
And about this part, we discussed and agreed already in the other
thread. :-)

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