* [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler
@ 2015-07-11 4:52 Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 1/4] xen: enable " Chong Li
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Chong Li @ 2015-07-11 4:52 UTC (permalink / raw)
To: xen-devel
Cc: Chong Li, wei.liu2, george.dunlap, dario.faggioli, ian.jackson,
ian.campbell, mengxu, jbeulich, lichong659, dgolomb
[Goal]
The current xl sched-rtds tool can only set the VCPUs of a domain
to the same parameter although the scheduler supports VCPUs with
different parameters. This patchset is to enable xl sched-rtds
tool to configure the VCPUs of a domain with different parameters.
This per-VCPU settings can be used in many scenarios. For example,
based on Dario's statement in our pervious discussion
(http://lists.xen.org/archives/html/xen-devel/2014-09/msg00423.html),
if there are two real-time applications, which have different timing
requirements, running in a multi-VCPU guest domain, it is beneficial
to pin these two applications to two seperate VCPUs with different
scheduling parameters.
What this patchset includes is a wanted and planned feature for RTDS
scheudler(http://wiki.xenproject.org/wiki/RTDS-Based-Scheduler) in
Xen 4.6. The interface design of the xl sched-rtds tool is based on
Meng's previous discussion with Dario, George and Wei
(http://lists.xen.org/archives/html/xen-devel/2015-02/msg02606.html).
Basically, there are two main changes:
1) in xl, we create an array that records all VCPUs whose parameters
are about to modify or output.
2) in libxl, we receive the array and call different xc functions to
handle it.
3) in xen and libxc, we use
XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo(introduced by this
patchset) as the hypercall for per-VCPU operations(get/set method).
[Usage]
With this patchset in use, xl sched-rtds tool can:
1) show the budget and period of each VCPU of each domain,
by using "xl sched-rtds -v all" command. An example would be like:
# xl sched-rtds -v all
Cpupool Pool-0: sched=RTDS
Name ID VCPU Period Budget
Domain-0 0 0 10000 4000
vm1 1 0 300 150
vm1 1 1 400 200
vm1 1 2 10000 4000
vm1 1 3 1000 500
vm2 2 0 10000 4000
vm2 2 1 10000 4000
Using "xl sched-rtds" will output the default scheduling parameters
for each domain. An example would be like:
# xl sched-rtds
Cpupool Pool-0: sched=RTDS
Name ID Period Budget
Domain-0 0 10000 4000
vm1 1 10000 4000
vm2 2 10000 4000
2) show the budget and period of each VCPU of a specific domain,
by using, e.g., "xl sched-rtds -d vm1 -v all" command. The output
would be like:
# xl sched-rtds -d vm1 -v all
Name ID VCPU Period Budget
vm1 1 0 300 150
vm1 1 1 400 200
vm1 1 2 10000 4000
vm1 1 3 1000 500
To show a subset of the parameters of the VCPUs of a specific domain,
please use, e.g.,"xl sched-rtds -d vm1 -v 0 -v 3" command.
The output would be:
# xl sched-rtds -d vm1 -v 0 -v 3
Name ID VCPU Period Budget
vm1 1 0 300 150
vm1 1 3 1000 500
Using command, e.g., "xl sched-rtds -d vm1" will output the default
scheduling parameters of vm1. An example would be like:
# xl sched-rtds -d vm1
Name ID Period Budget
vm1 1 10000 4000
3) Users can set the budget and period of multiple VCPUs of a
specific domain with only one command,
e.g., "xl sched-rtds -d vm1 -v 0 -p 100 -b 50 -v 3 -p 300 -b 150".
Users can set all VCPUs with the same parameters, by one command.
e.g., "xl sched-rtds -d vm1 -v all -p 500 -b 250".
---
CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <jbeulich@suse.com>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
CC: <ian.jackson@eu.citrix.com>
CC: <ian.campbell@eu.citrix.com>
Chong Li (4):
xen: enable per-VCPU parameter settings for RTDS scheduler
libxc: enable per-VCPU parameter settings for RTDS scheduler
libxl: enable per-VCPU parameter settings for RTDS scheduler
xl: enable per-VCPU parameter settings for RTDS scheduler
docs/man/xl.pod.1 | 4 +
tools/libxc/include/xenctrl.h | 9 ++
tools/libxc/xc_rt.c | 58 ++++++++-
tools/libxl/libxl.c | 265 ++++++++++++++++++++++++++++++++++----
tools/libxl/libxl.h | 17 +++
tools/libxl/libxl_types.idl | 16 +++
tools/libxl/xl_cmdimpl.c | 292 ++++++++++++++++++++++++++++++++++++++----
tools/libxl/xl_cmdtable.c | 10 +-
xen/common/domctl.c | 3 +
xen/common/sched_rt.c | 100 ++++++++++++++-
xen/common/schedule.c | 18 ++-
xen/include/public/domctl.h | 61 ++++++---
12 files changed, 770 insertions(+), 83 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
@ 2015-07-11 4:52 ` Chong Li
2015-07-13 8:37 ` Jan Beulich
2015-07-27 15:51 ` Dario Faggioli
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 2/4] libxc: " Chong Li
` (4 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Chong Li @ 2015-07-11 4:52 UTC (permalink / raw)
To: xen-devel
Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, Meng Xu,
jbeulich, lichong659, dgolomb
Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
to independently get and set the scheduling parameters of each
vCPU of a domain
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>
---
Changes on PATCH v3:
1) Remove struct xen_domctl_schedparam_t.
2) Change struct xen_domctl_scheduler_op.
3) Check if period/budget is within a validated range
CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <jbeulich@suse.com>
CC: <lichong659@gmail.com>
---
xen/common/domctl.c | 3 ++
xen/common/sched_rt.c | 100 +++++++++++++++++++++++++++++++++++++++++---
xen/common/schedule.c | 18 ++++++--
xen/include/public/domctl.h | 61 +++++++++++++++++++--------
4 files changed, 155 insertions(+), 27 deletions(-)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 2a2d203..349f68b 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -839,6 +839,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
case XEN_DOMCTL_scheduler_op:
ret = sched_adjust(d, &op->u.scheduler_op);
+ if ( ret == -ERESTART )
+ ret = hypercall_create_continuation(
+ __HYPERVISOR_domctl, "h", u_domctl);
copyback = 1;
break;
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 4372486..fed9e09 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -78,7 +78,6 @@
* vcpu_insert, vcpu_remove, context_saved, __runq_insert
*/
-
/*
* Default parameters:
* Period and budget in default is 10 and 4 ms, respectively
@@ -86,6 +85,18 @@
#define RTDS_DEFAULT_PERIOD (MICROSECS(10000))
#define RTDS_DEFAULT_BUDGET (MICROSECS(4000))
+/*
+ * Max period: max delta of time type
+ * Min period: 100 us, considering the scheduling overhead
+ */
+#define RTDS_MAX_PERIOD (STIME_DELTA_MAX)
+#define RTDS_MIN_PERIOD (MICROSECS(100))
+
+/*
+ * Min budget: 100 us
+ */
+#define RTDS_MIN_BUDGET (MICROSECS(100))
+
#define UPDATE_LIMIT_SHIFT 10
#define MAX_SCHEDULE (MILLISECS(1))
/*
@@ -1137,14 +1148,17 @@ rt_dom_cntl(
struct list_head *iter;
unsigned long flags;
int rc = 0;
+ int warn = 0;
+ xen_domctl_schedparam_vcpu_t local_sched;
+ s_time_t period, budget;
+ unsigned int index;
switch ( op->cmd )
{
- case XEN_DOMCTL_SCHEDOP_getinfo:
+ case XEN_DOMCTL_SCHEDOP_getinfo: /* return the default parameters */
spin_lock_irqsave(&prv->lock, flags);
- svc = list_entry(sdom->vcpu.next, struct rt_vcpu, sdom_elem);
- op->u.rtds.period = svc->period / MICROSECS(1); /* transfer to us */
- op->u.rtds.budget = svc->budget / MICROSECS(1);
+ op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1); /* transfer to us */
+ op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
spin_unlock_irqrestore(&prv->lock, flags);
break;
case XEN_DOMCTL_SCHEDOP_putinfo:
@@ -1162,8 +1176,82 @@ rt_dom_cntl(
}
spin_unlock_irqrestore(&prv->lock, flags);
break;
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ spin_lock_irqsave(&prv->lock, flags);
+ for ( index = 0; index < op->u.v.nr_vcpus; index++ )
+ {
+ if ( copy_from_guest_offset(&local_sched,
+ op->u.v.vcpus, index, 1) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+ if ( local_sched.vcpuid >= d->max_vcpus ||
+ d->vcpu[local_sched.vcpuid] == NULL )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
+
+ local_sched.s.rtds.budget = svc->budget / MICROSECS(1);
+ local_sched.s.rtds.period = svc->period / MICROSECS(1);
+
+ if ( __copy_to_guest_offset(op->u.v.vcpus, index,
+ &local_sched, 1) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+ if( hypercall_preempt_check() )
+ {
+ rc = -ERESTART;
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&prv->lock, flags);
+ break;
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ spin_lock_irqsave(&prv->lock, flags);
+ for( index = 0; index < op->u.v.nr_vcpus; index++ )
+ {
+ if ( copy_from_guest_offset(&local_sched,
+ op->u.v.vcpus, index, 1) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+ if ( local_sched.vcpuid >= d->max_vcpus ||
+ d->vcpu[local_sched.vcpuid] == NULL )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
+ period = MICROSECS(local_sched.s.rtds.period);
+ budget = MICROSECS(local_sched.s.rtds.budget);
+ if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD ||
+ budget < MICROSECS(10) || budget > period )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ if ( period < RTDS_MIN_PERIOD || budget < RTDS_MIN_BUDGET )
+ warn = 1;
+
+ svc->period = period;
+ svc->budget = budget;
+ if( hypercall_preempt_check() )
+ {
+ rc = -ERESTART;
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&prv->lock, flags);
+ break;
}
-
+ if ( rc == 0 && warn == 1 ) /* print warning in libxl */
+ rc = 1;
return rc;
}
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index ecf1545..886e1b5 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1052,10 +1052,22 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
if ( ret )
return ret;
- if ( (op->sched_id != DOM2OP(d)->sched_id) ||
- ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) &&
- (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
+ if ( op->sched_id != DOM2OP(d)->sched_id )
return -EINVAL;
+ else
+ switch ( op->cmd )
+ {
+ case XEN_DOMCTL_SCHEDOP_putinfo:
+ break;
+ case XEN_DOMCTL_SCHEDOP_getinfo:
+ break;
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ break;
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ break;
+ default:
+ return -EINVAL;
+ }
/* NB: the pluggable scheduler code needs to take care
* of locking by itself. */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index bc45ea5..bfb432f 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -330,31 +330,56 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
#define XEN_SCHEDULER_ARINC653 7
#define XEN_SCHEDULER_RTDS 8
+typedef struct xen_domctl_sched_sedf {
+ uint64_aligned_t period;
+ uint64_aligned_t slice;
+ uint64_aligned_t latency;
+ uint32_t extratime;
+ uint32_t weight;
+} xen_domctl_sched_sedf_t;
+
+typedef struct xen_domctl_sched_credit {
+ uint16_t weight;
+ uint16_t cap;
+} xen_domctl_sched_credit_t;
+
+typedef struct xen_domctl_sched_credit2 {
+ uint16_t weight;
+} xen_domctl_sched_credit2_t;
+
+typedef struct xen_domctl_sched_rtds {
+ uint32_t period;
+ uint32_t budget;
+} xen_domctl_sched_rtds_t;
+
+typedef struct xen_domctl_schedparam_vcpu {
+ union {
+ xen_domctl_sched_credit_t credit;
+ xen_domctl_sched_credit2_t credit2;
+ xen_domctl_sched_rtds_t rtds;
+ } s;
+ uint16_t vcpuid;
+ uint16_t padding;
+} xen_domctl_schedparam_vcpu_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
+
/* Set or get info? */
#define XEN_DOMCTL_SCHEDOP_putinfo 0
#define XEN_DOMCTL_SCHEDOP_getinfo 1
+#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2
+#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3
struct xen_domctl_scheduler_op {
uint32_t sched_id; /* XEN_SCHEDULER_* */
uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
union {
- struct xen_domctl_sched_sedf {
- uint64_aligned_t period;
- uint64_aligned_t slice;
- uint64_aligned_t latency;
- uint32_t extratime;
- uint32_t weight;
- } sedf;
- struct xen_domctl_sched_credit {
- uint16_t weight;
- uint16_t cap;
- } credit;
- struct xen_domctl_sched_credit2 {
- uint16_t weight;
- } credit2;
- struct xen_domctl_sched_rtds {
- uint32_t period;
- uint32_t budget;
- } rtds;
+ xen_domctl_sched_sedf_t sedf;
+ xen_domctl_sched_credit_t credit;
+ xen_domctl_sched_credit2_t credit2;
+ xen_domctl_sched_rtds_t rtds;
+ 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;
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 for Xen 4.6 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 1/4] xen: enable " Chong Li
@ 2015-07-11 4:52 ` Chong Li
2015-07-27 16:11 ` Dario Faggioli
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 3/4] libxl: " Chong Li
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Chong Li @ 2015-07-11 4:52 UTC (permalink / raw)
To: xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
Meng Xu, lichong659, dgolomb
Add xc_sched_rtds_vcpu_get/set functions to interact with
Xen to get/set a domain's per-VCPU parameters.
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>
---
CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
---
tools/libxc/include/xenctrl.h | 9 +++++++
tools/libxc/xc_rt.c | 58 +++++++++++++++++++++++++++++++++++++++++--
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index d1d2ab3..58f1a7a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -915,6 +915,15 @@ int xc_sched_rtds_domain_get(xc_interface *xch,
uint32_t domid,
struct xen_domctl_sched_rtds *sdom);
+int xc_sched_rtds_vcpu_set(xc_interface *xch,
+ uint32_t domid,
+ xen_domctl_schedparam_vcpu_t *vcpus,
+ uint16_t num_vcpus);
+int xc_sched_rtds_vcpu_get(xc_interface *xch,
+ uint32_t domid,
+ xen_domctl_schedparam_vcpu_t *vcpus,
+ uint16_t num_vcpus);
+
int
xc_sched_arinc653_schedule_set(
xc_interface *xch,
diff --git a/tools/libxc/xc_rt.c b/tools/libxc/xc_rt.c
index b2d1cc5..257a962 100644
--- a/tools/libxc/xc_rt.c
+++ b/tools/libxc/xc_rt.c
@@ -27,7 +27,7 @@
int xc_sched_rtds_domain_set(xc_interface *xch,
uint32_t domid,
- struct xen_domctl_sched_rtds *sdom)
+ xen_domctl_sched_rtds_t *sdom)
{
int rc;
DECLARE_DOMCTL;
@@ -46,7 +46,7 @@ int xc_sched_rtds_domain_set(xc_interface *xch,
int xc_sched_rtds_domain_get(xc_interface *xch,
uint32_t domid,
- struct xen_domctl_sched_rtds *sdom)
+ xen_domctl_sched_rtds_t *sdom)
{
int rc;
DECLARE_DOMCTL;
@@ -63,3 +63,57 @@ int xc_sched_rtds_domain_get(xc_interface *xch,
return rc;
}
+
+int xc_sched_rtds_vcpu_set(xc_interface *xch,
+ uint32_t domid,
+ xen_domctl_schedparam_vcpu_t *vcpus,
+ uint16_t num_vcpus)
+{
+ int rc;
+ DECLARE_DOMCTL;
+ DECLARE_HYPERCALL_BOUNCE(vcpus, sizeof(*vcpus) * num_vcpus,
+ XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( xc_hypercall_bounce_pre(xch, vcpus) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_scheduler_op;
+ domctl.domain = (domid_t) domid;
+ domctl.u.scheduler_op.sched_id = XEN_SCHEDULER_RTDS;
+ domctl.u.scheduler_op.cmd = XEN_DOMCTL_SCHEDOP_putvcpuinfo;
+ domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus;
+ set_xen_guest_handle(domctl.u.scheduler_op.u.v.vcpus, vcpus);
+
+ rc = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, vcpus);
+
+ return rc;
+}
+
+int xc_sched_rtds_vcpu_get(xc_interface *xch,
+ uint32_t domid,
+ xen_domctl_schedparam_vcpu_t *vcpus,
+ uint16_t num_vcpus)
+{
+ int rc;
+ DECLARE_DOMCTL;
+ DECLARE_HYPERCALL_BOUNCE(vcpus, sizeof(*vcpus) * num_vcpus,
+ XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+ if ( xc_hypercall_bounce_pre(xch, vcpus) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_scheduler_op;
+ domctl.domain = (domid_t) domid;
+ domctl.u.scheduler_op.sched_id = XEN_SCHEDULER_RTDS;
+ domctl.u.scheduler_op.cmd = XEN_DOMCTL_SCHEDOP_getvcpuinfo;
+ domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus;
+ set_xen_guest_handle(domctl.u.scheduler_op.u.v.vcpus, vcpus);
+
+ rc = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, vcpus);
+
+ return rc;
+}
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 1/4] xen: enable " Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 2/4] libxc: " Chong Li
@ 2015-07-11 4:52 ` Chong Li
2015-07-28 9:15 ` Dario Faggioli
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 4/4] xl: " Chong Li
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Chong Li @ 2015-07-11 4:52 UTC (permalink / raw)
To: xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
ian.jackson, ian.campbell, Meng Xu, lichong659, dgolomb
Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
functions to support 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>
---
Changes on PATCH v3:
1) Add sanity check on vcpuid
2) Add comments on per-domain and per-vcpu functions for libxl
users
CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
CC: <ian.jackson@eu.citrix.com>
CC: <ian.campbell@eu.citrix.com>
---
tools/libxl/libxl.c | 265 ++++++++++++++++++++++++++++++++++++++++----
tools/libxl/libxl.h | 17 +++
tools/libxl/libxl_types.idl | 16 +++
3 files changed, 276 insertions(+), 22 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e9a2d26..9f7f859 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5854,6 +5854,156 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
return 0;
}
+static int sched_rtds_validate_params(libxl__gc *gc, int period,
+ int 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 or equal to 1");
+ return 1;
+ }
+ *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 or equal to 1");
+ return 1;
+ }
+ *sdom_budget = budget;
+ }
+
+ if (budget > period) {
+ LOG(ERROR, "VCPU budget is larger than VCPU period");
+ return 1;
+ }
+
+ 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;
+ xen_domctl_schedparam_vcpu_t *vcpus;
+
+ rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (rc < 0) {
+ LOGE(ERROR, "getting domain info");
+ return ERROR_FAIL;
+ }
+
+ if (scinfo->num_vcpus == 0)
+ num_vcpus = info.max_vcpu_id + 1;
+ else
+ num_vcpus = scinfo->num_vcpus;
+
+ GCNEW_ARRAY(vcpus, num_vcpus);
+
+ if (scinfo->num_vcpus > 0)
+ for (i=0; i < num_vcpus; i++) {
+ if (scinfo->vcpus[i].vcpuid < 0 ||
+ scinfo->vcpus[i].vcpuid > info.max_vcpu_id) {
+ LOG(ERROR, "VCPU index is out of range, "
+ "valid values are within range from 0 to %d",
+ info.max_vcpu_id);
+ return ERROR_INVAL;
+ }
+ vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
+ } else
+ for (i=0; i < num_vcpus; i++)
+ vcpus[i].vcpuid = i;
+
+ rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus);
+ if (rc != 0) {
+ LOGE(ERROR, "getting vcpu sched rtds");
+ return ERROR_FAIL;
+ }
+
+ scinfo->sched = LIBXL_SCHEDULER_RTDS;
+ if (scinfo->num_vcpus == 0) {
+ scinfo->num_vcpus = num_vcpus;
+ scinfo->vcpus = libxl__calloc(NOGC, num_vcpus, sizeof(libxl_sched_params));
+ }
+ for(i = 0; i < num_vcpus; i++) {
+ scinfo->vcpus[i].period = vcpus[i].s.rtds.period;
+ scinfo->vcpus[i].budget = vcpus[i].s.rtds.budget;
+ scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
+ }
+
+ 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 max_vcpuid;
+ xc_dominfo_t info;
+ xen_domctl_schedparam_vcpu_t *vcpus;
+ int num_vcpus;
+
+ rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (rc < 0) {
+ LOGE(ERROR, "getting domain info");
+ return ERROR_FAIL;
+ }
+ max_vcpuid = info.max_vcpu_id;
+
+ if (scinfo->num_vcpus > 0) {
+ num_vcpus = scinfo->num_vcpus;
+ GCNEW_ARRAY(vcpus, num_vcpus);
+ for (i = 0; i < num_vcpus; i++) {
+ if (scinfo->vcpus[i].vcpuid < 0 ||
+ scinfo->vcpus[i].vcpuid > max_vcpuid) {
+ LOG(ERROR, "VCPU index is out of range, "
+ "valid values are within range from 0 to %d",
+ max_vcpuid);
+ return ERROR_INVAL;
+ }
+ vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
+
+ rc = sched_rtds_validate_params(gc,
+ scinfo->vcpus[i].period, scinfo->vcpus[i].budget,
+ &vcpus[i].s.rtds.period, &vcpus[i].s.rtds.budget);
+ if (rc)
+ return ERROR_INVAL;
+ }
+ } else {
+ num_vcpus = max_vcpuid + 1;
+ GCNEW_ARRAY(vcpus, num_vcpus);
+ rc = sched_rtds_validate_params(gc,
+ scinfo->vcpus[0].period, scinfo->vcpus[0].budget,
+ &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget);
+ if (rc)
+ return ERROR_INVAL;
+ for (i = 0; i < num_vcpus; i++) {
+ vcpus[i].vcpuid = i;
+ vcpus[i].s.rtds.period = scinfo->vcpus[0].period;
+ vcpus[i].s.rtds.budget = scinfo->vcpus[0].budget;
+ }
+ }
+
+ rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
+ vcpus, num_vcpus);
+ if (rc == 1) {
+ printf("WARN: too small period or budget may "
+ "result in large scheduling overhead\n");
+ rc = 0;
+ } else 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)
{
@@ -5887,29 +6037,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");
+ rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
+ &sdom.period, &sdom.budget);
+ if (rc)
return ERROR_INVAL;
- }
rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
if (rc < 0) {
@@ -5920,6 +6051,11 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
return 0;
}
+/* Set the per-domain scheduling parameters.
+* For schedulers that support per-vcpu settings (e.g., RTDS),
+* calling *_domain_set functions will set all vcpus with the same
+* scheduling parameters.
+*/
int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
const libxl_domain_sched_params *scinfo)
{
@@ -5956,6 +6092,52 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
return ret;
}
+/* Set the per-vcpu scheduling parameters */
+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_SEDF:
+ LOG(ERROR, "No per-vcpu set function provided");
+ ret = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_CREDIT:
+ LOG(ERROR, "No per-vcpu set function provided");
+ ret = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_CREDIT2:
+ LOG(ERROR, "No per-vcpu set function provided");
+ ret = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_ARINC653:
+ LOG(ERROR, "No per-vcpu set function provided");
+ ret = ERROR_INVAL;
+ break;
+ 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;
+}
+
+/* Get the per-domain scheduling parameters.
+* For schedulers that support per-vcpu settings (e.g., RTDS),
+* calling *_domain_get functions will get default scheduling
+* parameters.
+*/
int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
@@ -5989,6 +6171,45 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
return ret;
}
+/* Get the per-vcpu scheduling parameters */
+int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
+ libxl_vcpu_sched_params *scinfo)
+{
+ GC_INIT(ctx);
+ int ret;
+
+ scinfo->sched = libxl__domain_scheduler(gc, domid);
+
+ switch (scinfo->sched) {
+ case LIBXL_SCHEDULER_SEDF:
+ LOG(ERROR, "No per-vcpu get function provided");
+ ret = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_CREDIT:
+ LOG(ERROR, "No per-vcpu get function provided");
+ ret = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_CREDIT2:
+ LOG(ERROR, "No per-vcpu get function provided");
+ ret = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_ARINC653:
+ LOG(ERROR, "No per-vcpu get function provided");
+ ret = ERROR_INVAL;
+ break;
+ 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 a1c5d15..040a248 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -200,6 +200,16 @@
#define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
/*
+ * libxl_vcpu_sched_params is used to store per-vcpu params
+*/
+#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
+
+/*
+ * libxl_sched_params is used to store the array of per-vcpu params
+*/
+#define LIBXL_HAVE_SCHED_PARAMS 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
@@ -1554,10 +1564,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
+/* Per-VCPU parameters*/
+#define LIBXL_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 e1632fa..ff70372 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -351,6 +351,22 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
("checkpointed_stream", integer),
])
+libxl_sched_params = Struct("sched_params",[
+ ("vcpuid", integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
+ ("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",[
+ ("sched", libxl_scheduler),
+ ("vcpus", Array(libxl_sched_params, "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] 22+ messages in thread
* [PATCH v4 for Xen 4.6 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
` (2 preceding siblings ...)
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 3/4] libxl: " Chong Li
@ 2015-07-11 4:52 ` Chong Li
2015-07-28 9:25 ` Dario Faggioli
2015-07-11 14:33 ` [PATCH v4 for Xen 4.6 0/4] Enable " Wei Liu
2015-07-27 15:14 ` Dario Faggioli
5 siblings, 1 reply; 22+ messages in thread
From: Chong Li @ 2015-07-11 4:52 UTC (permalink / raw)
To: xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
Meng Xu, lichong659, dgolomb
Change main_sched_rtds and related output functions to support
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>
---
Changes on PATCH v3:
1) Support commands, e.g., "xl sched-rtds -d vm1" to output the
default scheduling parameters
CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
---
docs/man/xl.pod.1 | 4 +
tools/libxl/xl_cmdimpl.c | 292 +++++++++++++++++++++++++++++++++++++++++-----
tools/libxl/xl_cmdtable.c | 10 +-
3 files changed, 274 insertions(+), 32 deletions(-)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 4eb929d..d35e169 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1093,6 +1093,10 @@ B<OPTIONS>
Specify domain for which scheduler parameters are to be modified or retrieved.
Mandatory for modifying scheduler parameters.
+=item B<-v VCPUID/all>, B<--vcpuid=VCPUID/all>
+
+Specify vcpu for which scheduler parameters are to be modified or retrieved.
+
=item B<-p PERIOD>, B<--period=PERIOD>
Period of time, in microseconds, over which to replenish the budget.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c858068..da7c8a6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5630,6 +5630,37 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
return rc;
}
+static int sched_vcpu_get(libxl_scheduler sched, int domid,
+ libxl_vcpu_sched_params *scinfo)
+{
+ int rc;
+
+ rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo);
+ if (rc) {
+ fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n");
+ return rc;
+ }
+ if (scinfo->sched != sched) {
+ fprintf(stderr, "libxl_vcpu_sched_params_get returned %s not %s.\n",
+ libxl_scheduler_to_string(scinfo->sched),
+ libxl_scheduler_to_string(sched));
+ return ERROR_INVAL;
+ }
+
+ return 0;
+}
+
+static int sched_vcpu_set(int domid, const libxl_vcpu_sched_params *scinfo)
+{
+ int rc;
+
+ rc = libxl_vcpu_sched_params_set(ctx, domid, scinfo);
+ if (rc)
+ fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n");
+
+ return rc;
+}
+
static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
{
int rc;
@@ -5786,6 +5817,38 @@ out:
return rc;
}
+static int sched_rtds_vcpu_output(
+ int domid, libxl_vcpu_sched_params *scinfo)
+{
+ char *domname;
+ int rc = 0;
+ int i;
+
+ if (domid < 0) {
+ printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
+ "VCPU", "Period", "Budget");
+ return 0;
+ }
+
+ rc = sched_vcpu_get(LIBXL_SCHEDULER_RTDS, domid, scinfo);
+ if (rc)
+ goto out;
+
+ domname = libxl_domid_to_name(ctx, domid);
+ for( i = 0; i < scinfo->num_vcpus; i++ ) {
+ printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
+ domname,
+ domid,
+ scinfo->vcpus[i].vcpuid,
+ scinfo->vcpus[i].period,
+ scinfo->vcpus[i].budget);
+ }
+ free(domname);
+
+out:
+ return rc;
+}
+
static int sched_rtds_pool_output(uint32_t poolid)
{
char *poolname;
@@ -5859,6 +5922,65 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int),
return 0;
}
+static int sched_vcpu_output(libxl_scheduler sched,
+ int (*output)(int, libxl_vcpu_sched_params *),
+ int (*pooloutput)(uint32_t), const char *cpupool)
+{
+ libxl_dominfo *info;
+ libxl_cpupoolinfo *poolinfo = NULL;
+ uint32_t poolid;
+ int nb_domain, n_pools = 0, i, p;
+ int rc = 0;
+
+ if (cpupool) {
+ if (libxl_cpupool_qualifier_to_cpupoolid(ctx, cpupool, &poolid, NULL) ||
+ !libxl_cpupoolid_is_valid(ctx, poolid)) {
+ fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool);
+ return -ERROR_FAIL;
+ }
+ }
+
+ info = libxl_list_domain(ctx, &nb_domain);
+ if (!info) {
+ fprintf(stderr, "libxl_list_domain failed.\n");
+ return 1;
+ }
+ poolinfo = libxl_list_cpupool(ctx, &n_pools);
+ if (!poolinfo) {
+ fprintf(stderr, "error getting cpupool info\n");
+ libxl_dominfo_list_free(info, nb_domain);
+ return -ERROR_NOMEM;
+ }
+
+ for (p = 0; !rc && (p < n_pools); p++) {
+ if ((poolinfo[p].sched != sched) ||
+ (cpupool && (poolid != poolinfo[p].poolid)))
+ continue;
+
+ pooloutput(poolinfo[p].poolid);
+
+ libxl_vcpu_sched_params scinfo_out;
+ libxl_vcpu_sched_params_init(&scinfo_out);
+ output(-1, &scinfo_out);
+ libxl_vcpu_sched_params_dispose(&scinfo_out);
+ for (i = 0; i < nb_domain; i++) {
+ if (info[i].cpupool != poolinfo[p].poolid)
+ continue;
+ libxl_vcpu_sched_params scinfo;
+ libxl_vcpu_sched_params_init(&scinfo);
+ scinfo.num_vcpus = 0;
+ rc = output(info[i].domid, &scinfo);
+ libxl_vcpu_sched_params_dispose(&scinfo);
+ if (rc)
+ break;
+ }
+ }
+
+ libxl_cpupoolinfo_list_free(poolinfo, n_pools);
+ libxl_dominfo_list_free(info, nb_domain);
+ return 0;
+}
+
/*
* <nothing> : List all domain params and sched params from all pools
* -d [domid] : List domain params for domain
@@ -6173,76 +6295,190 @@ int main_sched_rtds(int argc, char **argv)
{
const char *dom = NULL;
const char *cpupool = NULL;
- int period = 0; /* period is in microsecond */
- int budget = 0; /* budget is in microsecond */
+
+ int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs that change */
+ int *periods = (int *)xmalloc(sizeof(int)); /* period is in microsecond */
+ int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in microsecond */
+ int v_size = 1; /* size of vcpus array */
+ int p_size = 1; /* size of periods array */
+ int b_size = 1; /* size of budgets array */
+ int v_index = 0; /* index in vcpus array */
+ int p_index =0; /* index in periods array */
+ int b_index =0; /* index for in budgets array */
bool opt_p = false;
bool opt_b = false;
- int opt, rc;
+ bool opt_v = false;
+ bool opt_all = false; /* output per-dom parameters */
+ int opt, i;
+ int rc = 0;
static struct option opts[] = {
{"domain", 1, 0, 'd'},
{"period", 1, 0, 'p'},
{"budget", 1, 0, 'b'},
+ {"vcpuid",1, 0, 'v'},
{"cpupool", 1, 0, 'c'},
COMMON_LONG_OPTS,
{0, 0, 0, 0}
};
- SWITCH_FOREACH_OPT(opt, "d:p:b:c:h", opts, "sched-rtds", 0) {
+ SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) {
case 'd':
dom = optarg;
break;
case 'p':
- period = strtol(optarg, NULL, 10);
+ if (p_index > b_index || p_index > v_index) {
+ /* budget or vcpuID is missed */
+ fprintf(stderr, "Must specify period, budget and vcpuID\n");
+ rc = 1;
+ goto out;
+ }
+ if (p_index >= p_size) {
+ p_size *= 2;
+ periods = xrealloc(periods, p_size);
+ if (!periods) {
+ fprintf(stderr, "Failed to realloc periods\n");
+ rc = 1;
+ goto out;
+ }
+ }
+ periods[p_index++] = strtol(optarg, NULL, 10);
opt_p = 1;
break;
case 'b':
- budget = strtol(optarg, NULL, 10);
+ if (b_index > p_index || b_index > v_index) {
+ /* period or vcpuID is missed */
+ fprintf(stderr, "Must specify period, budget and vcpuID\n");
+ rc = 1;
+ goto out;
+ }
+ if (b_index >= b_size) {
+ b_size *= 2;
+ budgets = xrealloc(budgets, b_size);
+ if (!budgets) {
+ fprintf(stderr, "Failed to realloc budgets\n");
+ rc = 1;
+ goto out;
+ }
+ }
+ budgets[b_index++] = strtol(optarg, NULL, 10);
opt_b = 1;
break;
+ case 'v':
+ if (!strcmp(optarg, "all")) { /* output per-dom parameters*/
+ opt_all = 1;
+ break;
+ }
+ if (v_index >= v_size) {
+ v_size *= 2;
+ vcpus = xrealloc(vcpus, v_size);
+ if (!vcpus) {
+ fprintf(stderr, "Failed to realloc vcpus\n");
+ rc = 1;
+ goto out;
+ }
+ }
+ vcpus[v_index++] = strtol(optarg, NULL, 10);
+ opt_v = 1;
+ break;
case 'c':
cpupool = optarg;
break;
}
- if (cpupool && (dom || opt_p || opt_b)) {
+ if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) {
fprintf(stderr, "Specifying a cpupool is not allowed with "
"other options.\n");
- return 1;
+ rc = 1;
+ goto out;
}
- if (!dom && (opt_p || opt_b)) {
- fprintf(stderr, "Must specify a domain.\n");
- return 1;
+ if (!dom && (opt_p || opt_b || opt_v)) {
+ fprintf(stderr, "Missing parameters.\n");
+ rc = 1;
+ goto out;
}
- if (opt_p != opt_b) {
- fprintf(stderr, "Must specify period and budget\n");
- return 1;
+ if (dom && !opt_v && !opt_all && (opt_p || opt_b)) {
+ fprintf(stderr, "Must specify VCPU.\n");
+ rc = 1;
+ goto out;
+ }
+ if (opt_v && opt_all) {
+ fprintf(stderr, "Incorrect VCPU IDs.\n");
+ rc = 1;
+ goto out;
+ }
+ if (((v_index > b_index) && opt_b) || ((v_index > p_index) && opt_p)
+ || p_index != b_index) {
+ fprintf(stderr, "Incorrect number of period and budget\n");
+ rc = 1;
+ goto out;
}
- if (!dom) { /* list all domain's rt scheduler info */
- return -sched_domain_output(LIBXL_SCHEDULER_RTDS,
+ if ((!dom) && opt_all) { /* list all domain's rtds scheduler info */
+ rc = -sched_vcpu_output(LIBXL_SCHEDULER_RTDS,
+ sched_rtds_vcpu_output,
+ sched_rtds_pool_output,
+ cpupool);
+ goto out;
+ } else if (!dom && !opt_all) {
+ /* list all domain's default scheduling parameters */
+ rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS,
sched_rtds_domain_output,
sched_rtds_pool_output,
cpupool);
+ goto out;
} else {
uint32_t domid = find_domain(dom);
- if (!opt_p && !opt_b) { /* output rt scheduler info */
+ if (!opt_v && !opt_all) { /* output default scheduling parameters */
sched_rtds_domain_output(-1);
- return -sched_rtds_domain_output(domid);
- } else { /* set rt scheduler paramaters */
- libxl_domain_sched_params scinfo;
- libxl_domain_sched_params_init(&scinfo);
+ rc = -sched_rtds_domain_output(domid);
+ goto out;
+ } else if (!opt_p && !opt_b) { /* output rtds scheduler info */
+ libxl_vcpu_sched_params scinfo;
+ libxl_vcpu_sched_params_init(&scinfo);
+ sched_rtds_vcpu_output(-1, &scinfo);
+ scinfo.num_vcpus = v_index;
+ if (v_index > 0)
+ scinfo.vcpus = (libxl_sched_params *)
+ xmalloc(sizeof(libxl_sched_params) * (v_index));
+ for (i = 0; i < v_index; i++)
+ scinfo.vcpus[i].vcpuid = vcpus[i];
+ rc = -sched_rtds_vcpu_output(domid, &scinfo);
+ libxl_vcpu_sched_params_dispose(&scinfo);
+ goto out;
+ } else if (opt_v || opt_all) { /* set per-vcpu rtds scheduler parameters */
+ libxl_vcpu_sched_params scinfo;
+ libxl_vcpu_sched_params_init(&scinfo);
scinfo.sched = LIBXL_SCHEDULER_RTDS;
- scinfo.period = period;
- scinfo.budget = budget;
+ scinfo.num_vcpus = v_index;
+ if (v_index > 0) {
+ scinfo.vcpus = (libxl_sched_params *)
+ xmalloc(sizeof(libxl_sched_params) * (v_index));
+ for (i = 0; i < v_index; i++) {
+ scinfo.vcpus[i].vcpuid = vcpus[i];
+ scinfo.vcpus[i].period = periods[i];
+ scinfo.vcpus[i].budget = budgets[i];
+ }
+ } else { /* set params for all vcpus*/
+ scinfo.vcpus = (libxl_sched_params *)
+ xmalloc(sizeof(libxl_sched_params));
+ scinfo.vcpus[0].period = periods[0];
+ scinfo.vcpus[0].budget = budgets[0];
+ }
- rc = sched_domain_set(domid, &scinfo);
- libxl_domain_sched_params_dispose(&scinfo);
- if (rc)
- return -rc;
+ rc = sched_vcpu_set(domid, &scinfo);
+ libxl_vcpu_sched_params_dispose(&scinfo);
+ if (rc) {
+ rc = -rc;
+ goto out;
+ }
}
}
- return 0;
+out:
+ free(vcpus);
+ free(periods);
+ free(budgets);
+ return rc;
}
int main_domid(int argc, char **argv)
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 7f4759b..c38da55 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -284,10 +284,12 @@ struct cmd_spec cmd_table[] = {
{ "sched-rtds",
&main_sched_rtds, 0, 1,
"Get/set rtds scheduler parameters",
- "[-d <Domain> [-p[=PERIOD]] [-b[=BUDGET]]]",
- "-d DOMAIN, --domain=DOMAIN Domain to modify\n"
- "-p PERIOD, --period=PERIOD Period (us)\n"
- "-b BUDGET, --budget=BUDGET Budget (us)\n"
+ "[-d <Domain> [-v[=VCPUID]] [-p[=PERIOD]] [-b[=BUDGET]]]",
+ "-d DOMAIN, --domain=DOMAIN Domain to modify\n"
+ "-v VCPUID/all, --vcpuid=VCPUID/all VCPU to modify or output;\n"
+ " Using '-v all' to modify/output all vcpus\n"
+ "-p PERIOD, --period=PERIOD Period (us)\n"
+ "-b BUDGET, --budget=BUDGET Budget (us)\n"
},
{ "domid",
&main_domid, 0, 0,
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
` (3 preceding siblings ...)
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 4/4] xl: " Chong Li
@ 2015-07-11 14:33 ` Wei Liu
2015-07-13 10:27 ` Dario Faggioli
2015-07-27 15:14 ` Dario Faggioli
5 siblings, 1 reply; 22+ messages in thread
From: Wei Liu @ 2015-07-11 14:33 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, george.dunlap, dario.faggioli, ian.jackson,
xen-devel, ian.campbell, mengxu, jbeulich, dgolomb
Hi Chong
This series is marked as "for 4.6", but we just hit feature freeze
yesterday.
Given the status of this series (missing many acks), I am sorry to say
this series will have to wait until next release.
We will review this series in timely manner provided there are no other
urgent matters for the release. Please keep up with your good work.
Wei.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 1/4] xen: enable " Chong Li
@ 2015-07-13 8:37 ` Jan Beulich
2015-08-09 15:45 ` Chong Li
2015-07-27 15:51 ` Dario Faggioli
1 sibling, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2015-07-13 8:37 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, xen-devel,
Meng Xu, dgolomb
>>> On 11.07.15 at 06:52, <lichong659@gmail.com> wrote:
> @@ -1162,8 +1176,82 @@ rt_dom_cntl(
> }
> spin_unlock_irqrestore(&prv->lock, flags);
> break;
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + spin_lock_irqsave(&prv->lock, flags);
> + for ( index = 0; index < op->u.v.nr_vcpus; index++ )
> + {
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> +
> + local_sched.s.rtds.budget = svc->budget / MICROSECS(1);
> + local_sched.s.rtds.period = svc->period / MICROSECS(1);
> +
> + if ( __copy_to_guest_offset(op->u.v.vcpus, index,
> + &local_sched, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if( hypercall_preempt_check() )
> + {
> + rc = -ERESTART;
> + break;
> + }
I still don't see how this is supposed to work.
> + }
> + spin_unlock_irqrestore(&prv->lock, flags);
> + break;
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + spin_lock_irqsave(&prv->lock, flags);
> + for( index = 0; index < op->u.v.nr_vcpus; index++ )
> + {
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> + period = MICROSECS(local_sched.s.rtds.period);
> + budget = MICROSECS(local_sched.s.rtds.budget);
> + if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD ||
> + budget < MICROSECS(10) || budget > period )
Apart from numerous coding style issues I think the first of the
checks in this if() is redundant (covered by the combination of
the last two ones) and hence would better be dropped.
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1052,10 +1052,22 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
> if ( ret )
> return ret;
>
> - if ( (op->sched_id != DOM2OP(d)->sched_id) ||
> - ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) &&
> - (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
> + if ( op->sched_id != DOM2OP(d)->sched_id )
> return -EINVAL;
> + else
> + switch ( op->cmd )
> + {
> + case XEN_DOMCTL_SCHEDOP_putinfo:
> + break;
> + case XEN_DOMCTL_SCHEDOP_getinfo:
> + break;
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + break;
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + break;
Only this break should stay, the three earlier ones should be dropped
as redundant.
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -330,31 +330,56 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
> #define XEN_SCHEDULER_ARINC653 7
> #define XEN_SCHEDULER_RTDS 8
>
> +typedef struct xen_domctl_sched_sedf {
> + uint64_aligned_t period;
> + uint64_aligned_t slice;
> + uint64_aligned_t latency;
> + uint32_t extratime;
> + uint32_t weight;
> +} xen_domctl_sched_sedf_t;
> +
> +typedef struct xen_domctl_sched_credit {
> + uint16_t weight;
> + uint16_t cap;
> +} xen_domctl_sched_credit_t;
> +
> +typedef struct xen_domctl_sched_credit2 {
> + uint16_t weight;
> +} xen_domctl_sched_credit2_t;
> +
> +typedef struct xen_domctl_sched_rtds {
> + uint32_t period;
> + uint32_t budget;
> +} xen_domctl_sched_rtds_t;
> +
> +typedef struct xen_domctl_schedparam_vcpu {
> + union {
> + xen_domctl_sched_credit_t credit;
> + xen_domctl_sched_credit2_t credit2;
> + xen_domctl_sched_rtds_t rtds;
> + } s;
> + uint16_t vcpuid;
> + uint16_t padding;
This pads to a 32-bit boundary, leaving another 32-bit hole.
> +} xen_domctl_schedparam_vcpu_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
> +
> /* Set or get info? */
> #define XEN_DOMCTL_SCHEDOP_putinfo 0
> #define XEN_DOMCTL_SCHEDOP_getinfo 1
> +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2
> +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3
> struct xen_domctl_scheduler_op {
> uint32_t sched_id; /* XEN_SCHEDULER_* */
> uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
> union {
> - struct xen_domctl_sched_sedf {
> - uint64_aligned_t period;
> - uint64_aligned_t slice;
> - uint64_aligned_t latency;
> - uint32_t extratime;
> - uint32_t weight;
> - } sedf;
> - struct xen_domctl_sched_credit {
> - uint16_t weight;
> - uint16_t cap;
> - } credit;
> - struct xen_domctl_sched_credit2 {
> - uint16_t weight;
> - } credit2;
> - struct xen_domctl_sched_rtds {
> - uint32_t period;
> - uint32_t budget;
> - } rtds;
> + xen_domctl_sched_sedf_t sedf;
> + xen_domctl_sched_credit_t credit;
> + xen_domctl_sched_credit2_t credit2;
> + xen_domctl_sched_rtds_t rtds;
> + struct {
> + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
> + uint16_t nr_vcpus;
> + } v;
And there's still no explicit padding here at all (nor am I convinced
that uint16_t is really a good choice for nr_vcpus - uint32_t would
seem more natural without causing any problems or structure
growth).
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 14:33 ` [PATCH v4 for Xen 4.6 0/4] Enable " Wei Liu
@ 2015-07-13 10:27 ` Dario Faggioli
2015-07-14 5:45 ` Meng Xu
0 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2015-07-13 10:27 UTC (permalink / raw)
To: Wei Liu
Cc: Chong Li, george.dunlap, ian.jackson, xen-devel, ian.campbell,
mengxu, jbeulich, Chong Li, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 2462 bytes --]
On Sat, 2015-07-11 at 15:33 +0100, Wei Liu wrote:
> Hi Chong
>
> This series is marked as "for 4.6", but we just hit feature freeze
> yesterday.
>
Yeah, I wanted to reply myself about this, but Wei beat me... Good job
as release manager, I would say. :-)
> Given the status of this series (missing many acks), I am sorry to say
> this series will have to wait until next release.
>
Indeed. The series is starting to look good, and, Chong, you're doing a
great work, especially by replying promptly to reviews, and reposting
new versions very quickly.
However, this series arrived a bit late in the dev cycle, and suffered
from some delay in reviewing (from me as well, sorry for that), but
(both) this things happen in (Open Source) software development, and we
can't do much about it.
Also, the original goal was to pull RTDS out of experimental, but, even
with this series in, we wouldn't get to there as:
- not enough testing: it entered OSSTest not so long ago, which, e.g.,
showed up it's failing on ARM!
- not enough benchmarks/performance figures: I'd like to have the
latency numbers, e.g., from cyclictest, we've spoke many times with
Meng, give our official blessing at using it
- the work Dagaen's doing is a rather fundamental restructuring, and it
makes sense to do all the above (testing and performance evaluation)
on top of the result of that for a bit, before declaring things
stable and supported (or we risk disrupting that because of it, and
since it's already ongoing, I'll really let him finish)
So, for the following reasons (coming from the above reasoning):
- the series is good, but certainly still not ready;
- having the series in, would not change much wrt RTDS in 4.6
I, as the maintainer of this feature, agree with Wei that we should work
toward merging this series really soon... at the beginning of 4.7
development cycle! :-D
> We will review this series in timely manner provided there are no other
> urgent matters for the release. Please keep up with your good work.
>
Indeed. Thanks a log again to you, Meng, Dagaen, and everyone.
I'll review the series ASAP.
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler
2015-07-13 10:27 ` Dario Faggioli
@ 2015-07-14 5:45 ` Meng Xu
2015-07-14 7:13 ` Dario Faggioli
0 siblings, 1 reply; 22+ messages in thread
From: Meng Xu @ 2015-07-14 5:45 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, George Dunlap, Ian Jackson,
xen-devel@lists.xen.org, Ian Campbell, mengxu@cis.upenn.edu,
Jan Beulich, Chong Li, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 637 bytes --]
Hi Dario,
- not enough benchmarks/performance figures: I'd like to have the
> latency numbers, e.g., from cyclictest, we've spoke many times with
> Meng, give our official blessing at using it
Considering t
he improved version Dagaen is working on should be able to improve the
efficiency of the scheduler "a lot"
,
I will evaluate the performance of the RTDS scheduler with cyclictest once
Dagaen have the improved version done.
Thanks,
Meng
-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/
[-- Attachment #1.2: Type: text/html, Size: 1558 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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler
2015-07-14 5:45 ` Meng Xu
@ 2015-07-14 7:13 ` Dario Faggioli
0 siblings, 0 replies; 22+ messages in thread
From: Dario Faggioli @ 2015-07-14 7:13 UTC (permalink / raw)
To: Meng Xu
Cc: Chong Li, Wei Liu, George Dunlap, Ian Jackson,
xen-devel@lists.xen.org, Ian Campbell, mengxu@cis.upenn.edu,
Jan Beulich, Chong Li, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 968 bytes --]
On Mon, 2015-07-13 at 22:45 -0700, Meng Xu wrote:
> Hi Dario,
>
Hi,
> - not enough benchmarks/performance figures: I'd like to have
> the
> latency numbers, e.g., from cyclictest, we've spoke many
> times with
> Meng, give our official blessing at using it
>
>
> Considering t
> he improved version Dagaen is working on should be able to improve the
> efficiency of the scheduler "a lot"
> ,
> I will evaluate the performance of the RTDS scheduler with cyclictest
> once Dagaen have the improved version done.
>
Sure thing! It actually was part of my own reasoning about why we want
to wait. :-)
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
` (4 preceding siblings ...)
2015-07-11 14:33 ` [PATCH v4 for Xen 4.6 0/4] Enable " Wei Liu
@ 2015-07-27 15:14 ` Dario Faggioli
2015-08-07 15:50 ` Chong Li
5 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2015-07-27 15:14 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, george.dunlap, ian.jackson, xen-devel,
ian.campbell, mengxu, jbeulich, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 2187 bytes --]
On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> Using "xl sched-rtds" will output the default scheduling parameters
> for each domain. An example would be like:
>
> # xl sched-rtds
> Cpupool Pool-0: sched=RTDS
> Name ID Period Budget
> Domain-0 0 10000 4000
> vm1 1 10000 4000
> vm2 2 10000 4000
> Using command, e.g., "xl sched-rtds -d vm1" will output the default
> scheduling parameters of vm1. An example would be like:
>
> # xl sched-rtds -d vm1
> Name ID Period Budget
> vm1 1 10000 4000
>
Thanks for making these two works.
I think it would be great if we could, in this case, output somehow and
somewhere that we are actually showing some defaults, instead than
anything that is really in use.
I don't know whether with something like this:
# xl sched-rtds
Cpupool Pool-0: sched=RTDS
Name ID Period Budget
Domain-0 (defaults) 0 10000 4000
vm1 (defaults) 1 10000 4000
vm2 (defaults) 2 10000 4000
# xl sched-rtds -d vm1
Name ID Period Budget
vm1 (defaults) 1 10000 4000
Or by "just" adding a note before the actual output:
# xl sched-rtds -d vm1
Showing per-vm(s) default scheduling parameters,
use `-v' for seeing the actual parameters of each vcpu
Name ID Period Budget
vm1 1 10000 4000
The latter is more verbose, but I think is what I see as more useful
(and, probably, the easier to implement).
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 1/4] xen: enable " Chong Li
2015-07-13 8:37 ` Jan Beulich
@ 2015-07-27 15:51 ` Dario Faggioli
2015-08-09 16:08 ` Chong Li
1 sibling, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2015-07-27 15:51 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, xen-devel, Meng Xu, jbeulich,
dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 10356 bytes --]
On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
> to independently get and set the scheduling parameters of each
> vCPU of a domain
>
I'd add a note about the fact that, for now, this is only supported and
being implemented for RTDs.
> 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>
>
> ---
> Changes on PATCH v3:
>
> 1) Remove struct xen_domctl_schedparam_t.
>
> 2) Change struct xen_domctl_scheduler_op.
>
> 3) Check if period/budget is within a validated range
>
Don't drop the 'Changes on PATCH v2' part, please. That helps having an
idea of the full history, reminding about previous comments being done,
etc.
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 2a2d203..349f68b 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -839,6 +839,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>
> case XEN_DOMCTL_scheduler_op:
> ret = sched_adjust(d, &op->u.scheduler_op);
> + if ( ret == -ERESTART )
> + ret = hypercall_create_continuation(
> + __HYPERVISOR_domctl, "h", u_domctl);
> copyback = 1;
> break;
>
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 4372486..fed9e09 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -78,7 +78,6 @@
> * vcpu_insert, vcpu_remove, context_saved, __runq_insert
> */
>
> -
> /*
> * Default parameters:
> * Period and budget in default is 10 and 4 ms, respectively
>
This hunk is spurious, just don't do these things. If you think the
white line should indeed be dropped, send a separate patch, i.e., let's
avoid mixing cosmetic and coding style fixes, with functional changes.
Of course, it's not really worthwhile to send a patch for a blank line.
If you, however, identify a bunch of coding style issues in the file,
you can well bundle them together and send a patch to fix them.
> @@ -86,6 +85,18 @@
> #define RTDS_DEFAULT_PERIOD (MICROSECS(10000))
> #define RTDS_DEFAULT_BUDGET (MICROSECS(4000))
>
> +/*
> + * Max period: max delta of time type
> + * Min period: 100 us, considering the scheduling overhead
> + */
> +#define RTDS_MAX_PERIOD (STIME_DELTA_MAX)
> +#define RTDS_MIN_PERIOD (MICROSECS(100))
> +
So, minimum acceptable value is 100, or so one understands, when seeing
this.
Then, below, you do this:
if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD ||
budget < MICROSECS(10) || budget > period )
{
rc = -EINVAL;
break;
}
if ( period < RTDS_MIN_PERIOD || budget < RTDS_MIN_BUDGET )
warn = 1;
I.e., we warn if the value is below the minimum acceptable, but we
accept it, while we error if the value is below some magic number (10,
in this case).
I'd do the opposite: make 10 the minimum, and #define it so. For gating
the warning, either use another #define, or open code 100, but put down
a comment about it.
> @@ -1137,14 +1148,17 @@ rt_dom_cntl(
> struct list_head *iter;
> unsigned long flags;
> int rc = 0;
> + int warn = 0;
> + xen_domctl_schedparam_vcpu_t local_sched;
> + s_time_t period, budget;
> + unsigned int index;
>
> switch ( op->cmd )
> {
> - case XEN_DOMCTL_SCHEDOP_getinfo:
> + case XEN_DOMCTL_SCHEDOP_getinfo: /* return the default parameters */
> spin_lock_irqsave(&prv->lock, flags);
> - svc = list_entry(sdom->vcpu.next, struct rt_vcpu, sdom_elem);
> - op->u.rtds.period = svc->period / MICROSECS(1); /* transfer to us */
> - op->u.rtds.budget = svc->budget / MICROSECS(1);
> + op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1); /* transfer to us */
> + op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
> spin_unlock_irqrestore(&prv->lock, flags);
> break;
> case XEN_DOMCTL_SCHEDOP_putinfo:
>
So, the interface is asymmetric:
- getinfo: returns defaults
- putinfo: sets params for all vcpus
This is fine, as there is no much else meaningful we can do with
getinfo, and I don't see putinfo changing the defaults that much useful.
However, I'd put down a brief comment about this here, and a less brief
one in the public header.
> @@ -1162,8 +1176,82 @@ rt_dom_cntl(
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + spin_lock_irqsave(&prv->lock, flags);
> + for( index = 0; index < op->u.v.nr_vcpus; index++ )
> + {
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> + period = MICROSECS(local_sched.s.rtds.period);
> + budget = MICROSECS(local_sched.s.rtds.budget);
> + if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD ||
> + budget < MICROSECS(10) || budget > period )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + if ( period < RTDS_MIN_PERIOD || budget < RTDS_MIN_BUDGET )
> + warn = 1;
> +
> + svc->period = period;
> + svc->budget = budget;
> + if( hypercall_preempt_check() )
> + {
> + rc = -ERESTART;
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&prv->lock, flags);
> + break;
> }
> -
> + if ( rc == 0 && warn == 1 ) /* print warning in libxl */
> + rc = 1;
> return rc;
> }
>
Mmm.. And where is the documentation about the fact that return value of
1 means that the call succeeded, but some upper layer should print a
warning?
Remember that, when working in Xen, libxl is not the only toolstack one
should bear in mind.
In any case, even if documented, I don't like it that much... Why can't
we print a warning here, and just return success?
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index ecf1545..886e1b5 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1052,10 +1052,22 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
> if ( ret )
> return ret;
>
> - if ( (op->sched_id != DOM2OP(d)->sched_id) ||
> - ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) &&
> - (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
> + if ( op->sched_id != DOM2OP(d)->sched_id )
> return -EINVAL;
> + else
> + switch ( op->cmd )
> + {
> + case XEN_DOMCTL_SCHEDOP_putinfo:
> + break;
> + case XEN_DOMCTL_SCHEDOP_getinfo:
> + break;
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + break;
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + break;
> + default:
> + return -EINVAL;
> + }
>
> /* NB: the pluggable scheduler code needs to take care
> * of locking by itself. */
>
So, what happens if someone calls XEN_DOMCTL_SCHEDOP_putvcpuinfo or
XEN_DOMCTL_SCHEDOP_getvcpuinfo on Credit or Credit2? The call reports
being successful, but no date is returned (by quickly inspecting
csched_dom_cntl() and csched2_dom_cntl())?
I think you should go there and add something like:
if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo ||
op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
return -ENOTSUP;
or something like that.
Again, I know this can't happen in libxl, but that does not really
matter much down here. :-)
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index bc45ea5..bfb432f 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -330,31 +330,56 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
> /* Set or get info? */
>
As I said, I'll expand the comment here a bit, mentioning what happens
in the various cases (schedulers supporting or not per-vcpu
information), as well as how, for supporting ones, _getvcpuinfo and
_putvcpuinfo behave.
> #define XEN_DOMCTL_SCHEDOP_putinfo 0
> #define XEN_DOMCTL_SCHEDOP_getinfo 1
> +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2
> +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3
> struct xen_domctl_scheduler_op {
> uint32_t sched_id; /* XEN_SCHEDULER_* */
> uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
> union {
> - struct xen_domctl_sched_sedf {
> - uint64_aligned_t period;
> - uint64_aligned_t slice;
> - uint64_aligned_t latency;
> - uint32_t extratime;
> - uint32_t weight;
> - } sedf;
> - struct xen_domctl_sched_credit {
> - uint16_t weight;
> - uint16_t cap;
> - } credit;
> - struct xen_domctl_sched_credit2 {
> - uint16_t weight;
> - } credit2;
> - struct xen_domctl_sched_rtds {
> - uint32_t period;
> - uint32_t budget;
> - } rtds;
> + xen_domctl_sched_sedf_t sedf;
> + xen_domctl_sched_credit_t credit;
> + xen_domctl_sched_credit2_t credit2;
> + xen_domctl_sched_rtds_t rtds;
> + 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;
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 2/4] libxc: " Chong Li
@ 2015-07-27 16:11 ` Dario Faggioli
2015-08-07 16:35 ` Chong Li
0 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2015-07-27 16:11 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, xen-devel, Meng Xu,
dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 1617 bytes --]
On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index d1d2ab3..58f1a7a 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -915,6 +915,15 @@ int xc_sched_rtds_domain_get(xc_interface *xch,
> uint32_t domid,
> struct xen_domctl_sched_rtds *sdom);
>
So, here in the header, you're not changing the prototype of
xc_sched_rtds_domain_{get,set} to use the typedef.
However...
> +int xc_sched_rtds_vcpu_set(xc_interface *xch,
> + uint32_t domid,
> + xen_domctl_schedparam_vcpu_t *vcpus,
> + uint16_t num_vcpus);
>
... you hare using the typedef for the newly introduced functions
xc_sched_rtds_vcpu_{get,set}(), and...
> --- a/tools/libxc/xc_rt.c
> +++ b/tools/libxc/xc_rt.c
> @@ -27,7 +27,7 @@
>
> int xc_sched_rtds_domain_set(xc_interface *xch,
> uint32_t domid,
> - struct xen_domctl_sched_rtds *sdom)
> + xen_domctl_sched_rtds_t *sdom)
>
...you are actually changing the _implementation_ of
xc_sched_rtds_domain_{get,set}().
That's all rather inconsistent.
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 3/4] libxl: " Chong Li
@ 2015-07-28 9:15 ` Dario Faggioli
2015-08-07 17:34 ` Chong Li
0 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2015-07-28 9:15 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, ian.jackson,
xen-devel, ian.campbell, Meng Xu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 13048 bytes --]
On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
>
This patch looks mostly fine. A few comments.
So, as for the other patches, I'd mention here in the changelog, that
only RTDS supports this for now.
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e9a2d26..9f7f859 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
>
> +static int sched_rtds_validate_params(libxl__gc *gc, int period,
> + int 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 or equal to 1");
>
That's probably a nit, but, if period is not set, as the error message
says, it means it stays LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT? In
which case you do not enter this branch, and you do not print this
message, do you?
What I mean is that, it looks to me that it's not accurate for the
message to say "period not set", or am I overlooking something?
> + return 1;
>
So, 1 is error, 0 is ok. That's fine, as this is an internal function,
but please, add a quick doc comment above it.
> + }
> + *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 or equal to 1");
> + return 1;
> + }
> + *sdom_budget = budget;
> + }
> +
> + if (budget > period) {
> + LOG(ERROR, "VCPU budget is larger than VCPU period");
>
I'd be more explicit: "VCPU budget must be smaller than VCPU period".
> + return 1;
> + }
> +
> + 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;
> + xen_domctl_schedparam_vcpu_t *vcpus;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> +
> + if (scinfo->num_vcpus == 0)
> + num_vcpus = info.max_vcpu_id + 1;
> + else
> + num_vcpus = scinfo->num_vcpus;
> +
num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus :
info->max_vcpu_id + 1;
(And I think there is a 'contracted' form of this, but I keep forgetting
it, and I'm not sure whether it's a GCC extension...)
> + GCNEW_ARRAY(vcpus, num_vcpus);
> +
> + if (scinfo->num_vcpus > 0)
> + for (i=0; i < num_vcpus; i++) {
> + if (scinfo->vcpus[i].vcpuid < 0 ||
> + scinfo->vcpus[i].vcpuid > info.max_vcpu_id) {
> + LOG(ERROR, "VCPU index is out of range, "
> + "valid values are within range from 0 to %d",
> + info.max_vcpu_id);
> + return ERROR_INVAL;
> + }
> + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
> + } else
> + for (i=0; i < num_vcpus; i++)
> + vcpus[i].vcpuid = i;
> +
> + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus);
> + if (rc != 0) {
> + LOGE(ERROR, "getting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + scinfo->sched = LIBXL_SCHEDULER_RTDS;
> + if (scinfo->num_vcpus == 0) {
> + scinfo->num_vcpus = num_vcpus;
> + scinfo->vcpus = libxl__calloc(NOGC, num_vcpus, sizeof(libxl_sched_params));
>
This line looks a long...
> + }
> + for(i = 0; i < num_vcpus; i++) {
> + scinfo->vcpus[i].period = vcpus[i].s.rtds.period;
> + scinfo->vcpus[i].budget = vcpus[i].s.rtds.budget;
> + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
> + }
> +
> + 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 max_vcpuid;
> + xc_dominfo_t info;
> + xen_domctl_schedparam_vcpu_t *vcpus;
> + int num_vcpus;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> + max_vcpuid = info.max_vcpu_id;
> +
> + if (scinfo->num_vcpus > 0) {
> + num_vcpus = scinfo->num_vcpus;
> + GCNEW_ARRAY(vcpus, num_vcpus);
> + for (i = 0; i < num_vcpus; i++) {
> + if (scinfo->vcpus[i].vcpuid < 0 ||
> + scinfo->vcpus[i].vcpuid > max_vcpuid) {
> + LOG(ERROR, "VCPU index is out of range, "
> + "valid values are within range from 0 to %d",
> + max_vcpuid);
> + return ERROR_INVAL;
> + }
> + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
> +
> + rc = sched_rtds_validate_params(gc,
> + scinfo->vcpus[i].period, scinfo->vcpus[i].budget,
> + &vcpus[i].s.rtds.period, &vcpus[i].s.rtds.budget);
> + if (rc)
> + return ERROR_INVAL;
>
if (sched_rtds_validate_params(gc, scinfo->vcpus[i].period,
scinfo->vcpus[i].budget,
&vcpus[i].s.rtds.period,
&vcpus[i].s.rtds.budget))
return ERROR_INVAL;
I.e., I don't think you need to save rc.
> + }
> + } else {
> + num_vcpus = max_vcpuid + 1;
> + GCNEW_ARRAY(vcpus, num_vcpus);
> + rc = sched_rtds_validate_params(gc,
> + scinfo->vcpus[0].period, scinfo->vcpus[0].budget,
> + &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget);
> + if (rc)
> + return ERROR_INVAL;
> + for (i = 0; i < num_vcpus; i++) {
> + vcpus[i].vcpuid = i;
> + vcpus[i].s.rtds.period = scinfo->vcpus[0].period;
> + vcpus[i].s.rtds.budget = scinfo->vcpus[0].budget;
> + }
> + }
> +
> + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
> + vcpus, num_vcpus);
> + if (rc == 1) {
> + printf("WARN: too small period or budget may "
> + "result in large scheduling overhead\n");
> + rc = 0;
>
As said, do the logging in Xen directly, and ditch this.
> + } else if (rc != 0) {
> + LOGE(ERROR, "setting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + return rc;
> +}
> @@ -5887,29 +6037,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");
> + rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
> + &sdom.period, &sdom.budget);
> + if (rc)
> return ERROR_INVAL;
>
Ditto.
> - }
>
> @@ -5920,6 +6051,11 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
> return 0;
> }
>
> +/* Set the per-domain scheduling parameters.
> +* For schedulers that support per-vcpu settings (e.g., RTDS),
> +* calling *_domain_set functions will set all vcpus with the same
> +* scheduling parameters.
> +*/
> int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> const libxl_domain_sched_params *scinfo)
> {
> @@ -5956,6 +6092,52 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> return ret;
> }
>
> +/* Set the per-vcpu scheduling parameters */
> +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_SEDF:
> + LOG(ERROR, "No per-vcpu set function provided");
>
I'd say: "per-VCPU parameter setting not supported for this scheduler".
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT:
> + LOG(ERROR, "No per-vcpu set function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT2:
> + LOG(ERROR, "No per-vcpu set function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_ARINC653:
> + LOG(ERROR, "No per-vcpu set function provided");
> + ret = ERROR_INVAL;
> + break;
BTW, Can't you unify all the !supported cases?
> + 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;
> +}
> @@ -5989,6 +6171,45 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> return ret;
> }
>
> +/* Get the per-vcpu scheduling parameters */
> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + GC_INIT(ctx);
> + int ret;
> +
> + scinfo->sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (scinfo->sched) {
> + case LIBXL_SCHEDULER_SEDF:
> + LOG(ERROR, "No per-vcpu get function provided");
>
Same here.
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT:
> + LOG(ERROR, "No per-vcpu get function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT2:
> + LOG(ERROR, "No per-vcpu get function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_ARINC653:
> + LOG(ERROR, "No per-vcpu get function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_RTDS:
> + ret = sched_rtds_vcpu_get(gc, domid, scinfo);
> + break;
And here.
> + default:
> + LOG(ERROR, "Unknown scheduler");
> + ret = ERROR_INVAL;
> + break;
> + }
> +
> + GC_FREE;
> + return ret;
> +}
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index a1c5d15..040a248 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -200,6 +200,16 @@
> #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>
> /*
> + * libxl_vcpu_sched_params is used to store per-vcpu params
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * libxl_sched_params is used to store the array of per-vcpu params
> +*/
> +#define LIBXL_HAVE_SCHED_PARAMS 1
> +
>
I may be misremembering, but did not we say that one of these
LIBXL_HAVE_* was enough?
If we want to have two, I'd much rather have a generic one
(LIBXL_HAVE_VCPU_SCHED_PARAMS) and one announcing that the feature is
supported by RTDS (e.g., LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS).
This way, if/when we'll add support for per-VCPU parameters in Credit,
we'd add LIBXL_HAVE_SCHED_CREDIT_VCPU_PARAMS.
But that's just an idea, it's best to see what tools maintainers think
of this.
BTW, be a little more verbose in commenting these macros. Just have a
look and take inspiration from the already existing ones.
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 4/4] xl: " Chong Li
@ 2015-07-28 9:25 ` Dario Faggioli
2015-08-09 14:53 ` Chong Li
0 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2015-07-28 9:25 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, xen-devel, Meng Xu,
dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 3379 bytes --]
On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU settings.
>
This patch also looks nice. A few comments provided inline.
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c858068..da7c8a6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5630,6 +5630,37 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
> return rc;
> }
>
> +static int sched_vcpu_get(libxl_scheduler sched, int domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> +
> + rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo);
> + if (rc) {
> + fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n");
> + return rc;
> + }
> + if (scinfo->sched != sched) {
> + fprintf(stderr, "libxl_vcpu_sched_params_get returned %s not %s.\n",
> + libxl_scheduler_to_string(scinfo->sched),
> + libxl_scheduler_to_string(sched));
> + return ERROR_INVAL;
>
We are in xl, so there's no need to use libxl error code. It's probably
not strictly forbidden either, and xl itself is really inconsistent
about this, I know.
Personally, I never use them, and I think that using them may be
confusing, and create even more inconsistency.
> +static int sched_vcpu_set(int domid, const libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> +
> + rc = libxl_vcpu_sched_params_set(ctx, domid, scinfo);
> + if (rc)
> + fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n");
> +
> + return rc;
>
In xl, when failing, most of the time we exit(-1), or something like
that. So, it's important that you log what's happening, but then there's
no need to save the return codes and propagate them.
> static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
> {
> int rc;
> @@ -5786,6 +5817,38 @@ out:
> return rc;
> }
>
> +static int sched_rtds_vcpu_output(
> + int domid, libxl_vcpu_sched_params *scinfo)
> +{
> + char *domname;
> + int rc = 0;
> + int i;
> +
> + if (domid < 0) {
>
Style (indentation). This is not the only instance of something like
this in this patch.
> + printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
> + "VCPU", "Period", "Budget");
> + return 0;
> + }
> +
> + rc = sched_vcpu_get(LIBXL_SCHEDULER_RTDS, domid, scinfo);
> + if (rc)
> + goto out;
> +
> + domname = libxl_domid_to_name(ctx, domid);
> + for( i = 0; i < scinfo->num_vcpus; i++ ) {
>
Style:
for (i = 0; i < scinfo->num_vcpus; i++) {
and indentation, I think.
> + printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
> + domname,
> + domid,
> + scinfo->vcpus[i].vcpuid,
> + scinfo->vcpus[i].period,
> + scinfo->vcpus[i].budget);
> + }
> + free(domname);
> +
> +out:
> + return rc;
> +}
> +
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler
2015-07-27 15:14 ` Dario Faggioli
@ 2015-08-07 15:50 ` Chong Li
0 siblings, 0 replies; 22+ messages in thread
From: Chong Li @ 2015-08-07 15:50 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, George Dunlap, Ian Jackson, xen-devel,
Ian Campbell, Meng Xu, Jan Beulich, Dagaen Golomb
On Mon, Jul 27, 2015 at 10:14 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
>
>
> Or by "just" adding a note before the actual output:
>
> # xl sched-rtds -d vm1
> Showing per-vm(s) default scheduling parameters,
> use `-v' for seeing the actual parameters of each vcpu
> Name ID Period Budget
> vm1 1 10000 4000
>
> The latter is more verbose, but I think is what I see as more useful
> (and, probably, the easier to implement).
I agree. I'll add it in the next version.
>
> 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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2015-07-27 16:11 ` Dario Faggioli
@ 2015-08-07 16:35 ` Chong Li
0 siblings, 0 replies; 22+ messages in thread
From: Chong Li @ 2015-08-07 16:35 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Dagaen Golomb
On Mon, Jul 27, 2015 at 11:11 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index d1d2ab3..58f1a7a 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -915,6 +915,15 @@ int xc_sched_rtds_domain_get(xc_interface *xch,
>> uint32_t domid,
>> struct xen_domctl_sched_rtds *sdom);
>>
> So, here in the header, you're not changing the prototype of
> xc_sched_rtds_domain_{get,set} to use the typedef.
>
> However...
>
>> +int xc_sched_rtds_vcpu_set(xc_interface *xch,
>> + uint32_t domid,
>> + xen_domctl_schedparam_vcpu_t *vcpus,
>> + uint16_t num_vcpus);
>>
> ... you hare using the typedef for the newly introduced functions
> xc_sched_rtds_vcpu_{get,set}(), and...
>
>> --- a/tools/libxc/xc_rt.c
>> +++ b/tools/libxc/xc_rt.c
>> @@ -27,7 +27,7 @@
>>
>> int xc_sched_rtds_domain_set(xc_interface *xch,
>> uint32_t domid,
>> - struct xen_domctl_sched_rtds *sdom)
>> + xen_domctl_sched_rtds_t *sdom)
>>
> ...you are actually changing the _implementation_ of
> xc_sched_rtds_domain_{get,set}().
>
> That's all rather inconsistent.
You're right. I'll change 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)
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2015-07-28 9:15 ` Dario Faggioli
@ 2015-08-07 17:34 ` Chong Li
0 siblings, 0 replies; 22+ messages in thread
From: Chong Li @ 2015-08-07 17:34 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson, xen-devel,
Ian Campbell, Meng Xu, Dagaen Golomb
On Tue, Jul 28, 2015 at 4:15 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index e9a2d26..9f7f859 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>>
>> +static int sched_rtds_validate_params(libxl__gc *gc, int period,
>> + int 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 or equal to 1");
>>
> That's probably a nit, but, if period is not set, as the error message
> says, it means it stays LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT? In
> which case you do not enter this branch, and you do not print this
> message, do you?
>
> What I mean is that, it looks to me that it's not accurate for the
> message to say "period not set", or am I overlooking something?
If the period is empty (which means its value == 0), we consider it as
"period not set". Actually, I think maybe we can just delete this "not
set" warning, because in xl, we've already done a sanity check to make
sure period is not empty.
>
>> + return 1;
>>
> So, 1 is error, 0 is ok. That's fine, as this is an internal function,
> but please, add a quick doc comment above it.
Sure.
>
>
>> + return 1;
>> + }
>
>> + }
>> + } else {
>> + num_vcpus = max_vcpuid + 1;
>> + GCNEW_ARRAY(vcpus, num_vcpus);
>> + rc = sched_rtds_validate_params(gc,
>> + scinfo->vcpus[0].period, scinfo->vcpus[0].budget,
>> + &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget);
>> + if (rc)
>> + return ERROR_INVAL;
>> + for (i = 0; i < num_vcpus; i++) {
>> + vcpus[i].vcpuid = i;
>> + vcpus[i].s.rtds.period = scinfo->vcpus[0].period;
>> + vcpus[i].s.rtds.budget = scinfo->vcpus[0].budget;
>> + }
>> + }
>> +
>> + rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
>> + vcpus, num_vcpus);
>> + if (rc == 1) {
>> + printf("WARN: too small period or budget may "
>> + "result in large scheduling overhead\n");
>> + rc = 0;
>>
> As said, do the logging in Xen directly, and ditch this.
Do I use "printk" function in Xen to output warning message?
>
>> + } else if (rc != 0) {
>> + LOGE(ERROR, "setting vcpu sched rtds");
>> + return ERROR_FAIL;
>> + }
>> +
>> + return rc;
>> +}
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index a1c5d15..040a248 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -200,6 +200,16 @@
>> #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>>
>> /*
>> + * libxl_vcpu_sched_params is used to store per-vcpu params
>> +*/
>> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
>> +
>> +/*
>> + * libxl_sched_params is used to store the array of per-vcpu params
>> +*/
>> +#define LIBXL_HAVE_SCHED_PARAMS 1
>> +
>>
> I may be misremembering, but did not we say that one of these
> LIBXL_HAVE_* was enough?
>
> If we want to have two, I'd much rather have a generic one
> (LIBXL_HAVE_VCPU_SCHED_PARAMS) and one announcing that the feature is
> supported by RTDS (e.g., LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS).
>
> This way, if/when we'll add support for per-VCPU parameters in Credit,
> we'd add LIBXL_HAVE_SCHED_CREDIT_VCPU_PARAMS.
>
> But that's just an idea, it's best to see what tools maintainers think
> of this.
>
> BTW, be a little more verbose in commenting these macros. Just have a
> look and take inspiration from the already existing ones.
Ok. I'll rewrite it.
Thanks,
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2015-07-28 9:25 ` Dario Faggioli
@ 2015-08-09 14:53 ` Chong Li
0 siblings, 0 replies; 22+ messages in thread
From: Chong Li @ 2015-08-09 14:53 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Dagaen Golomb
On Tue, Jul 28, 2015 at 4:25 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
>> Change main_sched_rtds and related output functions to support
>> per-VCPU settings.
>>
> This patch also looks nice. A few comments provided inline.
>
>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index c858068..da7c8a6 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -5630,6 +5630,37 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
>> return rc;
>> }
>>
>> +static int sched_vcpu_get(libxl_scheduler sched, int domid,
>> + libxl_vcpu_sched_params *scinfo)
>> +{
>> + int rc;
>> +
>> + rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo);
>> + if (rc) {
>> + fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n");
>> + return rc;
>> + }
>> + if (scinfo->sched != sched) {
>> + fprintf(stderr, "libxl_vcpu_sched_params_get returned %s not %s.\n",
>> + libxl_scheduler_to_string(scinfo->sched),
>> + libxl_scheduler_to_string(sched));
>> + return ERROR_INVAL;
>>
> We are in xl, so there's no need to use libxl error code. It's probably
> not strictly forbidden either, and xl itself is really inconsistent
> about this, I know.
>
> Personally, I never use them, and I think that using them may be
> confusing, and create even more inconsistency.
>
What error code should I use here? Do I also need to change the
"ERROR_INVAL" in function sched_domain_get?
> --
> <<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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2015-07-13 8:37 ` Jan Beulich
@ 2015-08-09 15:45 ` Chong Li
2015-08-11 9:39 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Chong Li @ 2015-08-09 15:45 UTC (permalink / raw)
To: Jan Beulich
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
On Mon, Jul 13, 2015 at 3:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 11.07.15 at 06:52, <lichong659@gmail.com> wrote:
>> @@ -1162,8 +1176,82 @@ rt_dom_cntl(
>> }
>> spin_unlock_irqrestore(&prv->lock, flags);
>> break;
>> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> + spin_lock_irqsave(&prv->lock, flags);
>> + for ( index = 0; index < op->u.v.nr_vcpus; index++ )
>> + {
>> + if ( copy_from_guest_offset(&local_sched,
>> + op->u.v.vcpus, index, 1) )
>> + {
>> + rc = -EFAULT;
>> + break;
>> + }
>> + if ( local_sched.vcpuid >= d->max_vcpus ||
>> + d->vcpu[local_sched.vcpuid] == NULL )
>> + {
>> + rc = -EINVAL;
>> + break;
>> + }
>> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>> +
>> + local_sched.s.rtds.budget = svc->budget / MICROSECS(1);
>> + local_sched.s.rtds.period = svc->period / MICROSECS(1);
>> +
>> + if ( __copy_to_guest_offset(op->u.v.vcpus, index,
>> + &local_sched, 1) )
>> + {
>> + rc = -EFAULT;
>> + break;
>> + }
>> + if( hypercall_preempt_check() )
>> + {
>> + rc = -ERESTART;
>> + break;
>> + }
>
> I still don't see how this is supposed to work.
I return -ERESTART here, and the upper layer function (do_domctl) will
handle this error code by calling hypercall_create_continuation.
>> +} xen_domctl_schedparam_vcpu_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
>> +
>> /* Set or get info? */
>> #define XEN_DOMCTL_SCHEDOP_putinfo 0
>> #define XEN_DOMCTL_SCHEDOP_getinfo 1
>> +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2
>> +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3
>> struct xen_domctl_scheduler_op {
>> uint32_t sched_id; /* XEN_SCHEDULER_* */
>> uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
>> union {
>> - struct xen_domctl_sched_sedf {
>> - uint64_aligned_t period;
>> - uint64_aligned_t slice;
>> - uint64_aligned_t latency;
>> - uint32_t extratime;
>> - uint32_t weight;
>> - } sedf;
>> - struct xen_domctl_sched_credit {
>> - uint16_t weight;
>> - uint16_t cap;
>> - } credit;
>> - struct xen_domctl_sched_credit2 {
>> - uint16_t weight;
>> - } credit2;
>> - struct xen_domctl_sched_rtds {
>> - uint32_t period;
>> - uint32_t budget;
>> - } rtds;
>> + xen_domctl_sched_sedf_t sedf;
>> + xen_domctl_sched_credit_t credit;
>> + xen_domctl_sched_credit2_t credit2;
>> + xen_domctl_sched_rtds_t rtds;
>> + struct {
>> + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
>> + uint16_t nr_vcpus;
>> + } v;
>
> And there's still no explicit padding here at all (nor am I convinced
> that uint16_t is really a good choice for nr_vcpus - uint32_t would
> seem more natural without causing any problems or structure
> growth).
I think the size of union u is equal to the size of
xen_domctl_sched_sedf_t, which is 64*4 bits (if "vcpus" in struct v is
just a pointer).
The "nr_vcpus" is indeed better to be uint32_t. I'll change it in the
next version.
Chong
>
> Jan
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2015-07-27 15:51 ` Dario Faggioli
@ 2015-08-09 16:08 ` Chong Li
0 siblings, 0 replies; 22+ messages in thread
From: Chong Li @ 2015-08-09 16:08 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu, Jan Beulich,
Dagaen Golomb
On Mon, Jul 27, 2015 at 10:51 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
>> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
>> to independently get and set the scheduling parameters of each
>> vCPU of a domain
>>
> I'd add a note about the fact that, for now, this is only supported and
> being implemented for RTDs.
>
>> 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>
>>
>> ---
>> Changes on PATCH v3:
>>
>> 1) Remove struct xen_domctl_schedparam_t.
>>
>> 2) Change struct xen_domctl_scheduler_op.
>>
>> 3) Check if period/budget is within a validated range
>>
> Don't drop the 'Changes on PATCH v2' part, please. That helps having an
> idea of the full history, reminding about previous comments being done,
> etc.
>
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 2a2d203..349f68b 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>
>> @@ -1162,8 +1176,82 @@ rt_dom_cntl(
>
>> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
>> + spin_lock_irqsave(&prv->lock, flags);
>> + for( index = 0; index < op->u.v.nr_vcpus; index++ )
>> + {
>> + if ( copy_from_guest_offset(&local_sched,
>> + op->u.v.vcpus, index, 1) )
>> + {
>> + rc = -EFAULT;
>> + break;
>> + }
>> + if ( local_sched.vcpuid >= d->max_vcpus ||
>> + d->vcpu[local_sched.vcpuid] == NULL )
>> + {
>> + rc = -EINVAL;
>> + break;
>> + }
>> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>> + period = MICROSECS(local_sched.s.rtds.period);
>> + budget = MICROSECS(local_sched.s.rtds.budget);
>> + if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD ||
>> + budget < MICROSECS(10) || budget > period )
>> + {
>> + rc = -EINVAL;
>> + break;
>> + }
>> + if ( period < RTDS_MIN_PERIOD || budget < RTDS_MIN_BUDGET )
>> + warn = 1;
>> +
>> + svc->period = period;
>> + svc->budget = budget;
>> + if( hypercall_preempt_check() )
>> + {
>> + rc = -ERESTART;
>> + break;
>> + }
>> + }
>> + spin_unlock_irqrestore(&prv->lock, flags);
>> + break;
>> }
>> -
>> + if ( rc == 0 && warn == 1 ) /* print warning in libxl */
>> + rc = 1;
>> return rc;
>> }
>>
> Mmm.. And where is the documentation about the fact that return value of
> 1 means that the call succeeded, but some upper layer should print a
> warning?
>
> Remember that, when working in Xen, libxl is not the only toolstack one
> should bear in mind.
>
> In any case, even if documented, I don't like it that much... Why can't
> we print a warning here, and just return success?
Do I just use printk here? Is there any other more appropriate print
function in xen?
>
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index ecf1545..886e1b5 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1052,10 +1052,22 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
>> if ( ret )
>> return ret;
>>
>> - if ( (op->sched_id != DOM2OP(d)->sched_id) ||
>> - ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) &&
>> - (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
>> + if ( op->sched_id != DOM2OP(d)->sched_id )
>> return -EINVAL;
>> + else
>> + switch ( op->cmd )
>> + {
>> + case XEN_DOMCTL_SCHEDOP_putinfo:
>> + break;
>> + case XEN_DOMCTL_SCHEDOP_getinfo:
>> + break;
>> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
>> + break;
>> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>>
>> /* NB: the pluggable scheduler code needs to take care
>> * of locking by itself. */
>>
> So, what happens if someone calls XEN_DOMCTL_SCHEDOP_putvcpuinfo or
> XEN_DOMCTL_SCHEDOP_getvcpuinfo on Credit or Credit2? The call reports
> being successful, but no date is returned (by quickly inspecting
> csched_dom_cntl() and csched2_dom_cntl())?
>
> I think you should go there and add something like:
>
> if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo ||
> op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
> return -ENOTSUP;
>
> or something like that.
I see. Will do.
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] 22+ messages in thread
* Re: [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2015-08-09 15:45 ` Chong Li
@ 2015-08-11 9:39 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2015-08-11 9:39 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
>>> On 09.08.15 at 17:45, <lichong659@gmail.com> wrote:
> On Mon, Jul 13, 2015 at 3:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 11.07.15 at 06:52, <lichong659@gmail.com> wrote:
>>> @@ -1162,8 +1176,82 @@ rt_dom_cntl(
>>> }
>>> spin_unlock_irqrestore(&prv->lock, flags);
>>> break;
>>> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>>> + spin_lock_irqsave(&prv->lock, flags);
>>> + for ( index = 0; index < op->u.v.nr_vcpus; index++ )
>>> + {
>>> + if ( copy_from_guest_offset(&local_sched,
>>> + op->u.v.vcpus, index, 1) )
>>> + {
>>> + rc = -EFAULT;
>>> + break;
>>> + }
>>> + if ( local_sched.vcpuid >= d->max_vcpus ||
>>> + d->vcpu[local_sched.vcpuid] == NULL )
>>> + {
>>> + rc = -EINVAL;
>>> + break;
>>> + }
>>> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>>> +
>>> + local_sched.s.rtds.budget = svc->budget / MICROSECS(1);
>>> + local_sched.s.rtds.period = svc->period / MICROSECS(1);
>>> +
>>> + if ( __copy_to_guest_offset(op->u.v.vcpus, index,
>>> + &local_sched, 1) )
>>> + {
>>> + rc = -EFAULT;
>>> + break;
>>> + }
>>> + if( hypercall_preempt_check() )
>>> + {
>>> + rc = -ERESTART;
>>> + break;
>>> + }
>>
>> I still don't see how this is supposed to work.
>
> I return -ERESTART here, and the upper layer function (do_domctl) will
> handle this error code by calling hypercall_create_continuation.
I have no idea where you found the upper layer (i.e. the
XEN_DOMCTL_scheduler_op case of do_domctl() to take care of
this).
>>> +} xen_domctl_schedparam_vcpu_t;
>>> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
>>> +
>>> /* Set or get info? */
>>> #define XEN_DOMCTL_SCHEDOP_putinfo 0
>>> #define XEN_DOMCTL_SCHEDOP_getinfo 1
>>> +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2
>>> +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3
>>> struct xen_domctl_scheduler_op {
>>> uint32_t sched_id; /* XEN_SCHEDULER_* */
>>> uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
>>> union {
>>> - struct xen_domctl_sched_sedf {
>>> - uint64_aligned_t period;
>>> - uint64_aligned_t slice;
>>> - uint64_aligned_t latency;
>>> - uint32_t extratime;
>>> - uint32_t weight;
>>> - } sedf;
>>> - struct xen_domctl_sched_credit {
>>> - uint16_t weight;
>>> - uint16_t cap;
>>> - } credit;
>>> - struct xen_domctl_sched_credit2 {
>>> - uint16_t weight;
>>> - } credit2;
>>> - struct xen_domctl_sched_rtds {
>>> - uint32_t period;
>>> - uint32_t budget;
>>> - } rtds;
>>> + xen_domctl_sched_sedf_t sedf;
>>> + xen_domctl_sched_credit_t credit;
>>> + xen_domctl_sched_credit2_t credit2;
>>> + xen_domctl_sched_rtds_t rtds;
>>> + struct {
>>> + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
>>> + uint16_t nr_vcpus;
>>> + } v;
>>
>> And there's still no explicit padding here at all (nor am I convinced
>> that uint16_t is really a good choice for nr_vcpus - uint32_t would
>> seem more natural without causing any problems or structure
>> growth).
>
> I think the size of union u is equal to the size of
> xen_domctl_sched_sedf_t, which is 64*4 bits (if "vcpus" in struct v is
> just a pointer).
Which doesn't in any way address to complaint about missing explicit
padding - I'm not asking you to pad to the size of the union, but to the
size of the unnamed structure you add.
Jan
> The "nr_vcpus" is indeed better to be uint32_t. I'll change it in the
> next version.
>
> Chong
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2015-08-11 9:39 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 1/4] xen: enable " Chong Li
2015-07-13 8:37 ` Jan Beulich
2015-08-09 15:45 ` Chong Li
2015-08-11 9:39 ` Jan Beulich
2015-07-27 15:51 ` Dario Faggioli
2015-08-09 16:08 ` Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 2/4] libxc: " Chong Li
2015-07-27 16:11 ` Dario Faggioli
2015-08-07 16:35 ` Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 3/4] libxl: " Chong Li
2015-07-28 9:15 ` Dario Faggioli
2015-08-07 17:34 ` Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 4/4] xl: " Chong Li
2015-07-28 9:25 ` Dario Faggioli
2015-08-09 14:53 ` Chong Li
2015-07-11 14:33 ` [PATCH v4 for Xen 4.6 0/4] Enable " Wei Liu
2015-07-13 10:27 ` Dario Faggioli
2015-07-14 5:45 ` Meng Xu
2015-07-14 7:13 ` Dario Faggioli
2015-07-27 15:14 ` Dario Faggioli
2015-08-07 15:50 ` 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.