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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxx 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@xxxxxxxxx>

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;
>      }
>  



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