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

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

On 07/06/2015 05:17 PM, Dario Faggioli wrote:
> On Mon, 2015-07-06 at 16:40 +0100, George Dunlap wrote:
>> On 07/06/2015 04:30 PM, Dario Faggioli wrote:
>>> only the interface is left in place, for backward
>>> compile-time compatibility, but every attempt to
>>> use it would throw an error.
>>>
>>> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>>> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
>>
>> This probably should have been dropped...
>>
>>> Chenges from v2:
>>>  - introduce and use ERROR_FEATURE_REMOVED, as requested
>>>    during review;
>>>  - mark the SEDF only parameter as deprecated in libxl_types.idl,
>>>    as requested during review.
>>
>> ...given these.  One question:
>>
> Really? I'm basically only adding commentary, not changing (or adding,
> or removing) a single line of code... I mean, the deprecation was
> de-facto there already, since v1, it just was not stated explicitly
> anywhere in that particular file.
> 
> That's why I didn't think a something like adding this comment would
> call for removal of the tag.
> 
> Anyway, sorry for this. :-)

Not a big deal of course, and as it happens I wouldn't have minded if
the patch went in as it is.  But what if I hadn't liked the name of the
error code?  It looks like I approve of it, which might sway some
maintainer's view, when in fact I haven't expressed an opinion.

I probably wouldn't even have bothered saying anything if I hadn't
already been replying to the e-mail because of the line below. :-)

 -G

> 
>>> @@ -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.
> 
> Thanks and Regards,
> Dario
>