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

[Chong Li <lichong659@gmail.com>]

On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> 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.
>>


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

If I follow the indentation rule, the second line would be longer than
80 characters.
The function name is just too long.


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

The current RTDS (xen 4.6) is:

1) Read the (per-domain) scheduling params from Xen, and store them to sdom
2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
-1), use sdom.period;
    else sdom.period = new period (if new period is valid);
    If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is
-1), use sdom.budget;
    else sdom.budget = new budget;
3) xc_sched_rtds_domain_set (sdom);

In our patch, my plan is:
1) Read the default params from Xen, and store them to sdom
2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
-1), use sdom.period;
    else sdom.period = new period (if new period is valid);
    If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is
-1), use sdom.budget;
    else sdom.budget = new budget;
3) xc_sched_rtds_domain_set (sdom);

Even though I made some mistakes in this post (forgot the two "else"s
in my plan), is this plan ok?

Chong




-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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