Dario Faggioli
2012-Jun-04 14:01 UTC
[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.
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;
Ian Campbell
2012-Jun-06 10:35 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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;
Ian Jackson
2012-Jun-06 10:41 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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.
Dario Faggioli
2012-Jun-06 10:48 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Jun-06 10:49 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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.
Ian Jackson
2012-Jun-06 10:57 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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.
Dario Faggioli
2012-Jun-06 11:05 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Jun-06 11:07 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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.
Dario Faggioli
2012-Jun-06 14:23 UTC
Re: [PATCH] xl: check for meaningful combination of sedf config file parameters
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel