Sanity checking of scheduling parameters

PATCH New Dario Faggioli 4 participants 10 messages Jun 8, 2012 – Jun 14, 2012

lore

Share & tools

lore

Dario Faggioli

author

2012-06-08 09:08 UTC #0 lore raw

PATCH v3 1/1 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 v2:
* actually propagate the error correctly instead of always returning INVAL (in patch #1).

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

Signature · 4 lines

<<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)

Dario Faggioli

author

2012-06-08 09:08 UTC #1 lore raw

PATCH v3 1/1 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.

Signed-off-by Dario Faggioli <[email protected]>

tools/libxl/libxl_dom.c +4 -2

@@ -173,9 +173,11 @@ int libxl__build_post(libxl__gc *gc, uin
     char *dom_path, *vm_path;
     xs_transaction_t t;
     char **ents, **hvm_ents;
-    int i;
+    int i, rc;
 
-    libxl_domain_sched_params_set(CTX, domid, &info->sched_params);
+    rc = libxl_domain_sched_params_set(CTX, domid, &info->sched_params);
+    if (rc)
+        return rc;
 
     libxl_cpuid_apply_policy(ctx, domid);
     if (info->cpuid != NULL)

Ian Jackson

Acked-by on v3 1/1

2012-06-08 11:10 UTC #2 lore raw

Re: [PATCH 1 of 2 v3] libxl: propagete down the error from libxl_domain_sched_params_set

Dario Faggioli writes ("[PATCH 1 of 2 v3] libxl: propagete down the error from libxl_domain_sched_params_set"):

Quoted 1 line

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

Acked-by Ian Jackson <[email protected]>

Dario Faggioli

author

2012-06-08 09:08 UTC #3 lore raw

PATCH v3 1/1 xl: check for meaningful combination of sedf config file parameters

In reply to Dario Faggioli

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 <[email protected]>

tools/libxl/xl_cmdimpl.c +29 -0

@@ -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;

Ian Jackson

Acked-by on v3 1/1

2012-06-08 11:10 UTC #4 lore raw

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

Dario Faggioli writes ("[PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):

Quoted 3 lines

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.

Thanks, I am happy with this from a libxl point of view. I would like to see an ack from George from a scheduler point of view.

Acked-by Ian Jackson <[email protected]>

Thanks,
Ian.

George Dunlap

Acked-by

2012-06-08 11:11 UTC #5 lore raw

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

Quoted 6 lines · Ian Jackson

Dario Faggioli writes ("[PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):
Quoted 3 lines
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.
Thanks, I am happy with this from a libxl point of view. I would like to see an ack from George from a scheduler point of view.

I think Dario knows more about what sedf wants than I do; and since it only does anything when the domain is using sedf, I think it's fine.

So FWIW:

Acked-by George Dunlap <[email protected]>

Ian Jackson

2012-06-08 14:26 UTC #6 lore raw

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

George Dunlap writes ("Re: [Xen-devel] [PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):

Quoted 5 lines · Ian Jackson

Quoted 4 lines
Dario Faggioli writes ("[PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):
Quoted 3 lines
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.

Quoted 8 lines

Quoted 2 lines
Thanks, I am happy with this from a libxl point of view. I would like to see an ack from George from a scheduler point of view.
I think Dario knows more about what sedf wants than I do; and since it only does anything when the domain is using sedf, I think it's fine.
So FWIW:
Acked-by George Dunlap <[email protected]>

Great, thanks.

Committed-by Ian Jackson <[email protected]>

Ian.

Ian Campbell

2012-06-12 14:58 UTC #7 lore raw

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

Quoted 11 lines · Ian Jackson

George Dunlap writes ("Re: [Xen-devel] [PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):
Quoted 10 lines · Ian Jackson
Quoted 6 lines
Dario Faggioli writes ("[PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):
Quoted 3 lines
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.
Thanks, I am happy with this from a libxl point of view. I would like to see an ack from George from a scheduler point of view.
I think Dario knows more about what sedf wants than I do; and since it only does anything when the domain is using sedf, I think it's fine.

Quoted 7 lines

Quoted 3 lines
So FWIW:
Acked-by George Dunlap <[email protected]>
Great, thanks.
Committed-by Ian Jackson <[email protected]>

Was it deliberate that you took this one but not the other patch from this series of two, namely "libxl: propagete down the error from libxl_domain_sched_params_set"?

Ian.

Ian Jackson

2012-06-14 15:06 UTC #8 lore raw

Re: [PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters [and 1 more messages]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):

Quoted 3 lines

Was it deliberate that you took this one but not the other patch from this series of two, namely "libxl: propagete down the error from libxl_domain_sched_params_set"?

No, sorry.

Dario Faggioli writes ("[Xen-devel] [PATCH 1 of 2 v3] libxl: propagete down the error from libxl_domain_sched_params_set"):

Quoted 1 line

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

Committed-by Ian Jackson <[email protected]>

Dario Faggioli

author

2012-06-14 15:13 UTC #9 lore raw

Re: [PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters [and 1 more messages]

Quoted 5 lines · Ian Jackson

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2 of 2 v3] xl: check for meaningful combination of sedf config file parameters"):
Quoted 3 lines
Was it deliberate that you took this one but not the other patch from this series of two, namely "libxl: propagete down the error from libxl_domain_sched_params_set"?

Quoted 7 lines

No, sorry.
Dario Faggioli writes ("[Xen-devel] [PATCH 1 of 2 v3] libxl: propagete down the error from libxl_domain_sched_params_set"):
Quoted 1 line
So that the caller (e.g., libxl__build_post() ) knows and can deal with it.
Committed-by Ian Jackson <[email protected]>

Cool. Thanks,
Dario

Signature · 4 lines

<<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
[email protected]
http://lists.xen.org/xen-devel