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

[Meng Xu <xumengpanda@xxxxxxxxx>]

Hi Dario,

2014-09-05 6:21 GMT-04:00 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...

So I also need to change all RT to RT_DS in the tool stack, except
keeping the command 'xl sched-rt'? If so, I will do that.

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

What does open coding of the value mean? Do you mean (2^32-1) instead
of 4294967295?

>
> 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.

I actually considered the possibility of adding it to
xen/include/public/domctl.h, but other schedulers do not have such
range macro in the domctl.h, so I'm not sure if it will cause
inconsistence?

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

Yes. Now I didn't use any array in toolstack or kernel to set/get
domain's parameters. So we don't use (budget == 0 && period == 0) to
indicate the vcpus not changed. (Whenever user set a domain's vcpu's
parameters, we set all vcpus' parameters of this domain, so we don't
need such implication for 4.5 release. :-P) The array comes "only"
when we allow users to set/get a specific vcpu's parameters and vcpus
have different periods and budgets.

As to the 4.5 release, because we decide to not having the
functionality of setting/getting each vcpu's parameters. The current
toolstack implementation is aimed to setting/getting each vcpu's
parameters, the code change could be a lot, (but the result tool stack
code for 4.5 will be very similar to the toolstack code of existing
schedulers.)  I think I will release a version soon to let you guys
have a look. What do you think?

>
> 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.

Right!

>
>> +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. :-)

Removed the ad-hoc error code. :-)

>
>> @@ -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?
>

Right! We agreed on RT_DS. I changed the hypervisor code and didn't
realize I also need to change the tool stack part. Will change it the
next patch.

>> --- 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. :-)
>
Yes. Modified for the next patch.

Thank you very much!

Meng


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

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