[PATCH] xl: fix sedf parameters checking

[PATCH] xl: fix sedf parameters checking

[Dario Faggioli]

9d1fd58ff602 was bogous in not letting a new domain being created
if its scheduling parameters --when running under the sedf scheduler--
were not fully specified, making creation fail like in this example
here below:

2012-06-16 07:37:47 Z executing ssh ... root@10.80.248.105 xl create /etc/xen/debian.guest.osstest.cfg
libxl: error: libxl.c:3619:sched_sedf_domain_set: setting domain sched sedf: Invalid argument
libxl: error: libxl_create.c:710:domcreate_bootloader_done: cannot (re-)build domain: -3
Parsing config from /etc/xen/debian.guest.osstest.cfg

This is due to the fact the values for period, slice, weight and
extratime should be consistent among each others, and if not all
are explicitly specified, someone has to make that happen. That
was right the purpose of the change in question, but it was failing
at achieving so.

This commit fixes things by forcing unspecified parameters to
sensible values, depending on the ones the user provided.

Signed-off-by: Dario Faggioli <dario.faggioli@citix.com>

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -555,6 +555,8 @@ static int sched_params_valid(libxl_doma
     int has_weight = scp->weight != LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT;
     int has_period = scp->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT;
     int has_slice = scp->slice != LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT;
+    int has_extratime =
+                scp->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT;
     libxl_domain_sched_params sci;
 
     libxl_domain_sched_params_get(ctx, domid, &sci);
@@ -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;
+
+        /*
+         * 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.
+         */
         if (has_weight) {
             scp->slice = 0;
             scp->period = 0;
         }
-        if (has_period || has_slice)
+        else if (!has_period) {
+            /* 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;
+            scp->period = 0;
+        }
+        if (has_period && has_slice)
             scp->weight = 0;
     }

Re: [PATCH] xl: fix sedf parameters checking

[Ian Campbell]

On Wed, 2012-06-20 at 18:09 +0100, Dario Faggioli wrote:
> 9d1fd58ff602 was bogous in not letting a new domain being created
> if its scheduling parameters --when running under the sedf scheduler--
> were not fully specified, making creation fail like in this example
> here below:
> 
> 2012-06-16 07:37:47 Z executing ssh ... root@10.80.248.105 xl create /etc/xen/debian.guest.osstest.cfg
> libxl: error: libxl.c:3619:sched_sedf_domain_set: setting domain sched sedf: Invalid argument
> libxl: error: libxl_create.c:710:domcreate_bootloader_done: cannot (re-)build domain: -3
> Parsing config from /etc/xen/debian.guest.osstest.cfg
> 
> This is due to the fact the values for period, slice, weight and
> extratime should be consistent among each others, and if not all
> are explicitly specified, someone has to make that happen. That
> was right the purpose of the change in question, but it was failing
> at achieving so.
> 
> This commit fixes things by forcing unspecified parameters to
> sensible values, depending on the ones the user provided.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citix.com>

Thanks for this. I'd like to get it in ASAP so we can start passing
tests again. I have a few queries though unfortunately...

(most of the comments below are just me thinking aloud following the
logic, because it's pretty subtle...)

> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -555,6 +555,8 @@ static int sched_params_valid(libxl_doma
>      int has_weight = scp->weight != LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT;
>      int has_period = scp->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT;
>      int has_slice = scp->slice != LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT;
> +    int has_extratime =
> +                scp->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT;
>      libxl_domain_sched_params sci;
>  
>      libxl_domain_sched_params_get(ctx, domid, &sci);
> @@ -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?

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

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

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

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

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.

> +            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 ;-))

>              scp->weight = 0;
>      }
>  

Re: [PATCH] xl: fix sedf parameters checking

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 5992 bytes --]

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] xl: fix sedf parameters checking

[Ian Campbell]

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.

Re: [PATCH] xl: fix sedf parameters checking

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2110 bytes --]

On Thu, 2012-06-21 at 11:40 +0100, Ian Campbell wrote:
> > 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.
> 
Exactly!

> 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).
> 
Ok.
 
> > Tricky eh?
> 
> Tricky isn't the half of it ;-)
>
:-)

> > 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  ;-)
> 
Me too. I'll add those bits and resend then.

Thanks and Regards,
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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] xl: fix sedf parameters checking

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xl: fix sedf parameters checking"):
> On Wed, 2012-06-20 at 18:09 +0100, Dario Faggioli wrote:
> > 9d1fd58ff602 was bogous in not letting a new domain being created
> > if its scheduling parameters --when running under the sedf scheduler--
> > were not fully specified, making creation fail like in this example
> > here below:
...
> 
> Thanks for this. I'd like to get it in ASAP so we can start passing
> tests again. I have a few queries though unfortunately...

You'll see I've applied it already.  I would be happy to take a
followup cleanup/fixup.

Ian.

Re: [PATCH] xl: fix sedf parameters checking

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1266 bytes --]

On Thu, 2012-06-21 at 18:21 +0100, Ian Jackson wrote: 
> Ian Campbell writes ("Re: [PATCH] xl: fix sedf parameters checking"):
> > On Wed, 2012-06-20 at 18:09 +0100, Dario Faggioli wrote:
> > > 9d1fd58ff602 was bogous in not letting a new domain being created
> > > if its scheduling parameters --when running under the sedf scheduler--
> > > were not fully specified, making creation fail like in this example
> > > here below:
> ...
> > 
> > Thanks for this. I'd like to get it in ASAP so we can start passing
> > tests again. I have a few queries though unfortunately...
> 
> You'll see I've applied it already.  
>
Yes, I saw that earlier in the afternoon.

> I would be happy to take a
> followup cleanup/fixup.
> 
I already sent something like that:

Subject: 	[PATCH] xl: improve the comments for sedf parameters checking
 Date: 	        Thu, 21 Jun 2012 16:19:50 +0200
 Message-id: 	<fe85a8d600800524917c.1340288390@Solace>

:-)

Thanks and Regards,
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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] xl: fix sedf parameters checking

[Ian Jackson]

Dario Faggioli writes ("[PATCH] xl: fix sedf parameters checking"):
> 9d1fd58ff602 was bogous in not letting a new domain being created
> if its scheduling parameters --when running under the sedf scheduler--
> were not fully specified, making creation fail like in this example
> here below:

Thanks,

Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

(I fix a typo in the commit message.)

Ian.