Re: [Xen-devel] [PATCH] xl: fix sedf parameters checking

[Dario Faggioli <raistlin@xxxxxxxx>]

On Thu, 2012-06-21 at 10:53 +0100, Ian Campbell wrote:
> Thanks for this. I'd like to get it in ASAP so we can start passing
> tests again. 
>
Hopefully! :-O

> I have a few queries though unfortunately...
> 
Sure.

> (most of the comments below are just me thinking aloud following the
> logic, because it's pretty subtle...)
> 
That's more than reasonable, it took a lot of this to me too. The point
is that the sedf scheduler is not, from POV of both the algorithm and
the interface, is a shape I'd call "good". Just think that if you _get()
dom0's scheduling parameters and you try to _set() back what you just
got, it will fail! :-(

Unfortunately, I can't think of any way to put some sense into it
without almost rewriting both (i.e., algorithm and interface), which I'm
still hoping to find the time to do at some point! :-)


> > @@ -563,12 +565,27 @@ static int sched_params_valid(libxl_doma
> >      if (sci.sched == LIBXL_SCHEDULER_SEDF) {
> >          if (has_weight && (has_period || has_slice))
> >              return 0;
> > -
> > +        if (has_period != has_slice)
> > +            return 0;
> 
> OK, so if you give period you _must_ give slice too, there is no
> default?
> 
Default for period is 100ms (although it's some sort of fake period, but
anyway). Default for slice is 0. So, theoretically, you could just
provide slice and rely on period being there, but not the vice versa.
However, if one wants an EDF real-time scheduler domain, I don't think
it's too much to ask to provide both slice and period, given it also
makes what follows easier to handle. Let me know if you think the other
way around. It's not that hard to deal with it here (without affecting
the logic below to much, which is already too complicated!).

> > +        /*
> > +         * Idea is, if we specify a weight, then both period and
> > +         * slice has to be zero. OTOH, if we do not specify a weight,
> > +         * that means we want a pure best effort domain or an actual
> > +         * real-time one. In the former case, it is period that needs
> > +         * to be zero, in the latter, weight should be.
> > +         */
> 
> I suspect there is a doc somewhere of which combinations of values are
> valid and what they mean etc -- perhaps we could link to it here?
> 
> It's docs/misc/sedf_scheduler_mini-HOWTO.txt I suppose?
> 
Yes, I can point the reader to it. Thanks.

> >          if (has_weight) {
> >              scp->slice = 0;
> >              scp->period = 0;
> >          }
> 
> If we have a weight then we've already checked that we don't has_period
> or has_slice so force them to zero. Makes sense.
> 
And besides making sense, it's needed for the set-scheduling-parameter
call to be successful (as it is the vice versa, if we have slice and
period, weight should be zeroed).


> > -        if (has_period || has_slice)
> > +        else if (!has_period) {
> 
> So here we don't have weight or period. We also know we don't have a
> slice either because we checked that we either have or don't have both
> of slice and period.
> 
Yep.

> > +            /* We can setup a proper best effort domain (extra time only)
> > +             * iff we either already have or are asking for some extra 
> > time. */
> > +            scp->weight = has_extratime ? scp->extratime : sci.extratime;
> 
> weight = extratime?
> 
> If I understand correctly then weight is an integer and extratime is a
> bool, which just seems wrong. Or is this subtly relying on the fact that
> True == 1 and therefore we set the weight to either 1 or 0?
> 
Exactly. As I tried to explain in the comment, not providing _anything_,
i.e., no slice/period and no weight, means you want a pure best effort
domain or whatever the default is, right? Ok, unfortunately, the default
is period=100,slice=0,weight=0,extratime=1, which would just fail, as
we're not providing any weight, we're providing a period but with zero
slice! Therefore I need to do something to fix things, or trying to
create a domain without passing any scheduling parameters would just
always fail (as it is actually happening).

Also, I can't just set period to zero, as the call will fail also if I
try to set weight=0, besides setting it internally in the scheduler code
(and that's why it is that that is returned when you ask for
actual/default scheduling parameters). :-O

However, if you set extratime=1 (with period=0 and slice=0), whatever
weight you provide, will be zeroed by the hypervisor, even if you can't
pass weight=0 yourself.

Tricky eh?

> If so then adding some !! on the bools would ensure that you really have
> 0 or 1 and not some other True value. Also expanding the comment to say
> that iff we have extratime then weight == 1 would make this clearer.
> 
Ok, I suppose I can do that. :-)

> > +            scp->period = 0;
> > +        }
> > +        if (has_period && has_slice)
> 
> is this also the same as "else if (has_period && has_slice)", which
> since we know has_period == has_slice is "else if (has_period")" which,
> given the previous "else if (!has_period)" is just "else" (plus a
> comment ;-))
> 
Probably. :-) I'll double check that and turn into what you suggest when
resubmitting.

Both when I did this in the first place and now, I tried to look at what
main_sched_sedf() does and replicate that logic as much as I can, to
make things easier to understand. The point here is we're being called
by a different context/situation, but I maybe can give it another shot
and see if I can quickly come up with something less mind blowing! :-P

Thanks for looking at the patch.
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/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