[PATCH] xl: check for meaningful combination of sedf config file parameters

[PATCH] xl: check for meaningful combination of sedf config file parameters

[Dario Faggioli]

As we do it in the implementation of `xl sched-sedf -d ...', some
consistency checking is needed while parsing the sedf scheduling
parameters provided via config file. Not doing this results in the call
libxl_domain_sched_params_set() to fail, and no parameters being
enforced for the domain.

Note we do this at config file parsing time as that gives us the chance
of bailing out early. It would have been pointless to add it within
sched_sedf_domain_set() (in libxl), as the very same thing is
done in the hypervisor, and the result is being checked and returned
to the caller already.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.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
@@ -561,6 +561,7 @@ static void parse_config_data(const char
     long l;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
+    int opt_w = 0, opt_p = 0, opt_s = 0;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 1;
     int pci_permissive = 0;
@@ -632,18 +633,36 @@ static void parse_config_data(const char
 
     /* the following is the actual config parsing with overriding
      * values in the structures */
-    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
+    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0)) {
         b_info->sched_params.weight = l;
+        opt_w = 1;
+    }
     if (!xlu_cfg_get_long (config, "cap", &l, 0))
         b_info->sched_params.cap = l;
-    if (!xlu_cfg_get_long (config, "period", &l, 0))
+    if (!xlu_cfg_get_long (config, "period", &l, 0)) {
         b_info->sched_params.period = l;
-    if (!xlu_cfg_get_long (config, "slice", &l, 0))
+        opt_p = 1;
+    }
+    if (!xlu_cfg_get_long (config, "slice", &l, 0)) {
         b_info->sched_params.slice = l;
+        opt_s = 1;
+    }
     if (!xlu_cfg_get_long (config, "latency", &l, 0))
         b_info->sched_params.latency = l;
     if (!xlu_cfg_get_long (config, "extratime", &l, 0))
         b_info->sched_params.extratime = l;
+    /* The sedf scheduler needs some more consistency checking */
+    if (opt_w && (opt_p || opt_s)) {
+        fprintf(stderr, "Either specify a weight OR a period and slice\n");
+        exit(1);
+    }
+    if (opt_w) {
+        b_info->sched_params.slice = 0;
+        b_info->sched_params.period = 0;
+    }
+    if (opt_p || opt_s)
+        b_info->sched_params.weight = 0;
+
 
     if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
         b_info->max_vcpus = l;

Re: [PATCH] xl: check for meaningful combination of sedf config file parameters

[Ian Campbell]

On Mon, 2012-06-04 at 15:01 +0100, Dario Faggioli wrote:
> As we do it in the implementation of `xl sched-sedf -d ...', some
> consistency checking is needed while parsing the sedf scheduling
> parameters provided via config file. Not doing this results in the call
> libxl_domain_sched_params_set() to fail, and no parameters being
> enforced for the domain.
> 
> Note we do this at config file parsing time as that gives us the chance
> of bailing out early. It would have been pointless to add it within
> sched_sedf_domain_set() (in libxl), as the very same thing is
> done in the hypervisor, and the result is being checked and returned
> to the caller already.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.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
> @@ -561,6 +561,7 @@ static void parse_config_data(const char
>      long l;
>      XLU_Config *config;
>      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
> +    int opt_w = 0, opt_p = 0, opt_s = 0;

These names don't make much sense in this context.

Perhaps you can just check each interesting option against the
corresponding LIBXL_DOAIN_SCHED_PARAM_DEFAULT? That might make some long
lines. Perhaps pulling this out into a separate valid_sched_params()
would help with that?

>      int pci_power_mgmt = 0;
>      int pci_msitranslate = 1;
>      int pci_permissive = 0;
> @@ -632,18 +633,36 @@ static void parse_config_data(const char
>  
>      /* the following is the actual config parsing with overriding
>       * values in the structures */
> -    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
> +    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0)) {
>          b_info->sched_params.weight = l;
> +        opt_w = 1;
> +    }
>      if (!xlu_cfg_get_long (config, "cap", &l, 0))
>          b_info->sched_params.cap = l;
> -    if (!xlu_cfg_get_long (config, "period", &l, 0))
> +    if (!xlu_cfg_get_long (config, "period", &l, 0)) {
>          b_info->sched_params.period = l;
> -    if (!xlu_cfg_get_long (config, "slice", &l, 0))
> +        opt_p = 1;
> +    }
> +    if (!xlu_cfg_get_long (config, "slice", &l, 0)) {
>          b_info->sched_params.slice = l;
> +        opt_s = 1;
> +    }
>      if (!xlu_cfg_get_long (config, "latency", &l, 0))
>          b_info->sched_params.latency = l;
>      if (!xlu_cfg_get_long (config, "extratime", &l, 0))
>          b_info->sched_params.extratime = l;
> +    /* The sedf scheduler needs some more consistency checking */
> +    if (opt_w && (opt_p || opt_s)) {
> +        fprintf(stderr, "Either specify a weight OR a period and slice\n");

Does this constrain you from setting valid combinations of credit*
parameters? I think not since period and slice are SEDF specific.


> +        exit(1);
> +    }
> +    if (opt_w) {
> +        b_info->sched_params.slice = 0;
> +        b_info->sched_params.period = 0;
> +    }
> +    if (opt_p || opt_s)
> +        b_info->sched_params.weight = 0;
> +
>  
>      if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
>          b_info->max_vcpus = l;

Re: [PATCH] xl: check for meaningful combination of sedf config file parameters

[Dario Faggioli]

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

On Wed, 2012-06-06 at 11:35 +0100, Ian Campbell wrote:
> > 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
> > @@ -561,6 +561,7 @@ static void parse_config_data(const char
> >      long l;
> >      XLU_Config *config;
> >      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
> > +    int opt_w = 0, opt_p = 0, opt_s = 0;
> 
> These names don't make much sense in this context.
> 
Yeah, I agree... It's just I needed something :-/

> Perhaps you can just check each interesting option against the
> corresponding LIBXL_DOAIN_SCHED_PARAM_DEFAULT? 
>
Mmm... I was mistakenly thinking these default values not to be there
yet, but I now see it. Yes, I guess I can do that.

> That might make some long
> lines. Perhaps pulling this out into a separate valid_sched_params()
> would help with that?
>
Maybe, but what to put here depends on your thought on the below...

> >      if (!xlu_cfg_get_long (config, "latency", &l, 0))
> >          b_info->sched_params.latency = l;
> >      if (!xlu_cfg_get_long (config, "extratime", &l, 0))
> >          b_info->sched_params.extratime = l;
> > +    /* The sedf scheduler needs some more consistency checking */
> > +    if (opt_w && (opt_p || opt_s)) {
> > +        fprintf(stderr, "Either specify a weight OR a period and slice\n");
> 
> Does this constrain you from setting valid combinations of credit*
> parameters? I think not since period and slice are SEDF specific.
> 
I'd say not at all, for the exact reason you're suggesting. Then, if you
ask what happens if you boot with sched=credit and then try to specify
both a cpu_weight and a period, then yes, it will kick you out.

The whole point is, period and slice are only meaningful for sedf so, if
you are using them, I take it like you meant to be using sedf, and thus
asking for a cpu_weight at the same time is wrong.

Of course, one can think at it the other way around (scheduler is
credit, so cpu_weight is fine and period and slice should be ignored).
If that is better, I can add a libxl_is_the_scheduler_credit? kind of
check to that if...

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: check for meaningful combination of sedf config file parameters

[Ian Campbell]

On Wed, 2012-06-06 at 12:05 +0100, Dario Faggioli wrote:
> On Wed, 2012-06-06 at 11:35 +0100, Ian Campbell wrote:
> > > 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
> > > @@ -561,6 +561,7 @@ static void parse_config_data(const char
> > >      long l;
> > >      XLU_Config *config;
> > >      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
> > > +    int opt_w = 0, opt_p = 0, opt_s = 0;
> > 
> > These names don't make much sense in this context.
> > 
> Yeah, I agree... It's just I needed something :-/
> 
> > Perhaps you can just check each interesting option against the
> > corresponding LIBXL_DOAIN_SCHED_PARAM_DEFAULT? 
> >
> Mmm... I was mistakenly thinking these default values not to be there
> yet, but I now see it. Yes, I guess I can do that.
> 
> > That might make some long
> > lines. Perhaps pulling this out into a separate valid_sched_params()
> > would help with that?
> >
> Maybe, but what to put here depends on your thought on the below...
> 
> > >      if (!xlu_cfg_get_long (config, "latency", &l, 0))
> > >          b_info->sched_params.latency = l;
> > >      if (!xlu_cfg_get_long (config, "extratime", &l, 0))
> > >          b_info->sched_params.extratime = l;
> > > +    /* The sedf scheduler needs some more consistency checking */
> > > +    if (opt_w && (opt_p || opt_s)) {
> > > +        fprintf(stderr, "Either specify a weight OR a period and slice\n");
> > 
> > Does this constrain you from setting valid combinations of credit*
> > parameters? I think not since period and slice are SEDF specific.
> > 
> I'd say not at all, for the exact reason you're suggesting. Then, if you
> ask what happens if you boot with sched=credit and then try to specify
> both a cpu_weight and a period, then yes, it will kick you out.
> 
> The whole point is, period and slice are only meaningful for sedf so, if
> you are using them, I take it like you meant to be using sedf, and thus
> asking for a cpu_weight at the same time is wrong.
> 
> Of course, one can think at it the other way around (scheduler is
> credit, so cpu_weight is fine and period and slice should be ignored).
> If that is better, I can add a libxl_is_the_scheduler_credit? kind of
> check to that if...

Lets keep it simple for now and go with the "don't do that" answer --
i.e. reject as invalid setting weight and period regardless of the
actual scheduler in use.

Ian.

Re: [PATCH] xl: check for meaningful combination of sedf config file parameters

[Ian Jackson]

Dario Faggioli writes ("[PATCH] xl: check for meaningful combination of sedf config file parameters"):
> As we do it in the implementation of `xl sched-sedf -d ...', some
> consistency checking is needed while parsing the sedf scheduling
> parameters provided via config file. Not doing this results in the call
> libxl_domain_sched_params_set() to fail, and no parameters being
> enforced for the domain.

Why does xl continue after libxl_domain_sched_params_set fails ?

Ian.

Re: [PATCH] xl: check for meaningful combination of sedf config file parameters

[Dario Faggioli]

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

On Wed, 2012-06-06 at 11:41 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH] xl: check for meaningful combination of sedf config file parameters"):
> > As we do it in the implementation of `xl sched-sedf -d ...', some
> > consistency checking is needed while parsing the sedf scheduling
> > parameters provided via config file. Not doing this results in the call
> > libxl_domain_sched_params_set() to fail, and no parameters being
> > enforced for the domain.
> 
> Why does xl continue after libxl_domain_sched_params_set fails ?
> 
Well, that I really don't know. It has always been like this I guess, it
just print the related error about inconsistent/wrong scheduling
parameters and then the domain is created with default ones.

Should we/I stop it?

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: check for meaningful combination of sedf config file parameters

[Ian Jackson]

Dario Faggioli writes ("Re: [PATCH] xl: check for meaningful combination of sedf config file parameters"):
> On Wed, 2012-06-06 at 11:41 +0100, Ian Jackson wrote:
> > Why does xl continue after libxl_domain_sched_params_set fails ?
> 
> Well, that I really don't know. It has always been like this I guess, it
> just print the related error about inconsistent/wrong scheduling
> parameters and then the domain is created with default ones.
> 
> Should we/I stop it?

Please, yes.  In general libxl's error handling is rather poor in
places (although a lot of it has been improved).

Where you notice that (a) the thing you wanted to do doesn't work and
(b) xl blunders on anyway, it's best to fix (b) first, while you still
have the test case, and then (a) :-).

Thanks,
Ian.

Re: [PATCH] xl: check for meaningful combination of sedf config file parameters

[Ian Campbell]

On Wed, 2012-06-06 at 11:41 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH] xl: check for meaningful combination of sedf config file parameters"):
> > As we do it in the implementation of `xl sched-sedf -d ...', some
> > consistency checking is needed while parsing the sedf scheduling
> > parameters provided via config file. Not doing this results in the call
> > libxl_domain_sched_params_set() to fail, and no parameters being
> > enforced for the domain.
> 
> Why does xl continue after libxl_domain_sched_params_set fails ?

Because libxl just carries on (in libxl__build_post) and doesn't
propagate the error... It most likely shouldn't. I think we can change
that separately though?

Ian.

Re: [PATCH] xl: check for meaningful combination of sedf config file parameters

[Dario Faggioli]

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

On Wed, 2012-06-06 at 11:49 +0100, Ian Campbell wrote:
> Because libxl just carries on (in libxl__build_post) and doesn't
> propagate the error... It most likely shouldn't. I think we can change
> that separately though?
> 
Taking care of this right now... it seems, next version of this patch
will be a series of two patches. :-)

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