support of setting scheduler parameters on domain creation

Support a new sysctl schedop sub-command to get the scheduling defaults of a specific scheduler.

Additionally correct parameter checking of the sysctl handling in schedule.c (checked wrong sub-commands: domctl instead of sysctl).

Signed-off-by Juergen Gross <[email protected]>

6 files changed, 69 insertions(+), 22 deletions(-)
xen/common/sched_credit.c   |    5 +++++
xen/common/sched_credit2.c  |   17 +++++++++++++++++
xen/common/sched_sedf.c     |   20 ++++++++++++++++++++
xen/common/schedule.c       |    5 +++--
xen/include/public/domctl.h |   38 ++++++++++++++++++++------------------
xen/include/public/sysctl.h |    6 ++++--

Quoted 2 lines · Juergen Gross

Support a new sysctl schedop sub-command to get the scheduling defaults of a specific scheduler.

Why is XEN_DOMCTL_SCHEDOP_getinfo not sufficient here?

Isn't it actually more useful/meaningful to get the current actual setting rather than a default?

# HG changeset patch
# User Juergen Gross <[email protected]>
# Date 1337677423 -7200
# Node ID 953383741ff44d97587c2e75da79b092523d6e83
# Parent 19aaa30d7fdce2f1b56cb13399d603d955a61fb8 full support of setting scheduler parameters on domain creation

Obtains default scheduler parameters before modifying any settings specified
via domain config.

Corrected an error for setting sedf parameters (setting .period multiple
times).

Signed-off-by Juergen Gross <[email protected]>

diff -r 19aaa30d7fdc -r 953383741ff4 tools/libxl/libxl.h

Ian.

Quoted 3 lines · Ian Campbell

Quoted 2 lines · Juergen Gross

Support a new sysctl schedop sub-command to get the scheduling defaults of a specific scheduler.

Quoted 4 lines

Why is XEN_DOMCTL_SCHEDOP_getinfo not sufficient here?

Isn't it actually more useful/meaningful to get the current actual setting rather than a default?

When setting the parameters from the config file we have no domain yet which we would have to specify for XEN_DOMCTL_SCHEDOP_getinfo. I didn't want to parse part of the config only after domain creation.

A default should be okay, as this is what we want to modify. :-) The scheduler should initialize a new domain with this default, of course.

Quoted 17 lines

# HG changeset patch
# User Juergen Gross<[email protected]>
# Date 1337677423 -7200
# Node ID 953383741ff44d97587c2e75da79b092523d6e83
# Parent 19aaa30d7fdce2f1b56cb13399d603d955a61fb8 full support of setting scheduler parameters on domain creation

Obtains default scheduler parameters before modifying any settings specified
via domain config.

Corrected an error for setting sedf parameters (setting .period multiple
times).

Signed-off-by Juergen Gross<[email protected]>

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 int libxl_primary_console_exec(libxl_ctx · line 610

Quoted 10 lines

diff -r 19aaa30d7fdc -r 953383741ff4 tools/libxl/libxl.h

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 renamed from tools/libxl/libxl.h Tue May 22 10:31:30 2012 +0200 +2 -0

@@ -605,6 +605,8 @@ int libxl_primary_console_exec(libxl_ctx
  /* May be called with info_r == NULL to check for domain's existance
 */
  int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
                        uint32_t domid);
+int libxl_sched_set_defaults(libxl_ctx*, uint32_t poolid,
+                             libxl_sched_params *scparams);

This interface really makes libxl_sched_params differ from all the other libxl structs (which have a public _init function and an internal setdefaults function). I'm not really sure its justified either, I was under the impression that you'd found that there were useful discriminating values?

If this function was called libxl_sched_init (replacing the autogenerated one) then it might be ok. Although I'm still not really sure what the issue is with having a discriminating value meaning default is, doing that keeps the _init function cheap too.

Quoted 11 lines

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 renamed from tools/libxl/libxl.h Tue May 22 10:31:30 2012 +0200 +0 -0

  int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
 libxl_sched_params *scparams)

  {
      libxl_ctx *ctx = libxl__gc_owner(gc);
-    libxl_scheduler sched;
      libxl_sched_sedf_domain sedf_info;
      libxl_sched_credit_domain credit_info;
      libxl_sched_credit2_domain credit2_info;
      int ret;

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200

Quoted 3 lines

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 renamed from tools/libxl/libxl.h Tue May 22 10:31:30 2012 +0200 +1 -2

-    sched = libxl_get_scheduler (ctx);
-    switch (sched) {
+    switch (scparams->sched) {

What happens if scparams->sched is not the scheduler used for this domain? Should it either be checked or set somewhere?

Quoted 17 lines · Ian Campbell

Quoted 17 lines

# HG changeset patch
# User Juergen Gross<[email protected]>
# Date 1337677423 -7200
# Node ID 953383741ff44d97587c2e75da79b092523d6e83
# Parent 19aaa30d7fdce2f1b56cb13399d603d955a61fb8 full support of setting scheduler parameters on domain creation

Obtains default scheduler parameters before modifying any settings specified
via domain config.

Corrected an error for setting sedf parameters (setting .period multiple
times).

Signed-off-by Juergen Gross<[email protected]>

Quoted 15 lines

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 int libxl_primary_console_exec(libxl_ctx · line 610

Quoted 10 lines

diff -r 19aaa30d7fdc -r 953383741ff4 tools/libxl/libxl.h

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 renamed from tools/libxl/libxl.h Tue May 22 10:31:30 2012 +0200 +2 -0

@@ -605,6 +605,8 @@ int libxl_primary_console_exec(libxl_ctx
  /* May be called with info_r == NULL to check for domain's existance
 */
  int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
                        uint32_t domid);
+int libxl_sched_set_defaults(libxl_ctx*, uint32_t poolid,
+                             libxl_sched_params *scparams);

This interface really makes libxl_sched_params differ from all the other libxl structs (which have a public _init function and an internal setdefaults function). I'm not really sure its justified either, I was under the impression that you'd found that there were useful discriminating values?

Dario opted for this solution, so I proposed a patch implementing it. I prefer this solution, too, as it isn't exporting scheduler internals to the tools.

Quoted 5 lines

If this function was called libxl_sched_init (replacing the autogenerated one) then it might be ok. Although I'm still not really sure what the issue is with having a discriminating value meaning default is, doing that keeps the _init function cheap too.

Okay, I can rename it.

Quoted 11 lines

Quoted 11 lines

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 renamed from tools/libxl/libxl.h Tue May 22 10:31:30 2012 +0200 +0 -0

  int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
 libxl_sched_params *scparams)

  {
      libxl_ctx *ctx = libxl__gc_owner(gc);
-    libxl_scheduler sched;
      libxl_sched_sedf_domain sedf_info;
      libxl_sched_credit_domain credit_info;
      libxl_sched_credit2_domain credit2_info;
      int ret;

Quoted 5 lines

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200

Quoted 3 lines

tools/libxl/libxl.h Tue May 22 11:03:43 2012 +0200 renamed from tools/libxl/libxl.h Tue May 22 10:31:30 2012 +0200 +1 -2

-    sched = libxl_get_scheduler (ctx);
-    switch (sched) {
+    switch (scparams->sched) {

What happens if scparams->sched is not the scheduler used for this domain? Should it either be checked or set somewhere?

The check would be the same as the original setting of scparams->sched. Setting of the scheduler parameters will be rejected by the hypervisor if the scheduler does not match.

Juergen

Quoted 6 lines · Juergen Gross

Quoted 5 lines · Ian Campbell

Quoted 2 lines · Juergen Gross

Support a new sysctl schedop sub-command to get the scheduling defaults of a specific scheduler.

Why is XEN_DOMCTL_SCHEDOP_getinfo not sufficient here?

Quoted 5 lines

Quoted 2 lines

Isn't it actually more useful/meaningful to get the current actual setting rather than a default?

When setting the parameters from the config file we have no domain yet which we would have to specify for XEN_DOMCTL_SCHEDOP_getinfo. I didn't want to parse part of the config only after domain creation.

That seems like another problem with doing this up front instead of doing read-modify-write when we come to set the values for the domain.

Quoted 2 lines

A default should be okay, as this is what we want to modify. :-) The scheduler should initialize a new domain with this default, of course.

So I can't use this interface to change the current one of the current settings for running a domain, while leaving the others at their current value?

Which interface can I use for that and why isn't it the same as this one?

Quoted 7 lines

Quoted 4 lines

When setting the parameters from the config file we have no domain yet which
we would have to specify for XEN_DOMCTL_SCHEDOP_getinfo. I didn't want to parse part of the config only after domain creation.

That seems like another problem with doing this up front instead of doing read-modify-write when we come to set the values for the domain.

Do you think it would be okay to parse the scheduler config data _after_ domain creation? If yes, the read-modify-write approach is simple and always correct. If no, you will have to initialize the parameters to something scheduler specific, and not domain specific.

Quoted 4 lines

Quoted 3 lines

A default should be okay, as this is what we want to modify. :-) The scheduler should initialize a new domain with this default, of course.

Quoted 3 lines

So I can't use this interface to change the current one of the current settings for running a domain, while leaving the others at their current value?

I don't understand this sentence :-) ...

Quoted 4 lines

Which interface can I use for that and why isn't it the same as this one?

... making it hard to answer to this one. :-)
Nevertheless trying: you can change the scheduling parameters of a running domain with xl sched-credit/sched-sedf/...

Quoted 5 lines · Juergen Gross

Quoted 5 lines

Quoted 4 lines

When setting the parameters from the config file we have no domain yet which
we would have to specify for XEN_DOMCTL_SCHEDOP_getinfo. I didn't want to parse part of the config only after domain creation.

Quoted 6 lines

Quoted 2 lines

That seems like another problem with doing this up front instead of doing read-modify-write when we come to set the values for the domain.

Do you think it would be okay to parse the scheduler config data _after_ domain creation?

If yes, the read-modify-write approach is simple and always

Quoted 13 lines

correct. If no, you will have to initialize the parameters to something scheduler specific, and not domain specific.

Quoted 7 lines

Quoted 3 lines

A default should be okay, as this is what we want to modify. :-) The scheduler should initialize a new domain with this default, of course.

So I can't use this interface to change the current one of the current settings for running a domain, while leaving the others at their current value?

Quoted 1 line

I don't understand this sentence :-) ...

I think he means this: Suppose after the domain is created, you want to set the weight. With the proposed interface, you have to manually read the values and change them yourself. With an interface that allows -1 to be default, libxl can do that for you (making programming easier).

Quoted 10 lines

Quoted 5 lines

Which interface can I use for that and why isn't it the same as this one?

... making it hard to answer to this one. :-)
Nevertheless trying: you can change the scheduling parameters of a running domain with xl sched-credit/sched-sedf/...

I think Ian has convinced me that just using -1 for default values would be the best option:
* It doesn't require a new special 'what are the defaults" hypercall
* The same interface can be used to set only a subset of the options (leaving the other ones alone) *after* the domain is created.

Thanks for doing the work to code it up, though Juergen.

-George

I think Ian has convinced me that just using -1 for default values would be the best option:

Quoted 3 lines · George Dunlap

I think Ian has convinced me that just using -1 for default values would be the best option:

Perhaps I should code up what I'm talking about for comparison.

Quoted 5 lines

This interface really makes libxl_sched_params differ from all the other libxl structs (which have a public _init function and an internal setdefaults function). I'm not really sure its justified either, I was under the impression that you'd found that there were useful discriminating values?

Dario opted for this solution, so I proposed a patch implementing it. I prefer this solution, too, as it isn't exporting scheduler internals to the tools.

Quoted 13 lines

Quoted 13 lines

int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
libxl_sched_params *scparams)

  {
      libxl_ctx *ctx = libxl__gc_owner(gc);

tools/libxl/libxl_dom.c +1 -3

-    libxl_scheduler sched;
      libxl_sched_sedf_domain sedf_info;
      libxl_sched_credit_domain credit_info;
      libxl_sched_credit2_domain credit2_info;
      int ret;
 
-    sched = libxl_get_scheduler (ctx);
-    switch (sched) {
+    switch (scparams->sched) {

Quoted 2 lines

What happens if scparams->sched is not the scheduler used for this domain? Should it either be checked or set somewhere?

The check would be the same as the original setting of scparams->sched. Setting of the scheduler parameters will be rejected by the hypervisor if the scheduler does not match.

Like the below. Lightly tested with the credit scheduler.

I think CAP is the only one for which 0 is a real valid value, but I'm not sure (especially with the SEDF ones which didn't have existing limits checks in the set function I could crib from...).

I'm pretty sure that libxl__sched_set_params needs to get the correct scheduler for the particular domain, but I've no idea how to get that...

8<---------------------------

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1337698727 -3600
# Node ID 355030f95eb313605a0e43aa7328e731b28a28b3
# Parent 426bbf58cea4559464b6e5d3ff0f65324a5f5926
libxl: make it possible to explicitly specify default sched params

To do so we define a descriminating value which is never a valid real value for each parameter.

While there:

- remove tslice_ms and ratelimit_us from libxl_sched_params and from the xl domain config parser. These are global scheduler properties, not per-domain ones (and were never read in any case).
- removed libxl_sched_*_domain in favour of libxl_sched_params.
- rename libxl_sched_params to libxl_sched_domain_params for clarity.
- use this new functionality for the various xl commands which set sched parameters, which saves an explicit read-modify-write in xl.
- removed call of xc_domain_getinfolist from a few functions which weren't actually using the result (looks like a cut and paste error)
- fix xl which was setting period for a variety of different config keys.

Signed-off-by Ian Campbell <[email protected]>

diff -r 426bbf58cea4 -r 355030f95eb3 tools/libxl/libxl.c

tools/libxl/libxl.c +0 -0

@@ -3168,19 +3168,19 @@ libxl_scheduler libxl_get_scheduler(libx
 }
 

int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,

tools/libxl/libxl.c +21 -15

-                                libxl_sched_sedf_domain *scinfo)
+                                libxl_sched_domain_params *scinfo)
 {
-    xc_domaininfo_t domaininfo;
-    int rc;
-
-    rc = xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo);
-    if (rc < 0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
+    uint64_t period;
+    uint64_t slice;
+    uint64_t latency;
+    uint16_t extratime;
+    uint16_t weight;
+
+    int ret;
+
+    ret = xc_sedf_domain_get(ctx->xch, domid, &period, &slice, &latency,
+                            &extratime, &weight);
+    if (ret != 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched sedf");
         return ERROR_FAIL;
     }
-    if (rc != 1 || domaininfo.domain != domid)
-        return ERROR_INVAL;
-
-
-    rc = xc_sedf_domain_set(ctx->xch, domid, scinfo->period * 1000000,
-                            scinfo->slice * 1000000, scinfo->latency * 1000000,
-                            scinfo->extratime, scinfo->weight);
-    if ( rc < 0 ) {
+
+    if (scinfo->period != LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT)
+        period = scinfo->period * 1000000;
+    if (scinfo->slice != LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT)
+        period = scinfo->slice * 1000000;
+    if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
+        period = scinfo->latency * 1000000;
+    if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)

Quoted 6 lines · Ian Campbell

Like the below. Lightly tested with the credit scheduler.

I think CAP is the only one for which 0 is a real valid value, but I'm not sure (especially with the SEDF ones which didn't have existing limits checks in the set function I could crib from...).

Yep, that's because they're mostly time values. xen/common/sched_sedf.c hosts some ranges for some of them, but I'm not sure we want to propagate those:

#define PERIOD_MAX MILLISECS(10000) /* 10s */
#define PERIOD_MIN (MICROSECS(10)) /* 10us */
#define SLICE_MIN (MICROSECS(5)) /* 5us */

Also, extratime is a flag, so I think 0 and 1 are both meaningful values, maybe we can go for -1 as for cap (I'll try and let you know).

Quoted 3 lines

I'm pretty sure that libxl__sched_set_params needs to get the correct scheduler for the particular domain, but I've no idea how to get that...

Again, I was thinking something like what Juergen did here could help (go getting the scheduler of the cpupool the domain belongs to)... O am I misunderstanding the issue?

(quoted diff) +3 -0

+    poolinfo = libxl_list_cpupool(ctx,&n_pools);
+    if (!poolinfo)
+        return ERROR_NOMEM;

Quoted 33 lines

8<---------------------------

# HG changeset patch
# User Ian Campbell<[email protected]>
# Date 1337698727 -3600
# Node ID 355030f95eb313605a0e43aa7328e731b28a28b3
# Parent 426bbf58cea4559464b6e5d3ff0f65324a5f5926
libxl: make it possible to explicitly specify default sched params

To do so we define a descriminating value which is never a valid real value for each parameter.

While there:

- remove tslice_ms and ratelimit_us from libxl_sched_params and from the xl domain config parser. These are global scheduler properties, not per-domain ones (and were never read in any case).
- removed libxl_sched_*_domain in favour of libxl_sched_params.
- rename libxl_sched_params to libxl_sched_domain_params for clarity.
- use this new functionality for the various xl commands which set sched parameters, which saves an explicit read-modify-write in xl.
- removed call of xc_domain_getinfolist from a few functions which weren't actually using the result (looks like a cut and paste error)
- fix xl which was setting period for a variety of different config keys.

Signed-off-by Ian Campbell<[email protected]>

diff -r 426bbf58cea4 -r 355030f95eb3 tools/libxl/libxl.c

tools/libxl/libxl.c +0 -0

@@ -3168,19 +3168,19 @@ libxl_scheduler libxl_get_scheduler(libx
  }
 

<snip>

Quoted 42 lines

int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,

tools/libxl/libxl.c +21 -15

-                                libxl_sched_sedf_domain *scinfo)
+                                libxl_sched_domain_params *scinfo)
  {
-    xc_domaininfo_t domaininfo;
-    int rc;
-
-    rc = xc_domain_getinfolist(ctx->xch, domid, 1,&domaininfo);
-    if (rc<  0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
+    uint64_t period;
+    uint64_t slice;
+    uint64_t latency;
+    uint16_t extratime;
+    uint16_t weight;
+
+    int ret;
+
+    ret = xc_sedf_domain_get(ctx->xch, domid,&period,&slice,&latency,
+&extratime,&weight);
+    if (ret != 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched sedf");
          return ERROR_FAIL;
      }
-    if (rc != 1 || domaininfo.domain != domid)
-        return ERROR_INVAL;
-
-
-    rc = xc_sedf_domain_set(ctx->xch, domid, scinfo->period * 1000000,
-                            scinfo->slice * 1000000, scinfo->latency * 1000000,
-                            scinfo->extratime, scinfo->weight);
-    if ( rc<  0 ) {
+
+    if (scinfo->period != LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT)
+        period = scinfo->period * 1000000;
+    if (scinfo->slice != LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT)
+        period = scinfo->slice * 1000000;
+    if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
+        period = scinfo->latency * 1000000;
+    if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)

tools/libxl/libxl.c int libxl_sched_sedf_domain_get(libxl_ct · line 3419

Quoted 10 lines

tools/libxl/libxl.c +7 -0

+        period = scinfo->extratime;
+    if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT)
+        period = scinfo->weight;
+
+    ret = xc_sedf_domain_set(ctx->xch, domid, period, slice, latency,
+                            extratime, weight);
+    if ( ret<  0 ) {
          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "setting domain sched sedf");
          return ERROR_FAIL;
      }

# xl create vm1.cfg
Parsing config from vm1.cfg
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument

And I'm getting the above independently on what I put in the config file (valid params, no params at all, etc.). I also can't change the scheduling parameters of a domain on-line anymore:

# xl sched-sedf -d 3 -p 100 -s 50
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
libxl_sched_sedf_domain_set failed.

I'll try digging a bit more into this ASAP.

On more thing, are we ok with the _set command failing and he domain being created anyway? Or perhaps the whole process should just abort?

Quoted 3 lines

#define PERIOD_MAX MILLISECS(10000) /* 10s */
#define PERIOD_MIN (MICROSECS(10)) /* 10us */
#define SLICE_MIN (MICROSECS(5)) /* 5us */

I think this should remain in the hypervisor only.

Quoted 2 lines

Also, extratime is a flag, so I think 0 and 1 are both meaningful values, maybe we can go for -1 as for cap (I'll try and let you know).

Why not -1 for all values?

Quoted 7 lines

Quoted 3 lines

I'm pretty sure that libxl__sched_set_params needs to get the correct scheduler for the particular domain, but I've no idea how to get that...

Again, I was thinking something like what Juergen did here could help (go getting the scheduler of the cpupool the domain belongs to)... O am I misunderstanding the issue?

Review

Quoted 13 lines

(quoted diff) +13 -0

+    poolinfo = libxl_list_cpupool(ctx,&n_pools);
+    if (!poolinfo)
+        return ERROR_NOMEM;
+
+    ret = ERROR_INVAL;
+    for (p = 0; p<  n_pools; p++) {
+        if (poolinfo[p].poolid == poolid) {
+            scparams->sched = poolinfo[p].sched;
+            ret = 0;
+        }
+        libxl_cpupoolinfo_dispose(poolinfo + p);
+    }
+

This was exactly the purpose of the sniplet.

Quoted 8 lines

# xl create vm1.cfg
Parsing config from vm1.cfg
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument

And I'm getting the above independently on what I put in the config file (valid params, no params at all, etc.). I also can't change the scheduling parameters of a domain on-line anymore:

Quoted 5 lines

# xl sched-sedf -d 3 -p 100 -s 50
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
libxl_sched_sedf_domain_set failed.

I'll try digging a bit more into this ASAP.

It's easy: Ian repeated an error he (and I) corrected elsewhere:
He's always setting period, regardless of the changed parameter...

Quoted 6 lines · Juergen Gross

Quoted 3 lines

#define PERIOD_MAX MILLISECS(10000) /* 10s */
#define PERIOD_MIN (MICROSECS(10)) /* 10us */
#define SLICE_MIN (MICROSECS(5)) /* 5us */

I think this should remain in the hypervisor only.

Me too.

Quoted 5 lines

Quoted 2 lines

Also, extratime is a flag, so I think 0 and 1 are both meaningful values, maybe we can go for -1 as for cap (I'll try and let you know).

Why not -1 for all values?

Would work, I guess, unless there's some collision with big weights represented on short unsigned value (not sure it's like that, and I've always been bad at this kind of math! :-P).

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I'm pretty sure that libxl__sched_set_params needs to get the correct scheduler for the particular domain, but I've no idea how to get that...

Again, I was thinking something like what Juergen did here could help (go getting the scheduler of the cpupool the domain belongs to)... O am I misunderstanding the issue?

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+    poolinfo = libxl_list_cpupool(ctx,&n_pools);
+    if (!poolinfo)
+        return ERROR_NOMEM;
+
+    ret = ERROR_INVAL;
+    for (p = 0; p<  n_pools; p++) {
+        if (poolinfo[p].poolid == poolid) {
+            scparams->sched = poolinfo[p].sched;
+            ret = 0;
+        }
+        libxl_cpupoolinfo_dispose(poolinfo + p);
+    }
+

This was exactly the purpose of the sniplet.

Good to hear that. :-)

Quoted 12 lines

Quoted 12 lines

# xl create vm1.cfg
Parsing config from vm1.cfg
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument

And I'm getting the above independently on what I put in the config file (valid params, no params at all, etc.). I also can't change the scheduling parameters of a domain on-line anymore:

# xl sched-sedf -d 3 -p 100 -s 50
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
libxl_sched_sedf_domain_set failed.

Quoted 5 lines

Quoted 1 line

I'll try digging a bit more into this ASAP.

It's easy: Ian repeated an error he (and I) corrected elsewhere:
He's always setting period, regardless of the changed parameter...

Not sure, I think Ian's patch fixes that:

tools/libxl/xl_cmdimpl.c Tue May 22 15:58:47 2012 +0100 renamed from tools/libxl/xl_cmdimpl.c Tue May 22 14:19:07 2012 +0100 +5 -8

@@ -627,23 +627,20 @@ static void parse_config_data(const char
 
     libxl_domain_build_info_init_type(b_info, c_info->type);
 
-    /* the following is the actual config parsing with overriding values in the structures */
+    /* the following is the actual config parsing with overriding
+     * values in the structures */
     if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
         b_info->sched_params.weight = l;
     if (!xlu_cfg_get_long (config, "cap", &l, 0))
         b_info->sched_params.cap = l;
-    if (!xlu_cfg_get_long (config, "tslice_ms", &l, 0))
-        b_info->sched_params.tslice_ms = l;
-    if (!xlu_cfg_get_long (config, "ratelimit_us", &l, 0))
-        b_info->sched_params.ratelimit_us = l;
     if (!xlu_cfg_get_long (config, "period", &l, 0))
         b_info->sched_params.period = l;
     if (!xlu_cfg_get_long (config, "slice", &l, 0))
-        b_info->sched_params.period = l;
+        b_info->sched_params.slice = l;
     if (!xlu_cfg_get_long (config, "latency", &l, 0))
-        b_info->sched_params.period = l;
+        b_info->sched_params.latency = l;
     if (!xlu_cfg_get_long (config, "extratime", &l, 0))
-        b_info->sched_params.period = l;
+        b_info->sched_params.extratime = l;
 
 

So it has to be something else...

Thanks and Regards,
Dario

# xl create vm1.cfg
Parsing config from vm1.cfg
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument

And I'm getting the above independently on what I put in the config file (valid params, no params at all, etc.). I also can't change the scheduling parameters of a domain on-line anymore:

# xl sched-sedf -d 3 -p 100 -s 50
libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
libxl_sched_sedf_domain_set failed.

I'll try digging a bit more into this ASAP.

Quoted 4 lines

Quoted 3 lines

It's easy: Ian repeated an error he (and I) corrected elsewhere:
He's always setting period, regardless of the changed parameter...

Not sure, I think Ian's patch fixes that:

I thought so to!

Quoted 30 lines

tools/libxl/xl_cmdimpl.c Tue May 22 15:58:47 2012 +0100 renamed from tools/libxl/xl_cmdimpl.c Tue May 22 14:19:07 2012 +0100 +5 -8

@@ -627,23 +627,20 @@ static void parse_config_data(const char
 
      libxl_domain_build_info_init_type(b_info, c_info->type);
 
-    /* the following is the actual config parsing with overriding values in the structures */
+    /* the following is the actual config parsing with overriding
+     * values in the structures */
      if (!xlu_cfg_get_long (config, "cpu_weight",&l, 0))
          b_info->sched_params.weight = l;
      if (!xlu_cfg_get_long (config, "cap",&l, 0))
          b_info->sched_params.cap = l;
-    if (!xlu_cfg_get_long (config, "tslice_ms",&l, 0))
-        b_info->sched_params.tslice_ms = l;
-    if (!xlu_cfg_get_long (config, "ratelimit_us",&l, 0))
-        b_info->sched_params.ratelimit_us = l;
      if (!xlu_cfg_get_long (config, "period",&l, 0))
          b_info->sched_params.period = l;
      if (!xlu_cfg_get_long (config, "slice",&l, 0))
-        b_info->sched_params.period = l;
+        b_info->sched_params.slice = l;
      if (!xlu_cfg_get_long (config, "latency",&l, 0))
-        b_info->sched_params.period = l;
+        b_info->sched_params.latency = l;
      if (!xlu_cfg_get_long (config, "extratime",&l, 0))
-        b_info->sched_params.period = l;
+        b_info->sched_params.extratime = l;
 
 

Quoted 1 line

So it has to be something else...

Quoted 5 lines · Juergen Gross

Quoted 3 lines

#define PERIOD_MAX MILLISECS(10000) /* 10s */
#define PERIOD_MIN (MICROSECS(10)) /* 10us */
#define SLICE_MIN (MICROSECS(5)) /* 5us */

I think this should remain in the hypervisor only.

Me too.

Quoted 4 lines

Quoted 2 lines

Also, extratime is a flag, so I think 0 and 1 are both meaningful values, maybe we can go for -1 as for cap (I'll try and let you know).

Why not -1 for all values?

Would work, I guess, unless there's some collision with big weights represented on short unsigned value (not sure it's like that, and I've always been bad at this kind of math! :-P).

Oh yes:

(quoted diff) +11 -0

+
+    if (scinfo->period != LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT)
+        period = scinfo->period * 1000000;
+    if (scinfo->slice != LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT)
+        period = scinfo->slice * 1000000;
+    if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
+        period = scinfo->latency * 1000000;
+    if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)
+        period = scinfo->extratime;
+    if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT)
+        period = scinfo->weight;

Regardless which parameter was changed, it is always 'period' which is set to the changed value.

Quoted 2 lines

Quoted 1 line

Not sure, I think Ian's patch fixes that:

I thought so to!

It fixes the original error.

Quoted 1 line

Quoted 1 line

So it has to be something else...

Oh yes:

(quoted diff) +11 -0

+
+    if (scinfo->period != LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT)
+        period = scinfo->period * 1000000;
+    if (scinfo->slice != LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT)
+        period = scinfo->slice * 1000000;
+    if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
+        period = scinfo->latency * 1000000;
+    if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)
+        period = scinfo->extratime;
+    if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT)
+        period = scinfo->weight;

Regardless which parameter was changed, it is always 'period' which is set to the changed value.