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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-06-21 at 11:26 +0100, Dario Faggioli wrote:

> > > @@ -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!).

This is fine as is.

> > > +            /* 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.

Ah, ok, so the key thing, which I think needs to be in the comment, is
that weight must be non-zero in this case but that the specific value is
irrelevant since it will be zeroed.

I think it might be worth also mentioning in the comment what best
effort means in practice.I think it means a domain which can use extra
time but which has no actual period/slice/weight of its own (despite the
wrinkle about how you must supply weight).

> 
> Tricky eh?

Tricky isn't the half of it ;-)

> > 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. :-)

I'm not sure it's necessary if you include the comment to explain the
thing about weight being non-zero and getting clearer, but it can't hurt
I suppose.

> 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

I think actually with your explanations the current way seems to make
sense to me, some more detail in the comments should be sufficient I
think. Rewriting it again into something (even if it were less mind
blowing) would likely still mean another round of this sort of
conversation I expect  ;-)

Ian.


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