* [PATCH v5 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler
@ 2016-02-04 22:50 Chong Li
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 1/4] xen: enable " Chong Li
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Chong Li @ 2016-02-04 22:50 UTC (permalink / raw)
To: xen-devel
Cc: 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".
---
Previous conclusion:
On PATCH v4, our concern was about the usage of hypercall_preemption_check
and the print of warning message (both in xen). These issues are addressed
in this patch.
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 | 8 ++
tools/libxc/xc_rt.c | 56 ++++++++
tools/libxl/libxl.c | 249 +++++++++++++++++++++++++++++++---
tools/libxl/libxl.h | 19 +++
tools/libxl/libxl_types.idl | 16 +++
tools/libxl/xl_cmdimpl.c | 305 +++++++++++++++++++++++++++++++++++++-----
tools/libxl/xl_cmdtable.c | 10 +-
xen/common/domctl.c | 5 +
xen/common/sched_credit.c | 4 +
xen/common/sched_credit2.c | 4 +
xen/common/sched_rt.c | 127 +++++++++++++++---
xen/common/schedule.c | 15 ++-
xen/include/public/domctl.h | 57 ++++++--
14 files changed, 787 insertions(+), 92 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 [PATCH v5 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
@ 2016-02-04 22:50 ` Chong Li
2016-02-09 18:17 ` Dario Faggioli
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 2/4] libxc: " Chong Li
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-02-04 22:50 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 v4:
1) Add uint32_t vcpu_index to struct xen_domctl_scheduler_op.
When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we call
hypercall_preemption_check in case the current hypercall lasts
too long. If we decide to preempt the current hypercall, we record
the index of the most-recent finished vcpu into the vcpu_index of
struct xen_domctl_scheduler_op. So when we resume the hypercall after
preemption, we start processing from the posion specified by vcpu_index,
and don't need to repeat the work that has already been done in the
hypercall before the preemption.
(This design is based on the do_grant_table_op() in grant_table.c)
2) Coding style changes
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
Changes on PATCH v2:
1) Change struct xen_domctl_scheduler_op, for transferring per-vcpu parameters
between libxc and hypervisor.
2) Handler of XEN_DOMCTL_SCHEDOP_getinfo now just returns the default budget and
period values of RTDS scheduler.
3) Handler of XEN_DOMCTL_SCHEDOP_getvcpuinfo now can return a random subset of
the parameters of the VCPUs of a specific domain
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 | 5 ++
xen/common/sched_credit.c | 4 ++
xen/common/sched_credit2.c | 4 ++
xen/common/sched_rt.c | 127 ++++++++++++++++++++++++++++++++++++++------
xen/common/schedule.c | 15 ++++--
xen/include/public/domctl.h | 57 +++++++++++++++-----
6 files changed, 180 insertions(+), 32 deletions(-)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 46b967e..b294221 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -847,9 +847,14 @@ 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;
+ }
case XEN_DOMCTL_getdomaininfo:
{
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 0dce790..455c684 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1054,6 +1054,10 @@ csched_dom_cntl(
* lock. Runq lock not needed anywhere in here. */
spin_lock_irqsave(&prv->lock, flags);
+ if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo ||
+ op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
+ return -EINVAL;
+
if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
{
op->u.credit.weight = sdom->weight;
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 3c49ffa..c3049a0 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1421,6 +1421,10 @@ csched2_dom_cntl(
* runq lock to update csvcs. */
spin_lock_irqsave(&prv->lock, flags);
+ if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo ||
+ op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
+ return -EINVAL;
+
if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
{
op->u.credit2.weight = sdom->weight;
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 3f1d047..34ae48d 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -86,8 +86,21 @@
#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(10))
+
+/*
+ * Min budget: 100 us
+ */
+#define RTDS_MIN_BUDGET (MICROSECS(10))
+
#define UPDATE_LIMIT_SHIFT 10
#define MAX_SCHEDULE (MILLISECS(1))
+
/*
* Flags
*/
@@ -1129,26 +1142,18 @@ rt_dom_cntl(
struct vcpu *v;
unsigned long flags;
int rc = 0;
-
+ xen_domctl_schedparam_vcpu_t local_sched;
+ s_time_t period, budget;
+ uint32_t index;
switch ( op->cmd )
{
- case XEN_DOMCTL_SCHEDOP_getinfo:
- if ( d->max_vcpus > 0 )
- {
- spin_lock_irqsave(&prv->lock, flags);
- svc = rt_vcpu(d->vcpu[0]);
- op->u.rtds.period = svc->period / MICROSECS(1);
- op->u.rtds.budget = svc->budget / MICROSECS(1);
- spin_unlock_irqrestore(&prv->lock, flags);
- }
- else
- {
- /* If we don't have vcpus yet, let's just return the defaults. */
- op->u.rtds.period = RTDS_DEFAULT_PERIOD;
- op->u.rtds.budget = RTDS_DEFAULT_BUDGET;
- }
+ case XEN_DOMCTL_SCHEDOP_getinfo: /* return the default parameters */
+ spin_lock_irqsave(&prv->lock, flags);
+ 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:
+ case XEN_DOMCTL_SCHEDOP_putinfo: /* set parameters for all vcpus */
if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
{
rc = -EINVAL;
@@ -1163,6 +1168,94 @@ rt_dom_cntl(
}
spin_unlock_irqrestore(&prv->lock, flags);
break;
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus; index++ )
+ {
+ spin_lock_irqsave(&prv->lock, flags);
+ if ( copy_from_guest_offset(&local_sched,
+ op->u.v.vcpus, index, 1) )
+ {
+ rc = -EFAULT;
+ spin_unlock_irqrestore(&prv->lock, flags);
+ break;
+ }
+ if ( local_sched.vcpuid >= d->max_vcpus ||
+ d->vcpu[local_sched.vcpuid] == NULL )
+ {
+ rc = -EINVAL;
+ spin_unlock_irqrestore(&prv->lock, flags);
+ 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;
+ spin_unlock_irqrestore(&prv->lock, flags);
+ break;
+ }
+ spin_unlock_irqrestore(&prv->lock, flags);
+ if ( hypercall_preempt_check() )
+ {
+ op->u.v.vcpu_index = index + 1;
+ /* hypercall (after preemption) will continue at vcpu_index */
+ rc = -ERESTART;
+ break;
+ }
+ }
+ break;
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus; index++ )
+ {
+ spin_lock_irqsave(&prv->lock, flags);
+ if ( copy_from_guest_offset(&local_sched,
+ op->u.v.vcpus, index, 1) )
+ {
+ rc = -EFAULT;
+ spin_unlock_irqrestore(&prv->lock, flags);
+ break;
+ }
+ if ( local_sched.vcpuid >= d->max_vcpus ||
+ d->vcpu[local_sched.vcpuid] == NULL )
+ {
+ rc = -EINVAL;
+ spin_unlock_irqrestore(&prv->lock, flags);
+ 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 > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET ||
+ budget > period )
+ {
+ rc = -EINVAL;
+ spin_unlock_irqrestore(&prv->lock, flags);
+ break;
+ }
+
+ /*
+ * We accept period/budget less than 100 us, but will warn users about
+ * the large scheduling overhead due to it
+ */
+ if ( period < MICROSECS(100) || budget < MICROSECS(100) )
+ printk("Warning: period/budget less than 100 micro-secs "
+ "results in large scheduling overhead.\n");
+
+ svc->period = period;
+ svc->budget = budget;
+ spin_unlock_irqrestore(&prv->lock, flags);
+ if ( hypercall_preempt_check() )
+ {
+ op->u.v.vcpu_index = index + 1;
+ rc = -ERESTART;
+ break;
+ }
+ }
+ break;
}
return rc;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c195129..f4a4032 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1148,10 +1148,19 @@ 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:
+ case XEN_DOMCTL_SCHEDOP_getinfo:
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ 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 7a56b3f..6f429ec 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -338,24 +338,57 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
#define XEN_SCHEDULER_ARINC653 7
#define XEN_SCHEDULER_RTDS 8
-/* Set or get info? */
+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[3];
+} xen_domctl_schedparam_vcpu_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
+
+/* Set or get info?
+ * For schedulers supporting per-vcpu settings (e.g., RTDS):
+ * using XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
+ * using XEN_DOMCTL_SCHEDOP_getinfo gets default params;
+ * using XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus;
+ * For schedulers not supporting per-vcpu settings:
+ * using XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
+ * using XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params;
+ * using XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error code;
+ */
#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_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_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;
+ uint32_t nr_vcpus;
+ uint32_t vcpu_index;
+ } v;
} u;
};
typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 for Xen 4.7 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 [PATCH v5 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-02-04 22:50 ` Chong Li
2016-02-05 14:09 ` Wei Liu
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 3/4] libxl: " Chong Li
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 4/4] xl: " Chong Li
3 siblings, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-02-04 22:50 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>
---
Changes on PATCH v4:
1) Minor modifications on the function parameters.
Changes on PATCH v2:
1) Minor modifications due to the change of struct xen_domctl_scheduler_op.
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 | 8 +++++++
tools/libxc/xc_rt.c | 56 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 01a6dda..db13434 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -893,6 +893,14 @@ 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);
+int xc_sched_rtds_vcpu_set(xc_interface *xch,
+ uint32_t domid,
+ struct xen_domctl_schedparam_vcpu *vcpus,
+ uint32_t num_vcpus);
+int xc_sched_rtds_vcpu_get(xc_interface *xch,
+ uint32_t domid,
+ struct xen_domctl_schedparam_vcpu *vcpus,
+ uint32_t num_vcpus);
int
xc_sched_arinc653_schedule_set(
diff --git a/tools/libxc/xc_rt.c b/tools/libxc/xc_rt.c
index d59e5ce..cb7bc1a 100644
--- a/tools/libxc/xc_rt.c
+++ b/tools/libxc/xc_rt.c
@@ -62,3 +62,59 @@ int xc_sched_rtds_domain_get(xc_interface *xch,
return rc;
}
+
+int xc_sched_rtds_vcpu_set(xc_interface *xch,
+ uint32_t domid,
+ struct xen_domctl_schedparam_vcpu *vcpus,
+ uint32_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);
+ domctl.u.scheduler_op.u.v.vcpu_index = 0;
+
+ 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,
+ struct xen_domctl_schedparam_vcpu *vcpus,
+ uint32_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);
+ domctl.u.scheduler_op.u.v.vcpu_index = 0;
+
+ rc = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, vcpus);
+
+ return rc;
+}
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 [PATCH v5 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 1/4] xen: enable " Chong Li
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 2/4] libxc: " Chong Li
@ 2016-02-04 22:50 ` Chong Li
2016-02-05 14:44 ` Wei Liu
2016-02-09 12:00 ` Dario Faggioli
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 4/4] xl: " Chong Li
3 siblings, 2 replies; 24+ messages in thread
From: Chong Li @ 2016-02-04 22:50 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 v4:
1) Coding style changes
Changes on PATCH v3:
1) Add sanity check on vcpuid
2) Add comments on per-domain and per-vcpu functions for libxl
users
Changes on PATCH v2:
1) New data structure (libxl_vcpu_sched_params and libxl_sched_params)
to help per-VCPU settings.
2) sched_rtds_vcpu_get now can return a random subset of the parameters
of the VCPUs of a specific domain.
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 | 249 ++++++++++++++++++++++++++++++++++++++++----
tools/libxl/libxl.h | 19 ++++
tools/libxl/libxl_types.idl | 16 +++
3 files changed, 262 insertions(+), 22 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bd3aac8..ac4a103 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5770,6 +5770,151 @@ static int sched_credit2_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 out of range, "
+ "valid values are larger than or equal to 1");
+ return 1; /* error scheduling parameter */
+ }
+ *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 must be smaller than "
+ "or equal to VCPU period");
+ return 1;
+ }
+
+ return 0; /* period and budget are valid */
+}
+
+static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
+ libxl_vcpu_sched_params *scinfo)
+{
+ uint32_t num_vcpus;
+ int rc, i;
+ xc_dominfo_t info;
+ struct xen_domctl_schedparam_vcpu *vcpus;
+
+ rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (rc < 0) {
+ LOGE(ERROR, "getting domain info");
+ return ERROR_FAIL;
+ }
+
+ num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus :
+ info.max_vcpu_id + 1;
+
+ 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;
+ struct xen_domctl_schedparam_vcpu *vcpus;
+ uint32_t 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);
+ if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
+ scinfo->vcpus[0].budget,
+ &vcpus[0].s.rtds.period,
+ &vcpus[0].s.rtds.budget))
+ 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 != 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)
{
@@ -5803,29 +5948,9 @@ 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");
+ if (sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
+ &sdom.period, &sdom.budget))
return ERROR_INVAL;
- }
rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
if (rc < 0) {
@@ -5836,6 +5961,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)
{
@@ -5873,6 +6003,47 @@ 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, "SEDF scheduler no longer available");
+ ret=ERROR_FEATURE_REMOVED;
+ break;
+ case LIBXL_SCHEDULER_CREDIT:
+ case LIBXL_SCHEDULER_CREDIT2:
+ case LIBXL_SCHEDULER_ARINC653:
+ LOG(ERROR, "per-VCPU parameter setting "
+ "not supported for this scheduler");
+ 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)
{
@@ -5907,6 +6078,40 @@ 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, "SEDF scheduler no longer available");
+ ret=ERROR_FEATURE_REMOVED;
+ break;
+ case LIBXL_SCHEDULER_CREDIT:
+ case LIBXL_SCHEDULER_CREDIT2:
+ case LIBXL_SCHEDULER_ARINC653:
+ LOG(ERROR, "per-VCPU parameter getting "
+ "not supported for this scheduler");
+ 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 6b73848..4ba30d3 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -206,6 +206,18 @@
#define LIBXL_HAVE_DEVICE_MODEL_USER 1
/*
+ * libxl_vcpu_sched_params is used to store per-vcpu params.
+ * The 'vcpuid' field specifies the vcpu to be set or read.
+*/
+#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
+
+/*
+ * LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS indicates RTDS scheduler
+ * now supports per-vcpu settings.
+*/
+#define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1
+
+/*
* libxl_domain_build_info has the arm.gic_version field.
*/
#define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1
@@ -1647,10 +1659,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 cf3730f..4e7210e 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -378,6 +378,22 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
("stream_version", uint32, {'init_val': '1'}),
])
+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] 24+ messages in thread
* [PATCH v5 for Xen 4.7 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 [PATCH v5 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
` (2 preceding siblings ...)
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-02-04 22:50 ` Chong Li
2016-02-05 14:51 ` Wei Liu
3 siblings, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-02-04 22:50 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 v4:
1) Coding style changes
Changes on PATCH v3:
1) Support commands, e.g., "xl sched-rtds -d vm1" to output the
default scheduling parameters
Changes on PATCH v2:
1) Remove per-domain output functions for RTDS scheduler.
2) Users now use '-v all' to specify all VCPUs.
3) Support outputting a subset of the parameters of the VCPUs
of a specific domain.
4) When setting all VCPUs with the same parameters (by only one
command), no per-domain function is invoked.
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 | 305 ++++++++++++++++++++++++++++++++++++++++------
tools/libxl/xl_cmdtable.c | 10 +-
3 files changed, 281 insertions(+), 38 deletions(-)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 4279c7c..f9ff917 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1051,6 +1051,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 2b6371d..b843fa5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5823,6 +5823,39 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
return 0;
}
+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");
+ exit(-1);
+ }
+ 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 1;
+ }
+
+ 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");
+ exit(-1);
+ }
+
+ return rc;
+}
+
static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
{
if (libxl_sched_credit_params_set(ctx, poolid, scinfo)) {
@@ -5942,6 +5975,38 @@ static int sched_rtds_domain_output(
return 0;
}
+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;
@@ -6015,6 +6080,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
@@ -6222,77 +6346,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
+ COMMON_LONG_OPTS,
+ {0, 0, 0, 0}
};
- SWITCH_FOREACH_OPT(opt, "d:p:b:c:", 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 EXIT_FAILURE;
+ rc = 1;
+ goto out;
}
- if (!dom && (opt_p || opt_b)) {
- fprintf(stderr, "Must specify a domain.\n");
- return EXIT_FAILURE;
+ 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 EXIT_FAILURE;
+ 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 */
- if (sched_domain_output(LIBXL_SCHEDULER_RTDS,
- sched_rtds_domain_output,
- sched_rtds_pool_output,
- cpupool))
- return EXIT_FAILURE;
+ 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);
- if (sched_rtds_domain_output(domid))
- return EXIT_FAILURE;
- } 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 EXIT_FAILURE;
+ rc = sched_vcpu_set(domid, &scinfo);
+ libxl_vcpu_sched_params_dispose(&scinfo);
+ if (rc) {
+ rc = -rc;
+ goto out;
+ }
}
}
- return EXIT_SUCCESS;
+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 fdc1ac6..c68b656 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -268,10 +268,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] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 2/4] libxc: " Chong Li
@ 2016-02-05 14:09 ` Wei Liu
2016-02-09 18:20 ` Dario Faggioli
0 siblings, 1 reply; 24+ messages in thread
From: Wei Liu @ 2016-02-05 14:09 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
xen-devel, Meng Xu, dgolomb
On Thu, Feb 04, 2016 at 04:50:42PM -0600, Chong Li wrote:
> 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>
These looks like sensible wrappers. I will defer this patch to Dario. If
he's happy with this I will just ack it.
> ---
> Changes on PATCH v4:
> 1) Minor modifications on the function parameters.
>
> Changes on PATCH v2:
> 1) Minor modifications due to the change of struct xen_domctl_scheduler_op.
>
> 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 | 8 +++++++
> tools/libxc/xc_rt.c | 56 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 01a6dda..db13434 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -893,6 +893,14 @@ 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);
> +int xc_sched_rtds_vcpu_set(xc_interface *xch,
> + uint32_t domid,
> + struct xen_domctl_schedparam_vcpu *vcpus,
> + uint32_t num_vcpus);
> +int xc_sched_rtds_vcpu_get(xc_interface *xch,
> + uint32_t domid,
> + struct xen_domctl_schedparam_vcpu *vcpus,
> + uint32_t num_vcpus);
>
Indentation looks wrong.
Wei.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-02-05 14:44 ` Wei Liu
2016-02-05 15:59 ` Dario Faggioli
2016-02-06 0:10 ` Chong Li
2016-02-09 12:00 ` Dario Faggioli
1 sibling, 2 replies; 24+ messages in thread
From: Wei Liu @ 2016-02-05 14:44 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
ian.jackson, xen-devel, ian.campbell, Meng Xu, dgolomb
On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
>
I will need Dario or George to review the logic of the code.
If some of the comments below don't make sense, just ask. I'm sure I
make stupid comments at times.
> 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 v4:
> 1) Coding style changes
>
> Changes on PATCH v3:
> 1) Add sanity check on vcpuid
>
> 2) Add comments on per-domain and per-vcpu functions for libxl
> users
>
> Changes on PATCH v2:
> 1) New data structure (libxl_vcpu_sched_params and libxl_sched_params)
> to help per-VCPU settings.
>
> 2) sched_rtds_vcpu_get now can return a random subset of the parameters
> of the VCPUs of a specific domain.
>
> 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 | 249 ++++++++++++++++++++++++++++++++++++++++----
> tools/libxl/libxl.h | 19 ++++
> tools/libxl/libxl_types.idl | 16 +++
> 3 files changed, 262 insertions(+), 22 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index bd3aac8..ac4a103 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5770,6 +5770,151 @@ static int sched_credit2_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)
Indentation.
> +{
> + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> + if (period < 1) {
> + LOG(ERROR, "VCPU period is out of range, "
> + "valid values are larger than or equal to 1");
> + return 1; /* error scheduling parameter */
Though this is internal function I would very like it to stick to
CODING_STYLE in libxl. In this particular case, the error handling
should be using goto and the return value should be a ERROR_* value.
BTW there is no upper bound check for this value? Just asking -- I don't
know enough to judge.
> + }
> + *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;
Same here.
> + }
> + *sdom_budget = budget;
> + }
> +
> + if (budget > period) {
> + LOG(ERROR, "VCPU budget must be smaller than "
> + "or equal to VCPU period");
> + return 1;
> + }
> +
> + return 0; /* period and budget are valid */
> +}
> +
> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + uint32_t num_vcpus;
> + int rc, i;
> + xc_dominfo_t info;
> + struct xen_domctl_schedparam_vcpu *vcpus;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
According to CODING_STYLE, the return value of a system call or libxc
call should be called "r";
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
Same here, please use goto style error handling.
> + }
> +
> + num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus :
> + info.max_vcpu_id + 1;
> +
Please document the semantics of this function.
> + 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) {
Indentation.
> + 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
The "}" doesn't seem to match the preceding "if". Either this doesn't
compile or the indentation is confusing.
> + 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)
> +{
Again, please document the semantics of this function.
> + int rc;
int r;
And please use it to store return value from xc_ functions.
> + int i;
> + uint16_t max_vcpuid;
> + xc_dominfo_t info;
> + struct xen_domctl_schedparam_vcpu *vcpus;
> + uint32_t num_vcpus;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
Please use goto style error handling.
> + }
> + 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);
> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> + scinfo->vcpus[0].budget,
This doesn't make sense. You take this path because scinfo->num_vcpus is
0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
> + &vcpus[0].s.rtds.period,
> + &vcpus[0].s.rtds.budget))
Indentation.
> + 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);
Indentation.
> + 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)
> {
> @@ -5803,29 +5948,9 @@ 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");
> + if (sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
> + &sdom.period, &sdom.budget))
> return ERROR_INVAL;
> - }
>
> rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
> if (rc < 0) {
> @@ -5836,6 +5961,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)
> {
> @@ -5873,6 +6003,47 @@ 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)
Indentation.
> +{
> + GC_INIT(ctx);
> + libxl_scheduler sched = scinfo->sched;
> + int ret;
ret => rc please.
> +
> + if (sched == LIBXL_SCHEDULER_UNKNOWN)
> + sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (sched) {
> + case LIBXL_SCHEDULER_SEDF:
> + LOG(ERROR, "SEDF scheduler no longer available");
> + ret=ERROR_FEATURE_REMOVED;
Space before and after "=".
> + break;
> + case LIBXL_SCHEDULER_CREDIT:
> + case LIBXL_SCHEDULER_CREDIT2:
> + case LIBXL_SCHEDULER_ARINC653:
> + LOG(ERROR, "per-VCPU parameter setting "
> + "not supported for this scheduler");
Join these two lines please.
> + 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.
> +*/
Indentation.
> int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> libxl_domain_sched_params *scinfo)
> {
> @@ -5907,6 +6078,40 @@ 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)
Indentation.
> +{
> + GC_INIT(ctx);
> + int ret;
According to CODING_STYLE, the return value should be called rc.
> +
> + scinfo->sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (scinfo->sched) {
> + case LIBXL_SCHEDULER_SEDF:
> + LOG(ERROR, "SEDF scheduler no longer available");
is no longer available
> + ret=ERROR_FEATURE_REMOVED;
Space before and after "=" please.
> + break;
> + case LIBXL_SCHEDULER_CREDIT:
> + case LIBXL_SCHEDULER_CREDIT2:
> + case LIBXL_SCHEDULER_ARINC653:
> + LOG(ERROR, "per-VCPU parameter getting "
> + "not supported for this scheduler");
Please join the two string into one. It would be easier for grepping.
> + 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 6b73848..4ba30d3 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -206,6 +206,18 @@
> #define LIBXL_HAVE_DEVICE_MODEL_USER 1
>
> /*
> + * libxl_vcpu_sched_params is used to store per-vcpu params.
> + * The 'vcpuid' field specifies the vcpu to be set or read.
The second sentence doesn't seem to be particularly useful. I think
deleting it would be fine.
The semantics of the function is better documented in the comment
preceding the function prototype or the implementation.
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS indicates RTDS scheduler
> + * now supports per-vcpu settings.
> +*/
> +#define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1
> +
> +/*
> * libxl_domain_build_info has the arm.gic_version field.
> */
> #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1
> @@ -1647,10 +1659,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);
Indentation.
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> + const libxl_vcpu_sched_params *params);
>
Indentation.
> 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 cf3730f..4e7210e 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -378,6 +378,22 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
> ("stream_version", uint32, {'init_val': '1'}),
> ])
>
> +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 [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 4/4] xl: " Chong Li
@ 2016-02-05 14:51 ` Wei Liu
2016-02-09 18:25 ` Dario Faggioli
0 siblings, 1 reply; 24+ messages in thread
From: Wei Liu @ 2016-02-05 14:51 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
xen-devel, Meng Xu, dgolomb
On Thu, Feb 04, 2016 at 04:50:44PM -0600, Chong Li wrote:
> 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 v4:
> 1) Coding style changes
>
> Changes on PATCH v3:
> 1) Support commands, e.g., "xl sched-rtds -d vm1" to output the
> default scheduling parameters
>
> Changes on PATCH v2:
> 1) Remove per-domain output functions for RTDS scheduler.
>
> 2) Users now use '-v all' to specify all VCPUs.
>
> 3) Support outputting a subset of the parameters of the VCPUs
> of a specific domain.
>
> 4) When setting all VCPUs with the same parameters (by only one
> command), no per-domain function is invoked.
>
> 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 | 305 ++++++++++++++++++++++++++++++++++++++++------
> tools/libxl/xl_cmdtable.c | 10 +-
> 3 files changed, 281 insertions(+), 38 deletions(-)
>
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 4279c7c..f9ff917 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1051,6 +1051,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 2b6371d..b843fa5 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5823,6 +5823,39 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
> return 0;
> }
>
> +static int sched_vcpu_get(libxl_scheduler sched, int domid,
> + libxl_vcpu_sched_params *scinfo)
Indentation.
> +{
> + int rc;
> +
> + rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo);
> + if (rc) {
> + fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n");
> + exit(-1);
> + }
> + 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 1;
> + }
> +
> + return 0;
> +}
> +
[...]
> +
> static int sched_rtds_pool_output(uint32_t poolid)
> {
> char *poolname;
> @@ -6015,6 +6080,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)
Indentation.
> +{
> + libxl_dominfo *info;
> + libxl_cpupoolinfo *poolinfo = NULL;
> + uint32_t poolid;
> + int nb_domain, n_pools = 0, i, p;
> + int rc = 0;
[...]
> @@ -6222,77 +6346,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
> + COMMON_LONG_OPTS,
> + {0, 0, 0, 0}
This is not needed because now COMMON_LONG_OPTS includes {0,0,0,0}.
> };
>
> - SWITCH_FOREACH_OPT(opt, "d:p:b:c:", 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) {
xreaalloc can't fail.
> + fprintf(stderr, "Failed to realloc periods\n");
> + rc = 1;
> + goto out;
> + }
> + }
> + periods[p_index++] = strtol(optarg, NULL, 10);
You mix option parsing and validation in same place. I think you need to
clearly separate them. It's very hard to review a function like this
one.
Wei.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-05 14:44 ` Wei Liu
@ 2016-02-05 15:59 ` Dario Faggioli
2016-02-05 16:19 ` Wei Liu
2016-02-06 0:10 ` Chong Li
1 sibling, 1 reply; 24+ messages in thread
From: Dario Faggioli @ 2016-02-05 15:59 UTC (permalink / raw)
To: Wei Liu, Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, ian.jackson, xen-devel,
ian.campbell, Meng Xu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 3837 bytes --]
On Fri, 2016-02-05 at 14:44 +0000, Wei Liu wrote:
> On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote:
> > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> > functions to support per-VCPU settings.
> >
>
> I will need Dario or George to review the logic of the code.
>
Sure, it's on my short TODO list. It's either going to be today or
Monday.
> If some of the comments below don't make sense, just ask. I'm sure I
> make stupid comments at times.
>
Yeah, I'm sure you've said plenty of stupid things! ;-P ;-P
> > +{
> > + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> > + if (period < 1) {
> > + LOG(ERROR, "VCPU period is out of range, "
> > + "valid values are larger than or equal to
> > 1");
> > + return 1; /* error scheduling parameter */
>
> Though this is internal function I would very like it to stick to
> CODING_STYLE in libxl. In this particular case, the error handling
> should be using goto and the return value should be a ERROR_* value.
>
> BTW there is no upper bound check for this value? Just asking -- I
> don't
> know enough to judge.
>
It's checked in the hypervisor. As usual, in these cases, checking in
tools as well would make things more robust, allow better error
reporting, etc, _BUT_ it would require to keep the limits in sync,
which is undesirable.
So, as long as type-related confusion is not a possibility, I would be
ok with no checks here in libxl.
And just to be sure that we are on the safe side wrt that: in Xen these
values are uint32, should we use uint32 here as well (in the idl,
instead of 'integer')?
> > + }
> > + 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);
> > + if (sched_rtds_validate_params(gc, scinfo-
> > >vcpus[0].period,
> > + scinfo->vcpus[0].budget,
>
> This doesn't make sense. You take this path because scinfo->num_vcpus
> is
> 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
>
IIRC, the idea here may be that this is how we set all the vcpus
parameters to the same values... But I'll get back to this when
properly reviewing the series.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-05 15:59 ` Dario Faggioli
@ 2016-02-05 16:19 ` Wei Liu
0 siblings, 0 replies; 24+ messages in thread
From: Wei Liu @ 2016-02-05 16:19 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Sisu Xi, george.dunlap, ian.jackson,
xen-devel, ian.campbell, Meng Xu, Chong Li, dgolomb
On Fri, Feb 05, 2016 at 04:59:43PM +0100, Dario Faggioli wrote:
> On Fri, 2016-02-05 at 14:44 +0000, Wei Liu wrote:
> > On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote:
> > > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> > > functions to support per-VCPU settings.
> > >
> >
> > I will need Dario or George to review the logic of the code.
> >
> Sure, it's on my short TODO list. It's either going to be today or
> Monday.
>
> > If some of the comments below don't make sense, just ask. I'm sure I
> > make stupid comments at times.
> >
> Yeah, I'm sure you've said plenty of stupid things! ;-P ;-P
>
Yeah. My trick is that when I say too many stupid things people don't
know which one to remember so I'm safe. :-)
> > > +{
> > > + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> > > + if (period < 1) {
> > > + LOG(ERROR, "VCPU period is out of range, "
> > > + "valid values are larger than or equal to
> > > 1");
> > > + return 1; /* error scheduling parameter */
> >
> > Though this is internal function I would very like it to stick to
> > CODING_STYLE in libxl. In this particular case, the error handling
> > should be using goto and the return value should be a ERROR_* value.
> >
> > BTW there is no upper bound check for this value? Just asking -- I
> > don't
> > know enough to judge.
> >
> It's checked in the hypervisor. As usual, in these cases, checking in
> tools as well would make things more robust, allow better error
> reporting, etc, _BUT_ it would require to keep the limits in sync,
> which is undesirable.
>
> So, as long as type-related confusion is not a possibility, I would be
> ok with no checks here in libxl.
>
> And just to be sure that we are on the safe side wrt that: in Xen these
> values are uint32, should we use uint32 here as well (in the idl,
> instead of 'integer')?
>
> > > + }
> > > + 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);
> > > + if (sched_rtds_validate_params(gc, scinfo-
> > > >vcpus[0].period,
> > > + scinfo->vcpus[0].budget,
> >
> > This doesn't make sense. You take this path because scinfo->num_vcpus
> > is
> > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
> >
> IIRC, the idea here may be that this is how we set all the vcpus
> parameters to the same values... But I'll get back to this when
> properly reviewing the series.
>
It's one thing that when ->num_vcpus == 0 you allocate array, it's
another when the array is non-NULL but num_vcpus == 0.
Such usage is bad. What if I need to iterate through the array at some
point? How do you know if it is really a NULL array?
Wei.
> Thanks and Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-05 14:44 ` Wei Liu
2016-02-05 15:59 ` Dario Faggioli
@ 2016-02-06 0:10 ` Chong Li
2016-02-08 11:07 ` Wei Liu
1 sibling, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-02-06 0:10 UTC (permalink / raw)
To: Wei Liu
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, Ian Jackson,
xen-devel, Ian Campbell, Meng Xu, Dagaen Golomb
I'll fix these coding style issues.
On Fri, Feb 5, 2016 at 8:44 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote:
>> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
>> functions to support per-VCPU settings.
>>
>
> I will need Dario or George to review the logic of the code.
>
> If some of the comments below don't make sense, just ask. I'm sure I
> make stupid comments at times.
>
>> 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 v4:
>> 1) Coding style changes
>>
>> Changes on PATCH v3:
>> 1) Add sanity check on vcpuid
>>
>> 2) Add comments on per-domain and per-vcpu functions for libxl
>> users
>>
>> Changes on PATCH v2:
>> 1) New data structure (libxl_vcpu_sched_params and libxl_sched_params)
>> to help per-VCPU settings.
>>
>> 2) sched_rtds_vcpu_get now can return a random subset of the parameters
>> of the VCPUs of a specific domain.
>>
>> 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 | 249 ++++++++++++++++++++++++++++++++++++++++----
>> tools/libxl/libxl.h | 19 ++++
>> tools/libxl/libxl_types.idl | 16 +++
>> 3 files changed, 262 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index bd3aac8..ac4a103 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> +
>> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
>> + const libxl_vcpu_sched_params *scinfo)
>> +{
>
> Again, please document the semantics of this function.
>
>> + int rc;
>
> int r;
>
> And please use it to store return value from xc_ functions.
>
>> + int i;
>> + uint16_t max_vcpuid;
>> + xc_dominfo_t info;
>> + struct xen_domctl_schedparam_vcpu *vcpus;
>> + uint32_t num_vcpus;
>> +
>> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
>> + if (rc < 0) {
>> + LOGE(ERROR, "getting domain info");
>> + return ERROR_FAIL;
>
> Please use goto style error handling.
>
>> + }
>> + 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);
>> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
>> + scinfo->vcpus[0].budget,
>
> This doesn't make sense. You take this path because scinfo->num_vcpus is
> 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000"
(which sets all vcpus with
the same scheduling parameters), we pass the budget and period via
scinfo->vcpus[0].
I'll add more explanation here.
>
>> + &vcpus[0].s.rtds.period,
>> + &vcpus[0].s.rtds.budget))
>
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-06 0:10 ` Chong Li
@ 2016-02-08 11:07 ` Wei Liu
2016-02-08 22:59 ` Chong Li
0 siblings, 1 reply; 24+ messages in thread
From: Wei Liu @ 2016-02-08 11:07 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, dario.faggioli,
Ian Jackson, xen-devel, Ian Campbell, Meng Xu, Dagaen Golomb
On Fri, Feb 05, 2016 at 06:10:33PM -0600, Chong Li wrote:
> I'll fix these coding style issues.
>
Thank you. :-)
[...]
> >> + num_vcpus = max_vcpuid + 1;
> >> + GCNEW_ARRAY(vcpus, num_vcpus);
> >> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> >> + scinfo->vcpus[0].budget,
> >
> > This doesn't make sense. You take this path because scinfo->num_vcpus is
> > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
> For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000"
> (which sets all vcpus with
> the same scheduling parameters), we pass the budget and period via
> scinfo->vcpus[0].
>
> I'll add more explanation here.
No, adding more explanation won't help.
Let me explain a bit. Libxl is the library that can be used by multiple
applications. Xl is just one of the applications. The other application
that I know of is libvirt.
So, the incarnation of a particular xl command is of no concern how we
define the semantics of a libxl API. That is, you can come up with an
unambiguous API but still support the same xl command.
Currently the semantics of this (new?) libxl API seems to be broken,
because you're (ab)using num_vcpus to represent a special case. In
effect you can't really whether the array is empty. When num_vcpus is 0,
you shouldn't dereference vcpus array, at all, because the semantics of
num_vcpus == 0 is that the array is empty.
Wei.
> >
> >> + &vcpus[0].s.rtds.period,
> >> + &vcpus[0].s.rtds.budget))
> >
>
> --
> Chong Li
> Department of Computer Science and Engineering
> Washington University in St.louis
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-08 11:07 ` Wei Liu
@ 2016-02-08 22:59 ` Chong Li
2016-02-09 10:19 ` Wei Liu
0 siblings, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-02-08 22:59 UTC (permalink / raw)
To: Wei Liu
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, Ian Jackson,
xen-devel, Ian Campbell, Meng Xu, Dagaen Golomb
On Mon, Feb 8, 2016 at 5:07 AM, Wei Liu <wei.liu2@citrix.com> wrote:
>
>
> [...]
> > >> + num_vcpus = max_vcpuid + 1;
> > >> + GCNEW_ARRAY(vcpus, num_vcpus);
> > >> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> > >> + scinfo->vcpus[0].budget,
> > >
> > > This doesn't make sense. You take this path because scinfo->num_vcpus is
> > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
> > For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000"
> > (which sets all vcpus with
> > the same scheduling parameters), we pass the budget and period via
> > scinfo->vcpus[0].
> >
> > I'll add more explanation here.
>
> No, adding more explanation won't help.
>
> Let me explain a bit. Libxl is the library that can be used by multiple
> applications. Xl is just one of the applications. The other application
> that I know of is libvirt.
>
> So, the incarnation of a particular xl command is of no concern how we
> define the semantics of a libxl API. That is, you can come up with an
> unambiguous API but still support the same xl command.
>
> Currently the semantics of this (new?) libxl API seems to be broken,
> because you're (ab)using num_vcpus to represent a special case. In
> effect you can't really whether the array is empty. When num_vcpus is 0,
> you shouldn't dereference vcpus array, at all, because the semantics of
> num_vcpus == 0 is that the array is empty.
>
> Wei.
I see. I'll think about re-designing the data structure of
libxl_vcpu_sched_params.
Chong
>
> > >
> > >> + &vcpus[0].s.rtds.period,
> > >> + &vcpus[0].s.rtds.budget))
> > >
> >
> > --
> > Chong Li
> > Department of Computer Science and Engineering
> > Washington University in St.louis
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-08 22:59 ` Chong Li
@ 2016-02-09 10:19 ` Wei Liu
2016-02-09 11:05 ` Dario Faggioli
0 siblings, 1 reply; 24+ messages in thread
From: Wei Liu @ 2016-02-09 10:19 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, dario.faggioli,
Ian Jackson, xen-devel, Ian Campbell, Meng Xu, Dagaen Golomb
On Mon, Feb 08, 2016 at 04:59:46PM -0600, Chong Li wrote:
> On Mon, Feb 8, 2016 at 5:07 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> >
> >
> > [...]
> > > >> + num_vcpus = max_vcpuid + 1;
> > > >> + GCNEW_ARRAY(vcpus, num_vcpus);
> > > >> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> > > >> + scinfo->vcpus[0].budget,
> > > >
> > > > This doesn't make sense. You take this path because scinfo->num_vcpus is
> > > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
> > > For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000"
> > > (which sets all vcpus with
> > > the same scheduling parameters), we pass the budget and period via
> > > scinfo->vcpus[0].
> > >
> > > I'll add more explanation here.
> >
> > No, adding more explanation won't help.
> >
> > Let me explain a bit. Libxl is the library that can be used by multiple
> > applications. Xl is just one of the applications. The other application
> > that I know of is libvirt.
> >
> > So, the incarnation of a particular xl command is of no concern how we
> > define the semantics of a libxl API. That is, you can come up with an
> > unambiguous API but still support the same xl command.
> >
> > Currently the semantics of this (new?) libxl API seems to be broken,
> > because you're (ab)using num_vcpus to represent a special case. In
> > effect you can't really whether the array is empty. When num_vcpus is 0,
> > you shouldn't dereference vcpus array, at all, because the semantics of
> > num_vcpus == 0 is that the array is empty.
> >
> > Wei.
>
> I see. I'll think about re-designing the data structure of
> libxl_vcpu_sched_params.
>
Or you can come up with a new API (function) that sets parameter for all
vcpus at once? Just a random thought.
You can post your proposed data structure and / or the API here. We
can discuss this a bit before you actually start writing code, so that
you avoid wasting effort.
Wei.
> Chong
> >
> > > >
> > > >> + &vcpus[0].s.rtds.period,
> > > >> + &vcpus[0].s.rtds.budget))
> > > >
> > >
> > > --
> > > Chong Li
> > > Department of Computer Science and Engineering
> > > Washington University in St.louis
>
>
>
>
> --
> Chong Li
> Department of Computer Science and Engineering
> Washington University in St.louis
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-09 10:19 ` Wei Liu
@ 2016-02-09 11:05 ` Dario Faggioli
0 siblings, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-02-09 11:05 UTC (permalink / raw)
To: Wei Liu, Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, Ian Jackson, xen-devel,
Ian Campbell, Meng Xu, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 977 bytes --]
On Tue, 2016-02-09 at 10:19 +0000, Wei Liu wrote:
> On Mon, Feb 08, 2016 at 04:59:46PM -0600, Chong Li wrote:
> >
> > I see. I'll think about re-designing the data structure of
> > libxl_vcpu_sched_params.
> >
>
> Or you can come up with a new API (function) that sets parameter for
> all
> vcpus at once? Just a random thought.
>
Exactly!
> You can post your proposed data structure and / or the API here. We
> can discuss this a bit before you actually start writing code, so
> that
> you avoid wasting effort.
>
I'm looking at the patches right now, and I don't think there is the
need to redesign the data structures!
I'll send in my comments as soon as I'm done...
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] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 3/4] libxl: " Chong Li
2016-02-05 14:44 ` Wei Liu
@ 2016-02-09 12:00 ` Dario Faggioli
2016-02-09 16:48 ` Chong Li
2016-02-09 17:38 ` Wei Liu
1 sibling, 2 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-02-09 12:00 UTC (permalink / raw)
To: Chong Li, xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, ian.jackson,
ian.campbell, Meng Xu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 7793 bytes --]
On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote:
>
> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
> + const libxl_vcpu_sched_params
> *scinfo)
>
I'd call this sched_rtds_vcpus_params_set().
I know, it's longer, which is bad, but it's a lot more evident what it
does.
> +{
> + int rc;
> + int i;
> + uint16_t max_vcpuid;
> + xc_dominfo_t info;
> + struct xen_domctl_schedparam_vcpu *vcpus;
> + uint32_t 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 {
>
So, it looks to me that this function can be split in two. One would be
the actual sched_rtds_vcpus_params_set(), and it will do what is being
done above here.
The other one would be something like
sched_rtds_vcpus_params_set_all(), and it will do what is being done
below here.
About scinfo->num_vcpus, I think it would be fine for
sched_rtds_vcpus_params_set() to enforce it being > 0, and erroring out
if not.
On the other hand, in sched_rtds_vcpus_params_set_all(), since the
semantic is "use this set of params for all vcpus", I think it would be
fine to enforce scinfo->num_vcpus == 1 (and maybe even
scinfo.vcpus[0].vcpuid == LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT).
Now, for external callers (like xl, but also like any other toolstack
wanting to build on top of libxl).
If you think a 'set all vcpus' function would be useufl (as it is
probably the case), you can define a libxl API function called
libxl_vcpus_params_set_all(), doing exactly the same thing that
libxl_vcpus_params_set() is doing, but calling the
sched_rtds_vcpus_params_set_all() internal function.
Chong, do you think this could work?
Wei, what do you think of the resulting API?
> + num_vcpus = max_vcpuid + 1;
> + GCNEW_ARRAY(vcpus, num_vcpus);
> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> + scinfo->vcpus[0].budget,
> + &vcpus[0].s.rtds.period,
> + &vcpus[0].s.rtds.budget))
> + 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 != 0) {
> + LOGE(ERROR, "setting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + return rc;
> +}
> @@ -5836,6 +5961,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)
>
I think this comment would be better put in libxl.h.
> @@ -5873,6 +6003,47 @@ int libxl_domain_sched_params_set(libxl_ctx
> *ctx, uint32_t domid,
>
> +/* 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)
>
(same as above: move in libxl.h)
> diff --git a/tools/libxl/libxl_types.idl
> b/tools/libxl/libxl_types.idl
> index cf3730f..4e7210e 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -378,6 +378,22 @@ libxl_domain_restore_params =
> Struct("domain_restore_params", [
> ("stream_version", uint32, {'init_val': '1'}),
> ])
>
> +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'}),
>
So, 'slice', 'latency' and 'extratime' are not in use by any scheduler.
They were for SEDF, which is now removed from all the places where we
could remove it, and deprecated elsewhere (e.g., in the definition
of libxl_domain_sched_params, here in this file).
So, unless we want to keep this struct sort-of in sync
with libxl_domain_sched_params, I think we don't need to have these
fields here.
I defer this to the tools maintainers, in case there is something I'm
missing, but I'd say we can get rid of them from here.
> + ("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'}),
>
(Apart from the nit above) This looks ok to me as a set of data
structures.
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] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-09 12:00 ` Dario Faggioli
@ 2016-02-09 16:48 ` Chong Li
2016-02-09 17:38 ` Wei Liu
1 sibling, 0 replies; 24+ messages in thread
From: Chong Li @ 2016-02-09 16:48 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, Feb 9, 2016 at 6:00 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote:
>>
>> +{
>> + int rc;
>> + int i;
>> + uint16_t max_vcpuid;
>> + xc_dominfo_t info;
>> + struct xen_domctl_schedparam_vcpu *vcpus;
>> + uint32_t 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 {
>>
> So, it looks to me that this function can be split in two. One would be
> the actual sched_rtds_vcpus_params_set(), and it will do what is being
> done above here.
>
> The other one would be something like
> sched_rtds_vcpus_params_set_all(), and it will do what is being done
> below here.
>
> About scinfo->num_vcpus, I think it would be fine for
> sched_rtds_vcpus_params_set() to enforce it being > 0, and erroring out
> if not.
>
> On the other hand, in sched_rtds_vcpus_params_set_all(), since the
> semantic is "use this set of params for all vcpus", I think it would be
> fine to enforce scinfo->num_vcpus == 1 (and maybe even
> scinfo.vcpus[0].vcpuid == LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT).
>
>
> Now, for external callers (like xl, but also like any other toolstack
> wanting to build on top of libxl).
>
> If you think a 'set all vcpus' function would be useufl (as it is
> probably the case), you can define a libxl API function called
> libxl_vcpus_params_set_all(), doing exactly the same thing that
> libxl_vcpus_params_set() is doing, but calling the
> sched_rtds_vcpus_params_set_all() internal function.
>
> Chong, do you think this could work?
I think it would work. Thanks for this suggestion.
Chong
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-09 12:00 ` Dario Faggioli
2016-02-09 16:48 ` Chong Li
@ 2016-02-09 17:38 ` Wei Liu
1 sibling, 0 replies; 24+ messages in thread
From: Wei Liu @ 2016-02-09 17:38 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, ian.jackson,
xen-devel, ian.campbell, Meng Xu, Chong Li, dgolomb
On Tue, Feb 09, 2016 at 01:00:37PM +0100, Dario Faggioli wrote:
[...]
> So, it looks to me that this function can be split in two. One would be
> the actual sched_rtds_vcpus_params_set(), and it will do what is being
> done above here.
>
> The other one would be something like
> sched_rtds_vcpus_params_set_all(), and it will do what is being done
> below here.
>
> About scinfo->num_vcpus, I think it would be fine for
> sched_rtds_vcpus_params_set() to enforce it being > 0, and erroring out
> if not.
>
> On the other hand, in sched_rtds_vcpus_params_set_all(), since the
> semantic is "use this set of params for all vcpus", I think it would be
> fine to enforce scinfo->num_vcpus == 1 (and maybe even
> scinfo.vcpus[0].vcpuid == LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT).
>
>
> Now, for external callers (like xl, but also like any other toolstack
> wanting to build on top of libxl).
>
> If you think a 'set all vcpus' function would be useufl (as it is
> probably the case), you can define a libxl API function called
> libxl_vcpus_params_set_all(), doing exactly the same thing that
> libxl_vcpus_params_set() is doing, but calling the
> sched_rtds_vcpus_params_set_all() internal function.
>
> Chong, do you think this could work?
> Wei, what do you think of the resulting API?
Introducing a _all function sounds reasonable.
Wei.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-02-09 18:17 ` Dario Faggioli
2016-03-01 17:58 ` Chong Li
0 siblings, 1 reply; 24+ messages in thread
From: Dario Faggioli @ 2016-02-09 18:17 UTC (permalink / raw)
To: Chong Li, xen-devel
Cc: Chong Li, Sisu Xi, george.dunlap, Meng Xu, jbeulich, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 14716 bytes --]
On Thu, 2016-02-04 at 16:50 -0600, 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
>
> 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 v4:
> 1) Add uint32_t vcpu_index to struct xen_domctl_scheduler_op.
> When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we call
> hypercall_preemption_check in case the current hypercall lasts
> too long. If we decide to preempt the current hypercall, we record
> the index of the most-recent finished vcpu into the vcpu_index of
> struct xen_domctl_scheduler_op. So when we resume the hypercall after
> preemption, we start processing from the posion specified by
> vcpu_index,
> and don't need to repeat the work that has already been done in the
> hypercall before the preemption.
> (This design is based on the do_grant_table_op() in grant_table.c)
>
Ok.
This now looks like it could be fine now. However, I remember asking
you to look at how XEN_SYSCTL_pcitopoinfo is handled, because I thought
that way of doing things is a better fit for this usecase.
So, I've got to ask to check that and give it a try implementing things
that way.
In fact, apart from being a better suited arrangement of the code, it
should, if I'm not mistaken, allow you to avoid having to introduce the
vcpu_index field in the domctl struct.
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 46b967e..b294221 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -847,9 +847,14 @@ 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;
> + }
>
As said, have a look at the XEN_SYSCTL_pcitopoinfo case in do_sysctl(),
and at xc_pcitopoinfo().
That basically works by re-issueing the hypercall in libxc, rather than
on create_continuation.
Elaborating a bit more, I think that is a better fit in this case
because:
- like in that case, you know how many vcpus you want to act upon in
libxc already, and you can take advantage of that in there;
- it makes the xc_ call do something useful;
- it avoids (again, if I'm not wrong) having to clobber the Xen
interface with a useless and weirdly-looking vcpu_index field;
- it makes it very easy to process a bunch of vcpus, and then check
for preemption, instead of checking for that at each iteration
(of course, this can be implemented with the 'standard' contination
approach as well, but again, I like how it's done there).
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 3f1d047..34ae48d 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -86,8 +86,21 @@
> #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
> + */
>
Comments like these are not super useful. You need to explain why you
are using the maximum delta ("because period is added to the time a
vcpu activates, so this must not overflow etc..."), and express a
little bit better what you mean with "considering the scheduling
overhead".
Moreover...
> +#define RTDS_MAX_PERIOD (STIME_DELTA_MAX)
> +#define RTDS_MIN_PERIOD (MICROSECS(10))
> +
Comment says 100us, but then it's 10?
> +/*
> + * Min budget: 100 us
> + */
> +#define RTDS_MIN_BUDGET (MICROSECS(10))
> +
As above, why? (scheduling overhead again, I would say).
> #define UPDATE_LIMIT_SHIFT 10
> #define MAX_SCHEDULE (MILLISECS(1))
> +
>
Spurious blank line addition.
> /*
> * Flags
> */
> @@ -1163,6 +1168,94 @@ rt_dom_cntl(
> }
> spin_unlock_irqrestore(&prv->lock, flags);
> break;
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus;
> index++ )
> + {
> + spin_lock_irqsave(&prv->lock, flags);
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
>
vcpus is an handle, and it needs to be validated with
guest_handle_is_null().
> + {
> + rc = -EFAULT;
> + spin_unlock_irqrestore(&prv->lock, flags);
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + spin_unlock_irqrestore(&prv->lock, flags);
> + 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;
> + spin_unlock_irqrestore(&prv->lock, flags);
> + break;
>
In any case, i.e., no matter whether the code keeps using this
approach, or becomes more similar to XEN_SYSCTL_pcitopoinfo, can these
failing paths be factored instead than repeated?
> + }
> + spin_unlock_irqrestore(&prv->lock, flags);
> + if ( hypercall_preempt_check() )
> + {
> + op->u.v.vcpu_index = index + 1;
> + /* hypercall (after preemption) will continue at
> vcpu_index */
> + rc = -ERESTART;
> + break;
> + }
> + }
> + break;
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
>
All what I've said about adopting the XEN_SYSCTL_pcitopoinfo, handle
validation and (attempting to) factoring the error paths applies to
both get and put.
> + for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus;
> index++ )
> + {
> + spin_lock_irqsave(&prv->lock, flags);
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
> + {
> + rc = -EFAULT;
> + spin_unlock_irqrestore(&prv->lock, flags);
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + spin_unlock_irqrestore(&prv->lock, flags);
> + 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 > RTDS_MAX_PERIOD || budget <
> RTDS_MIN_BUDGET ||
> + budget > period )
>
Isn't checking against RTDS_MIN_PERIOD missing?
> + {
> + rc = -EINVAL;
> + spin_unlock_irqrestore(&prv->lock, flags);
> + break;
> + }
> +
> + /*
> + * We accept period/budget less than 100 us, but will
> warn users about
> + * the large scheduling overhead due to it
> + */
> + if ( period < MICROSECS(100) || budget < MICROSECS(100)
> )
> + printk("Warning: period/budget less than 100 micro-
> secs "
> + "results in large scheduling overhead.\n");
> +
"WARNING: period or budget set to less than 100us.\n"
" This may result in high scheduling overhead"
Or something like this. But try to make the two lines as independent as
possible. In general, we try not to break these messages, so that
grepping the source code for any substring of the will work. In this
case, we indeed need to break this, but we still keep grep-ability in
mind.
See, for example, sched_credit2:2147.
> + svc->period = period;
> + svc->budget = budget;
> + spin_unlock_irqrestore(&prv->lock, flags);
> + if ( hypercall_preempt_check() )
> + {
> + op->u.v.vcpu_index = index + 1;
> + rc = -ERESTART;
> + break;
> + }
> + }
> + break;
> }
>
> return rc;
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index c195129..f4a4032 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1148,10 +1148,19 @@ 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:
> + case XEN_DOMCTL_SCHEDOP_getinfo:
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + break;
> + default:
> + return -EINVAL;
> + }
>
Maybe I'm misremembering (in which case, sorry), was using a switch
like this instead of an if suggested during one of the previous rounds?
> diff --git a/xen/include/public/domctl.h
> b/xen/include/public/domctl.h
> index 7a56b3f..6f429ec 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -338,24 +338,57 @@
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
> #define XEN_SCHEDULER_ARINC653 7
> #define XEN_SCHEDULER_RTDS 8
>
> -/* Set or get info? */
> +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[3];
> +} xen_domctl_schedparam_vcpu_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
> +
> +/* Set or get info?
>
Comment style (opening '/*').
> + * For schedulers supporting per-vcpu settings (e.g., RTDS):
> + * using XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
>
kill the "using", and maybe indent the three lines by 1 and/or put a
marker (like '-' or '+') to make this looks like a bulleted list.
> + * using XEN_DOMCTL_SCHEDOP_getinfo gets default params;
> + * using XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of
> vcpus;
>
I'd leave an empty line here (still within the comment).
> + * For schedulers not supporting per-vcpu settings:
> + * using XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
> + * using XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params;
>
The wording of these two items above should be the same.
> + * using XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error code;
>
Just "returns error".
> + */
> #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_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_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;
> + uint32_t nr_vcpus;
> + uint32_t vcpu_index;
> + } v;
> } u;
> };
> typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2016-02-05 14:09 ` Wei Liu
@ 2016-02-09 18:20 ` Dario Faggioli
0 siblings, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-02-09 18:20 UTC (permalink / raw)
To: Wei Liu, Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, xen-devel, Meng Xu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 1046 bytes --]
On Fri, 2016-02-05 at 14:09 +0000, Wei Liu wrote:
> On Thu, Feb 04, 2016 at 04:50:42PM -0600, Chong Li wrote:
> > 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>
>
> These looks like sensible wrappers. I will defer this patch to Dario.
> If
> he's happy with this I will just ack it.
>
They seem fine to me as well.
However, as said when reviewing patch 1, I'd like to see an attempt for
these hypercalls to be dealt with in the same way as
XEN_SYSCTL_pcitopoinfo is.
If doing that, these wrappers need to be changed accordingly.
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] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2016-02-05 14:51 ` Wei Liu
@ 2016-02-09 18:25 ` Dario Faggioli
0 siblings, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-02-09 18:25 UTC (permalink / raw)
To: Wei Liu, Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, xen-devel, Meng Xu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 2344 bytes --]
On Fri, 2016-02-05 at 14:51 +0000, Wei Liu wrote:
> On Thu, Feb 04, 2016 at 04:50:44PM -0600, Chong Li wrote:
> >
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 2b6371d..b843fa5 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> >
> > - SWITCH_FOREACH_OPT(opt, "d:p:b:c:", 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) {
>
> xreaalloc can't fail.
>
> > + fprintf(stderr, "Failed to realloc periods\n");
> > + rc = 1;
> > + goto out;
> > + }
> > + }
> > + periods[p_index++] = strtol(optarg, NULL, 10);
>
> You mix option parsing and validation in same place. I think you need
> to
> clearly separate them. It's very hard to review a function like this
> one.
>
Exactly.
So, considering this, and the fact that the libxl API is changing (the
_set_all() function is being added), I don't think there is much point
in reviewing this version of this patch.
So, good work following up on this! I know, it takes more time than one
would think... but interfaces are very important, so it's normal it
takes a bit to get them right (and convince people that that's the
case! :-P).
Looking forward to v6.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-02-09 18:17 ` Dario Faggioli
@ 2016-03-01 17:58 ` Chong Li
2016-03-02 13:36 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-03-01 17:58 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Jan Beulich, Dagaen Golomb
On Tue, Feb 9, 2016 at 12:17 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Thu, 2016-02-04 at 16:50 -0600, 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
>>
>> 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 v4:
>> 1) Add uint32_t vcpu_index to struct xen_domctl_scheduler_op.
>> When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we call
>> hypercall_preemption_check in case the current hypercall lasts
>> too long. If we decide to preempt the current hypercall, we record
>> the index of the most-recent finished vcpu into the vcpu_index of
>> struct xen_domctl_scheduler_op. So when we resume the hypercall after
>> preemption, we start processing from the posion specified by
>> vcpu_index,
>> and don't need to repeat the work that has already been done in the
>> hypercall before the preemption.
>> (This design is based on the do_grant_table_op() in grant_table.c)
>>
>
>> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
>> index 3f1d047..34ae48d 100644
>> --- a/xen/common/sched_rt.c
>> +++ b/xen/common/sched_rt.c
>
>> + for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus;
>> index++ )
>> + {
>> + spin_lock_irqsave(&prv->lock, flags);
>> + if ( copy_from_guest_offset(&local_sched,
>> + op->u.v.vcpus, index, 1) )
>> + {
>> + rc = -EFAULT;
>> + spin_unlock_irqrestore(&prv->lock, flags);
>> + break;
>> + }
>> + if ( local_sched.vcpuid >= d->max_vcpus ||
>> + d->vcpu[local_sched.vcpuid] == NULL )
>> + {
>> + rc = -EINVAL;
>> + spin_unlock_irqrestore(&prv->lock, flags);
>> + 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 > RTDS_MAX_PERIOD || budget <
>> RTDS_MIN_BUDGET ||
>> + budget > period )
>>
> Isn't checking against RTDS_MIN_PERIOD missing?
Because RTDS_MIN_PERIOD==RTDS_MIN_BUDGET, by checking budget <
RTDS_MIN_BUDGET and budget > period, the checking against
RTDS_MIN_PERIOD is already covered.
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index c195129..f4a4032 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1148,10 +1148,19 @@ 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:
>> + case XEN_DOMCTL_SCHEDOP_getinfo:
>> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
>> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>>
> Maybe I'm misremembering (in which case, sorry), was using a switch
> like this instead of an if suggested during one of the previous rounds?
>
Yes. In patch v3, Jan suggested that.
--
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-01 17:58 ` Chong Li
@ 2016-03-02 13:36 ` Jan Beulich
2016-03-02 14:06 ` George Dunlap
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2016-03-02 13:36 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, Dario Faggioli, xen-devel,
Meng Xu, Dagaen Golomb
>>> On 01.03.16 at 18:58, <lichong659@gmail.com> wrote:
> On Tue, Feb 9, 2016 at 12:17 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
>> On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote:
>>> --- a/xen/common/sched_rt.c
>>> +++ b/xen/common/sched_rt.c
>
>>
>>> + for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus;
>>> index++ )
>>> + {
>>> + spin_lock_irqsave(&prv->lock, flags);
>>> + if ( copy_from_guest_offset(&local_sched,
>>> + op->u.v.vcpus, index, 1) )
>>> + {
>>> + rc = -EFAULT;
>>> + spin_unlock_irqrestore(&prv->lock, flags);
>>> + break;
>>> + }
>>> + if ( local_sched.vcpuid >= d->max_vcpus ||
>>> + d->vcpu[local_sched.vcpuid] == NULL )
>>> + {
>>> + rc = -EINVAL;
>>> + spin_unlock_irqrestore(&prv->lock, flags);
>>> + 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 > RTDS_MAX_PERIOD || budget <
>>> RTDS_MIN_BUDGET ||
>>> + budget > period )
>>>
>> Isn't checking against RTDS_MIN_PERIOD missing?
>
> Because RTDS_MIN_PERIOD==RTDS_MIN_BUDGET, by checking budget <
> RTDS_MIN_BUDGET and budget > period, the checking against
> RTDS_MIN_PERIOD is already covered.
If you make code dependent upon such value matches, the
dependency should be documented and enforced to be
noticed if broken by a BUILD_BUG_ON().
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-02 13:36 ` Jan Beulich
@ 2016-03-02 14:06 ` George Dunlap
0 siblings, 0 replies; 24+ messages in thread
From: George Dunlap @ 2016-03-02 14:06 UTC (permalink / raw)
To: Jan Beulich, Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, Dario Faggioli, xen-devel,
Meng Xu, Dagaen Golomb
On 02/03/16 13:36, Jan Beulich wrote:
>>>> On 01.03.16 at 18:58, <lichong659@gmail.com> wrote:
>> On Tue, Feb 9, 2016 at 12:17 PM, Dario Faggioli
>> <dario.faggioli@citrix.com> wrote:
>>> On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote:
>>>> --- a/xen/common/sched_rt.c
>>>> +++ b/xen/common/sched_rt.c
>>
>>>
>>>> + for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus;
>>>> index++ )
>>>> + {
>>>> + spin_lock_irqsave(&prv->lock, flags);
>>>> + if ( copy_from_guest_offset(&local_sched,
>>>> + op->u.v.vcpus, index, 1) )
>>>> + {
>>>> + rc = -EFAULT;
>>>> + spin_unlock_irqrestore(&prv->lock, flags);
>>>> + break;
>>>> + }
>>>> + if ( local_sched.vcpuid >= d->max_vcpus ||
>>>> + d->vcpu[local_sched.vcpuid] == NULL )
>>>> + {
>>>> + rc = -EINVAL;
>>>> + spin_unlock_irqrestore(&prv->lock, flags);
>>>> + 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 > RTDS_MAX_PERIOD || budget <
>>>> RTDS_MIN_BUDGET ||
>>>> + budget > period )
>>>>
>>> Isn't checking against RTDS_MIN_PERIOD missing?
>>
>> Because RTDS_MIN_PERIOD==RTDS_MIN_BUDGET, by checking budget <
>> RTDS_MIN_BUDGET and budget > period, the checking against
>> RTDS_MIN_PERIOD is already covered.
>
> If you make code dependent upon such value matches, the
> dependency should be documented and enforced to be
> noticed if broken by a BUILD_BUG_ON().
To expand upon this:
Code changes. At the moment RTDS_MIN_PERIOD == RTDS_MIN_BUDGET, but the
very fact that you have two different macros implies to anyone coming
along later that you can change one. If someone does change one but not
the other, then that will create a bug in the program which will be very
difficult to detect. It is likely not to be noticed during patch review
(since it probably won't change the code you're now introducing), and it
may not even be noticed in follow-up testing for some time.
After you've been bitten several times by this sort of bug, you learn to
be paranoid about this sort of thing (which is why Dario noticed it).
Two ways to proceed:
1. Don't assume RTDS_MIN_PERIOD == RTDS_MIN_BUDGET here, and add the
extra check Dario mentioned.
2. Assume RTDS_MIN_PERIOD == RTDS_MIN_BUDGET, and add something to the
code which will break the build if this is ever false (as Jan suggested).
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2016-03-02 14:06 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-04 22:50 [PATCH v5 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 1/4] xen: enable " Chong Li
2016-02-09 18:17 ` Dario Faggioli
2016-03-01 17:58 ` Chong Li
2016-03-02 13:36 ` Jan Beulich
2016-03-02 14:06 ` George Dunlap
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 2/4] libxc: " Chong Li
2016-02-05 14:09 ` Wei Liu
2016-02-09 18:20 ` Dario Faggioli
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 3/4] libxl: " Chong Li
2016-02-05 14:44 ` Wei Liu
2016-02-05 15:59 ` Dario Faggioli
2016-02-05 16:19 ` Wei Liu
2016-02-06 0:10 ` Chong Li
2016-02-08 11:07 ` Wei Liu
2016-02-08 22:59 ` Chong Li
2016-02-09 10:19 ` Wei Liu
2016-02-09 11:05 ` Dario Faggioli
2016-02-09 12:00 ` Dario Faggioli
2016-02-09 16:48 ` Chong Li
2016-02-09 17:38 ` Wei Liu
2016-02-04 22:50 ` [PATCH v5 for Xen 4.7 4/4] xl: " Chong Li
2016-02-05 14:51 ` Wei Liu
2016-02-09 18:25 ` Dario Faggioli
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.