Re: [PATCH 1 of 3] Support of getting scheduler defaults

[Juergen Gross <juergen.gross@ts.fujitsu.com>]

On 05/23/2012 01:46 AM, Dario Faggioli wrote:
> On Tue, 2012-05-22 at 15:59 +0100, Ian Campbell wrote:
>> Like the below. Lightly tested with the credit scheduler.
>>
>> I think CAP is the only one for which 0 is a real valid value, but I'm
>> not sure (especially with the SEDF ones which didn't have existing
>> limits checks in the set function I could crib from...).
>>
> Yep, that's because they're mostly time values. xen/common/sched_sedf.c
> hosts some ranges for some of them, but I'm not sure we want to
> propagate those:
>
> #define PERIOD_MAX MILLISECS(10000) /* 10s  */
> #define PERIOD_MIN (MICROSECS(10))  /* 10us */
> #define SLICE_MIN (MICROSECS(5))    /*  5us */

I think this should remain in the hypervisor only.

> Also, extratime is a flag, so I think 0 and 1 are both meaningful
> values, maybe we can go for -1 as for cap (I'll try and let you know).

Why not -1 for all values?

>> I'm pretty sure that libxl__sched_set_params needs to get the correct
>> scheduler for the particular domain, but I've no idea how to get that...
>>
> Again, I was thinking something like what Juergen did here could help
> (go getting the scheduler of the cpupool the domain belongs to)... O am
> I misunderstanding the issue?
>
> +    poolinfo = libxl_list_cpupool(ctx,&n_pools);
> +    if (!poolinfo)
> +        return ERROR_NOMEM;
> +
> +    ret = ERROR_INVAL;
> +    for (p = 0; p<  n_pools; p++) {
> +        if (poolinfo[p].poolid == poolid) {
> +            scparams->sched = poolinfo[p].sched;
> +            ret = 0;
> +        }
> +        libxl_cpupoolinfo_dispose(poolinfo + p);
> +    }
> +

This was exactly the purpose of the sniplet.

>> 8<---------------------------
>>
>> # HG changeset patch
>> # User Ian Campbell<ian.campbell@citrix.com>
>> # Date 1337698727 -3600
>> # Node ID 355030f95eb313605a0e43aa7328e731b28a28b3
>> # Parent  426bbf58cea4559464b6e5d3ff0f65324a5f5926
>> libxl: make it possible to explicitly specify default sched params
>>
>> To do so we define a descriminating value which is never a valid real value for
>> each parameter.
>>
>> While there:
>>
>>   - remove tslice_ms and ratelimit_us from libxl_sched_params and from the xl
>>     domain config parser. These are global scheduler properties, not per-domain
>>     ones (and were never read in any case).
>>   - removed libxl_sched_*_domain in favour of libxl_sched_params.
>>   - rename libxl_sched_params to libxl_sched_domain_params for clarity.
>>   - use this new functionality for the various xl commands which set sched
>>     parameters, which saves an explicit read-modify-write in xl.
>>   - removed call of xc_domain_getinfolist from a few functions which weren't
>>     actually using the result (looks like a cut and paste error)
>>   - fix xl which was setting period for a variety of different config keys.
>>
>> Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
>>
>> diff -r 426bbf58cea4 -r 355030f95eb3 tools/libxl/libxl.c
>> --- a/tools/libxl/libxl.c	Tue May 22 14:19:07 2012 +0100
>> +++ b/tools/libxl/libxl.c	Tue May 22 15:58:47 2012 +0100
>> @@ -3168,19 +3168,19 @@ libxl_scheduler libxl_get_scheduler(libx
>>   }
>>
> <snip>
>>
>>   int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,
>> -                                libxl_sched_sedf_domain *scinfo)
>> +                                libxl_sched_domain_params *scinfo)
>>   {
>> -    xc_domaininfo_t domaininfo;
>> -    int rc;
>> -
>> -    rc = xc_domain_getinfolist(ctx->xch, domid, 1,&domaininfo);
>> -    if (rc<  0) {
>> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
>> +    uint64_t period;
>> +    uint64_t slice;
>> +    uint64_t latency;
>> +    uint16_t extratime;
>> +    uint16_t weight;
>> +
>> +    int ret;
>> +
>> +    ret = xc_sedf_domain_get(ctx->xch, domid,&period,&slice,&latency,
>> +&extratime,&weight);
>> +    if (ret != 0) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched sedf");
>>           return ERROR_FAIL;
>>       }
>> -    if (rc != 1 || domaininfo.domain != domid)
>> -        return ERROR_INVAL;
>> -
>> -
>> -    rc = xc_sedf_domain_set(ctx->xch, domid, scinfo->period * 1000000,
>> -                            scinfo->slice * 1000000, scinfo->latency * 1000000,
>> -                            scinfo->extratime, scinfo->weight);
>> -    if ( rc<  0 ) {
>> +
>> +    if (scinfo->period != LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT)
>> +        period = scinfo->period * 1000000;
>> +    if (scinfo->slice != LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT)
>> +        period = scinfo->slice * 1000000;
>> +    if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
>> +        period = scinfo->latency * 1000000;
>> +    if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)
>> +        period = scinfo->extratime;
>> +    if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT)
>> +        period = scinfo->weight;
>> +
>> +    ret = xc_sedf_domain_set(ctx->xch, domid, period, slice, latency,
>> +                            extratime, weight);
>> +    if ( ret<  0 ) {
>>           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "setting domain sched sedf");
>>           return ERROR_FAIL;
>>       }
>>
> # xl create vm1.cfg
> Parsing config from vm1.cfg
> libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
>
> And I'm getting the above independently on what I put in the config file
> (valid params, no params at all, etc.). I also can't change the
> scheduling parameters of a domain on-line anymore:
>
> # xl sched-sedf -d 3 -p 100 -s 50
> libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
> libxl_sched_sedf_domain_set failed.
>
> I'll try digging a bit more into this ASAP.

It's easy: Ian repeated an error he (and I) corrected elsewhere:
He's always setting period, regardless of the changed parameter...

> On more thing, are we ok with the _set command failing and he domain
> being created anyway? Or perhaps the whole process should just abort?

Perhaps a warning might be okay.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html