Dario Faggioli
2012-Jun-22 16:16 UTC
[PATCH] libxl: fix validation of scheduling parameters for sedf
2205914617cb does its job in correcting the "wrong domain being
considered" issue introduced by 9d1fd58ff602. Unfortunately, when
dealing (again!) with the sedf scheduler, it is required for the
vCPUs of a domain to have been allocated and setup already (in
the hypervisor), when the first call to libxl_domain_sched_params_get()
happens, and that is not true.
This fixes that by avoiding calling that function at all, as we
only need to know which scheduler the domain is running under,
and that is provided by libxl__domain_scheduler() which is safe
to be called there.
While at it, also improve a bit the comments about the whole
sedf parameter validation and mangling process.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -80,36 +80,48 @@ static int sched_params_valid(libxl__gc
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);
/* The sedf scheduler needs some more consistency checking */
- if (sci.sched == LIBXL_SCHEDULER_SEDF) {
+ if (libxl__domain_scheduler(gc, domid) == LIBXL_SCHEDULER_SEDF) {
if (has_weight && (has_period || has_slice))
return 0;
+ /* If you want a real-time domain, with its own period and
+ * slice, please, do provide both! */
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.
+ * slice has to be zero. OTOH, if we do specify a period and
+ * slice, it is weight that should be zeroed. See
+ * docs/misc/sedf_scheduler_mini-HOWTO.txt for more details
+ * on the meaningful combinations and their meanings.
*/
if (has_weight) {
scp->slice = 0;
scp->period = 0;
}
else if (!has_period) {
+ /* No weight nor slice/period means best effort. Parameters needs
+ * some mangling in order to properly ask for that, though. */
+
+ /*
+ * Providing no weight does not make any sense if we do not allow
+ * the domain to run in extra time. On the other hand, if we have
+ * extra time, weight will be ignored (and zeroed) by Xen, but it
+ * can''t be zero here, or the call for setting the
scheduling
+ * parameters will fail. So, avoid the latter by setting a random
+ * weight (namely, 1), as it will be ignored anyway.
+ */
+
/* 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->weight = has_extratime ? scp->extratime : 1;
scp->period = 0;
+ } else {
+ /* Real-time domain: will get slice CPU time over every period */
+ scp->weight = 0;
}
- if (has_period && has_slice)
- scp->weight = 0;
}
return 1;
Ian Campbell
2012-Jun-22 16:44 UTC
Re: [PATCH] libxl: fix validation of scheduling parameters for sedf
On Fri, 2012-06-22 at 17:16 +0100, Dario Faggioli wrote:> 2205914617cb does its job in correcting the "wrong domain being > considered" issue introduced by 9d1fd58ff602. Unfortunately, when > dealing (again!) with the sedf scheduler, it is required for the > vCPUs of a domain to have been allocated and setup already (in > the hypervisor), when the first call to libxl_domain_sched_params_get() > happens, and that is not true. > > This fixes that by avoiding calling that function at all, as we > only need to know which scheduler the domain is running under, > and that is provided by libxl__domain_scheduler() which is safe > to be called there. > > While at it, also improve a bit the comments about the whole > sedf parameter validation and mangling process. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>Acked-by: Ian Campbell <ian.campbell@citrix.com> Committed-by: Ian Campbell <ian.campbell@citrix.com>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -80,36 +80,48 @@ static int sched_params_valid(libxl__gc > 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); > > /* The sedf scheduler needs some more consistency checking */ > - if (sci.sched == LIBXL_SCHEDULER_SEDF) { > + if (libxl__domain_scheduler(gc, domid) == LIBXL_SCHEDULER_SEDF) { > if (has_weight && (has_period || has_slice)) > return 0; > + /* If you want a real-time domain, with its own period and > + * slice, please, do provide both! */ > 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. > + * slice has to be zero. OTOH, if we do specify a period and > + * slice, it is weight that should be zeroed. See > + * docs/misc/sedf_scheduler_mini-HOWTO.txt for more details > + * on the meaningful combinations and their meanings. > */ > if (has_weight) { > scp->slice = 0; > scp->period = 0; > } > else if (!has_period) { > + /* No weight nor slice/period means best effort. Parameters needs > + * some mangling in order to properly ask for that, though. */ > + > + /* > + * Providing no weight does not make any sense if we do not allow > + * the domain to run in extra time. On the other hand, if we have > + * extra time, weight will be ignored (and zeroed) by Xen, but it > + * can''t be zero here, or the call for setting the scheduling > + * parameters will fail. So, avoid the latter by setting a random > + * weight (namely, 1), as it will be ignored anyway. > + */ > + > /* 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->weight = has_extratime ? scp->extratime : 1; > scp->period = 0; > + } else { > + /* Real-time domain: will get slice CPU time over every period */ > + scp->weight = 0; > } > - if (has_period && has_slice) > - scp->weight = 0; > } > > return 1;