Re: [PATCH v3 1/7] libxl: get rid of the SEDF scheduler

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/07/2015 03:15 PM, Ian Campbell wrote:
> On Tue, 2015-07-07 at 14:48 +0100, Ian Campbell wrote:
>> On Mon, 2015-07-06 at 18:17 +0200, Dario Faggioli wrote:
>>>>> @@ -356,9 +357,13 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
>>>>>      ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>>>>>      ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
>>>>>      ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>>>>> -    ("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'}),
>>>>> +    # The following three parameters ('slice', 'latency' and 'extratime') are deprecated,
>>>>> +    # and will have no effect if used, since the SEDF scheduler has been removed.
>>>>> +    # Note that 'period' was an SDF parameter too, but it is still effective as it is
>>>>> +    # now used (together with 'budget') by the RTDS scheduler.
>>>>> +    ("slice",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),     # deprecated
>>>>> +    ("latency",      integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),   # deprecated
>>>>> +    ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}), # deprecated
>>>>>      ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
>>>>
>>>> Since we're aiming for API compatibility rather than ABI compatibility,
>>>> is it allowable to move 'budget' up above the comment, so that it's more
>>>> obvious that it hasn't been deprecated?
>>>>
>>> It's tool's people call, I guess. My opinion is that, yes, it should be
>>> possible without any issue, and yes, I also would like the end result
>>> better.
>>
>> Yes, I think you can move it up.
>>
>> You should also add a blank line before "# The following three" and you
>> can now also drop the per line "# deprecated" since it will be visually
>> obvious which three the bigger comment refers to.
>>
>> Nit:
>>>     (-25, "FEATURE_REMOVED"), # For functionallities that are no longer there
>>
>> The correct spelling would be "functionalities", but the correct meaning
>> would be "functionality that is", I'd probably also go with "that has
>> been removed".
>>
>> Unless there is some reason to resend
> 
> Which of course I gave myself in the first couple of paras!

Oh, took you to mean that you were offering to make both modifications
on check-in, and I thought "Well, that's generous of him."  I guess
you're not so generous after all. ;-)

 -G