* [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
@ 2015-05-26 0:09 Chong Li
2015-06-02 12:53 ` George Dunlap
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Chong Li @ 2015-05-26 0:09 UTC (permalink / raw)
To: xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
ian.jackson, ian.campbell, mengxu, lichong659, dgolomb
Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support
per-VCPU settings for RTDS scheduler.
Add a new data structure (libxl_vcpu_sched_params) to help per-VCPU settings.
Signed-off-by: Chong Li <chong.li@wustl.edu>
Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
Signed-off-by: Sisu Xi <xisisu@gmail.com>
---
tools/libxl/libxl.c | 189 ++++++++++++++++++++++++++++++++++++++------
tools/libxl/libxl.h | 19 +++++
tools/libxl/libxl_types.idl | 11 +++
3 files changed, 196 insertions(+), 23 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index feb3aa9..169901a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5797,6 +5797,120 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
return 0;
}
+static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period,
+ uint32_t budget, uint32_t *sdom_period,
+ uint32_t *sdom_budget)
+{
+ if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
+ if (period < 1) {
+ LOG(ERROR, "VCPU period is not set or out of range, "
+ "valid values are larger than 1");
+ return ERROR_INVAL;
+ }
+ *sdom_period = period;
+ }
+
+ if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
+ if (budget < 1) {
+ LOG(ERROR, "VCPU budget is not set or out of range, "
+ "valid values are larger than 1");
+ return ERROR_INVAL;
+ }
+ *sdom_budget = budget;
+ }
+
+ if (budget > period) {
+ LOG(ERROR, "VCPU budget is larger than VCPU period, "
+ "VCPU budget should be no larger than VCPU period");
+ return ERROR_INVAL;
+ }
+
+ return 0;
+}
+
+static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
+ libxl_vcpu_sched_params *scinfo)
+{
+ uint16_t num_vcpus;
+ int rc, i;
+ xc_dominfo_t info;
+
+ rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (rc < 0) {
+ LOGE(ERROR, "getting domain info");
+ return ERROR_FAIL;
+ }
+ num_vcpus = info.max_vcpu_id + 1;
+
+ struct xen_domctl_sched_rtds_params *sdom = libxl__malloc(NOGC,
+ sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus);
+ rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus);
+ if (rc != 0) {
+ LOGE(ERROR, "getting vcpu sched rtds");
+ return ERROR_FAIL;
+ }
+
+ libxl_vcpu_sched_params_init(scinfo);
+
+ scinfo->sched = LIBXL_SCHEDULER_RTDS;
+ scinfo->num_vcpus = num_vcpus;
+ scinfo->vcpus = (libxl_rtds_vcpu *)
+ libxl__malloc(NOGC, sizeof(libxl_rtds_vcpu) * num_vcpus);
+ for(i = 0; i < num_vcpus; i++) {
+ scinfo->vcpus[i].period = sdom[i].period;
+ scinfo->vcpus[i].budget = sdom[i].budget;
+ }
+
+ return 0;
+}
+
+static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
+ const libxl_vcpu_sched_params *scinfo)
+{
+ int rc;
+ int i;
+ uint16_t num_vcpus;
+ int vcpuid;
+ uint32_t budget, period;
+ xc_dominfo_t info;
+
+ rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (rc < 0) {
+ LOGE(ERROR, "getting domain info");
+ return ERROR_FAIL;
+ }
+ num_vcpus = info.max_vcpu_id + 1;
+
+ struct xen_domctl_sched_rtds_params *sdom =
+ libxl__malloc(NOGC, scinfo->num_vcpus);
+ for (i = 0; i < scinfo->num_vcpus; i++) {
+ vcpuid = scinfo->vcpus[i].vcpuid;
+ budget = scinfo->vcpus[i].budget;
+ period = scinfo->vcpus[i].period;
+ if (vcpuid < 0 || vcpuid >= num_vcpus) {
+ LOG(ERROR, "VCPU index is out of range, "
+ "valid values are within range from 0 to %d",
+ num_vcpus);
+ return ERROR_INVAL;
+ }
+ sdom[i].vcpuid = vcpuid;
+
+ rc = sched_rtds_validate_params(gc, period, budget,
+ &sdom[i].period, &sdom[i].budget);
+ if (rc == ERROR_INVAL)
+ return rc;
+ }
+
+ rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
+ sdom, scinfo->num_vcpus);
+ if (rc != 0) {
+ LOGE(ERROR, "setting vcpu sched rtds");
+ return ERROR_FAIL;
+ }
+
+ return rc;
+}
+
static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
@@ -5830,29 +5944,10 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
return ERROR_FAIL;
}
- if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
- if (scinfo->period < 1) {
- LOG(ERROR, "VCPU period is not set or out of range, "
- "valid values are larger than 1");
- return ERROR_INVAL;
- }
- sdom.period = scinfo->period;
- }
-
- if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
- if (scinfo->budget < 1) {
- LOG(ERROR, "VCPU budget is not set or out of range, "
- "valid values are larger than 1");
- return ERROR_INVAL;
- }
- sdom.budget = scinfo->budget;
- }
-
- if (sdom.budget > sdom.period) {
- LOG(ERROR, "VCPU budget is larger than VCPU period, "
- "VCPU budget should be no larger than VCPU period");
- return ERROR_INVAL;
- }
+ rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
+ &sdom.period, &sdom.budget);
+ if (rc == ERROR_INVAL)
+ return rc;
rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
if (rc < 0) {
@@ -5899,6 +5994,30 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
return ret;
}
+int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
+ const libxl_vcpu_sched_params *scinfo)
+{
+ GC_INIT(ctx);
+ libxl_scheduler sched = scinfo->sched;
+ int ret;
+
+ if (sched == LIBXL_SCHEDULER_UNKNOWN)
+ sched = libxl__domain_scheduler(gc, domid);
+
+ switch (sched) {
+ case LIBXL_SCHEDULER_RTDS:
+ ret=sched_rtds_vcpu_set(gc, domid, scinfo);
+ break;
+ default:
+ LOG(ERROR, "Unknown scheduler");
+ ret=ERROR_INVAL;
+ break;
+ }
+
+ GC_FREE;
+ return ret;
+}
+
int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
@@ -5932,6 +6051,30 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
return ret;
}
+int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
+ libxl_vcpu_sched_params *scinfo)
+{
+ GC_INIT(ctx);
+ int ret;
+
+ libxl_vcpu_sched_params_init(scinfo);
+
+ scinfo->sched = libxl__domain_scheduler(gc, domid);
+
+ switch (scinfo->sched) {
+ case LIBXL_SCHEDULER_RTDS:
+ ret=sched_rtds_vcpu_get(gc, domid, scinfo);
+ break;
+ default:
+ LOG(ERROR, "Unknown scheduler");
+ ret=ERROR_INVAL;
+ break;
+ }
+
+ GC_FREE;
+ return ret;
+}
+
static int libxl__domain_s3_resume(libxl__gc *gc, int domid)
{
int rc = 0;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 44bd8e2..e565814 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -192,6 +192,18 @@
* is not present, instead of ERROR_INVAL.
*/
#define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
+
+/*
+ * libxl_vcpu_sched_params is used to get/set per-vcpu params
+*/
+#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
+
+/*
+ * libxl_rtds_vcpu is used to represent the rtds scheduling params
+ * of a vcpu
+*/
+#define LIBXL_HAVE_RTDS_VCPU 1
+
/*
* libxl ABI compatibility
*
@@ -1460,10 +1472,17 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
#define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
#define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1
+/*RTDS Per-VCPU parameters*/
+#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1
+
int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
libxl_domain_sched_params *params);
int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
const libxl_domain_sched_params *params);
+int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
+ libxl_vcpu_sched_params *params);
+int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
+ const libxl_vcpu_sched_params *params);
int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
libxl_trigger trigger, uint32_t vcpuid);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 117b61d..d28d274 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
("checkpointed_stream", integer),
])
+libxl_rtds_vcpu = Struct("rtds_vcpu",[
+ ("period", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
+ ("budget", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
+ ("vcpuid", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
+ ])
+
+libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
+ ("sched", libxl_scheduler),
+ ("vcpus", Array(libxl_rtds_vcpu, "num_vcpus")),
+ ])
+
libxl_domain_sched_params = Struct("domain_sched_params",[
("sched", libxl_scheduler),
("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
--
1.9.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-05-26 0:09 [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler Chong Li
@ 2015-06-02 12:53 ` George Dunlap
2015-06-02 14:10 ` Chong Li
2015-06-04 23:48 ` Dario Faggioli
2015-06-05 11:37 ` Ian Campbell
2 siblings, 1 reply; 15+ messages in thread
From: George Dunlap @ 2015-06-02 12:53 UTC (permalink / raw)
To: Chong Li, xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, dario.faggioli, ian.jackson,
ian.campbell, mengxu, dgolomb
On 05/26/2015 01:09 AM, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support
> per-VCPU settings for RTDS scheduler.
>
> Add a new data structure (libxl_vcpu_sched_params) to help per-VCPU settings.
>
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
This doesn't apply cleanly for me anymore -- can you refresh and resend?
Thanks,
-George
> ---
> tools/libxl/libxl.c | 189 ++++++++++++++++++++++++++++++++++++++------
> tools/libxl/libxl.h | 19 +++++
> tools/libxl/libxl_types.idl | 11 +++
> 3 files changed, 196 insertions(+), 23 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index feb3aa9..169901a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5797,6 +5797,120 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
> return 0;
> }
>
> +static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period,
> + uint32_t budget, uint32_t *sdom_period,
> + uint32_t *sdom_budget)
> +{
> + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> + if (period < 1) {
> + LOG(ERROR, "VCPU period is not set or out of range, "
> + "valid values are larger than 1");
> + return ERROR_INVAL;
> + }
> + *sdom_period = period;
> + }
> +
> + if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> + if (budget < 1) {
> + LOG(ERROR, "VCPU budget is not set or out of range, "
> + "valid values are larger than 1");
> + return ERROR_INVAL;
> + }
> + *sdom_budget = budget;
> + }
> +
> + if (budget > period) {
> + LOG(ERROR, "VCPU budget is larger than VCPU period, "
> + "VCPU budget should be no larger than VCPU period");
> + return ERROR_INVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + uint16_t num_vcpus;
> + int rc, i;
> + xc_dominfo_t info;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> + num_vcpus = info.max_vcpu_id + 1;
> +
> + struct xen_domctl_sched_rtds_params *sdom = libxl__malloc(NOGC,
> + sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus);
> + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus);
> + if (rc != 0) {
> + LOGE(ERROR, "getting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + libxl_vcpu_sched_params_init(scinfo);
> +
> + scinfo->sched = LIBXL_SCHEDULER_RTDS;
> + scinfo->num_vcpus = num_vcpus;
> + scinfo->vcpus = (libxl_rtds_vcpu *)
> + libxl__malloc(NOGC, sizeof(libxl_rtds_vcpu) * num_vcpus);
> + for(i = 0; i < num_vcpus; i++) {
> + scinfo->vcpus[i].period = sdom[i].period;
> + scinfo->vcpus[i].budget = sdom[i].budget;
> + }
> +
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> + int i;
> + uint16_t num_vcpus;
> + int vcpuid;
> + uint32_t budget, period;
> + xc_dominfo_t info;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> + num_vcpus = info.max_vcpu_id + 1;
> +
> + struct xen_domctl_sched_rtds_params *sdom =
> + libxl__malloc(NOGC, scinfo->num_vcpus);
> + for (i = 0; i < scinfo->num_vcpus; i++) {
> + vcpuid = scinfo->vcpus[i].vcpuid;
> + budget = scinfo->vcpus[i].budget;
> + period = scinfo->vcpus[i].period;
> + if (vcpuid < 0 || vcpuid >= num_vcpus) {
> + LOG(ERROR, "VCPU index is out of range, "
> + "valid values are within range from 0 to %d",
> + num_vcpus);
> + return ERROR_INVAL;
> + }
> + sdom[i].vcpuid = vcpuid;
> +
> + rc = sched_rtds_validate_params(gc, period, budget,
> + &sdom[i].period, &sdom[i].budget);
> + if (rc == ERROR_INVAL)
> + return rc;
> + }
> +
> + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
> + sdom, scinfo->num_vcpus);
> + if (rc != 0) {
> + LOGE(ERROR, "setting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + return rc;
> +}
> +
> static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid,
> libxl_domain_sched_params *scinfo)
> {
> @@ -5830,29 +5944,10 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
> return ERROR_FAIL;
> }
>
> - if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> - if (scinfo->period < 1) {
> - LOG(ERROR, "VCPU period is not set or out of range, "
> - "valid values are larger than 1");
> - return ERROR_INVAL;
> - }
> - sdom.period = scinfo->period;
> - }
> -
> - if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> - if (scinfo->budget < 1) {
> - LOG(ERROR, "VCPU budget is not set or out of range, "
> - "valid values are larger than 1");
> - return ERROR_INVAL;
> - }
> - sdom.budget = scinfo->budget;
> - }
> -
> - if (sdom.budget > sdom.period) {
> - LOG(ERROR, "VCPU budget is larger than VCPU period, "
> - "VCPU budget should be no larger than VCPU period");
> - return ERROR_INVAL;
> - }
> + rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
> + &sdom.period, &sdom.budget);
> + if (rc == ERROR_INVAL)
> + return rc;
>
> rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
> if (rc < 0) {
> @@ -5899,6 +5994,30 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> return ret;
> }
>
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + GC_INIT(ctx);
> + libxl_scheduler sched = scinfo->sched;
> + int ret;
> +
> + if (sched == LIBXL_SCHEDULER_UNKNOWN)
> + sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (sched) {
> + case LIBXL_SCHEDULER_RTDS:
> + ret=sched_rtds_vcpu_set(gc, domid, scinfo);
> + break;
> + default:
> + LOG(ERROR, "Unknown scheduler");
> + ret=ERROR_INVAL;
> + break;
> + }
> +
> + GC_FREE;
> + return ret;
> +}
> +
> int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> libxl_domain_sched_params *scinfo)
> {
> @@ -5932,6 +6051,30 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> return ret;
> }
>
> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + GC_INIT(ctx);
> + int ret;
> +
> + libxl_vcpu_sched_params_init(scinfo);
> +
> + scinfo->sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (scinfo->sched) {
> + case LIBXL_SCHEDULER_RTDS:
> + ret=sched_rtds_vcpu_get(gc, domid, scinfo);
> + break;
> + default:
> + LOG(ERROR, "Unknown scheduler");
> + ret=ERROR_INVAL;
> + break;
> + }
> +
> + GC_FREE;
> + return ret;
> +}
> +
> static int libxl__domain_s3_resume(libxl__gc *gc, int domid)
> {
> int rc = 0;
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 44bd8e2..e565814 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -192,6 +192,18 @@
> * is not present, instead of ERROR_INVAL.
> */
> #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
> +
> +/*
> + * libxl_vcpu_sched_params is used to get/set per-vcpu params
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * libxl_rtds_vcpu is used to represent the rtds scheduling params
> + * of a vcpu
> +*/
> +#define LIBXL_HAVE_RTDS_VCPU 1
> +
> /*
> * libxl ABI compatibility
> *
> @@ -1460,10 +1472,17 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
> #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
> #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1
>
> +/*RTDS Per-VCPU parameters*/
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1
> +
> int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> libxl_domain_sched_params *params);
> int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> const libxl_domain_sched_params *params);
> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> + libxl_vcpu_sched_params *params);
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> + const libxl_vcpu_sched_params *params);
>
> int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
> libxl_trigger trigger, uint32_t vcpuid);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 117b61d..d28d274 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
> ("checkpointed_stream", integer),
> ])
>
> +libxl_rtds_vcpu = Struct("rtds_vcpu",[
> + ("period", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> + ("budget", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> + ("vcpuid", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> + ])
> +
> +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> + ("sched", libxl_scheduler),
> + ("vcpus", Array(libxl_rtds_vcpu, "num_vcpus")),
> + ])
> +
> libxl_domain_sched_params = Struct("domain_sched_params",[
> ("sched", libxl_scheduler),
> ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-02 12:53 ` George Dunlap
@ 2015-06-02 14:10 ` Chong Li
0 siblings, 0 replies; 15+ messages in thread
From: Chong Li @ 2015-06-02 14:10 UTC (permalink / raw)
To: George Dunlap
Cc: Chong Li, Wei Liu, Sisu Xi, dario.faggioli, Ian Jackson,
xen-devel, Ian Campbell, Meng Xu, Dagaen Golomb
On Tue, Jun 2, 2015 at 7:53 AM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 05/26/2015 01:09 AM, Chong Li wrote:
>> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support
>> per-VCPU settings for RTDS scheduler.
>>
>> Add a new data structure (libxl_vcpu_sched_params) to help per-VCPU settings.
>>
>> Signed-off-by: Chong Li <chong.li@wustl.edu>
>> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>
> This doesn't apply cleanly for me anymore -- can you refresh and resend?
>
> Thanks,
> -George
>
Yes, sure. I'm working on the comments that have already been posted
in this series and will send out a new version soon.
Chong
>> ---
>> tools/libxl/libxl.c | 189 ++++++++++++++++++++++++++++++++++++++------
>> tools/libxl/libxl.h | 19 +++++
>> tools/libxl/libxl_types.idl | 11 +++
>> 3 files changed, 196 insertions(+), 23 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index feb3aa9..169901a 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -5797,6 +5797,120 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
>> return 0;
>> }
>>
>> +static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period,
>> + uint32_t budget, uint32_t *sdom_period,
>> + uint32_t *sdom_budget)
>> +{
>> + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
>> + if (period < 1) {
>> + LOG(ERROR, "VCPU period is not set or out of range, "
>> + "valid values are larger than 1");
>> + return ERROR_INVAL;
>> + }
>> + *sdom_period = period;
>> + }
>> +
>> + if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
>> + if (budget < 1) {
>> + LOG(ERROR, "VCPU budget is not set or out of range, "
>> + "valid values are larger than 1");
>> + return ERROR_INVAL;
>> + }
>> + *sdom_budget = budget;
>> + }
>> +
>> + if (budget > period) {
>> + LOG(ERROR, "VCPU budget is larger than VCPU period, "
>> + "VCPU budget should be no larger than VCPU period");
>> + return ERROR_INVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
>> + libxl_vcpu_sched_params *scinfo)
>> +{
>> + uint16_t num_vcpus;
>> + int rc, i;
>> + xc_dominfo_t info;
>> +
>> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
>> + if (rc < 0) {
>> + LOGE(ERROR, "getting domain info");
>> + return ERROR_FAIL;
>> + }
>> + num_vcpus = info.max_vcpu_id + 1;
>> +
>> + struct xen_domctl_sched_rtds_params *sdom = libxl__malloc(NOGC,
>> + sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus);
>> + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus);
>> + if (rc != 0) {
>> + LOGE(ERROR, "getting vcpu sched rtds");
>> + return ERROR_FAIL;
>> + }
>> +
>> + libxl_vcpu_sched_params_init(scinfo);
>> +
>> + scinfo->sched = LIBXL_SCHEDULER_RTDS;
>> + scinfo->num_vcpus = num_vcpus;
>> + scinfo->vcpus = (libxl_rtds_vcpu *)
>> + libxl__malloc(NOGC, sizeof(libxl_rtds_vcpu) * num_vcpus);
>> + for(i = 0; i < num_vcpus; i++) {
>> + scinfo->vcpus[i].period = sdom[i].period;
>> + scinfo->vcpus[i].budget = sdom[i].budget;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
>> + const libxl_vcpu_sched_params *scinfo)
>> +{
>> + int rc;
>> + int i;
>> + uint16_t num_vcpus;
>> + int vcpuid;
>> + uint32_t budget, period;
>> + xc_dominfo_t info;
>> +
>> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
>> + if (rc < 0) {
>> + LOGE(ERROR, "getting domain info");
>> + return ERROR_FAIL;
>> + }
>> + num_vcpus = info.max_vcpu_id + 1;
>> +
>> + struct xen_domctl_sched_rtds_params *sdom =
>> + libxl__malloc(NOGC, scinfo->num_vcpus);
>> + for (i = 0; i < scinfo->num_vcpus; i++) {
>> + vcpuid = scinfo->vcpus[i].vcpuid;
>> + budget = scinfo->vcpus[i].budget;
>> + period = scinfo->vcpus[i].period;
>> + if (vcpuid < 0 || vcpuid >= num_vcpus) {
>> + LOG(ERROR, "VCPU index is out of range, "
>> + "valid values are within range from 0 to %d",
>> + num_vcpus);
>> + return ERROR_INVAL;
>> + }
>> + sdom[i].vcpuid = vcpuid;
>> +
>> + rc = sched_rtds_validate_params(gc, period, budget,
>> + &sdom[i].period, &sdom[i].budget);
>> + if (rc == ERROR_INVAL)
>> + return rc;
>> + }
>> +
>> + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
>> + sdom, scinfo->num_vcpus);
>> + if (rc != 0) {
>> + LOGE(ERROR, "setting vcpu sched rtds");
>> + return ERROR_FAIL;
>> + }
>> +
>> + return rc;
>> +}
>> +
>> static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid,
>> libxl_domain_sched_params *scinfo)
>> {
>> @@ -5830,29 +5944,10 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
>> return ERROR_FAIL;
>> }
>>
>> - if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
>> - if (scinfo->period < 1) {
>> - LOG(ERROR, "VCPU period is not set or out of range, "
>> - "valid values are larger than 1");
>> - return ERROR_INVAL;
>> - }
>> - sdom.period = scinfo->period;
>> - }
>> -
>> - if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
>> - if (scinfo->budget < 1) {
>> - LOG(ERROR, "VCPU budget is not set or out of range, "
>> - "valid values are larger than 1");
>> - return ERROR_INVAL;
>> - }
>> - sdom.budget = scinfo->budget;
>> - }
>> -
>> - if (sdom.budget > sdom.period) {
>> - LOG(ERROR, "VCPU budget is larger than VCPU period, "
>> - "VCPU budget should be no larger than VCPU period");
>> - return ERROR_INVAL;
>> - }
>> + rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
>> + &sdom.period, &sdom.budget);
>> + if (rc == ERROR_INVAL)
>> + return rc;
>>
>> rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
>> if (rc < 0) {
>> @@ -5899,6 +5994,30 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>> return ret;
>> }
>>
>> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>> + const libxl_vcpu_sched_params *scinfo)
>> +{
>> + GC_INIT(ctx);
>> + libxl_scheduler sched = scinfo->sched;
>> + int ret;
>> +
>> + if (sched == LIBXL_SCHEDULER_UNKNOWN)
>> + sched = libxl__domain_scheduler(gc, domid);
>> +
>> + switch (sched) {
>> + case LIBXL_SCHEDULER_RTDS:
>> + ret=sched_rtds_vcpu_set(gc, domid, scinfo);
>> + break;
>> + default:
>> + LOG(ERROR, "Unknown scheduler");
>> + ret=ERROR_INVAL;
>> + break;
>> + }
>> +
>> + GC_FREE;
>> + return ret;
>> +}
>> +
>> int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>> libxl_domain_sched_params *scinfo)
>> {
>> @@ -5932,6 +6051,30 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>> return ret;
>> }
>>
>> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>> + libxl_vcpu_sched_params *scinfo)
>> +{
>> + GC_INIT(ctx);
>> + int ret;
>> +
>> + libxl_vcpu_sched_params_init(scinfo);
>> +
>> + scinfo->sched = libxl__domain_scheduler(gc, domid);
>> +
>> + switch (scinfo->sched) {
>> + case LIBXL_SCHEDULER_RTDS:
>> + ret=sched_rtds_vcpu_get(gc, domid, scinfo);
>> + break;
>> + default:
>> + LOG(ERROR, "Unknown scheduler");
>> + ret=ERROR_INVAL;
>> + break;
>> + }
>> +
>> + GC_FREE;
>> + return ret;
>> +}
>> +
>> static int libxl__domain_s3_resume(libxl__gc *gc, int domid)
>> {
>> int rc = 0;
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 44bd8e2..e565814 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -192,6 +192,18 @@
>> * is not present, instead of ERROR_INVAL.
>> */
>> #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
>> +
>> +/*
>> + * libxl_vcpu_sched_params is used to get/set per-vcpu params
>> +*/
>> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
>> +
>> +/*
>> + * libxl_rtds_vcpu is used to represent the rtds scheduling params
>> + * of a vcpu
>> +*/
>> +#define LIBXL_HAVE_RTDS_VCPU 1
>> +
>> /*
>> * libxl ABI compatibility
>> *
>> @@ -1460,10 +1472,17 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
>> #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
>> #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1
>>
>> +/*RTDS Per-VCPU parameters*/
>> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1
>> +
>> int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>> libxl_domain_sched_params *params);
>> int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>> const libxl_domain_sched_params *params);
>> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>> + libxl_vcpu_sched_params *params);
>> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
>> + const libxl_vcpu_sched_params *params);
>>
>> int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
>> libxl_trigger trigger, uint32_t vcpuid);
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 117b61d..d28d274 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
>> ("checkpointed_stream", integer),
>> ])
>>
>> +libxl_rtds_vcpu = Struct("rtds_vcpu",[
>> + ("period", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>> + ("budget", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
>> + ("vcpuid", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
>> + ])
>> +
>> +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
>> + ("sched", libxl_scheduler),
>> + ("vcpus", Array(libxl_rtds_vcpu, "num_vcpus")),
>> + ])
>> +
>> libxl_domain_sched_params = Struct("domain_sched_params",[
>> ("sched", libxl_scheduler),
>> ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>>
>
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-05-26 0:09 [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler Chong Li
2015-06-02 12:53 ` George Dunlap
@ 2015-06-04 23:48 ` Dario Faggioli
2015-06-05 11:37 ` Ian Campbell
2 siblings, 0 replies; 15+ messages in thread
From: Dario Faggioli @ 2015-06-04 23:48 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, ian.jackson,
xen-devel, ian.campbell, mengxu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 10455 bytes --]
On Mon, 2015-05-25 at 19:09 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support
> per-VCPU settings for RTDS scheduler.
>
> Add a new data structure (libxl_vcpu_sched_params) to help per-VCPU settings.
>
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> ---
>
For v3, don't forget the summary of what changed between v2 and v3 that
Jan also asked, please. :-)
> tools/libxl/libxl.c | 189 ++++++++++++++++++++++++++++++++++++++------
> tools/libxl/libxl.h | 19 +++++
> tools/libxl/libxl_types.idl | 11 +++
> 3 files changed, 196 insertions(+), 23 deletions(-)
>
The subject says 'XL', but then you are only touching libxl. I probably
see what you meant, and in fact, the 'libxl:' prefix is correct. Just
don't mention xl at all... there could be users of libxl different than
xl, and you're enabling all of them to use per-vcpu parameters, not just
xl. :-)
> +static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period,
> + uint32_t budget, uint32_t *sdom_period,
> + uint32_t *sdom_budget)
> +{
>
This is better, thanks. One thing I'm not sure about...
> + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> + if (period < 1) {
> + LOG(ERROR, "VCPU period is not set or out of range, "
> + "valid values are larger than 1");
> + return ERROR_INVAL;
> + }
> + *sdom_period = period;
> + }
> +
... is whether I like it or not for this to have side effects. It's
rather easy to spot that from the prototype, I know, but probably, I'd
prefer if a function called *_validate_* would actually just do the
validation.
> + if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> + if (budget < 1) {
> + LOG(ERROR, "VCPU budget is not set or out of range, "
> + "valid values are larger than 1");
> + return ERROR_INVAL;
>
It's a utility function, and you never return its return value directly,
so you can just go with 0==error, 1==ok.
> + }
> + *sdom_budget = budget;
> + }
> +
> + if (budget > period) {
> + LOG(ERROR, "VCPU budget is larger than VCPU period, "
> + "VCPU budget should be no larger than VCPU period");
> + return ERROR_INVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + uint16_t num_vcpus;
> + int rc, i;
> + xc_dominfo_t info;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> + num_vcpus = info.max_vcpu_id + 1;
> +
>
And as in the hypervisor, get and set are asymmetrical: get reads
everything, set operates on a well defined subset.
Symmetry is a value here too, IMO. So, please, make *both* _get and _set
able to deal with (sparse?) clusters of VCPU.
> + struct xen_domctl_sched_rtds_params *sdom = libxl__malloc(NOGC,
> + sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus);
>
As I said already, please, don't mix variable definition and code. This
belongs above, next to all the others, perhaps initialized to NULL, if
you need it.
BTW, do we really need this? I still haven't looked at the libxc patch,
but can't we make things such as this can be done 'in place'?
> + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus);
> + if (rc != 0) {
> + LOGE(ERROR, "getting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + libxl_vcpu_sched_params_init(scinfo);
> +
> + scinfo->sched = LIBXL_SCHEDULER_RTDS;
> + scinfo->num_vcpus = num_vcpus;
> + scinfo->vcpus = (libxl_rtds_vcpu *)
> + libxl__malloc(NOGC, sizeof(libxl_rtds_vcpu) * num_vcpus);
> + for(i = 0; i < num_vcpus; i++) {
> + scinfo->vcpus[i].period = sdom[i].period;
> + scinfo->vcpus[i].budget = sdom[i].budget;
> + }
> +
>
You're using scinfo for reporting the parameters back, so what about
sdom? It looks to me that you're using it internally and leaking it,
while (provided it's really necessary), you should free it, or let the
automatic garbage collector do so.
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> + int i;
> + uint16_t num_vcpus;
> + int vcpuid;
> + uint32_t budget, period;
> + xc_dominfo_t info;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> + num_vcpus = info.max_vcpu_id + 1;
> +
Why doing the +1 and translating this from vcpu ID to number-of, if then
all you do with the result is comparing it with IDs?
> + struct xen_domctl_sched_rtds_params *sdom =
> + libxl__malloc(NOGC, scinfo->num_vcpus);
>
Definition mixed with code.
I also don't like sdom as a name for the variable here... 'vcpus',
perhaps? But most important, why NOGC? This is internal only, right? I
mean you're preparing the array for libxc and pass it there, but you
never hand it to the caller, AFAICT. Therefore, you should go with an
allocation which is automatically garbage collected (see GCNEW_ARRAY(),
as already pointed out), and use GC_INIT and GC_FREE to properly make
that happen.
> + for (i = 0; i < scinfo->num_vcpus; i++) {
> + vcpuid = scinfo->vcpus[i].vcpuid;
> + budget = scinfo->vcpus[i].budget;
> + period = scinfo->vcpus[i].period;
> + if (vcpuid < 0 || vcpuid >= num_vcpus) {
>
vcpuid should probably be of unsigned type, when defining the struct.
This will save you the '< 0' part.
> + LOG(ERROR, "VCPU index is out of range, "
> + "valid values are within range from 0 to %d",
> + num_vcpus);
> + return ERROR_INVAL;
>
And in fact, you're leaking the memory allocated for sdom, here and on
every exit path. You really should use GCNEW_ARRAY, and then use the
'goto out:' pattern for exit/error handling (look around, there are
thousands of examples :-D ).
> + }
> + sdom[i].vcpuid = vcpuid;
> +
> + rc = sched_rtds_validate_params(gc, period, budget,
> + &sdom[i].period, &sdom[i].budget);
> + if (rc == ERROR_INVAL)
>
You don't need to check for an exact match with ERROR_INVAL, especially
considering that the helper, right now in this patch, if returning
non-0, is _only_ returning ERROR_INVAL (or did I miss something?).
If you simplify the helper as I suggested, this will become just:
if (!rc)
Or, even better:
if (!sched_rtds_validate_params(...))
> + return rc;
> + }
> +
> + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
> + sdom, scinfo->num_vcpus);
> + if (rc != 0) {
> + LOGE(ERROR, "setting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + return rc;
> +}
> +
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + GC_INIT(ctx);
> + libxl_scheduler sched = scinfo->sched;
> + int ret;
> +
> + if (sched == LIBXL_SCHEDULER_UNKNOWN)
> + sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (sched) {
> + case LIBXL_SCHEDULER_RTDS:
> + ret=sched_rtds_vcpu_set(gc, domid, scinfo);
>
Coding style.
> + break;
> + default:
> + LOG(ERROR, "Unknown scheduler");
> + ret=ERROR_INVAL;
>
And again.
> + break;
> + }
> +
> + GC_FREE;
> + return ret;
> +}
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 44bd8e2..e565814 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -192,6 +192,18 @@
> * is not present, instead of ERROR_INVAL.
> */
> #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
> +
> +/*
> + * libxl_vcpu_sched_params is used to get/set per-vcpu params
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * libxl_rtds_vcpu is used to represent the rtds scheduling params
> + * of a vcpu
> +*/
> +#define LIBXL_HAVE_RTDS_VCPU 1
> +
>
I don't think you need 2. Also, I'd make the whole thing more generic
(see below), with RTDS being the only scheduler implementing this new
interface.
> /*
> * libxl ABI compatibility
> *
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 117b61d..d28d274 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
> ("checkpointed_stream", integer),
> ])
>
> +libxl_rtds_vcpu = Struct("rtds_vcpu",[
> + ("period", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> + ("budget", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> + ("vcpuid", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> + ])
> +
> +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> + ("sched", libxl_scheduler),
> + ("vcpus", Array(libxl_rtds_vcpu, "num_vcpus")),
> + ])
> +
> libxl_domain_sched_params = Struct("domain_sched_params",[
> ("sched", libxl_scheduler),
> ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>
I'd do something similar to what I suggested for Xen interface already.
I.e, I'd put together something that can accommodate per-vcpu parameters
for all schedulers and use it.
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.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: 181 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] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-05-26 0:09 [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler Chong Li
2015-06-02 12:53 ` George Dunlap
2015-06-04 23:48 ` Dario Faggioli
@ 2015-06-05 11:37 ` Ian Campbell
2015-06-08 15:56 ` Dario Faggioli
2 siblings, 1 reply; 15+ messages in thread
From: Ian Campbell @ 2015-06-05 11:37 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
ian.jackson, xen-devel, mengxu, dgolomb
On Mon, 2015-05-25 at 19:09 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support
> per-VCPU settings for RTDS scheduler.
>
> Add a new data structure (libxl_vcpu_sched_params) to help per-VCPU settings.
>
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> ---
> tools/libxl/libxl.c | 189 ++++++++++++++++++++++++++++++++++++++------
> tools/libxl/libxl.h | 19 +++++
> tools/libxl/libxl_types.idl | 11 +++
> 3 files changed, 196 insertions(+), 23 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index feb3aa9..169901a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5797,6 +5797,120 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
> return 0;
> }
>
> +static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period,
> + uint32_t budget, uint32_t *sdom_period,
> + uint32_t *sdom_budget)
> +{
> + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT is -1, while period appears to
be unsigned, so I think think this will ever be false.
This means you will be setting the period to 2^32 if the default is
requested.
> + if (period < 1) {
> + LOG(ERROR, "VCPU period is not set or out of range, "
> + "valid values are larger than 1");
According to the check they are 1 or greater i.e. 1 is considered valid
while the message suggests that 2 is the first valid value.
> + return ERROR_INVAL;
> + }
> + *sdom_period = period;
> + }
> +
> + if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
Likewise.
> + if (budget < 1) {
> + LOG(ERROR, "VCPU budget is not set or out of range, "
> + "valid values are larger than 1");
Likewise.
> + return ERROR_INVAL;
> + }
> + *sdom_budget = budget;
> + }
> +
> + if (budget > period) {
> + LOG(ERROR, "VCPU budget is larger than VCPU period, "
> + "VCPU budget should be no larger than VCPU period");
The second half of this statement is redundant I think.
> + return ERROR_INVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + uint16_t num_vcpus;
> + int rc, i;
> + xc_dominfo_t info;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> + num_vcpus = info.max_vcpu_id + 1;
> +
> + struct xen_domctl_sched_rtds_params *sdom = libxl__malloc(NOGC,
^ one space only please.
> + sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus);
GCNEW_ARRAY, I think.
Please define sdom at the top (but init it here)
Also, I think you leak sdom here, since it is not returned to the user
it should be gc'd.
> + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus);
> + if (rc != 0) {
> + LOGE(ERROR, "getting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + libxl_vcpu_sched_params_init(scinfo);
> +
> + scinfo->sched = LIBXL_SCHEDULER_RTDS;
> + scinfo->num_vcpus = num_vcpus;
> + scinfo->vcpus = (libxl_rtds_vcpu *)
> + libxl__malloc(NOGC, sizeof(libxl_rtds_vcpu) * num_vcpus);
You don't need to cast the result of malloc (in C at least)
Also libxl__calloc is better for allocating arrays, but as above you
want GCNEW_ARRAY anyway.
NOGC is correct for this allocation.
> + for(i = 0; i < num_vcpus; i++) {
> + scinfo->vcpus[i].period = sdom[i].period;
> + scinfo->vcpus[i].budget = sdom[i].budget;
> + }
> +
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> + int i;
> + uint16_t num_vcpus;
> + int vcpuid;
> + uint32_t budget, period;
> + xc_dominfo_t info;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> + num_vcpus = info.max_vcpu_id + 1;
> +
> + struct xen_domctl_sched_rtds_params *sdom =
> + libxl__malloc(NOGC, scinfo->num_vcpus);
GCNEW_ARRAY, and not NOGC, define sdom at the top please and the same
double space issue in the type definition as before too.
> + for (i = 0; i < scinfo->num_vcpus; i++) {
> + vcpuid = scinfo->vcpus[i].vcpuid;
Isn't scinfo->vcpus[i].vcpuid unsigned, while the local var is an int.
> + rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
> + &sdom.period, &sdom.budget);
While this is ok, we normally align the parameters under each other,
i.e. align the second line with "gc" in this case. There are a few of
these here.
> + if (rc == ERROR_INVAL)
> + return rc;
>
> rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
> if (rc < 0) {
> @@ -5899,6 +5994,30 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> return ret;
> }
>
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + GC_INIT(ctx);
> + libxl_scheduler sched = scinfo->sched;
> + int ret;
> +
> + if (sched == LIBXL_SCHEDULER_UNKNOWN)
> + sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (sched) {
> + case LIBXL_SCHEDULER_RTDS:
> + ret=sched_rtds_vcpu_set(gc, domid, scinfo);
Spaces around = please (in several places)
> + break;
> + default:
> + LOG(ERROR, "Unknown scheduler");
> + ret=ERROR_INVAL;
I think unknown scheduler is distinct from a scheduler having no
per-vcpu parameters.
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 117b61d..d28d274 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
> ("checkpointed_stream", integer),
> ])
>
> +libxl_rtds_vcpu = Struct("rtds_vcpu",[
> + ("period", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> + ("budget", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> + ("vcpuid", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> + ])
> +
> +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> + ("sched", libxl_scheduler),
> + ("vcpus", Array(libxl_rtds_vcpu, "num_vcpus")),
How will we handle a second scheduler with per-vcpu parameters being
added in the future without breaking the API?
> + ])
> +
> libxl_domain_sched_params = Struct("domain_sched_params",[
> ("sched", libxl_scheduler),
> ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-05 11:37 ` Ian Campbell
@ 2015-06-08 15:56 ` Dario Faggioli
2015-06-08 20:55 ` Chong Li
0 siblings, 1 reply; 15+ messages in thread
From: Dario Faggioli @ 2015-06-08 15:56 UTC (permalink / raw)
To: Ian Campbell
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, ian.jackson,
xen-devel, mengxu, Chong Li, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 5683 bytes --]
On Fri, 2015-06-05 at 12:37 +0100, Ian Campbell wrote:
> On Mon, 2015-05-25 at 19:09 -0500, Chong Li wrote:
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 117b61d..d28d274 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
> > ("checkpointed_stream", integer),
> > ])
> >
> > +libxl_rtds_vcpu = Struct("rtds_vcpu",[
> > + ("period", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> > + ("budget", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> > + ("vcpuid", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> > + ])
> > +
> > +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> > + ("sched", libxl_scheduler),
> > + ("vcpus", Array(libxl_rtds_vcpu, "num_vcpus")),
>
> How will we handle a second scheduler with per-vcpu parameters being
> added in the future without breaking the API?
>
Yeah, this is an issue here, as it is at the hypercall level. In my own
review of this patch, I said "do something similar to the suggested
hypervisor interface".
Let me try to be a bit more specific.
Ideally, I'd like this to look sort of as follows (let's call this
'option 0'):
libxl_sched_params = Struct("sched_params",[
("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
])
libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
("sched", libxl_scheduler),
("vcpus", Array(libxl_sched_params, "num_vcpus")),
])
libxl_domain_sched_params = Struct("domain_sched_params",[
("sched", libxl_scheduler),
("schedparams", libxl_sched_params),
])
But this would mean breaking the API too much, I guess (see
libxl_domain_sched_params being completely redefined, the LIBXL_*_PARAMS
constants being changed, etc.)
So, I think I'd be fine with something like this (let's call this
'option 1'):
libxl_domain_sched_params = Struct("domain_sched_params",[ //left right as it is now!!
("sched", libxl_scheduler),
("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
("cap", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
("period", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
("slice", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),
("latency", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),
("extratime", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
("budget", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
])
libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
("vcpus", Array(libxl_domain_sched_params, "num_vcpus")),
])
The mix of domain and vcpu in the type (and hence) function names may
not appear ideal, but it's certainly not meaningless nor incorrect:
these are scheduling parameters for a domain (as opposed to parameters
global to a scheduler, like the ones in libxl_sched_credit_params), and
are (now) applied at a per-vcpu level for such domain.
There is some amount of redundancy, as the sched field, of
libxl_scheduler type, is repeated for all vcpus, but it's not possible
to use different schedulers for different vcpus, so it'll have to always
be the same, or just be unused, which is indeed rather ugly an API.
Alternatevily we can just add the per-vcpu stuff (as in 'option 0'), for
all schedulers, and really leave libxl_domain_sched_params alone (let's
call this 'option 2'):
libxl_sched_params = Struct("sched_params",[
("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
])
libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
("sched", libxl_scheduler),
("vcpus", Array(libxl_sched_params, "num_vcpus")),
])
In this case the redundancy comes from having libxl_domain_sched_params
and libxl_sched_params looking a lot similar, but not shared code in
declaring them.
Maybe we can also consider putting an union inside
libl_domain_sched_params... but again, quite a severe breakage, and I
wouldn't be sure about how to 'key it'...
So, Thoughts? What do you think the best way forward could be?
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.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: 181 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] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-08 15:56 ` Dario Faggioli
@ 2015-06-08 20:55 ` Chong Li
2015-06-09 16:18 ` Dario Faggioli
0 siblings, 1 reply; 15+ messages in thread
From: Chong Li @ 2015-06-08 20:55 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Ian Campbell, Sisu Xi, George Dunlap,
Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb
On Mon, Jun 8, 2015 at 10:56 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2015-06-05 at 12:37 +0100, Ian Campbell wrote:
>> On Mon, 2015-05-25 at 19:09 -0500, Chong Li wrote:
>
>> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> > index 117b61d..d28d274 100644
>> > --- a/tools/libxl/libxl_types.idl
>> > +++ b/tools/libxl/libxl_types.idl
>> > @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
>> > ("checkpointed_stream", integer),
>> > ])
>> >
>
> Alternatevily we can just add the per-vcpu stuff (as in 'option 0'), for
> all schedulers, and really leave libxl_domain_sched_params alone (let's
> call this 'option 2'):
>
> libxl_sched_params = Struct("sched_params",[
> ("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
> ("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
> ("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
> ("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
> ("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
> ("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
> ("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
> ])
>
> libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> ("sched", libxl_scheduler),
> ("vcpus", Array(libxl_sched_params, "num_vcpus")),
> ])
>
> In this case the redundancy comes from having libxl_domain_sched_params
> and libxl_sched_params looking a lot similar, but not shared code in
> declaring them.
>
> Maybe we can also consider putting an union inside
> libl_domain_sched_params... but again, quite a severe breakage, and I
> wouldn't be sure about how to 'key it'...
>
> So, Thoughts? What do you think the best way forward could be?
I like option 2 more. But I think we may also need a 'vcpuid' field in
libxl_sched_params.
Chong
>
> Regards,
> Dario
>
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-08 20:55 ` Chong Li
@ 2015-06-09 16:18 ` Dario Faggioli
2015-06-12 20:48 ` Chong Li
2015-06-17 12:08 ` Ian Campbell
0 siblings, 2 replies; 15+ messages in thread
From: Dario Faggioli @ 2015-06-09 16:18 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Wei Liu, Ian Campbell, Sisu Xi, George Dunlap,
Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 2745 bytes --]
On Mon, 2015-06-08 at 15:55 -0500, Chong Li wrote:
> On Mon, Jun 8, 2015 at 10:56 AM, Dario Faggioli
> > So, Thoughts? What do you think the best way forward could be?
>
> I like option 2 more. But I think we may also need a 'vcpuid' field in
> libxl_sched_params.
>
For sparse array support, yes. At which point, I would flip the names as
well, i.e., something like this:
libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
("vcpuid", integer, { xxx some init val xxx}),
("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
])
libxl_sched_params = Struct("sched_params",[
("sched", libxl_scheduler),
("vcpus", Array(libxl_sched_params, "num_vcpus")),
])
With the possibility of naming the latter 'libxl_vcpus_sched_params',
which is more descriptive, but perhaps is too similar to
libxl_vcpu_sched_params.
Ian, George, what do you think?
While we're here, another thing we would appreciate some feedback on is
what should happen to libxl_domain_sched_params_get(). This occurred to
my mind while reviewing patch 4 of this series. Actually, I think we've
discussed this before, but can't find the reference now.
Anyway, my view is that, for a scheduler that uses per-vcpu parameters,
libxl_domain_sched_params_set() should set the same parameters for all
the vcpus.
When it comes to _get(), however, I'm not sure. To match the _set()
case, we'd need to return the parameters of all the vcpus, but we can't,
because the function takes a libxl_domain_sched_params argument, which
just holds 1 tuple.
Should we just WARN and ask, when on that specific scheduler, to use the
per-vcpu variant being introduced in this patch
(libxl_vcpu_sched_params_get())?
This does not look ideal, but without changing the prototype of
libxl_domain_sched_params_get(), I don't see what else sensible we could
do... :-/
Should we change it, and do the LIBXL_API_VERSION "trick"?
So, again, thoughts?
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.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: 181 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] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-09 16:18 ` Dario Faggioli
@ 2015-06-12 20:48 ` Chong Li
2015-06-15 10:12 ` Dario Faggioli
2015-06-17 12:08 ` Ian Campbell
1 sibling, 1 reply; 15+ messages in thread
From: Chong Li @ 2015-06-12 20:48 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Ian Campbell, Sisu Xi, George Dunlap,
Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb
If no more feedbacks, let me summarize the design for the next version.
For "get" operations, we will implement the following features:
1) Use " xl sched-rtds -v all " to output the per-dom parameters of
all domains. And use, e.g., " xl sched-rtds -d vm1 -v all ", to output
the per-dom parameters of one specific domain. When a domain (say vm1)
has vcpus with different scheduling parameters but meanwhile the user
uses "xl sched-rtds -d vm1 -v all " to show the per-dom parameters,
the actual output result is just the parameters of vcpu with ID=0
(which is pointless, and should be made clear to the users).
These two kinds of "get" operations would be implemented through
libxl_domain_sched_params_get() and other domain-related functions (no
changes to all these functions).
2) For example, use " xl sched-rtds -d vm1 -v 0 -v 2 -v 4 " to show
the per-vcpu parameters of vcpu"0", vcpu"2" and vcpu"4" of vm1.
This kind of "get" operation would be implemented through
libxl_vcpu_sched_params_get() and other newly-added vcpu-related
functions.
For "set" operations, no new feature is added, against patch v2.
We need some new data structures to support per-vcpu operations (for
all schedulers, not just RTDS).
1) In libxl, we will introduce:
libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
("vcpuid", integer, { xxx some init val xxx}),
("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
])
libxl_sched_params = Struct("sched_params",[
("sched", libxl_scheduler),
("vcpus", Array(libxl_sched_params, "num_vcpus")),
])
and use libxl_sched_params to store and transfer vcpu array with
parameters to change/output.
2) In xen, we will introduce:
struct xen_domctl_scheduler_op {
uint32_t sched_id; /* XEN_SCHEDULER_* */
uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
union {
xen_domctl_schedparam_t d;
struct {
XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
uint16_t nr_vcpus;
} v;
} u;
};
typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_scheduler_op_t);
and some others (details can be found in
http://www.gossamer-threads.com/lists/xen/devel/380726?do=post_view_threaded
). Because of this new xen_domctl_scheduler_op_t, some changes have to
be done for credit and credit2 schedulers (for the
XEN_DOMCTL_scheduler_op processing there).
Please correct me if something is wrong.
Thanks,
Chong
On Tue, Jun 9, 2015 at 11:18 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Mon, 2015-06-08 at 15:55 -0500, Chong Li wrote:
>> On Mon, Jun 8, 2015 at 10:56 AM, Dario Faggioli
>
>> > So, Thoughts? What do you think the best way forward could be?
>>
>> I like option 2 more. But I think we may also need a 'vcpuid' field in
>> libxl_sched_params.
>>
> For sparse array support, yes. At which point, I would flip the names as
> well, i.e., something like this:
>
> libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> ("vcpuid", integer, { xxx some init val xxx}),
> ("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
> ("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
> ("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
> ("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
> ("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
> ("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
> ("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
> ])
>
> libxl_sched_params = Struct("sched_params",[
> ("sched", libxl_scheduler),
> ("vcpus", Array(libxl_sched_params, "num_vcpus")),
> ])
>
> With the possibility of naming the latter 'libxl_vcpus_sched_params',
> which is more descriptive, but perhaps is too similar to
> libxl_vcpu_sched_params.
>
> Ian, George, what do you think?
>
> While we're here, another thing we would appreciate some feedback on is
> what should happen to libxl_domain_sched_params_get(). This occurred to
> my mind while reviewing patch 4 of this series. Actually, I think we've
> discussed this before, but can't find the reference now.
>
> Anyway, my view is that, for a scheduler that uses per-vcpu parameters,
> libxl_domain_sched_params_set() should set the same parameters for all
> the vcpus.
> When it comes to _get(), however, I'm not sure. To match the _set()
> case, we'd need to return the parameters of all the vcpus, but we can't,
> because the function takes a libxl_domain_sched_params argument, which
> just holds 1 tuple.
>
> Should we just WARN and ask, when on that specific scheduler, to use the
> per-vcpu variant being introduced in this patch
> (libxl_vcpu_sched_params_get())?
>
> This does not look ideal, but without changing the prototype of
> libxl_domain_sched_params_get(), I don't see what else sensible we could
> do... :-/
>
> Should we change it, and do the LIBXL_API_VERSION "trick"?
>
> So, again, thoughts?
>
> Regards,
> Dario
>
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-12 20:48 ` Chong Li
@ 2015-06-15 10:12 ` Dario Faggioli
2015-06-17 12:14 ` Ian Campbell
0 siblings, 1 reply; 15+ messages in thread
From: Dario Faggioli @ 2015-06-15 10:12 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Wei Liu, Ian Campbell, Sisu Xi, George Dunlap,
Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 8850 bytes --]
On Fri, 2015-06-12 at 15:48 -0500, Chong Li wrote:
> If no more feedbacks, let me summarize the design for the next version.
>
> For "get" operations, we will implement the following features:
>
> 1) Use " xl sched-rtds -v all " to output the per-dom parameters of
> all domains. And use, e.g., " xl sched-rtds -d vm1 -v all ", to output
> the per-dom parameters of one specific domain.
>
Well, I said already that I think the distinction between per-dom and
per-vcpu parameters is meaningless, if done this way.
A parameter is either per-domain or per-vcpu, no matter how the user try
to set or get it. In RTDS, all parameters are per-domain now and, with
your work, all of them are becoming per-vcpu, and that's ok. But then,
per-dom parameters should just no longer exist.
This IMO should look as follows:
# sched-rtds -d vm1 --> (a) outputs nothing (no per-domain params)
(b) outputs params for all vcpus
# sched-rtds -d vm1 -v 2 --> outputs params for vcpu 2
# sched-rtds -d vm1 -v all --> outputs params for all vcpus
# sched-rtds -d vm1 -b 100 -p 1000 --> (c) errors out, as there's no
per-dom param!
(d) sets params for all vcpus
# sched-rtds -d vm1 -v all -b 100 -p 1000 --> sets params for all vcpus
# sched-rtds -d vm1 -v 2 -b 100 -p 1000 --> sets params for vcpu 2
If, OTOH, e.g. in another scheduler, there are both kind of parameters,
then each set should be handled with its own cli params and library
interface.
For instance, let's assume that, for Credit1 and/or, we'll want to have
(implement, in case of Credit2) per-vcpu caps, but leave the weight
per-dom (which is not that unlikely). At the xl level, that should IMO
look as follows:
# sched-credit2 -d vm1 --> (e) outputs per-dom params only (i.e., the
weight)
(f) outputs per-dom params (weight) and the
per-vcpu params (the cap) for all vcpus
# sched-credit2 -d vm1 -v 2 --> outputs cap of vcpu 2
# sched-credit2 -d vm1 -v all --> outputs caps of all vcpus
# sched-credit2 -d vm1 -w 128 --> set the weight to 128
# sched-credit2 -d vm1 -v 2 -c 25 --> set the cap to 25% for vcpu 2
# sched-credit2 -d vm1 -v 2 -w 128 --> errors out, weight is per-dom!
# sched-credit2 -d vm1 -v all -c 25 --> set the cap to 25% for all
vcpus
# sched-credit2 -d vm1 -c 25 --> (g) errors out, cap is per-vcpu!
(h) sets the cap to 25% for all vcpus
And, for consistency, I'd go either with (a), (c), (e) and (g) OR with
(b), (d), (f) and (h).
> When a domain (say vm1)
> has vcpus with different scheduling parameters but meanwhile the user
> uses "xl sched-rtds -d vm1 -v all " to show the per-dom parameters,
> the actual output result is just the parameters of vcpu with ID=0
> (which is pointless, and should be made clear to the users).
>
No, this just does not make any sense to me, even if you tell it to the
user. This is especially true at the xl level, where we can do pretty
much what we want, by calling the proper libxl functions.
> These two kinds of "get" operations would be implemented through
> libxl_domain_sched_params_get() and other domain-related functions (no
> changes to all these functions).
>
This is the most tricky part, IMO, because of the stability
requirements, and I reiterate my request for feedback from George and
the tools' maintainers.
My view is the one I stated above (with all the differences due to the
fact that we are in a library and not in a command line tool). But
still, I think that the distinction between per-vcpu and per-domain
parameters should hit the library rather explicitly.
> We need some new data structures to support per-vcpu operations (for
> all schedulers, not just RTDS).
>
> 1) In libxl, we will introduce:
>
> libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> ("vcpuid", integer, { xxx some init val xxx}),
> ("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
> ("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
> ("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
> ("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
> ("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
> ("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
> ("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
> ])
>
> libxl_sched_params = Struct("sched_params",[
> ("sched", libxl_scheduler),
> ("vcpus", Array(libxl_sched_params, "num_vcpus")),
> ])
>
This would allow, at some point, to turn all the existing scheduling
parameters of all the existing schedulers into per-vcpu parameters. This
may actually be a good thing, as, for instance (sticking to the example
above) if at some point we decide to also support per-vcpu weights, in
Credit1 and Credit2, the API won't have to change.
But then, in the implementation, you'll have, e.g., to deal with the
situation where someone tries to set the weight of a vcpu in Credit1
(and error out). Whether you do it in libxl, or rely on Xen telling
libxl that such thing is forbidden and having libxl propagate the error,
it's probably not a great deal. I'd recommend putting sanity checks in
libxl, though.
You'll have to do something similar for per-domain parameters in any
case, st least for the get side. In fact, we just can't touch:
libxl_domain_sched_params = Struct("domain_sched_params",[
("sched", libxl_scheduler),
("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
("cap", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
("period", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
("slice", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),
("latency", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),
("extratime", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
("budget", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
])
So, if for the set case, you can decide to loop through all the vcpus
_inside_ libxl, when one calls libxl_domain_sched_params_get() under
RTDS, where there are no per-domain parameters, and since you can't
return an array of per-vcpu parameters, you need to error out. This is a
change in the behavior of libxl_domain_sched_params_get(), and it's what
I'm less sure of about this interface I'm proposing, but it's how I'd do
it.
The alternative would be to avoid exposing per-domain only parameters in
the new libxl_vcpu_sched_params struct (which would mean, as far as your
work is concerned, only putting RTDS stuff in there). That would make
the API 'cleaner', for now, perhaps, but would require breaking it again
in future (e.g., it's rather likely that we will want per-vcpu caps in
Credit 1 and 2, sooner rather than later).
So, yes, I'd go for what you're showing above, but handle it as I
explained. Thoughts?
> 2) In xen, we will introduce:
>
> struct xen_domctl_scheduler_op {
> uint32_t sched_id; /* XEN_SCHEDULER_* */
> uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
> union {
> xen_domctl_schedparam_t d;
> struct {
> XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
> uint16_t nr_vcpus;
> } v;
> } u;
> };
> typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_scheduler_op_t);
>
Again, this allows for all parameters of all schedulers to be per-vcpu.
It may look too broad, considering that, for now, only a small subset of
them are, but it makes sense to me for this interface to be generic...
Then, inside the scheduling and the parameter handling code, you'll
enforce the proper semantic.
> Please correct me if something is wrong.
>
BTW, thanks for this summary... It's important that we agree on what we
want, in order to avoid re-doing things too many times! :-)
What I tried to describe is what I think fits best, let me know if
there's something that is not clear.
Maintainers, your views?
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.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: 181 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] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-09 16:18 ` Dario Faggioli
2015-06-12 20:48 ` Chong Li
@ 2015-06-17 12:08 ` Ian Campbell
2015-06-17 12:32 ` Dario Faggioli
1 sibling, 1 reply; 15+ messages in thread
From: Ian Campbell @ 2015-06-17 12:08 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
xen-devel, Meng Xu, Chong Li, Dagaen Golomb
On Tue, 2015-06-09 at 18:18 +0200, Dario Faggioli wrote:
> On Mon, 2015-06-08 at 15:55 -0500, Chong Li wrote:
> > On Mon, Jun 8, 2015 at 10:56 AM, Dario Faggioli
>
> > > So, Thoughts? What do you think the best way forward could be?
> >
> > I like option 2 more. But I think we may also need a 'vcpuid' field in
> > libxl_sched_params.
> >
> For sparse array support, yes. At which point, I would flip the names as
> well, i.e., something like this:
>
> libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> ("vcpuid", integer, { xxx some init val xxx}),
> ("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
> ("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
> ("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
> ("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
> ("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
> ("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
> ("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
Is a full new set of defaults really necessary? I don't think it would
be semantically all that strange to say that an unspecified per-vcpu
value will default to the domain default, at which point having
LIBXL_DOMAIN_SCHED_PARAM_FOO_DEFAULT mentioned doesn't seem all that
strange.
> ])
>
> libxl_sched_params = Struct("sched_params",[
> ("sched", libxl_scheduler),
> ("vcpus", Array(libxl_sched_params, "num_vcpus")),
> ])
>
> With the possibility of naming the latter 'libxl_vcpus_sched_params',
> which is more descriptive, but perhaps is too similar to
> libxl_vcpu_sched_params.
I think I'd go the other way and name the thing with all the values in
it "libxl_sched_params" and the thing with the per-vcpu array in it
"libxl_vcpu_sched_params".
> Ian, George, what do you think?
>
> While we're here, another thing we would appreciate some feedback on is
> what should happen to libxl_domain_sched_params_get(). This occurred to
> my mind while reviewing patch 4 of this series. Actually, I think we've
> discussed this before, but can't find the reference now.
>
> Anyway, my view is that, for a scheduler that uses per-vcpu parameters,
> libxl_domain_sched_params_set() should set the same parameters for all
> the vcpus.
> When it comes to _get(), however, I'm not sure. To match the _set()
> case, we'd need to return the parameters of all the vcpus, but we can't,
> because the function takes a libxl_domain_sched_params argument, which
> just holds 1 tuple.
Wouldn't domain_get/set be manipulating the domain's default values for
things, i.e. what a vcpu gets if nothing more specific is specified? Or
is it too late to say that?
For set overall I don't really think it matters too much if it sets
everything (i..e all vcpus and the defaults) so long as is a documented
effect of the API -- anyone who adds per-vcpu support would then be
aware of this and can adjust their code accordingly.
For get I think saying it returns the defaults used for vcpus which
don't have something more explicit set is perfectly fine and doesn't
pose an upgrade wrinkle, since only those who are aware of the vcpu
settings would care about the distinction.
> Should we just WARN and ask, when on that specific scheduler, to use the
> per-vcpu variant being introduced in this patch
> (libxl_vcpu_sched_params_get())?
>
> This does not look ideal, but without changing the prototype of
> libxl_domain_sched_params_get(), I don't see what else sensible we could
> do... :-/
>
> Should we change it, and do the LIBXL_API_VERSION "trick"?
>
> So, again, thoughts?
>
> Regards,
> Dario
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-15 10:12 ` Dario Faggioli
@ 2015-06-17 12:14 ` Ian Campbell
2015-06-17 12:26 ` Dario Faggioli
0 siblings, 1 reply; 15+ messages in thread
From: Ian Campbell @ 2015-06-17 12:14 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
xen-devel, Meng Xu, Chong Li, Dagaen Golomb
On Mon, 2015-06-15 at 12:12 +0200, Dario Faggioli wrote:
> A parameter is either per-domain or per-vcpu, no matter how the user try
> to set or get it. In RTDS, all parameters are per-domain now and, with
> your work, all of them are becoming per-vcpu, and that's ok. But then,
> per-dom parameters should just no longer exist.
Are you saying there is going to be no domain wide default for a given
per-vcpu parameter?
if that is the case then what happens if I hotplug a new VCPU without
settings its per-vcpu properties?
I expected something like.
domain_params_set (weight=10)
add vcpu0 (weight will be 10)
add vcpu1 (weight will be 10)
vcpu_params_set(0, weight=20)
add vcpu2 (weight will be 10, from domain wide default)
If not that then what weight should vcpu2 have at this point?
Ian.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-17 12:14 ` Ian Campbell
@ 2015-06-17 12:26 ` Dario Faggioli
0 siblings, 0 replies; 15+ messages in thread
From: Dario Faggioli @ 2015-06-17 12:26 UTC (permalink / raw)
To: Ian Campbell
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
xen-devel, Meng Xu, Chong Li, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 1509 bytes --]
On Wed, 2015-06-17 at 13:14 +0100, Ian Campbell wrote:
> On Mon, 2015-06-15 at 12:12 +0200, Dario Faggioli wrote:
> > A parameter is either per-domain or per-vcpu, no matter how the user try
> > to set or get it. In RTDS, all parameters are per-domain now and, with
> > your work, all of them are becoming per-vcpu, and that's ok. But then,
> > per-dom parameters should just no longer exist.
>
> Are you saying there is going to be no domain wide default for a given
> per-vcpu parameter?
>
No, no, there sure is a default, and I certainly would make it domain
wide.
> if that is the case then what happens if I hotplug a new VCPU without
> settings its per-vcpu properties?
>
Exactly.
> I expected something like.
>
> domain_params_set (weight=10)
> add vcpu0 (weight will be 10)
> add vcpu1 (weight will be 10)
> vcpu_params_set(0, weight=20)
> add vcpu2 (weight will be 10, from domain wide default)
>
> If not that then what weight should vcpu2 have at this point?
>
Absolutely.
What I'm aiming at here is the per-domain API to have a well defined and
valuable behavior, and the one you suggest in your other email (i.e.,
let's return the default) is exactly that! :-)
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.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: 181 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] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-17 12:08 ` Ian Campbell
@ 2015-06-17 12:32 ` Dario Faggioli
2015-06-17 15:47 ` Chong Li
0 siblings, 1 reply; 15+ messages in thread
From: Dario Faggioli @ 2015-06-17 12:32 UTC (permalink / raw)
To: Ian Campbell
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
xen-devel, Meng Xu, Chong Li, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 4156 bytes --]
On Wed, 2015-06-17 at 13:08 +0100, Ian Campbell wrote:
> On Tue, 2015-06-09 at 18:18 +0200, Dario Faggioli wrote:
> > On Mon, 2015-06-08 at 15:55 -0500, Chong Li wrote:
> > > On Mon, Jun 8, 2015 at 10:56 AM, Dario Faggioli
> >
> > > > So, Thoughts? What do you think the best way forward could be?
> > >
> > > I like option 2 more. But I think we may also need a 'vcpuid' field in
> > > libxl_sched_params.
> > >
> > For sparse array support, yes. At which point, I would flip the names as
> > well, i.e., something like this:
> >
> > libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> > ("vcpuid", integer, { xxx some init val xxx}),
> > ("weight", integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
> > ("cap", integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
> > ("period", integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
> > ("slice", integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
> > ("latency", integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
> > ("extratime", integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
> > ("budget", integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
>
> Is a full new set of defaults really necessary?
>
No, it's not... I was 'sketching' the various alternatives and just went
too far. :-)
> I don't think it would
> be semantically all that strange to say that an unspecified per-vcpu
> value will default to the domain default, at which point having
> LIBXL_DOMAIN_SCHED_PARAM_FOO_DEFAULT mentioned doesn't seem all that
> strange.
>
Indeed.
> > libxl_sched_params = Struct("sched_params",[
> > ("sched", libxl_scheduler),
> > ("vcpus", Array(libxl_sched_params, "num_vcpus")),
> > ])
> >
> > With the possibility of naming the latter 'libxl_vcpus_sched_params',
> > which is more descriptive, but perhaps is too similar to
> > libxl_vcpu_sched_params.
>
> I think I'd go the other way and name the thing with all the values in
> it "libxl_sched_params" and the thing with the per-vcpu array in it
> "libxl_vcpu_sched_params".
>
Fine with me.
> > While we're here, another thing we would appreciate some feedback on is
> > what should happen to libxl_domain_sched_params_get(). This occurred to
> > my mind while reviewing patch 4 of this series. Actually, I think we've
> > discussed this before, but can't find the reference now.
> >
> > Anyway, my view is that, for a scheduler that uses per-vcpu parameters,
> > libxl_domain_sched_params_set() should set the same parameters for all
> > the vcpus.
> > When it comes to _get(), however, I'm not sure. To match the _set()
> > case, we'd need to return the parameters of all the vcpus, but we can't,
> > because the function takes a libxl_domain_sched_params argument, which
> > just holds 1 tuple.
>
> Wouldn't domain_get/set be manipulating the domain's default values for
> things, i.e. what a vcpu gets if nothing more specific is specified? Or
> is it too late to say that?
>
I actually like this. A lot.
> For set overall I don't really think it matters too much if it sets
> everything (i..e all vcpus and the defaults) so long as is a documented
> effect of the API -- anyone who adds per-vcpu support would then be
> aware of this and can adjust their code accordingly.
>
Exactly. Personally, I think I'd make per-domain set behave
consistently, and hence use it for set the default.
> For get I think saying it returns the defaults used for vcpus which
> don't have something more explicit set is perfectly fine and doesn't
> pose an upgrade wrinkle, since only those who are aware of the vcpu
> settings would care about the distinction.
>
As said, I like this, great idea.
Chong, what do you think? Can you make this happen?
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.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: 181 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] 15+ messages in thread
* Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
2015-06-17 12:32 ` Dario Faggioli
@ 2015-06-17 15:47 ` Chong Li
0 siblings, 0 replies; 15+ messages in thread
From: Chong Li @ 2015-06-17 15:47 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Ian Campbell, Sisu Xi, George Dunlap,
Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb
On Wed, Jun 17, 2015 at 7:32 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Wed, 2015-06-17 at 13:08 +0100, Ian Campbell wrote:
>> On Tue, 2015-06-09 at 18:18 +0200, Dario Faggioli wrote:
>> > On Mon, 2015-06-08 at 15:55 -0500, Chong Li wrote:
>> > > On Mon, Jun 8, 2015 at 10:56 AM, Dario Faggioli
>> >
>
>> For set overall I don't really think it matters too much if it sets
>> everything (i..e all vcpus and the defaults) so long as is a documented
>> effect of the API -- anyone who adds per-vcpu support would then be
>> aware of this and can adjust their code accordingly.
>>
> Exactly. Personally, I think I'd make per-domain set behave
> consistently, and hence use it for set the default.
>
>> For get I think saying it returns the defaults used for vcpus which
>> don't have something more explicit set is perfectly fine and doesn't
>> pose an upgrade wrinkle, since only those who are aware of the vcpu
>> settings would care about the distinction.
>>
> As said, I like this, great idea.
>
> Chong, what do you think? Can you make this happen?
I like this design, too. Will make it happen soon.
Thanks for these feedbacks.
Chong
>
> Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-06-17 15:47 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-26 0:09 [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler Chong Li
2015-06-02 12:53 ` George Dunlap
2015-06-02 14:10 ` Chong Li
2015-06-04 23:48 ` Dario Faggioli
2015-06-05 11:37 ` Ian Campbell
2015-06-08 15:56 ` Dario Faggioli
2015-06-08 20:55 ` Chong Li
2015-06-09 16:18 ` Dario Faggioli
2015-06-12 20:48 ` Chong Li
2015-06-15 10:12 ` Dario Faggioli
2015-06-17 12:14 ` Ian Campbell
2015-06-17 12:26 ` Dario Faggioli
2015-06-17 12:08 ` Ian Campbell
2015-06-17 12:32 ` Dario Faggioli
2015-06-17 15:47 ` Chong Li
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.