Re: [Xen-devel] [RFC PATCH v2 0/7] Repurpose SEDF Scheduler for Real-time Use

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On gio, 2014-07-10 at 15:43 +0100, Ian Campbell wrote:
> On Wed, 2014-07-09 at 16:55 -0400, Josh Whitehead wrote:
> > Finally, the amount of name changes has been significantly reduced,
> > the most prominent being to keep the "sedf" name and not switch to a
> > new name as was also decided through previous discussion.
> 
> This is unfortunate since renaming would have meant a new member of the
> libxl_scheduler enum which would have made the API
> deprecation/compatibility stuff much simpler -- since you could just
> return ERROR_INVAL (or ERROR_SEDF_WENT_AWAY ;-)) whenever you saw the
> SEDF enum value, rather than having to check each field individually.
> 
*** Short (well, sort of!) Answer ***
Personally, I would prefer to keep SEDF alive at least for a few
versions (warning the user about some params being ignored, etc.).

However, whatever route we take, concentrating on this series, I don't
think the renaming+SEDF deprecation should happen until proper SMP
support is implemented, and probably also not until support for per-VCPU
scheduling parameters (quite important for an advanced real-time
scheduling solution) is there. It's fine for an RFC to be incomplete, so
I'm not complaining or saying this can't be a step in the right
direction.

However, the biggest problem of SEDF is its poor support for SMP, and
lack of capability for setting scheduling parameters on a per-VCPU
basis, which this series (rightfully) does not address.
Imagine if, after proper review and cleanups, the set of changes from
this series goes in, and no other follow up series makes it by feature
freeze, 4.5 will have:
 - SCHED_SEDF deprecated, because it was lacking SMP support and support
   for per-VCPU scheduling parameters
 - SCHED_CBS, _lacking_ SMP support and support for per-VCPU scheduling 
   parameters
Not ideal. :-/

[That, of course, does not mean that I won't be reviewing the series
itself. On the contrary, I'm down to it, especially patches 1, 2 and 3.]

So, to summarize, I'm ok with whatever route we decide to take, included
full deprecation and renaming, but let's make it worth it!
If wanting to do that, Josh and Robert, you can well assume that the
unnecessary parameters and the messy code currently in sched_sedf.c
related to them can go away (either being ignored by libxl, or because
this will be a new scheduler), and so it's fine for you to kill them in
one of your patches. This is something I was not so sure myself, so,
thanks for bringing the discussion up on it with this series! :-)

Finally, I also apologize if my comments were not explicitly along this
line during v1 review... it just took a bit of thinking to wrap my head
around what the best way forward could be here. :-(

Dario

*** Long (Really Long!!) Answer ***
The _core_ of the contribution of this series, as of now, is not a new
scheduling policy. It is a slight (although, to me, very relevant)
modification to SEDF internals in order to fix a few problems the
scheduler has.

In fact, I like the fact that the subject itself is 'Repurpose SEDF
Scheduler'. :-)

The problems SEDF has are:
 1. it has really really really poor SMP support
 2. it does not allow to specify scheduling parameters on a per-VCPU
    basis, but only on a domain basis. This is fine for general purpose
    schedulers, but can be quite important in real-time workloads
 3. it tries to be both a real-time and a general purpose scheduler
    which is:
    a) very hard, and often means adding a lot of complexity for
       nothing, since you're not going to be that good at both anyway...
       and this is one of the cases;
    b) not necessary any longer, now that we have good general purpose 
       schedulers, like credit and (hopefully ready soon) credit2;
 4. it deals with VCPU wake-ups in an hacky, unnecessarily complex and,
    if you ask me, incorrect way.

Note that, in order to achieve 3, not only a lot of complexity (hacks!)
were introduced inside sched_sedf.c in Xen, it is also the interface
that contains parameters that are not useful for a real-time scheduler
which only aims at doing its own job well, i.e.,
_being_a_good_real-time_scheduler_. :-D

The issues are, in my opinion, sort-of in priority order, with 3 and 4
being related and almost equally important. 4 is probably the easiest,
as it only involves changes in xen/common/sched_sedf.c, while 3 is not
hard to do per se, but has interface implications (i.e., exactly what
we're discussing here).

* Just to make sure everyone understand, even if not into SEDF and RT
* scheduling, this patch series, as it stands now, fixes 4, and tries
* to deal with 2.
*
* That's why I'm saying "let's discuss renaming later".

In the long run, what we want is a good, SMP capable, real-time
scheduler. This good SMP capable real-time scheduler won't ever need
some of the parameters that the SEDF interface includes or, if we make
it digest them, semantic and behavior will be different from now anyway.

As far as I can tell, this can be implemented:
 A) modifying SEDF, i.e., actually modifying sched_sedf.c, fixing the
    problems above
 B) introducing something completely new (sched_rt.c ?), which leaves
    open the question of what to do with sched_sedf.c and
    {LIBXL,XEN}_SCHEDULER_SEDF as a scheduling policy.

If it were me doing it, I probably would have tried to modify SEDF, in a
very similar way to what this series does, and then, probably, even in
the long run, I'd have kept the name and deprecated (or changed the
meaning, and warning the user about it) some of the params. And, in
fact, Josh's work springs from a conversation we had about this a while
back, right Josh? :-)

In fact, I don't like the option of getting rid of SEDF all together and
all of a sudden, even given the issues it has. I think that would not be
in line with what we've done up to now, in terms of API stability/not
breaking things for users (see xm-->xl migr.), and I don't think it's
necessary. Fixing it internally and warning the user that a few
parameters are now being ignored seems a good solution to me, at least
for a few versions.

And even if we decide that SEDF has to go, in favor of SCHED_CBS,
SCHED_RT, SCHED_<whatever>, I personally would _NOT_ do that until such
a new scheduler:
 - is a good and SMP capable real-time scheduler
 - has an interface we're happy with, and can commit to keep it stable

This series is an RFC, so it's ok for it not to be complete and, since
it tries to fix 2 of the 4 issues I identified myself, I certainly am
not saying it's not a step in (one of the possible) right direction. For
sure, thanks to the discussion we've had as a consequence of this
series, we now know that deprecating SEDF is --either sooner or later--
an option... probably even the best option!

So go ahead with the items you have in your 'FUTURE DEVELOPMENT' section
already, and we'll kill SEDF as soon as it will be worth it. I'll let
you have my comments on patches 1, 2 and 3 ASAP.

Thanks and Regards,
Dario


-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel