Xen devel - Jun 2012 - [PATCH 0 of 2 v2] Sanity checking of scheduling parameters

Hi,

This small series achieves two goals:
 - check the return value of libxl_domain_sched_params_set() in
   libxl__build_post() and deal with the error, if that is the
   case (patch #1);
 - check and ensue we are passing along a meaningful set of sedf
   scheduling parameters when they come directly from the config file
   (patch #2)

Tested on both credit and sedf schedulers.

Changes from v1:
 * patch #1: it was not there at all in v1! :-P
 * patch #2: the if-s have been moved into an helper function. Also,
   they only happen if the domain is actually being scheduled with
   sedf (IanC, yes, I decided to do it... At the end of the day, it is
   simple enough I think).


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)

So that the caller (e.g., libxl__build_post() ) knows and can deal with it.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -175,7 +175,8 @@ int libxl__build_post(libxl__gc *gc, uin
     char **ents, **hvm_ents;
     int i;
 
-    libxl_domain_sched_params_set(CTX, domid, &info->sched_params);
+    if (libxl_domain_sched_params_set(CTX, domid, &info->sched_params))
+        return ERROR_INVAL;
 
     libxl_cpuid_apply_policy(ctx, domid);
     if (info->cpuid != NULL)

As it happens in the implementation of `xl sched-sedf -d ...'', some
consistency checking is needed for the scheduling parameters when
they come from the config file.

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
@@ -550,6 +550,31 @@ vcpp_out:
     return rc;
 }
 
+static int sched_params_valid(libxl_domain_sched_params *scp)
+{
+    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;
+    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 (has_weight && (has_period || has_slice))
+            return 0;
+
+        if (has_weight) {
+            scp->slice = 0;
+            scp->period = 0;
+        }
+        if (has_period || has_slice)
+            scp->weight = 0;
+    }
+
+    return 1;
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -644,6 +669,10 @@ static void parse_config_data(const char
         b_info->sched_params.latency = l;
     if (!xlu_cfg_get_long (config, "extratime", &l, 0))
         b_info->sched_params.extratime = l;
+    if (!sched_params_valid(&b_info->sched_params)) {
+        fprintf(stderr, "Invalid scheduling parameters\n");
+        exit(1);
+    }
 
     if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
         b_info->max_vcpus = l;

Dario Faggioli writes ("[PATCH 1 of 2 v2] libxl: propagete down the error
from libxl_domain_sched_params_set"):
> So that the caller (e.g., libxl__build_post() ) knows and can deal with it.
...
> -    libxl_domain_sched_params_set(CTX, domid, &info->sched_params);
> +    if (libxl_domain_sched_params_set(CTX, domid,
&info->sched_params))
> +        return ERROR_INVAL;
This is not correct.  It should return whatever error code
libxl_domain_sched_params_set returns, not unconditionally
ERROR_INVAL.

Ian.

On Thu, 2012-06-07 at 17:41 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH 1 of 2 v2] libxl: propagete down the
error from libxl_domain_sched_params_set"):
> > So that the caller (e.g., libxl__build_post() ) knows and can deal
with it.
> ...
> > -    libxl_domain_sched_params_set(CTX, domid,
&info->sched_params);
> > +    if (libxl_domain_sched_params_set(CTX, domid,
&info->sched_params))
> > +        return ERROR_INVAL;
> 
> This is not correct.  It should return whatever error code
> libxl_domain_sched_params_set returns, not unconditionally
> ERROR_INVAL.
> 
Ops, you''re right, sorry... New version coming!

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