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; }
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; > } >
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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! :-PI 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.
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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.
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.
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Reasonably Related Threads
- xl doesn't honour the parameter cpu_weight from my config file while xm does honour it
- xl doesn't honour the parameter cpu_weight from my config file while xm does honour it
- How to get actual DOMU scheduling parameters ?
- [PATCH] xm,xend: flesh out xm sched-sedf
- [xen-unstable test] 13339: regressions - FAIL