Re: [RFC PATCH 3/4] Updated comments/variables to reflect cbs, fixed formatting and confusing comments/variables

[Joshua Whitehead <josh.whitehead@dornerworks.com>]

On 6/18/2014 7:18 AM, George Dunlap wrote:
> On 06/17/2014 05:11 PM, Dario Faggioli wrote:
>> On lun, 2014-06-16 at 16:29 +0100, George Dunlap wrote:
>>> On 06/16/2014 10:33 AM, Jan Beulich wrote:
>>>>>>> On 13.06.14 at 21:58, <josh.whitehead@dornerworks.com> wrote:
>>>>> --- a/xen/include/public/trace.h
>>>>> +++ b/xen/include/public/trace.h
>>>>> @@ -75,7 +75,7 @@
>>>>>    /* Per-scheduler IDs, to identify scheduler specific events */
>>>>>    #define TRC_SCHED_CSCHED   0
>>>>>    #define TRC_SCHED_CSCHED2  1
>>>>> -#define TRC_SCHED_SEDF     2
>>>>> +#define TRC_SCHED_CBS      2
>>>> While the change to domctl.h is fine, I'm not sure we can allow simple
>>>> renaming elsewhere in the public headers (i.e. the old name may need
>>>> to remain there, guarded with a __XEN_INTERFACE_VERSION__
>>>> conditional).
>>> I think the tracing stuff is fine too -- we've always considered that
>>> non-stable (and have made incompatible changes across versions).
>>>
>>> But the libxl interfaces *do* need to have something sensible done with
>>> them.
>>>
>>> Given that,  I think it would probably be better to make this patch series:
>>>
>>> 1/N: Add sched_cbs.c to Xen
>>> 2/N: Add cbs to toolstack
>>> 3/N: Remove sedf scheduler (with appropriate backwards-compatibility bits)
>>>
>>> I think that would make it a bit easier to review as well.
>>>
>> As far as this patch is concerned, I agree with George.
>>
>> However... Is removing SEDF an option? Is radically changing, if not
>> it's behavior (as it's known to be pretty broken), the expectations of
>> an user, e.g., of an old application being compiled with a new version
>> of xen+libxl an option?
>>
>> If yes, what's the process to do that?
>> Personally, I'm all for having a really working real-time scheduling
>> solution, and you all know that. :-) However, especially considering
>> Josh's and Robbie's series, I think I would not remove or rename SEDF, I
>> rather "just" amend the implementation.
>>
>> In future, it would be interesting to introduce more advanced real-time
>> scheduling features an capabilities, like the ones coming from RT-Xen
>> (and the RT-Xen guys are working on doing that), but I think that can be
>> done step-by-step, and without any massive renaming or removal.
>>
>> So, I'm asking, mostly to George, about the overall scheduling aspects
>> and implications, and to the tools maintainer, as that's where API
>> stability is to be enforced: should this be a concern? In what sense API
>> stability applies here? Can we, for example, start to ignore one or more
>> SEDF scheduling parameters?
Just wanted to throw in a few comments:

> 
> Well the primary thing is that programs using the old interface still 
> need to compile.  This could be done with the libxl interface version 
> #ifdef-ery.
> 
This should be simple to add, we'll try to take a look at that for V2, but it
may be better to wait until V3 so we can get some more feedback on the series
before making too many of those types of changes and can see what's actually
necessary.

> One of the things that makes this handy is that the only way a user can 
> *set* SEDF via libxl is when they create a cpupool; all the other uses 
> are just recognizing SEDF if it's actually the scheduler already.
> 
> Re the functionality: for one, if nobody is actually using it, we don't 
> need to keep it around.  One advantage of adding a CBS scheduler and 
> just removing the SEDF scheduler is that it should make it pretty clear 
> who those people are, and make putting it back pretty easy.
> 
> If someone is using it -- I think asking them to switch to the 
> (presumably) better CBS scheduler makes sense, since it accomplishes 
> much the same thing.
> 
One of our motivations behind modifying the SEDF was our general impression that
it was no longer in use in any serious way by anyone in the Xen community as
there were better general purpose schedules available (credit/credit2) and the
modifications to it made it unsuitable for real-time use.

> On the whole, I'm inclined to say we should just go ahead and change the 
> SEDF interface in place rather than introducing a new scheduler.
> 
> The question then comes with the parameters: whether it's better to 
> attempt to keep working the best you can, or whether it's better to 
> alert the admin to the fact that the scheduler has changed significantly 
> and it's time to re-tweak the system.
> 
> I'm inclined to say that we should:
> * Leave the name and number as sedf
> * Remove parameters no longer being used, gated with the LIBXL_API_VERSION.
> * For all schedulers, if parameters that are not used are set to 
> anything other than LIBXL_DOMAIN_SCHED_PARAM_{FOO}_DEFAULT, 
> libxl_domain_set_sched_params() should return an error.
> 
I commented on it in an earlier e-mail as well, but I thought I'd put it here
too- I think this is the path our V2 of the patch series will focus on.  Keep
the SEDF naming scheme in general, update names and parameters only on a limited
basis ("change the SEDF interface in place" as George said), and reorganize the
patch to be easier to review.  This should also put it in a good position to add
functionality in the future (from RT-Xen or otherwise).

> Alternately, we could have libxl throw a warning that we're ignoring a 
> parameter in most cases; and in the case of no longer used parameters, 
> warn specifically that it's no longer implemented.  We could maybe also 
> point them to some documentation, and in the documentation say to 
> contact the xen-devel list if you're still using it.
> 
We would also be willing to add this as desired :-)

Thanks again George for your input as well!

- Josh Whitehead

> If people complain, then we can try to accomodate them.  But at very 
> least we get an idea if anyone actually cares. :-)
> 
>   -George
>