* [PATCH 0 of 3] libxl: make it possible to explicitly specify default sched params
@ 2012-05-23 9:26 Ian Campbell
2012-05-23 9:26 ` [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler Ian Campbell
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Ian Campbell @ 2012-05-23 9:26 UTC (permalink / raw)
To: xen-devel; +Cc: Dario.Faggioli, Ian.Jackson, Juergen Gross, George.Dunlap
This series defines a descriminating value for each scheduler paramter
and uses it to fix a warning when starting a guest:
"Cpu weight out of range, valid values are within range from 1 to 65535"
It also cleans up a few things and adds some convience interfaces for
cpupools.
I'm slightly reticent about this change during the freeze, but since
it fixes a warning which needs to be fixed for release and makes
(useful, I think) changes to the libxl API I think it is ok.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler
2012-05-23 9:26 [PATCH 0 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
@ 2012-05-23 9:26 ` Ian Campbell
2012-05-23 19:47 ` George Dunlap
2012-05-23 9:26 ` [PATCH 2 of 3] libxl: rename libxl_sched_params to libxl_sched_domain_params Ian Campbell
2012-05-23 9:26 ` [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2012-05-23 9:26 UTC (permalink / raw)
To: xen-devel; +Cc: Dario.Faggioli, Ian.Jackson, Juergen Gross, George.Dunlap
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1337764129 -3600
# Node ID 6533501734be011cb1f7669b7840bfdf5b2eb801
# Parent 3c9221c5b82e53d010452d1208a9f9acaf8981cd
libxl: add internal function to get a domain's scheduler.
This takes into account cpupools.
Add a helper to get the info for a single cpu pool, refactor libxl_list_cpupool
t use this. While there fix the leaks due to not disposing the partial list on
realloc failure in that function.
Fix the failure of sched_domain_output to free the poolinfo list.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 3c9221c5b82e -r 6533501734be tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Tue May 22 16:15:06 2012 +0100
+++ b/tools/libxl/libxl.c Wed May 23 10:08:49 2012 +0100
@@ -552,41 +552,72 @@ int libxl_domain_info(libxl_ctx *ctx, li
return 0;
}
+static int cpupool_info(libxl__gc *gc,
+ libxl_cpupoolinfo *info,
+ uint32_t poolid,
+ bool exact /* exactly poolid or >= poolid */)
+{
+ xc_cpupoolinfo_t *xcinfo;
+ int rc = ERROR_FAIL;
+
+ xcinfo = xc_cpupool_getinfo(CTX->xch, poolid);
+ if (xcinfo == NULL)
+ return ERROR_FAIL;
+
+ if (exact && xcinfo->cpupool_id != poolid)
+ goto out;
+
+ info->poolid = xcinfo->cpupool_id;
+ info->sched = xcinfo->sched_id;
+ info->n_dom = xcinfo->n_dom;
+ if (libxl_cpumap_alloc(CTX, &info->cpumap))
+ goto out;
+ memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
+
+ rc = 0;
+out:
+ xc_cpupool_infofree(CTX->xch, xcinfo);
+ return rc;
+}
+
+int libxl_cpupool_info(libxl_ctx *ctx,
+ libxl_cpupoolinfo *info, uint32_t poolid)
+{
+ GC_INIT(ctx);
+ int rc = cpupool_info(gc, info, poolid, true);
+ GC_FREE;
+ return rc;
+}
+
libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool)
{
- libxl_cpupoolinfo *ptr, *tmp;
+ GC_INIT(ctx);
+ libxl_cpupoolinfo info, *ptr, *tmp;
int i;
- xc_cpupoolinfo_t *info;
uint32_t poolid;
ptr = NULL;
poolid = 0;
for (i = 0;; i++) {
- info = xc_cpupool_getinfo(ctx->xch, poolid);
- if (info == NULL)
+ if (cpupool_info(gc, &info, poolid, false))
break;
tmp = realloc(ptr, (i + 1) * sizeof(libxl_cpupoolinfo));
if (!tmp) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
+ while(--i>0)
+ libxl_cpupoolinfo_dispose(ptr+i);
free(ptr);
- xc_cpupool_infofree(ctx->xch, info);
- return NULL;
+ goto out;
}
ptr = tmp;
- ptr[i].poolid = info->cpupool_id;
- ptr[i].sched = info->sched_id;
- ptr[i].n_dom = info->n_dom;
- if (libxl_cpumap_alloc(ctx, &ptr[i].cpumap)) {
- xc_cpupool_infofree(ctx->xch, info);
- break;
- }
- memcpy(ptr[i].cpumap.map, info->cpumap, ptr[i].cpumap.size);
- poolid = info->cpupool_id + 1;
- xc_cpupool_infofree(ctx->xch, info);
+ ptr[i] = info;
+ poolid = info.poolid + 1;
}
*nb_pool = i;
+out:
+ GC_FREE;
return ptr;
}
diff -r 3c9221c5b82e -r 6533501734be tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Tue May 22 16:15:06 2012 +0100
+++ b/tools/libxl/libxl.h Wed May 23 10:08:49 2012 +0100
@@ -860,6 +860,7 @@ int libxl_cpupool_cpuadd_node(libxl_ctx
int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
+int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);
int libxl_domid_valid_guest(uint32_t domid);
diff -r 3c9221c5b82e -r 6533501734be tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Tue May 22 16:15:06 2012 +0100
+++ b/tools/libxl/libxl_dom.c Wed May 23 10:08:49 2012 +0100
@@ -93,6 +93,41 @@ int libxl__domain_shutdown_reason(libxl_
return (info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask;
}
+int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid)
+{
+ xc_domaininfo_t info;
+ int ret;
+
+ ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info);
+ if (ret != 1)
+ return ERROR_FAIL;
+ if (info.domain != domid)
+ return ERROR_FAIL;
+
+ return info.cpupool;
+}
+
+libxl_scheduler libxl__domain_scheduler(libxl__gc *gc, uint32_t domid)
+{
+ uint32_t cpupool = libxl__domain_cpupool(gc, domid);
+ libxl_cpupoolinfo poolinfo;
+ libxl_scheduler sched = LIBXL_SCHEDULER_UNKNOWN;
+ int rc;
+
+ if (cpupool < 0)
+ return sched;
+
+ rc = libxl_cpupool_info(CTX, &poolinfo, cpupool);
+ if (rc < 0)
+ goto out;
+
+ sched = poolinfo.sched;
+
+out:
+ libxl_cpupoolinfo_dispose(&poolinfo);
+ return sched;
+}
+
int libxl__build_pre(libxl__gc *gc, uint32_t domid,
libxl_domain_build_info *info, libxl__domain_build_state *state)
{
diff -r 3c9221c5b82e -r 6533501734be tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Tue May 22 16:15:06 2012 +0100
+++ b/tools/libxl/libxl_internal.h Wed May 23 10:08:49 2012 +0100
@@ -716,6 +716,8 @@ _hidden int libxl__atfork_init(libxl_ctx
/* from xl_dom */
_hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
_hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
+_hidden int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid);
+_hidden libxl_scheduler libxl__domain_scheduler(libxl__gc *gc, uint32_t domid);
_hidden int libxl__sched_set_params(libxl__gc *gc, uint32_t domid, libxl_sched_params *scparams);
#define LIBXL__DOMAIN_IS_TYPE(gc, domid, type) \
libxl__domain_type((gc), (domid)) == LIBXL_DOMAIN_TYPE_##type
diff -r 3c9221c5b82e -r 6533501734be tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl Tue May 22 16:15:06 2012 +0100
+++ b/tools/libxl/libxl_types.idl Wed May 23 10:08:49 2012 +0100
@@ -109,7 +109,9 @@ libxl_bios_type = Enumeration("bios_type
])
# Consistent with values defined in domctl.h
+# Except unknown which we have made up
libxl_scheduler = Enumeration("scheduler", [
+ (0, "unknown"),
(4, "sedf"),
(5, "credit"),
(6, "credit2"),
diff -r 3c9221c5b82e -r 6533501734be tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Tue May 22 16:15:06 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c Wed May 23 10:08:49 2012 +0100
@@ -4603,6 +4603,7 @@ static int sched_domain_output(libxl_sch
for (p = 0; p < n_pools; p++) {
libxl_cpupoolinfo_dispose(poolinfo + p);
}
+ free(poolinfo);
}
return 0;
}
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2 of 3] libxl: rename libxl_sched_params to libxl_sched_domain_params
2012-05-23 9:26 [PATCH 0 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2012-05-23 9:26 ` [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler Ian Campbell
@ 2012-05-23 9:26 ` Ian Campbell
2012-05-23 19:49 ` George Dunlap
2012-05-23 9:26 ` [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2012-05-23 9:26 UTC (permalink / raw)
To: xen-devel; +Cc: Dario.Faggioli, Ian.Jackson, Juergen Gross, George.Dunlap
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1337764319 -3600
# Node ID 7d8428388b775a0b26cf88f89ec8f99f5fc8ce25
# Parent 6533501734be011cb1f7669b7840bfdf5b2eb801
libxl: rename libxl_sched_params to libxl_sched_domain_params
Remove credit scheduler global options from the struct, they were never used
anyway.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 6533501734be -r 7d8428388b77 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Wed May 23 10:08:49 2012 +0100
+++ b/tools/libxl/libxl_dom.c Wed May 23 10:11:59 2012 +0100
@@ -42,7 +42,8 @@ libxl_domain_type libxl__domain_type(lib
return LIBXL_DOMAIN_TYPE_PV;
}
-int libxl__sched_set_params(libxl__gc *gc, uint32_t domid, libxl_sched_params *scparams)
+int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
+ libxl_sched_domain_params *scparams)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
libxl_scheduler sched;
diff -r 6533501734be -r 7d8428388b77 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Wed May 23 10:08:49 2012 +0100
+++ b/tools/libxl/libxl_internal.h Wed May 23 10:11:59 2012 +0100
@@ -718,7 +718,8 @@ _hidden libxl_domain_type libxl__domain_
_hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
_hidden int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid);
_hidden libxl_scheduler libxl__domain_scheduler(libxl__gc *gc, uint32_t domid);
-_hidden int libxl__sched_set_params(libxl__gc *gc, uint32_t domid, libxl_sched_params *scparams);
+_hidden int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
+ libxl_sched_domain_params *scparams);
#define LIBXL__DOMAIN_IS_TYPE(gc, domid, type) \
libxl__domain_type((gc), (domid)) == LIBXL_DOMAIN_TYPE_##type
typedef struct {
diff -r 6533501734be -r 7d8428388b77 tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl Wed May 23 10:08:49 2012 +0100
+++ b/tools/libxl/libxl_types.idl Wed May 23 10:11:59 2012 +0100
@@ -226,11 +226,9 @@ libxl_domain_create_info = Struct("domai
MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
-libxl_sched_params = Struct("sched_params",[
+libxl_sched_domain_params = Struct("sched_domain_params",[
("weight", integer),
("cap", integer),
- ("tslice_ms", integer),
- ("ratelimit_us", integer),
("period", integer),
("slice", integer),
("latency", integer),
@@ -269,7 +267,7 @@ libxl_domain_build_info = Struct("domain
# extra parameters pass directly to qemu for HVM guest, NULL terminated
("extra_hvm", libxl_string_list),
# parameters for all type of scheduler
- ("sched_params", libxl_sched_params),
+ ("sched_params", libxl_sched_domain_params),
("u", KeyedUnion(None, libxl_domain_type, "type",
[("hvm", Struct(None, [("firmware", string),
diff -r 6533501734be -r 7d8428388b77 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed May 23 10:08:49 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c Wed May 23 10:11:59 2012 +0100
@@ -632,10 +632,6 @@ static void parse_config_data(const char
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))
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 9:26 [PATCH 0 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2012-05-23 9:26 ` [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler Ian Campbell
2012-05-23 9:26 ` [PATCH 2 of 3] libxl: rename libxl_sched_params to libxl_sched_domain_params Ian Campbell
@ 2012-05-23 9:26 ` Ian Campbell
2012-05-23 10:51 ` Ian Jackson
` (2 more replies)
2 siblings, 3 replies; 19+ messages in thread
From: Ian Campbell @ 2012-05-23 9:26 UTC (permalink / raw)
To: xen-devel; +Cc: Dario.Faggioli, Ian.Jackson, Juergen Gross, George.Dunlap
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1337764865 -3600
# Node ID b6221bcdf9a9045b49a2ddd7877602788f657bad
# Parent 7d8428388b775a0b26cf88f89ec8f99f5fc8ce25
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:
- removed libxl_sched_*_domain in favour of libxl_sched_params.
- 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 <ian.campbell@citrix.com>
diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Wed May 23 10:11:59 2012 +0100
+++ b/tools/libxl/libxl.c Wed May 23 10:21:05 2012 +0100
@@ -3199,19 +3199,19 @@ libxl_scheduler libxl_get_scheduler(libx
}
int libxl_sched_credit_domain_get(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit_domain *scinfo)
+ libxl_sched_domain_params *scinfo)
{
struct xen_domctl_sched_credit sdom;
int rc;
- libxl_sched_credit_domain_init(scinfo);
-
rc = xc_sched_credit_domain_get(ctx->xch, domid, &sdom);
if (rc != 0) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched credit");
return ERROR_FAIL;
}
+ libxl_sched_domain_params_init(scinfo);
+ scinfo->sched = LIBXL_SCHEDULER_CREDIT;
scinfo->weight = sdom.weight;
scinfo->cap = sdom.cap;
@@ -3219,7 +3219,7 @@ int libxl_sched_credit_domain_get(libxl_
}
int libxl_sched_credit_domain_set(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit_domain *scinfo)
+ libxl_sched_domain_params *scinfo)
{
struct xen_domctl_sched_credit sdom;
xc_domaininfo_t domaininfo;
@@ -3233,22 +3233,33 @@ int libxl_sched_credit_domain_set(libxl_
if (rc != 1 || domaininfo.domain != domid)
return ERROR_INVAL;
-
- if (scinfo->weight < 1 || scinfo->weight > 65535) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "Cpu weight out of range, valid values are within range from 1 to 65535");
- return ERROR_INVAL;
+ rc = xc_sched_credit_domain_get(ctx->xch, domid, &sdom);
+ if (rc != 0) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched credit");
+ return ERROR_FAIL;
}
- if (scinfo->cap < 0 || scinfo->cap > (domaininfo.max_vcpu_id + 1) * 100) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "Cpu cap out of range, valid range is from 0 to %d for specified number of vcpus",
- ((domaininfo.max_vcpu_id + 1) * 100));
- return ERROR_INVAL;
+ if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT) {
+ if (scinfo->weight < 1 || scinfo->weight > 65535) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "Cpu weight out of range, "
+ "valid values are within range from 1 to 65535");
+ return ERROR_INVAL;
+ }
+ sdom.weight = scinfo->weight;
}
- sdom.weight = scinfo->weight;
- sdom.cap = scinfo->cap;
+ if (scinfo->cap != LIBXL_SCHED_DOMAIN_PARAM_CAP_DEFAULT) {
+ if (scinfo->cap < 0
+ || scinfo->cap > (domaininfo.max_vcpu_id + 1) * 100) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "Cpu cap out of range, "
+ "valid range is from 0 to %d for specified number of vcpus",
+ ((domaininfo.max_vcpu_id + 1) * 100));
+ return ERROR_INVAL;
+ }
+ sdom.cap = scinfo->cap;
+ }
rc = xc_sched_credit_domain_set(ctx->xch, domid, &sdom);
if ( rc < 0 ) {
@@ -3321,13 +3332,11 @@ int libxl_sched_credit_params_set(libxl_
}
int libxl_sched_credit2_domain_get(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit2_domain *scinfo)
+ libxl_sched_domain_params *scinfo)
{
struct xen_domctl_sched_credit2 sdom;
int rc;
- libxl_sched_credit2_domain_init(scinfo);
-
rc = xc_sched_credit2_domain_get(ctx->xch, domid, &sdom);
if (rc != 0) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
@@ -3335,36 +3344,37 @@ int libxl_sched_credit2_domain_get(libxl
return ERROR_FAIL;
}
+ libxl_sched_domain_params_init(scinfo);
+ scinfo->sched = LIBXL_SCHEDULER_CREDIT2;
scinfo->weight = sdom.weight;
return 0;
}
int libxl_sched_credit2_domain_set(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit2_domain *scinfo)
+ libxl_sched_domain_params *scinfo)
{
struct xen_domctl_sched_credit2 sdom;
- 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");
+ rc = xc_sched_credit2_domain_get(ctx->xch, domid, &sdom);
+ if (rc != 0) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "getting domain sched credit2");
return ERROR_FAIL;
}
- if (rc != 1 || domaininfo.domain != domid)
- return ERROR_INVAL;
-
-
- if (scinfo->weight < 1 || scinfo->weight > 65535) {
- LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
- "Cpu weight out of range, valid values are within range from "
- "1 to 65535");
- return ERROR_INVAL;
+
+ if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT) {
+ if (scinfo->weight < 1 || scinfo->weight > 65535) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "Cpu weight out of range, "
+ "valid values are within range from "
+ "1 to 65535");
+ return ERROR_INVAL;
+ }
+ sdom.weight = scinfo->weight;
}
- sdom.weight = scinfo->weight;
-
rc = xc_sched_credit2_domain_set(ctx->xch, domid, &sdom);
if ( rc < 0 ) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
@@ -3376,7 +3386,7 @@ int libxl_sched_credit2_domain_set(libxl
}
int libxl_sched_sedf_domain_get(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_sedf_domain *scinfo)
+ libxl_sched_domain_params *scinfo)
{
uint64_t period;
uint64_t slice;
@@ -3385,8 +3395,6 @@ int libxl_sched_sedf_domain_get(libxl_ct
uint16_t weight;
int rc;
- libxl_sched_sedf_domain_init(scinfo);
-
rc = xc_sedf_domain_get(ctx->xch, domid, &period, &slice, &latency,
&extratime, &weight);
if (rc != 0) {
@@ -3394,6 +3402,8 @@ int libxl_sched_sedf_domain_get(libxl_ct
return ERROR_FAIL;
}
+ libxl_sched_domain_params_init(scinfo);
+ scinfo->sched = LIBXL_SCHEDULER_SEDF;
scinfo->period = period / 1000000;
scinfo->slice = slice / 1000000;
scinfo->latency = latency / 1000000;
@@ -3404,24 +3414,37 @@ int libxl_sched_sedf_domain_get(libxl_ct
}
int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,
- 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)
+ slice = scinfo->slice * 1000000;
+ if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
+ latency = scinfo->latency * 1000000;
+ if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)
+ extratime = scinfo->extratime;
+ if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT)
+ weight = 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;
}
diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Wed May 23 10:11:59 2012 +0100
+++ b/tools/libxl/libxl.h Wed May 23 10:21:05 2012 +0100
@@ -805,23 +805,33 @@ int libxl_set_vcpuonline(libxl_ctx *ctx,
libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx);
-
-int libxl_sched_credit_domain_get(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit_domain *scinfo);
-int libxl_sched_credit_domain_set(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit_domain *scinfo);
+/* Per-scheduler parameters */
int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
+
+/* Scheduler Per-domain parameters */
+
+#define LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT 0
+#define LIBXL_SCHED_DOMAIN_PARAM_CAP_DEFAULT -1
+#define LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT 0
+#define LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT 0
+#define LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT 0
+#define LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT -1
+
+int libxl_sched_credit_domain_get(libxl_ctx *ctx, uint32_t domid,
+ libxl_sched_domain_params *scinfo);
+int libxl_sched_credit_domain_set(libxl_ctx *ctx, uint32_t domid,
+ libxl_sched_domain_params *scinfo);
int libxl_sched_credit2_domain_get(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit2_domain *scinfo);
+ libxl_sched_domain_params *scinfo);
int libxl_sched_credit2_domain_set(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_credit2_domain *scinfo);
+ libxl_sched_domain_params *scinfo);
int libxl_sched_sedf_domain_get(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_sedf_domain *scinfo);
+ libxl_sched_domain_params *scinfo);
int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,
- libxl_sched_sedf_domain *scinfo);
+ libxl_sched_domain_params *scinfo);
int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
libxl_trigger trigger, uint32_t vcpuid);
int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq);
diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Wed May 23 10:11:59 2012 +0100
+++ b/tools/libxl/libxl_dom.c Wed May 23 10:21:05 2012 +0100
@@ -45,34 +45,26 @@ libxl_domain_type libxl__domain_type(lib
int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
libxl_sched_domain_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;
+ libxl_scheduler sched = scparams->sched;
int ret;
- sched = libxl_get_scheduler (ctx);
+ if (sched == LIBXL_SCHEDULER_UNKNOWN)
+ sched = libxl__domain_scheduler(gc, domid);
+
switch (sched) {
case LIBXL_SCHEDULER_SEDF:
- sedf_info.period = scparams->period;
- sedf_info.slice = scparams->slice;
- sedf_info.latency = scparams->latency;
- sedf_info.extratime = scparams->extratime;
- sedf_info.weight = scparams->weight;
- ret=libxl_sched_sedf_domain_set(ctx, domid, &sedf_info);
- break;
+ ret=libxl_sched_sedf_domain_set(CTX, domid, scparams);
+ break;
case LIBXL_SCHEDULER_CREDIT:
- credit_info.weight = scparams->weight;
- credit_info.cap = scparams->cap;
- ret=libxl_sched_credit_domain_set(ctx, domid, &credit_info);
- break;
+ ret=libxl_sched_credit_domain_set(CTX, domid, scparams);
+ break;
case LIBXL_SCHEDULER_CREDIT2:
- credit2_info.weight = scparams->weight;
- ret=libxl_sched_credit2_domain_set(ctx, domid, &credit2_info);
- break;
+ ret=libxl_sched_credit2_domain_set(CTX, domid, scparams);
+ break;
default:
- ret=-1;
+ LOG(ERROR, "Unknown scheduler");
+ ret=ERROR_INVAL;
+ break;
}
return ret;
}
diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl Wed May 23 10:11:59 2012 +0100
+++ b/tools/libxl/libxl_types.idl Wed May 23 10:21:05 2012 +0100
@@ -227,12 +227,13 @@ libxl_domain_create_info = Struct("domai
MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
libxl_sched_domain_params = Struct("sched_domain_params",[
- ("weight", integer),
- ("cap", integer),
- ("period", integer),
- ("slice", integer),
- ("latency", integer),
- ("extratime", integer),
+ ("sched", libxl_scheduler),
+ ("weight", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT'}),
+ ("cap", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_CAP_DEFAULT'}),
+ ("period", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT'}),
+ ("slice", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT'}),
+ ("latency", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT'}),
+ ("extratime", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT'}),
], dir=DIR_IN)
# Instances of libxl_file_reference contained in this struct which
@@ -432,28 +433,11 @@ libxl_cputopology = Struct("cputopology"
("node", uint32),
], dir=DIR_OUT)
-libxl_sched_credit_domain = Struct("sched_credit_domain", [
- ("weight", integer),
- ("cap", integer),
- ])
-
libxl_sched_credit_params = Struct("sched_credit_params", [
("tslice_ms", integer),
("ratelimit_us", integer),
], dispose_fn=None)
-libxl_sched_credit2_domain = Struct("sched_credit2_domain", [
- ("weight", integer),
- ])
-
-libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
- ("period", integer),
- ("slice", integer),
- ("latency", integer),
- ("extratime", integer),
- ("weight", integer),
- ])
-
libxl_domain_remus_info = Struct("domain_remus_info",[
("interval", integer),
("blackhole", bool),
diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed May 23 10:11:59 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c Wed May 23 10:21:05 2012 +0100
@@ -627,7 +627,8 @@ 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))
@@ -635,11 +636,11 @@ static void parse_config_data(const char
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;
if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
b_info->max_vcpus = l;
@@ -4351,7 +4352,7 @@ int main_sharing(int argc, char **argv)
return 0;
}
-static int sched_credit_domain_get(int domid, libxl_sched_credit_domain *scinfo)
+static int sched_credit_domain_get(int domid, libxl_sched_domain_params *scinfo)
{
int rc;
@@ -4362,7 +4363,7 @@ static int sched_credit_domain_get(int d
return rc;
}
-static int sched_credit_domain_set(int domid, libxl_sched_credit_domain *scinfo)
+static int sched_credit_domain_set(int domid, libxl_sched_domain_params *scinfo)
{
int rc;
@@ -4398,7 +4399,7 @@ static int sched_credit_params_get(int p
static int sched_credit_domain_output(int domid)
{
char *domname;
- libxl_sched_credit_domain scinfo;
+ libxl_sched_domain_params scinfo;
int rc;
if (domid < 0) {
@@ -4415,7 +4416,7 @@ static int sched_credit_domain_output(in
scinfo.weight,
scinfo.cap);
free(domname);
- libxl_sched_credit_domain_dispose(&scinfo);
+ libxl_sched_domain_params_dispose(&scinfo);
return 0;
}
@@ -4441,7 +4442,7 @@ static int sched_credit_pool_output(uint
}
static int sched_credit2_domain_get(
- int domid, libxl_sched_credit2_domain *scinfo)
+ int domid, libxl_sched_domain_params *scinfo)
{
int rc;
@@ -4453,7 +4454,7 @@ static int sched_credit2_domain_get(
}
static int sched_credit2_domain_set(
- int domid, libxl_sched_credit2_domain *scinfo)
+ int domid, libxl_sched_domain_params *scinfo)
{
int rc;
@@ -4468,7 +4469,7 @@ static int sched_credit2_domain_output(
int domid)
{
char *domname;
- libxl_sched_credit2_domain scinfo;
+ libxl_sched_domain_params scinfo;
int rc;
if (domid < 0) {
@@ -4484,12 +4485,12 @@ static int sched_credit2_domain_output(
domid,
scinfo.weight);
free(domname);
- libxl_sched_credit2_domain_dispose(&scinfo);
+ libxl_sched_domain_params_dispose(&scinfo);
return 0;
}
static int sched_sedf_domain_get(
- int domid, libxl_sched_sedf_domain *scinfo)
+ int domid, libxl_sched_domain_params *scinfo)
{
int rc;
@@ -4501,7 +4502,7 @@ static int sched_sedf_domain_get(
}
static int sched_sedf_domain_set(
- int domid, libxl_sched_sedf_domain *scinfo)
+ int domid, libxl_sched_domain_params *scinfo)
{
int rc;
@@ -4515,7 +4516,7 @@ static int sched_sedf_domain_output(
int domid)
{
char *domname;
- libxl_sched_sedf_domain scinfo;
+ libxl_sched_domain_params scinfo;
int rc;
if (domid < 0) {
@@ -4536,7 +4537,7 @@ static int sched_sedf_domain_output(
scinfo.extratime,
scinfo.weight);
free(domname);
- libxl_sched_sedf_domain_dispose(&scinfo);
+ libxl_sched_domain_params_dispose(&scinfo);
return 0;
}
@@ -4617,7 +4618,6 @@ static int sched_domain_output(libxl_sch
*/
int main_sched_credit(int argc, char **argv)
{
- libxl_sched_credit_domain scinfo;
const char *dom = NULL;
const char *cpupool = NULL;
int weight = 256, cap = 0, opt_w = 0, opt_c = 0;
@@ -4692,7 +4692,7 @@ int main_sched_credit(int argc, char **a
}
if (opt_s) {
- libxl_sched_credit_params scparam;
+ libxl_sched_credit_params scparam;
uint32_t poolid = 0;
if (cpupool) {
@@ -4728,20 +4728,19 @@ int main_sched_credit(int argc, char **a
} else {
find_domain(dom);
- rc = sched_credit_domain_get(domid, &scinfo);
- if (rc)
- return -rc;
-
if (!opt_w && !opt_c) { /* output credit scheduler info */
sched_credit_domain_output(-1);
return -sched_credit_domain_output(domid);
} else { /* set credit scheduler paramaters */
+ libxl_sched_domain_params scinfo;
+ libxl_sched_domain_params_init(&scinfo);
+ scinfo.sched = LIBXL_SCHEDULER_CREDIT;
if (opt_w)
scinfo.weight = weight;
if (opt_c)
scinfo.cap = cap;
rc = sched_credit_domain_set(domid, &scinfo);
- libxl_sched_credit_domain_dispose(&scinfo);
+ libxl_sched_domain_params_dispose(&scinfo);
if (rc)
return -rc;
}
@@ -4752,7 +4751,6 @@ int main_sched_credit(int argc, char **a
int main_sched_credit2(int argc, char **argv)
{
- libxl_sched_credit2_domain scinfo;
const char *dom = NULL;
const char *cpupool = NULL;
int weight = 256, opt_w = 0;
@@ -4807,18 +4805,17 @@ int main_sched_credit2(int argc, char **
} else {
find_domain(dom);
- rc = sched_credit2_domain_get(domid, &scinfo);
- if (rc)
- return -rc;
-
if (!opt_w) { /* output credit2 scheduler info */
sched_credit2_domain_output(-1);
return -sched_credit2_domain_output(domid);
} else { /* set credit2 scheduler paramaters */
+ libxl_sched_domain_params scinfo;
+ libxl_sched_domain_params_init(&scinfo);
+ scinfo.sched = LIBXL_SCHEDULER_CREDIT2;
if (opt_w)
scinfo.weight = weight;
rc = sched_credit2_domain_set(domid, &scinfo);
- libxl_sched_credit2_domain_dispose(&scinfo);
+ libxl_sched_domain_params_dispose(&scinfo);
if (rc)
return -rc;
}
@@ -4829,7 +4826,6 @@ int main_sched_credit2(int argc, char **
int main_sched_sedf(int argc, char **argv)
{
- libxl_sched_sedf_domain scinfo;
const char *dom = NULL;
const char *cpupool = NULL;
int period = 0, opt_p = 0;
@@ -4912,15 +4908,15 @@ int main_sched_sedf(int argc, char **arg
} else {
find_domain(dom);
- rc = sched_sedf_domain_get(domid, &scinfo);
- if (rc)
- return -rc;
-
if (!opt_p && !opt_s && !opt_l && !opt_e && !opt_w) {
/* output sedf scheduler info */
sched_sedf_domain_output(-1);
return -sched_sedf_domain_output(domid);
} else { /* set sedf scheduler paramaters */
+ libxl_sched_domain_params scinfo;
+ libxl_sched_domain_params_init(&scinfo);
+ scinfo.sched = LIBXL_SCHEDULER_SEDF;
+
if (opt_p) {
scinfo.period = period;
scinfo.weight = 0;
@@ -4939,7 +4935,7 @@ int main_sched_sedf(int argc, char **arg
scinfo.slice = 0;
}
rc = sched_sedf_domain_set(domid, &scinfo);
- libxl_sched_sedf_domain_dispose(&scinfo);
+ libxl_sched_domain_params_dispose(&scinfo);
if (rc)
return -rc;
}
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 9:26 ` [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
@ 2012-05-23 10:51 ` Ian Jackson
2012-05-23 11:12 ` Dario Faggioli
2012-05-23 19:28 ` George Dunlap
2 siblings, 0 replies; 19+ messages in thread
From: Ian Jackson @ 2012-05-23 10:51 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Juergen Gross, Dario Faggioli, xen-devel
Ian Campbell writes ("[PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params"):
> libxl: make it possible to explicitly specify default sched params
I think this API change is valuable and I would be willing to make a
freeze exception for it. But I'd like to see George's comments on the
actual code.
> To do so we define a descriminating value which is never a valid
> real value for each parameter.
"Discriminating" would be the correct spelling but I think the word
"distinguished" would be more correct semantically.
> +#define LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_CAP_DEFAULT -1
> +#define LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT -1
Is there some reason these can't all be -1 ? The code has now happily
abstracted this away but anyone looking at one of these structs in a
debugger or trace log or something is going to be quite confused.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 9:26 ` [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2012-05-23 10:51 ` Ian Jackson
@ 2012-05-23 11:12 ` Dario Faggioli
2012-05-24 8:52 ` Ian Campbell
2012-05-23 19:28 ` George Dunlap
2 siblings, 1 reply; 19+ messages in thread
From: Dario Faggioli @ 2012-05-23 11:12 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Juergen Gross, Ian Jackson, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 3059 bytes --]
On Wed, 2012-05-23 at 10:26 +0100, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1337764865 -3600
> # Node ID b6221bcdf9a9045b49a2ddd7877602788f657bad
> # Parent 7d8428388b775a0b26cf88f89ec8f99f5fc8ce25
> 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:
>
> - removed libxl_sched_*_domain in favour of libxl_sched_params.
> - 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 <ian.campbell@citrix.com>
>
Might be my fault, but I can't get this patch to build:
xl_cmdimpl.c:4365:47: error: unknown type name ‘libxl_sched_credit_domain’
xl_cmdimpl.c: In function ‘sched_credit_domain_output’:
xl_cmdimpl.c:4401:5: error: unknown type name ‘libxl_sched_credit_domain’
xl_cmdimpl.c:4408:5: error: implicit declaration of function ‘sched_credit_domain_get’ [-Werror=implicit-function-declaration]
xl_cmdimpl.c:4415:15: error: request for member ‘weight’ in something not a structure or union
xl_cmdimpl.c:4416:15: error: request for member ‘cap’ in something not a structure or union
xl_cmdimpl.c:4418:5: error: implicit declaration of function ‘libxl_sched_credit_domain_dispose’ [-Werror=implicit-function-declaration]
xl_cmdimpl.c: At top level:
xl_cmdimpl.c:4444:16: error: unknown type name ‘libxl_sched_credit2_domain’
xl_cmdimpl.c:4456:16: error: unknown type name ‘libxl_sched_credit2_domain’
xl_cmdimpl.c: In function ‘sched_credit2_domain_output’:
xl_cmdimpl.c:4471:5: error: unknown type name ‘libxl_sched_credit2_domain’
xl_cmdimpl.c:4478:5: error: implicit declaration of function ‘sched_credit2_domain_get’ [-Werror=implicit-function-declaration]
xl_cmdimpl.c:4485:15: error: request for member ‘weight’ in something not a structure or union
xl_cmdimpl.c:4487:5: error: implicit declaration of function ‘libxl_sched_credit2_domain_dispose’ [-Werror=implicit-function-declaration]
xl_cmdimpl.c: At top level:
xl_cmdimpl.c:4492:16: error: unknown type name ‘libxl_sched_sedf_domain’
xl_cmdimpl.c:4504:16: error: unknown type name ‘libxl_sched_sedf_domain’
...
...
It seems like there are some leftovers of the old
libxl_sched_{credit,...}_domain in xl_cmdimpl.c ... Perhaps a missing
add/refresh on our side?
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 9:26 ` [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2012-05-23 10:51 ` Ian Jackson
2012-05-23 11:12 ` Dario Faggioli
@ 2012-05-23 19:28 ` George Dunlap
2012-05-23 19:34 ` George Dunlap
` (2 more replies)
2 siblings, 3 replies; 19+ messages in thread
From: George Dunlap @ 2012-05-23 19:28 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian.Jackson, Dario.Faggioli, Juergen Gross, George.Dunlap, xen-devel
On Wed, May 23, 2012 at 10:26 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1337764865 -3600
> # Node ID b6221bcdf9a9045b49a2ddd7877602788f657bad
> # Parent 7d8428388b775a0b26cf88f89ec8f99f5fc8ce25
> 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:
>
> - removed libxl_sched_*_domain in favour of libxl_sched_params.
> - 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 <ian.campbell@citrix.com>
Overall the idea of the patch looks good. There's just the thing
about shoving all the various schedulers' parameters into one struct.
One fall-out from it is that if you specify weight in your config file
(or during domain creation), it will set the weight for credit or
credit2, but use the defaults for sedf. This might be nice; but we're
implicitly baking in an assumption that parameters with the same name
have to have roughly similar meanings across all schedulers.
Furthermore, if someone sets a "cap" in the config file, for example,
but starts the VM in a pool running credit2, should we really just
silently ignore it, or should we alert the user in some way?
In any case, this patch only takes things half-way. If we're really
going to have One Struct to Rule Them All, we don't need different
domain_set/domain_get functions for the different schedulers -- we
just need a libxl_sched_domain_get(), which will both figure out what
scheduler the domain is running, and fill in the appropriate
parameters, and a libxl_sched_domain_set(), which will check to see
that you've asked for the right scheduler (or marked "unknown" if you
aren't afraid), and set what it can set. We could also have a unified
"xl sched" command that would set various parameters without the user
having to know what scheduler was currently running (perhaps throwing
a warning if you're trying to set a parameter that doesn't exist for
that scheduler).
I'm not really sure which way I think is best. I can see the
advantage of not having to know which scheduler is actually running,
but I'm a bit wary of baking in assumptions about the equivalence of
parameters; it seems like it could lead to some nasty surprises.
But I think whichever way we choose, we should take it to its logical
conclusion. Which in the "One Struct" way, would mean having a single
domain_get/domain_set function, and in the "separate struct" way would
probably mean specifying the scheduler -- i.e., "credit_weight",
"credit2_weight" or something like that. (Obviously we need xm
compatibility, but we can throw a warning to encourage people to
change their config files.)
-George
>
> diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c Wed May 23 10:11:59 2012 +0100
> +++ b/tools/libxl/libxl.c Wed May 23 10:21:05 2012 +0100
> @@ -3199,19 +3199,19 @@ libxl_scheduler libxl_get_scheduler(libx
> }
>
> int libxl_sched_credit_domain_get(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit_domain *scinfo)
> + libxl_sched_domain_params *scinfo)
> {
> struct xen_domctl_sched_credit sdom;
> int rc;
>
> - libxl_sched_credit_domain_init(scinfo);
> -
> rc = xc_sched_credit_domain_get(ctx->xch, domid, &sdom);
> if (rc != 0) {
> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched credit");
> return ERROR_FAIL;
> }
>
> + libxl_sched_domain_params_init(scinfo);
> + scinfo->sched = LIBXL_SCHEDULER_CREDIT;
> scinfo->weight = sdom.weight;
> scinfo->cap = sdom.cap;
>
> @@ -3219,7 +3219,7 @@ int libxl_sched_credit_domain_get(libxl_
> }
>
> int libxl_sched_credit_domain_set(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit_domain *scinfo)
> + libxl_sched_domain_params *scinfo)
> {
> struct xen_domctl_sched_credit sdom;
> xc_domaininfo_t domaininfo;
> @@ -3233,22 +3233,33 @@ int libxl_sched_credit_domain_set(libxl_
> if (rc != 1 || domaininfo.domain != domid)
> return ERROR_INVAL;
>
> -
> - if (scinfo->weight < 1 || scinfo->weight > 65535) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> - "Cpu weight out of range, valid values are within range from 1 to 65535");
> - return ERROR_INVAL;
> + rc = xc_sched_credit_domain_get(ctx->xch, domid, &sdom);
> + if (rc != 0) {
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched credit");
> + return ERROR_FAIL;
> }
>
> - if (scinfo->cap < 0 || scinfo->cap > (domaininfo.max_vcpu_id + 1) * 100) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> - "Cpu cap out of range, valid range is from 0 to %d for specified number of vcpus",
> - ((domaininfo.max_vcpu_id + 1) * 100));
> - return ERROR_INVAL;
> + if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT) {
> + if (scinfo->weight < 1 || scinfo->weight > 65535) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "Cpu weight out of range, "
> + "valid values are within range from 1 to 65535");
> + return ERROR_INVAL;
> + }
> + sdom.weight = scinfo->weight;
> }
>
> - sdom.weight = scinfo->weight;
> - sdom.cap = scinfo->cap;
> + if (scinfo->cap != LIBXL_SCHED_DOMAIN_PARAM_CAP_DEFAULT) {
> + if (scinfo->cap < 0
> + || scinfo->cap > (domaininfo.max_vcpu_id + 1) * 100) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "Cpu cap out of range, "
> + "valid range is from 0 to %d for specified number of vcpus",
> + ((domaininfo.max_vcpu_id + 1) * 100));
> + return ERROR_INVAL;
> + }
> + sdom.cap = scinfo->cap;
> + }
>
> rc = xc_sched_credit_domain_set(ctx->xch, domid, &sdom);
> if ( rc < 0 ) {
> @@ -3321,13 +3332,11 @@ int libxl_sched_credit_params_set(libxl_
> }
>
> int libxl_sched_credit2_domain_get(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit2_domain *scinfo)
> + libxl_sched_domain_params *scinfo)
> {
> struct xen_domctl_sched_credit2 sdom;
> int rc;
>
> - libxl_sched_credit2_domain_init(scinfo);
> -
> rc = xc_sched_credit2_domain_get(ctx->xch, domid, &sdom);
> if (rc != 0) {
> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> @@ -3335,36 +3344,37 @@ int libxl_sched_credit2_domain_get(libxl
> return ERROR_FAIL;
> }
>
> + libxl_sched_domain_params_init(scinfo);
> + scinfo->sched = LIBXL_SCHEDULER_CREDIT2;
> scinfo->weight = sdom.weight;
>
> return 0;
> }
>
> int libxl_sched_credit2_domain_set(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit2_domain *scinfo)
> + libxl_sched_domain_params *scinfo)
> {
> struct xen_domctl_sched_credit2 sdom;
> - 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");
> + rc = xc_sched_credit2_domain_get(ctx->xch, domid, &sdom);
> + if (rc != 0) {
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> + "getting domain sched credit2");
> return ERROR_FAIL;
> }
> - if (rc != 1 || domaininfo.domain != domid)
> - return ERROR_INVAL;
> -
> -
> - if (scinfo->weight < 1 || scinfo->weight > 65535) {
> - LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> - "Cpu weight out of range, valid values are within range from "
> - "1 to 65535");
> - return ERROR_INVAL;
> +
> + if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT) {
> + if (scinfo->weight < 1 || scinfo->weight > 65535) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "Cpu weight out of range, "
> + "valid values are within range from "
> + "1 to 65535");
> + return ERROR_INVAL;
> + }
> + sdom.weight = scinfo->weight;
> }
>
> - sdom.weight = scinfo->weight;
> -
> rc = xc_sched_credit2_domain_set(ctx->xch, domid, &sdom);
> if ( rc < 0 ) {
> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> @@ -3376,7 +3386,7 @@ int libxl_sched_credit2_domain_set(libxl
> }
>
> int libxl_sched_sedf_domain_get(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_sedf_domain *scinfo)
> + libxl_sched_domain_params *scinfo)
> {
> uint64_t period;
> uint64_t slice;
> @@ -3385,8 +3395,6 @@ int libxl_sched_sedf_domain_get(libxl_ct
> uint16_t weight;
> int rc;
>
> - libxl_sched_sedf_domain_init(scinfo);
> -
> rc = xc_sedf_domain_get(ctx->xch, domid, &period, &slice, &latency,
> &extratime, &weight);
> if (rc != 0) {
> @@ -3394,6 +3402,8 @@ int libxl_sched_sedf_domain_get(libxl_ct
> return ERROR_FAIL;
> }
>
> + libxl_sched_domain_params_init(scinfo);
> + scinfo->sched = LIBXL_SCHEDULER_SEDF;
> scinfo->period = period / 1000000;
> scinfo->slice = slice / 1000000;
> scinfo->latency = latency / 1000000;
> @@ -3404,24 +3414,37 @@ int libxl_sched_sedf_domain_get(libxl_ct
> }
>
> int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,
> - 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)
> + slice = scinfo->slice * 1000000;
> + if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
> + latency = scinfo->latency * 1000000;
> + if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)
> + extratime = scinfo->extratime;
> + if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT)
> + weight = 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;
> }
> diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h Wed May 23 10:11:59 2012 +0100
> +++ b/tools/libxl/libxl.h Wed May 23 10:21:05 2012 +0100
> @@ -805,23 +805,33 @@ int libxl_set_vcpuonline(libxl_ctx *ctx,
>
> libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx);
>
> -
> -int libxl_sched_credit_domain_get(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit_domain *scinfo);
> -int libxl_sched_credit_domain_set(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit_domain *scinfo);
> +/* Per-scheduler parameters */
> int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
> libxl_sched_credit_params *scinfo);
> int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
> libxl_sched_credit_params *scinfo);
> +
> +/* Scheduler Per-domain parameters */
> +
> +#define LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_CAP_DEFAULT -1
> +#define LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT 0
> +#define LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT -1
> +
> +int libxl_sched_credit_domain_get(libxl_ctx *ctx, uint32_t domid,
> + libxl_sched_domain_params *scinfo);
> +int libxl_sched_credit_domain_set(libxl_ctx *ctx, uint32_t domid,
> + libxl_sched_domain_params *scinfo);
> int libxl_sched_credit2_domain_get(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit2_domain *scinfo);
> + libxl_sched_domain_params *scinfo);
> int libxl_sched_credit2_domain_set(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_credit2_domain *scinfo);
> + libxl_sched_domain_params *scinfo);
> int libxl_sched_sedf_domain_get(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_sedf_domain *scinfo);
> + libxl_sched_domain_params *scinfo);
> int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,
> - libxl_sched_sedf_domain *scinfo);
> + libxl_sched_domain_params *scinfo);
> int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
> libxl_trigger trigger, uint32_t vcpuid);
> int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq);
> diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Wed May 23 10:11:59 2012 +0100
> +++ b/tools/libxl/libxl_dom.c Wed May 23 10:21:05 2012 +0100
> @@ -45,34 +45,26 @@ libxl_domain_type libxl__domain_type(lib
> int libxl__sched_set_params(libxl__gc *gc, uint32_t domid,
> libxl_sched_domain_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;
> + libxl_scheduler sched = scparams->sched;
> int ret;
>
> - sched = libxl_get_scheduler (ctx);
> + if (sched == LIBXL_SCHEDULER_UNKNOWN)
> + sched = libxl__domain_scheduler(gc, domid);
> +
> switch (sched) {
> case LIBXL_SCHEDULER_SEDF:
> - sedf_info.period = scparams->period;
> - sedf_info.slice = scparams->slice;
> - sedf_info.latency = scparams->latency;
> - sedf_info.extratime = scparams->extratime;
> - sedf_info.weight = scparams->weight;
> - ret=libxl_sched_sedf_domain_set(ctx, domid, &sedf_info);
> - break;
> + ret=libxl_sched_sedf_domain_set(CTX, domid, scparams);
> + break;
> case LIBXL_SCHEDULER_CREDIT:
> - credit_info.weight = scparams->weight;
> - credit_info.cap = scparams->cap;
> - ret=libxl_sched_credit_domain_set(ctx, domid, &credit_info);
> - break;
> + ret=libxl_sched_credit_domain_set(CTX, domid, scparams);
> + break;
> case LIBXL_SCHEDULER_CREDIT2:
> - credit2_info.weight = scparams->weight;
> - ret=libxl_sched_credit2_domain_set(ctx, domid, &credit2_info);
> - break;
> + ret=libxl_sched_credit2_domain_set(CTX, domid, scparams);
> + break;
> default:
> - ret=-1;
> + LOG(ERROR, "Unknown scheduler");
> + ret=ERROR_INVAL;
> + break;
> }
> return ret;
> }
> diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl Wed May 23 10:11:59 2012 +0100
> +++ b/tools/libxl/libxl_types.idl Wed May 23 10:21:05 2012 +0100
> @@ -227,12 +227,13 @@ libxl_domain_create_info = Struct("domai
> MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
>
> libxl_sched_domain_params = Struct("sched_domain_params",[
> - ("weight", integer),
> - ("cap", integer),
> - ("period", integer),
> - ("slice", integer),
> - ("latency", integer),
> - ("extratime", integer),
> + ("sched", libxl_scheduler),
> + ("weight", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT'}),
> + ("cap", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_CAP_DEFAULT'}),
> + ("period", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT'}),
> + ("slice", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT'}),
> + ("latency", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT'}),
> + ("extratime", integer, {'init_val': 'LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT'}),
> ], dir=DIR_IN)
>
> # Instances of libxl_file_reference contained in this struct which
> @@ -432,28 +433,11 @@ libxl_cputopology = Struct("cputopology"
> ("node", uint32),
> ], dir=DIR_OUT)
>
> -libxl_sched_credit_domain = Struct("sched_credit_domain", [
> - ("weight", integer),
> - ("cap", integer),
> - ])
> -
> libxl_sched_credit_params = Struct("sched_credit_params", [
> ("tslice_ms", integer),
> ("ratelimit_us", integer),
> ], dispose_fn=None)
>
> -libxl_sched_credit2_domain = Struct("sched_credit2_domain", [
> - ("weight", integer),
> - ])
> -
> -libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
> - ("period", integer),
> - ("slice", integer),
> - ("latency", integer),
> - ("extratime", integer),
> - ("weight", integer),
> - ])
> -
> libxl_domain_remus_info = Struct("domain_remus_info",[
> ("interval", integer),
> ("blackhole", bool),
> diff -r 7d8428388b77 -r b6221bcdf9a9 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c Wed May 23 10:11:59 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c Wed May 23 10:21:05 2012 +0100
> @@ -627,7 +627,8 @@ 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))
> @@ -635,11 +636,11 @@ static void parse_config_data(const char
> 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;
>
> if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
> b_info->max_vcpus = l;
> @@ -4351,7 +4352,7 @@ int main_sharing(int argc, char **argv)
> return 0;
> }
>
> -static int sched_credit_domain_get(int domid, libxl_sched_credit_domain *scinfo)
> +static int sched_credit_domain_get(int domid, libxl_sched_domain_params *scinfo)
> {
> int rc;
>
> @@ -4362,7 +4363,7 @@ static int sched_credit_domain_get(int d
> return rc;
> }
>
> -static int sched_credit_domain_set(int domid, libxl_sched_credit_domain *scinfo)
> +static int sched_credit_domain_set(int domid, libxl_sched_domain_params *scinfo)
> {
> int rc;
>
> @@ -4398,7 +4399,7 @@ static int sched_credit_params_get(int p
> static int sched_credit_domain_output(int domid)
> {
> char *domname;
> - libxl_sched_credit_domain scinfo;
> + libxl_sched_domain_params scinfo;
> int rc;
>
> if (domid < 0) {
> @@ -4415,7 +4416,7 @@ static int sched_credit_domain_output(in
> scinfo.weight,
> scinfo.cap);
> free(domname);
> - libxl_sched_credit_domain_dispose(&scinfo);
> + libxl_sched_domain_params_dispose(&scinfo);
> return 0;
> }
>
> @@ -4441,7 +4442,7 @@ static int sched_credit_pool_output(uint
> }
>
> static int sched_credit2_domain_get(
> - int domid, libxl_sched_credit2_domain *scinfo)
> + int domid, libxl_sched_domain_params *scinfo)
> {
> int rc;
>
> @@ -4453,7 +4454,7 @@ static int sched_credit2_domain_get(
> }
>
> static int sched_credit2_domain_set(
> - int domid, libxl_sched_credit2_domain *scinfo)
> + int domid, libxl_sched_domain_params *scinfo)
> {
> int rc;
>
> @@ -4468,7 +4469,7 @@ static int sched_credit2_domain_output(
> int domid)
> {
> char *domname;
> - libxl_sched_credit2_domain scinfo;
> + libxl_sched_domain_params scinfo;
> int rc;
>
> if (domid < 0) {
> @@ -4484,12 +4485,12 @@ static int sched_credit2_domain_output(
> domid,
> scinfo.weight);
> free(domname);
> - libxl_sched_credit2_domain_dispose(&scinfo);
> + libxl_sched_domain_params_dispose(&scinfo);
> return 0;
> }
>
> static int sched_sedf_domain_get(
> - int domid, libxl_sched_sedf_domain *scinfo)
> + int domid, libxl_sched_domain_params *scinfo)
> {
> int rc;
>
> @@ -4501,7 +4502,7 @@ static int sched_sedf_domain_get(
> }
>
> static int sched_sedf_domain_set(
> - int domid, libxl_sched_sedf_domain *scinfo)
> + int domid, libxl_sched_domain_params *scinfo)
> {
> int rc;
>
> @@ -4515,7 +4516,7 @@ static int sched_sedf_domain_output(
> int domid)
> {
> char *domname;
> - libxl_sched_sedf_domain scinfo;
> + libxl_sched_domain_params scinfo;
> int rc;
>
> if (domid < 0) {
> @@ -4536,7 +4537,7 @@ static int sched_sedf_domain_output(
> scinfo.extratime,
> scinfo.weight);
> free(domname);
> - libxl_sched_sedf_domain_dispose(&scinfo);
> + libxl_sched_domain_params_dispose(&scinfo);
> return 0;
> }
>
> @@ -4617,7 +4618,6 @@ static int sched_domain_output(libxl_sch
> */
> int main_sched_credit(int argc, char **argv)
> {
> - libxl_sched_credit_domain scinfo;
> const char *dom = NULL;
> const char *cpupool = NULL;
> int weight = 256, cap = 0, opt_w = 0, opt_c = 0;
> @@ -4692,7 +4692,7 @@ int main_sched_credit(int argc, char **a
> }
>
> if (opt_s) {
> - libxl_sched_credit_params scparam;
> + libxl_sched_credit_params scparam;
> uint32_t poolid = 0;
>
> if (cpupool) {
> @@ -4728,20 +4728,19 @@ int main_sched_credit(int argc, char **a
> } else {
> find_domain(dom);
>
> - rc = sched_credit_domain_get(domid, &scinfo);
> - if (rc)
> - return -rc;
> -
> if (!opt_w && !opt_c) { /* output credit scheduler info */
> sched_credit_domain_output(-1);
> return -sched_credit_domain_output(domid);
> } else { /* set credit scheduler paramaters */
> + libxl_sched_domain_params scinfo;
> + libxl_sched_domain_params_init(&scinfo);
> + scinfo.sched = LIBXL_SCHEDULER_CREDIT;
> if (opt_w)
> scinfo.weight = weight;
> if (opt_c)
> scinfo.cap = cap;
> rc = sched_credit_domain_set(domid, &scinfo);
> - libxl_sched_credit_domain_dispose(&scinfo);
> + libxl_sched_domain_params_dispose(&scinfo);
> if (rc)
> return -rc;
> }
> @@ -4752,7 +4751,6 @@ int main_sched_credit(int argc, char **a
>
> int main_sched_credit2(int argc, char **argv)
> {
> - libxl_sched_credit2_domain scinfo;
> const char *dom = NULL;
> const char *cpupool = NULL;
> int weight = 256, opt_w = 0;
> @@ -4807,18 +4805,17 @@ int main_sched_credit2(int argc, char **
> } else {
> find_domain(dom);
>
> - rc = sched_credit2_domain_get(domid, &scinfo);
> - if (rc)
> - return -rc;
> -
> if (!opt_w) { /* output credit2 scheduler info */
> sched_credit2_domain_output(-1);
> return -sched_credit2_domain_output(domid);
> } else { /* set credit2 scheduler paramaters */
> + libxl_sched_domain_params scinfo;
> + libxl_sched_domain_params_init(&scinfo);
> + scinfo.sched = LIBXL_SCHEDULER_CREDIT2;
> if (opt_w)
> scinfo.weight = weight;
> rc = sched_credit2_domain_set(domid, &scinfo);
> - libxl_sched_credit2_domain_dispose(&scinfo);
> + libxl_sched_domain_params_dispose(&scinfo);
> if (rc)
> return -rc;
> }
> @@ -4829,7 +4826,6 @@ int main_sched_credit2(int argc, char **
>
> int main_sched_sedf(int argc, char **argv)
> {
> - libxl_sched_sedf_domain scinfo;
> const char *dom = NULL;
> const char *cpupool = NULL;
> int period = 0, opt_p = 0;
> @@ -4912,15 +4908,15 @@ int main_sched_sedf(int argc, char **arg
> } else {
> find_domain(dom);
>
> - rc = sched_sedf_domain_get(domid, &scinfo);
> - if (rc)
> - return -rc;
> -
> if (!opt_p && !opt_s && !opt_l && !opt_e && !opt_w) {
> /* output sedf scheduler info */
> sched_sedf_domain_output(-1);
> return -sched_sedf_domain_output(domid);
> } else { /* set sedf scheduler paramaters */
> + libxl_sched_domain_params scinfo;
> + libxl_sched_domain_params_init(&scinfo);
> + scinfo.sched = LIBXL_SCHEDULER_SEDF;
> +
> if (opt_p) {
> scinfo.period = period;
> scinfo.weight = 0;
> @@ -4939,7 +4935,7 @@ int main_sched_sedf(int argc, char **arg
> scinfo.slice = 0;
> }
> rc = sched_sedf_domain_set(domid, &scinfo);
> - libxl_sched_sedf_domain_dispose(&scinfo);
> + libxl_sched_domain_params_dispose(&scinfo);
> if (rc)
> return -rc;
> }
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 19:28 ` George Dunlap
@ 2012-05-23 19:34 ` George Dunlap
2012-05-23 21:19 ` Dario Faggioli
2012-05-24 13:57 ` Ian Jackson
2 siblings, 0 replies; 19+ messages in thread
From: George Dunlap @ 2012-05-23 19:34 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian.Jackson, Dario.Faggioli, Juergen Gross, George.Dunlap, xen-devel
On Wed, May 23, 2012 at 8:28 PM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> But I think whichever way we choose, we should take it to its logical
> conclusion. Which in the "One Struct" way, would mean having a single
> domain_get/domain_set function, and in the "separate struct" way would
> probably mean specifying the scheduler -- i.e., "credit_weight",
> "credit2_weight" or something like that. (Obviously we need xm
> compatibility, but we can throw a warning to encourage people to
> change their config files.)
Er, in case this wasn't clear, I meant specifying the scheduler with
the parameter in the config file.
-George
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler
2012-05-23 9:26 ` [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler Ian Campbell
@ 2012-05-23 19:47 ` George Dunlap
2012-05-24 8:55 ` Ian Campbell
0 siblings, 1 reply; 19+ messages in thread
From: George Dunlap @ 2012-05-23 19:47 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian.Jackson, Dario.Faggioli, Juergen Gross, George.Dunlap, xen-devel
On Wed, May 23, 2012 at 10:26 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> if (!tmp) {
> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
> + while(--i>0)
> + libxl_cpupoolinfo_dispose(ptr+i);
I'm not a fan of putting state-changes and conditionals in the same
expression and relying on prefix/postifx precedence to sort things
out. It seems like it's laying a trap for some poor tired programmer
in the future to make a thinko. Would it really be that bad to just
write "for(i--; i>0; i--)"? :-)
Actually -- walk me through this one. Won't this fail to call
libxl_cpupoolinfo_dispose() on element 0?
-G
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 of 3] libxl: rename libxl_sched_params to libxl_sched_domain_params
2012-05-23 9:26 ` [PATCH 2 of 3] libxl: rename libxl_sched_params to libxl_sched_domain_params Ian Campbell
@ 2012-05-23 19:49 ` George Dunlap
0 siblings, 0 replies; 19+ messages in thread
From: George Dunlap @ 2012-05-23 19:49 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian.Jackson, Dario.Faggioli, Juergen Gross, George.Dunlap, xen-devel
On Wed, May 23, 2012 at 10:26 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1337764319 -3600
> # Node ID 7d8428388b775a0b26cf88f89ec8f99f5fc8ce25
> # Parent 6533501734be011cb1f7669b7840bfdf5b2eb801
> libxl: rename libxl_sched_params to libxl_sched_domain_params
>
> Remove credit scheduler global options from the struct, they were never used
> anyway.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 19:28 ` George Dunlap
2012-05-23 19:34 ` George Dunlap
@ 2012-05-23 21:19 ` Dario Faggioli
2012-05-24 9:13 ` Ian Campbell
2012-05-24 13:57 ` Ian Jackson
2 siblings, 1 reply; 19+ messages in thread
From: Dario Faggioli @ 2012-05-23 21:19 UTC (permalink / raw)
To: George Dunlap
Cc: Ian.Jackson, Juergen Gross, xen-devel, Ian Campbell, George.Dunlap
[-- Attachment #1.1: Type: text/plain, Size: 3467 bytes --]
On Wed, 2012-05-23 at 20:28 +0100, George Dunlap wrote:
> Overall the idea of the patch looks good. There's just the thing
> about shoving all the various schedulers' parameters into one struct.
>
Yep, I really don't like that either.
> One fall-out from it is that if you specify weight in your config file
> (or during domain creation), it will set the weight for credit or
> credit2, but use the defaults for sedf. This might be nice; but we're
> implicitly baking in an assumption that parameters with the same name
> have to have roughly similar meanings across all schedulers.
> Furthermore, if someone sets a "cap" in the config file, for example,
> but starts the VM in a pool running credit2, should we really just
> silently ignore it, or should we alert the user in some way?
>
I agree... Some mechanism for providing the user at least with a warning
would be useful.
> In any case, this patch only takes things half-way. If we're really
> going to have One Struct to Rule Them All, we don't need different
> domain_set/domain_get functions for the different schedulers -- we
> just need a libxl_sched_domain_get(), which will both figure out what
> scheduler the domain is running, and fill in the appropriate
> parameters, and a libxl_sched_domain_set(), which will check to see
> that you've asked for the right scheduler (or marked "unknown" if you
> aren't afraid), and set what it can set.
>
According to my personal taste, that would be quite ugly, not to mention
that every time we might be adding/removing/modifying a scheduler, we
would need to update this Frankenstein-struct, potentially affecting all
the other ones... :-(
> I'm not really sure which way I think is best. I can see the
> advantage of not having to know which scheduler is actually running,
> but I'm a bit wary of baking in assumptions about the equivalence of
> parameters; it seems like it could lead to some nasty surprises.
>
I agree again: the fact that, right now, _almost_ all the existing
schedulers have a parameter called weight with _almost_ the same meaning
shouldn't allow us to assume that to be true now and forever.
> But I think whichever way we choose, we should take it to its logical
> conclusion. Which in the "One Struct" way, would mean having a single
> domain_get/domain_set function, and in the "separate struct" way would
> probably mean specifying the scheduler -- i.e., "credit_weight",
> "credit2_weight" or something like that. (Obviously we need xm
> compatibility, but we can throw a warning to encourage people to
> change their config files.)
>
For what it counts, I'm all for option #2, i.e., each scheduler with its
own struct, set of helper functions, xl sub-command, etc. Something like
'credit.cap = XX', 'credit2.weight = XX' or 'sedf.period = XXX' would be
nice, for discriminating them in the config file. It'd remain to decide
what to do with things like 'weight = XX', which we need to support for
backward compatibility, but I guess almost anything is fine, provided we
warn the user about what's happening and ask him to update the syntax.
Just my 2 cents. :-)
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 11:12 ` Dario Faggioli
@ 2012-05-24 8:52 ` Ian Campbell
2012-05-24 9:15 ` Dario Faggioli
0 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2012-05-24 8:52 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Juergen Gross, Ian Jackson, xen-devel
On Wed, 2012-05-23 at 12:12 +0100, Dario Faggioli wrote:
> On Wed, 2012-05-23 at 10:26 +0100, Ian Campbell wrote:
> > # HG changeset patch
> > # User Ian Campbell <ian.campbell@citrix.com>
> > # Date 1337764865 -3600
> > # Node ID b6221bcdf9a9045b49a2ddd7877602788f657bad
> > # Parent 7d8428388b775a0b26cf88f89ec8f99f5fc8ce25
> > 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:
> >
> > - removed libxl_sched_*_domain in favour of libxl_sched_params.
> > - 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 <ian.campbell@citrix.com>
> >
> Might be my fault, but I can't get this patch to build:
>
> xl_cmdimpl.c:4365:47: error: unknown type name ‘libxl_sched_credit_domain’
> xl_cmdimpl.c: In function ‘sched_credit_domain_output’:
> xl_cmdimpl.c:4401:5: error: unknown type name ‘libxl_sched_credit_domain’
My local version definitely doesn't use this type name any more and I
can see the hunk in the patch which renamed the use in that function to
libxl_sched_domain_params. Did you get rejects when you applied perhaps?
> xl_cmdimpl.c:4408:5: error: implicit declaration of function ‘sched_credit_domain_get’ [-Werror=implicit-function-declaration]
> xl_cmdimpl.c:4415:15: error: request for member ‘weight’ in something not a structure or union
> xl_cmdimpl.c:4416:15: error: request for member ‘cap’ in something not a structure or union
> xl_cmdimpl.c:4418:5: error: implicit declaration of function ‘libxl_sched_credit_domain_dispose’ [-Werror=implicit-function-declaration]
> xl_cmdimpl.c: At top level:
> xl_cmdimpl.c:4444:16: error: unknown type name ‘libxl_sched_credit2_domain’
> xl_cmdimpl.c:4456:16: error: unknown type name ‘libxl_sched_credit2_domain’
> xl_cmdimpl.c: In function ‘sched_credit2_domain_output’:
> xl_cmdimpl.c:4471:5: error: unknown type name ‘libxl_sched_credit2_domain’
> xl_cmdimpl.c:4478:5: error: implicit declaration of function ‘sched_credit2_domain_get’ [-Werror=implicit-function-declaration]
> xl_cmdimpl.c:4485:15: error: request for member ‘weight’ in something not a structure or union
> xl_cmdimpl.c:4487:5: error: implicit declaration of function ‘libxl_sched_credit2_domain_dispose’ [-Werror=implicit-function-declaration]
> xl_cmdimpl.c: At top level:
> xl_cmdimpl.c:4492:16: error: unknown type name ‘libxl_sched_sedf_domain’
> xl_cmdimpl.c:4504:16: error: unknown type name ‘libxl_sched_sedf_domain’
> ...
> ...
>
> It seems like there are some leftovers of the old
> libxl_sched_{credit,...}_domain in xl_cmdimpl.c ... Perhaps a missing
> add/refresh on our side?
>
> Regards,
> Dario
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler
2012-05-23 19:47 ` George Dunlap
@ 2012-05-24 8:55 ` Ian Campbell
2012-05-24 9:16 ` Dario Faggioli
0 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2012-05-24 8:55 UTC (permalink / raw)
To: George Dunlap; +Cc: Dario Faggioli, Ian Jackson, Juergen Gross, xen-devel
On Wed, 2012-05-23 at 20:47 +0100, George Dunlap wrote:
> On Wed, May 23, 2012 at 10:26 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> > if (!tmp) {
> > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
> > + while(--i>0)
> > + libxl_cpupoolinfo_dispose(ptr+i);
>
> I'm not a fan of putting state-changes and conditionals in the same
> expression and relying on prefix/postifx precedence to sort things
> out. It seems like it's laying a trap for some poor tired programmer
> in the future to make a thinko. Would it really be that bad to just
> write "for(i--; i>0; i--)"? :-)
>
> Actually -- walk me through this one. Won't this fail to call
> libxl_cpupoolinfo_dispose() on element 0?
That's certainly not impossible! I'll double check as I rewrite it along
the lines you suggest.
>
> -G
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 21:19 ` Dario Faggioli
@ 2012-05-24 9:13 ` Ian Campbell
2012-05-24 9:36 ` Dario Faggioli
0 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2012-05-24 9:13 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Juergen Gross, Ian Jackson, xen-devel
On Wed, 2012-05-23 at 22:19 +0100, Dario Faggioli wrote:
> On Wed, 2012-05-23 at 20:28 +0100, George Dunlap wrote:
> > Overall the idea of the patch looks good. There's just the thing
> > about shoving all the various schedulers' parameters into one struct.
> >
> Yep, I really don't like that either.
I think we reached to opposite conclusion when Deiter implemented the
sched params support, but I don't really mind either way. I'm happy to
go with whichever you guys think is best (which seems to be leaning
toward separate structs?).
> > One fall-out from it is that if you specify weight in your config file
> > (or during domain creation), it will set the weight for credit or
> > credit2, but use the defaults for sedf. This might be nice; but we're
> > implicitly baking in an assumption that parameters with the same name
> > have to have roughly similar meanings across all schedulers.
> > Furthermore, if someone sets a "cap" in the config file, for example,
> > but starts the VM in a pool running credit2, should we really just
> > silently ignore it, or should we alert the user in some way?
> >
> I agree... Some mechanism for providing the user at least with a warning
> would be useful.
So xl should query the scheduler for the pool which has been specified
in the config and parse the appropriate options? That seems doable.
At the libxl level I think this would end up being a KeyedUnion in the
build info, selecting the appropriate per-sched params struct. Does that
sound reasonable?
> > But I think whichever way we choose, we should take it to its logical
> > conclusion. Which in the "One Struct" way, would mean having a single
> > domain_get/domain_set function, and in the "separate struct" way would
> > probably mean specifying the scheduler -- i.e., "credit_weight",
> > "credit2_weight" or something like that. (Obviously we need xm
> > compatibility, but we can throw a warning to encourage people to
> > change their config files.)
> >
> For what it counts, I'm all for option #2, i.e., each scheduler with its
> own struct, set of helper functions, xl sub-command, etc. Something like
> 'credit.cap = XX', 'credit2.weight = XX' or 'sedf.period = XXX' would be
> nice, for discriminating them in the config file. It'd remain to decide
> what to do with things like 'weight = XX', which we need to support for
> backward compatibility, but I guess almost anything is fine, provided we
> warn the user about what's happening and ask him to update the syntax.
That all sounds fine, but not for 4.2 IMHO.
Ian.
>
> Just my 2 cents. :-)
>
> Regards,
> Dario
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-24 8:52 ` Ian Campbell
@ 2012-05-24 9:15 ` Dario Faggioli
0 siblings, 0 replies; 19+ messages in thread
From: Dario Faggioli @ 2012-05-24 9:15 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Juergen Gross, Ian Jackson, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1218 bytes --]
On Thu, 2012-05-24 at 09:52 +0100, Ian Campbell wrote:
> > Might be my fault, but I can't get this patch to build:
> >
> > xl_cmdimpl.c:4365:47: error: unknown type name ‘libxl_sched_credit_domain’
> > xl_cmdimpl.c: In function ‘sched_credit_domain_output’:
> > xl_cmdimpl.c:4401:5: error: unknown type name ‘libxl_sched_credit_domain’
>
> My local version definitely doesn't use this type name any more and I
> can see the hunk in the patch which renamed the use in that function to
> libxl_sched_domain_params.
>
Mmm... You're ight, looking at the patch here in the message, the
xl_cmdimpl.c hunks are there. This is not the case of my `hg mimport'-ed
version. Strange. :-/
> Did you get rejects when you applied perhaps?
>
Nope, but the imported patch has some garbage at the end... Not sue
whether that is normal o not with `hg mimport'. :-O
Anyway, sorry for bothering, I'll give it another try!
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler
2012-05-24 8:55 ` Ian Campbell
@ 2012-05-24 9:16 ` Dario Faggioli
0 siblings, 0 replies; 19+ messages in thread
From: Dario Faggioli @ 2012-05-24 9:16 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Juergen Gross, Ian Jackson, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 550 bytes --]
On Thu, 2012-05-24 at 09:55 +0100, Ian Campbell wrote:
> That's certainly not impossible! I'll double check as I rewrite it along
> the lines you suggest.
>
And which one of the two ways George outlined are you considering for
such a rewrite (just curious :-P) ?
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-24 9:13 ` Ian Campbell
@ 2012-05-24 9:36 ` Dario Faggioli
0 siblings, 0 replies; 19+ messages in thread
From: Dario Faggioli @ 2012-05-24 9:36 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Juergen Gross, Ian Jackson, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 3124 bytes --]
On Thu, 2012-05-24 at 10:13 +0100, Ian Campbell wrote:
> On Wed, 2012-05-23 at 22:19 +0100, Dario Faggioli wrote:
> > On Wed, 2012-05-23 at 20:28 +0100, George Dunlap wrote:
> > > Overall the idea of the patch looks good. There's just the thing
> > > about shoving all the various schedulers' parameters into one struct.
> > >
> > Yep, I really don't like that either.
>
> I think we reached to opposite conclusion when Deiter implemented the
> sched params support, but I don't really mind either way.
>
Yes, you're thinking right. Unfortunately, besides starting to
participate to that thread, I was off when consensus on that particular
aspect was reached. After that, I decided it is no such a big deal that
would worth a rewrite of the whole thing per-se, but if we are rewriting
it anyway, well... :-)
> I'm happy to
> go with whichever you guys think is best (which seems to be leaning
> toward separate structs?).
>
I am definitely leaning toward that, but of course it's George's opinion
the one that we should care most.
> > > One fall-out from it is that if you specify weight in your config file
> > > (or during domain creation), it will set the weight for credit or
> > > credit2, but use the defaults for sedf. This might be nice; but we're
> > > implicitly baking in an assumption that parameters with the same name
> > > have to have roughly similar meanings across all schedulers.
> > > Furthermore, if someone sets a "cap" in the config file, for example,
> > > but starts the VM in a pool running credit2, should we really just
> > > silently ignore it, or should we alert the user in some way?
> > >
> > I agree... Some mechanism for providing the user at least with a warning
> > would be useful.
>
> So xl should query the scheduler for the pool which has been specified
> in the config and parse the appropriate options? That seems doable.
>
That appears right to me, yes.
> At the libxl level I think this would end up being a KeyedUnion in the
> build info, selecting the appropriate per-sched params struct. Does that
> sound reasonable?
>
To me, definitely.
> > For what it counts, I'm all for option #2, i.e., each scheduler with its
> > own struct, set of helper functions, xl sub-command, etc. Something like
> > 'credit.cap = XX', 'credit2.weight = XX' or 'sedf.period = XXX' would be
> > nice, for discriminating them in the config file. It'd remain to decide
> > what to do with things like 'weight = XX', which we need to support for
> > backward compatibility, but I guess almost anything is fine, provided we
> > warn the user about what's happening and ask him to update the syntax.
>
> That all sounds fine, but not for 4.2 IMHO.
>
Yes, let's just put what xm has together for now, we can add all the
other stuff later.
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-23 19:28 ` George Dunlap
2012-05-23 19:34 ` George Dunlap
2012-05-23 21:19 ` Dario Faggioli
@ 2012-05-24 13:57 ` Ian Jackson
2012-05-24 14:02 ` Ian Campbell
2 siblings, 1 reply; 19+ messages in thread
From: Ian Jackson @ 2012-05-24 13:57 UTC (permalink / raw)
To: George Dunlap; +Cc: Dario Faggioli, Ian Campbell, Juergen Gross, xen-devel
George Dunlap writes ("Re: [Xen-devel] [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params"):
> This might be nice; but we're
> implicitly baking in an assumption that parameters with the same name
> have to have roughly similar meanings across all schedulers.
We can make this assumption true in the libxl API even if it's false
at the Xen level, simply by renaming parameters which have
"sufficiently" divergent semantics.
> Furthermore, if someone sets a "cap" in the config file, for example,
> but starts the VM in a pool running credit2, should we really just
> silently ignore it, or should we alert the user in some way?
We don't currently have any way to warn anyone about unused parameter
settings in xl config files.
> But I think whichever way we choose, we should take it to its logical
> conclusion. Which in the "One Struct" way, would mean having a single
> domain_get/domain_set function, and in the "separate struct" way would
> probably mean specifying the scheduler -- i.e., "credit_weight",
> "credit2_weight" or something like that. (Obviously we need xm
> compatibility, but we can throw a warning to encourage people to
> change their config files.)
Yes.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params
2012-05-24 13:57 ` Ian Jackson
@ 2012-05-24 14:02 ` Ian Campbell
0 siblings, 0 replies; 19+ messages in thread
From: Ian Campbell @ 2012-05-24 14:02 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Dario Faggioli, Juergen Gross, xen-devel
On Thu, 2012-05-24 at 14:57 +0100, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params"):
> > This might be nice; but we're
> > implicitly baking in an assumption that parameters with the same name
> > have to have roughly similar meanings across all schedulers.
>
> We can make this assumption true in the libxl API even if it's false
> at the Xen level, simply by renaming parameters which have
> "sufficiently" divergent semantics.
>
> > Furthermore, if someone sets a "cap" in the config file, for example,
> > but starts the VM in a pool running credit2, should we really just
> > silently ignore it, or should we alert the user in some way?
>
> We don't currently have any way to warn anyone about unused parameter
> settings in xl config files.
Warning about known parameters which don't effect the current scheduler
should be pretty easy to do though.
>
> > But I think whichever way we choose, we should take it to its logical
> > conclusion. Which in the "One Struct" way, would mean having a single
> > domain_get/domain_set function, and in the "separate struct" way would
> > probably mean specifying the scheduler -- i.e., "credit_weight",
> > "credit2_weight" or something like that. (Obviously we need xm
> > compatibility, but we can throw a warning to encourage people to
> > change their config files.)
>
> Yes.
>
> Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-05-24 14:02 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-23 9:26 [PATCH 0 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2012-05-23 9:26 ` [PATCH 1 of 3] libxl: add internal function to get a domain's scheduler Ian Campbell
2012-05-23 19:47 ` George Dunlap
2012-05-24 8:55 ` Ian Campbell
2012-05-24 9:16 ` Dario Faggioli
2012-05-23 9:26 ` [PATCH 2 of 3] libxl: rename libxl_sched_params to libxl_sched_domain_params Ian Campbell
2012-05-23 19:49 ` George Dunlap
2012-05-23 9:26 ` [PATCH 3 of 3] libxl: make it possible to explicitly specify default sched params Ian Campbell
2012-05-23 10:51 ` Ian Jackson
2012-05-23 11:12 ` Dario Faggioli
2012-05-24 8:52 ` Ian Campbell
2012-05-24 9:15 ` Dario Faggioli
2012-05-23 19:28 ` George Dunlap
2012-05-23 19:34 ` George Dunlap
2012-05-23 21:19 ` Dario Faggioli
2012-05-24 9:13 ` Ian Campbell
2012-05-24 9:36 ` Dario Faggioli
2012-05-24 13:57 ` Ian Jackson
2012-05-24 14:02 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.