Re: [PATCH] full support of setting scheduler parameters on domain creation

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

On 05/21/2012 03:34 PM, Ian Campbell wrote:
> On Mon, 2012-05-21 at 14:23 +0100, Juergen Gross wrote:
>> On 05/21/2012 02:07 PM, Ian Campbell wrote:
>>> On Mon, 2012-05-21 at 12:46 +0100, Juergen Gross wrote:
>>>> Obtains current scheduler parameters before modifying any settings specified
>>>> via domain config. Only specified settings must be modified, of course!
>>>>
>>> I presume this will fix the
>>> libxl: error: libxl.c:3208:libxl_sched_credit_domain_set: Cpu weight out
>>> of range, valid values are within range from 1 to 65535
>>> warning we are currently seeing? Thanks!
>>>
>>>> @@ -233,6 +233,14 @@ libxl_sched_params = Struct("sched_param
>>>>        ("slice",        integer),
>>>>        ("latency",      integer),
>>>>        ("extratime",    integer),
>>>> +    ("set_weight",       bool),
>>>> +    ("set_cap",          bool),
>>>> +    ("set_tslice_ms",    bool),
>>>> +    ("set_ratelimit_us", bool),
>>>> +    ("set_period",       bool),
>>>> +    ("set_slice",        bool),
>>>> +    ("set_latency",      bool),
>>>> +    ("set_extratime",    bool),
>>> Rather than doing this it would be preferable to identify some specific
>>> value which means "default" for each of these fields. Generally this
>>> would be either 0 (preferred if possible) or ~0 or -1. You can then
>>> describe this in the IDL using the "init_val" property on each field.
>>> e.g.:
>>>
>>> @@ -225,7 +225,7 @@ libxl_domain_create_info = Struct("domai
>>>    MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
>>>
>>>    libxl_sched_params = Struct("sched_params",[
>>> -    ("weight",       integer),
>>> +    ("weight",       integer, {'init_val': -1}),
>>>        ("cap",          integer),
>>>        ("tslice_ms",    integer),
>>>        ("ratelimit_us", integer),
>>>
>>> (just as an illustration of a non-zero default, I suspect 0 would
>>> actually be a fine default value for weight, and 0 is the default
>>> init_val)
>>>
>>> Then libxl_sched_params_init, which xl must call (perhaps indirectly,
>>> e.g. via libxl_domain_build_info_init) would set these defaults and xl
>>> would override them from the config and libxl would only set those which
>>> were non-default.
>> Hmm. Scheduling parameters are handled in the hypervisor. I don't want to
>> export the knowledge about semantics to the tools. If this is no problem,
>> why can't I just set the defaults in the tools and omit asking the
>> hypervisor for the current settings?
> Exporting the idea that 0 is not a valid weight is (IMHO at least)
> better than exporting the fact that the default weight is (e.g.) 200 and
> hard coding that in multiple places.

Agreed.

>> Additionally there are parameters (well, one parameter) which apply to
>> multiple schedulers. Just by chance -1 would be invalid for all of them,
>> but I don't like to hard code this coincidence.
> Which parameter is it? Is there any reasonable possibility that a
> scheduler would ever use -1 (or +4 billion) for it?

It's the cpu_weight. :-)
I don't object using an invalid value instead of a boolean, I just thought
it would be cleaner to say "parameter xy was specified". This would include
the case when a user specifies 0 for the cpu_weight: it would be rejected
instead of just ignored...

> You could define a symbolic name if that would make you more comfortable
> (that would allow us to change the specific value without changing the
> API)

As the "invalid" value would be used at least twice this is a must.


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