* [PATCH v7 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler
@ 2016-03-16 16:47 Chong Li
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 1/4] xen: enable " Chong Li
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Chong Li @ 2016-03-16 16:47 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.7. The interface design of the xl sched-rtds tool is based on
Meng's previous discussion with Dario, George and Wei
(http://lists.xen.org/archives/html/xen-devel/2015-02/msg02606.html).
Basically, there are two main changes:
1) in xl, we create an array that records all VCPUs whose parameters
are about to modify or output.
2) in libxl, we receive the array and call different xc functions to
handle it.
3) in xen and libxc, we use
XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo(introduced by this
patchset) as the hypercall for per-VCPU operations(get/set method).
[Usage]
With this patchset in use, xl sched-rtds tool can:
1) show the budget and period of each VCPU of each domain,
by using "xl sched-rtds -v all" command. An example would be like:
# xl sched-rtds -v all
Cpupool Pool-0: sched=RTDS
Name ID VCPU Period Budget
Domain-0 0 0 10000 4000
vm1 1 0 300 150
vm1 1 1 400 200
vm1 1 2 10000 4000
vm1 1 3 1000 500
vm2 2 0 10000 4000
vm2 2 1 10000 4000
Using "xl sched-rtds" will output the default scheduling parameters
for each domain. An example would be like:
# xl sched-rtds
Cpupool Pool-0: sched=RTDS
Name ID Period Budget
Domain-0 0 10000 4000
vm1 1 10000 4000
vm2 2 10000 4000
2) show the budget and period of each VCPU of a specific domain,
by using, e.g., "xl sched-rtds -d vm1 -v all" command. The output
would be like:
# xl sched-rtds -d vm1 -v all
Name ID VCPU Period Budget
vm1 1 0 300 150
vm1 1 1 400 200
vm1 1 2 10000 4000
vm1 1 3 1000 500
To show a subset of the parameters of the VCPUs of a specific domain,
please use, e.g.,"xl sched-rtds -d vm1 -v 0 -v 3" command.
The output would be:
# xl sched-rtds -d vm1 -v 0 -v 3
Name ID VCPU Period Budget
vm1 1 0 300 150
vm1 1 3 1000 500
Using command, e.g., "xl sched-rtds -d vm1" will output the default
scheduling parameters of vm1. An example would be like:
# xl sched-rtds -d vm1
Name ID Period Budget
vm1 1 10000 4000
3) Users can set the budget and period of multiple VCPUs of a
specific domain with only one command,
e.g., "xl sched-rtds -d vm1 -v 0 -p 100 -b 50 -v 3 -p 300 -b 150".
Users can set all VCPUs with the same parameters, by one command.
e.g., "xl sched-rtds -d vm1 -v all -p 500 -b 250".
---
CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <jbeulich@suse.com>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
CC: <ian.jackson@eu.citrix.com>
CC: <ian.campbell@eu.citrix.com>
Chong Li (4):
xen: enable per-VCPU parameter settings for RTDS scheduler
libxc: enable per-VCPU parameter settings for RTDS scheduler
libxl: enable per-VCPU parameter settings for RTDS scheduler
xl: enable per-VCPU parameter settings for RTDS scheduler
docs/man/xl.pod.1 | 38 +++++
tools/libxc/include/xenctrl.h | 8 ++
tools/libxc/xc_rt.c | 68 +++++++++
tools/libxl/libxl.c | 321 +++++++++++++++++++++++++++++++++++++++---
tools/libxl/libxl.h | 37 +++++
tools/libxl/libxl_types.idl | 14 ++
tools/libxl/xl_cmdimpl.c | 301 ++++++++++++++++++++++++++++++++++-----
tools/libxl/xl_cmdtable.c | 16 ++-
xen/common/sched_credit.c | 17 ++-
xen/common/sched_credit2.c | 16 ++-
xen/common/sched_rt.c | 114 +++++++++++++--
xen/common/schedule.c | 15 +-
xen/include/public/domctl.h | 63 +++++++--
13 files changed, 928 insertions(+), 100 deletions(-)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 [PATCH v7 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
@ 2016-03-16 16:47 ` Chong Li
2016-03-17 10:03 ` Dario Faggioli
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 2/4] libxc: " Chong Li
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Chong Li @ 2016-03-16 16:47 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 v6:
1) Add explain for nr_vcpus in struct xen_domctl_scheduler_op, because it is used
in both IN and OUT ways.
2) Remove the check and warning for vcpu settings with low budget or budget. Making this
feature "per-domain" or "per-operation" is one of the future work.
3) In the *cntl() functions in sched_credit.c and sched_credit2.c, change the "if-else"
structure to "switch" structure.
4) In rt_dom_cntl(), use copy_to_guest* instead of __copy_to_guest*, because the latter one
requires lock protection.
Changes on PATCH v5:
1) When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we do
preemption check in a similar way to XEN_SYSCTL_pcitopoinfo
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/sched_credit.c | 17 ++++---
xen/common/sched_credit2.c | 16 ++++---
xen/common/sched_rt.c | 114 ++++++++++++++++++++++++++++++++++++++------
xen/common/schedule.c | 15 ++++--
xen/include/public/domctl.h | 63 +++++++++++++++++++-----
5 files changed, 183 insertions(+), 42 deletions(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 0dce790..82b0d14 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1054,15 +1054,16 @@ csched_dom_cntl(
* lock. Runq lock not needed anywhere in here. */
spin_lock_irqsave(&prv->lock, flags);
- if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
+ switch ( op->cmd )
{
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ return -EINVAL;
+ case XEN_DOMCTL_SCHEDOP_getinfo:
op->u.credit.weight = sdom->weight;
op->u.credit.cap = sdom->cap;
- }
- else
- {
- ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
-
+ break;
+ case XEN_DOMCTL_SCHEDOP_putinfo:
if ( op->u.credit.weight != 0 )
{
if ( !list_empty(&sdom->active_sdom_elem) )
@@ -1075,7 +1076,9 @@ csched_dom_cntl(
if ( op->u.credit.cap != (uint16_t)~0U )
sdom->cap = op->u.credit.cap;
-
+ break;
+ default:
+ return -EINVAL;
}
spin_unlock_irqrestore(&prv->lock, flags);
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 3c49ffa..46d54bc 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1421,14 +1421,15 @@ csched2_dom_cntl(
* runq lock to update csvcs. */
spin_lock_irqsave(&prv->lock, flags);
- if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
+ switch ( op->cmd )
{
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ return -EINVAL;
+ case XEN_DOMCTL_SCHEDOP_getinfo:
op->u.credit2.weight = sdom->weight;
- }
- else
- {
- ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
-
+ break;
+ case XEN_DOMCTL_SCHEDOP_putinfo:
if ( op->u.credit2.weight != 0 )
{
struct vcpu *v;
@@ -1457,6 +1458,9 @@ csched2_dom_cntl(
vcpu_schedule_unlock(lock, svc->vcpu);
}
}
+ break;
+ default:
+ return -EINVAL;
}
spin_unlock_irqrestore(&prv->lock, flags);
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 3f1d047..359c2db 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -86,6 +86,22 @@
#define RTDS_DEFAULT_PERIOD (MICROSECS(10000))
#define RTDS_DEFAULT_BUDGET (MICROSECS(4000))
+/*
+ * Max period: max delta of time type, because period is added to the time
+ * a vcpu activates, so this must not overflow.
+ * Min period: 10 us, considering the scheduling overhead (when period is
+ * too low, scheduling is invoked too frequently, causing high overhead).
+ */
+#define RTDS_MAX_PERIOD (STIME_DELTA_MAX)
+#define RTDS_MIN_PERIOD (MICROSECS(10))
+
+/*
+ * Min budget: 10 us, considering the scheduling overhead (when budget is
+ * consumed too fast, scheduling is invoked too frequently, causing
+ * high overhead).
+ */
+#define RTDS_MIN_BUDGET (MICROSECS(10))
+
#define UPDATE_LIMIT_SHIFT 10
#define MAX_SCHEDULE (MILLISECS(1))
/*
@@ -1129,24 +1145,22 @@ 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 = 0;
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;
- }
+ /* Return the default parameters.
+ * A previous bug fixed here:
+ * The default PERIOD or BUDGET should be divided by MICROSECS(1),
+ * before returned to upper caller.
+ */
+ spin_lock_irqsave(&prv->lock, flags);
+ op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1);
+ op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
+ spin_unlock_irqrestore(&prv->lock, flags);
break;
case XEN_DOMCTL_SCHEDOP_putinfo:
if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
@@ -1163,6 +1177,78 @@ rt_dom_cntl(
}
spin_unlock_irqrestore(&prv->lock, flags);
break;
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ while ( index < op->u.v.nr_vcpus )
+ {
+ if ( copy_from_guest_offset(&local_sched,
+ op->u.v.vcpus, index, 1) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+ if ( local_sched.vcpuid >= d->max_vcpus ||
+ d->vcpu[local_sched.vcpuid] == NULL )
+ {
+ rc = -EINVAL;
+ break;
+ }
+
+ spin_lock_irqsave(&prv->lock, flags);
+ svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
+ local_sched.u.rtds.budget = svc->budget / MICROSECS(1);
+ local_sched.u.rtds.period = svc->period / MICROSECS(1);
+ spin_unlock_irqrestore(&prv->lock, flags);
+
+ if ( copy_to_guest_offset(op->u.v.vcpus, index,
+ &local_sched, 1) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+ if ( (++index > 0x3f) && hypercall_preempt_check() )
+ break;
+ }
+ if ( !rc ) /* notify upper caller how many vcpus have been processed */
+ op->u.v.nr_vcpus = index;
+ break;
+
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ while ( index < op->u.v.nr_vcpus )
+ {
+ if ( copy_from_guest_offset(&local_sched,
+ op->u.v.vcpus, index, 1) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+ if ( local_sched.vcpuid >= d->max_vcpus ||
+ d->vcpu[local_sched.vcpuid] == NULL )
+ {
+ rc = -EINVAL;
+ break;
+ }
+
+ period = MICROSECS(local_sched.u.rtds.period);
+ budget = MICROSECS(local_sched.u.rtds.budget);
+ if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET ||
+ budget > period || period < RTDS_MIN_PERIOD )
+ {
+ rc = -EINVAL;
+ break;
+ }
+
+ spin_lock_irqsave(&prv->lock, flags);
+ svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
+ svc->period = period;
+ svc->budget = budget;
+ spin_unlock_irqrestore(&prv->lock, flags);
+
+ if ( (++index > 0x3f) && hypercall_preempt_check() )
+ break;
+ }
+ if ( !rc ) /* notify upper caller how many vcpus have been processed */
+ op->u.v.nr_vcpus = index;
+ break;
}
return rc;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index c195129..cd25e08 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1148,11 +1148,20 @@ 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;
+ 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. */
if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 7a56b3f..9297c01 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -338,24 +338,63 @@ 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;
+ } u;
+ uint32_t vcpuid;
+ uint16_t padding[2];
+} 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):
+ * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
+ * XEN_DOMCTL_SCHEDOP_getinfo gets default params;
+ * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus;
+ *
+ * For schedulers not supporting per-vcpu settings:
+ * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
+ * XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params;
+ * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo 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;
+ /*
+ * IN: Number of elements in vcpus array.
+ * OUT: Number of processed elements of vcpus array.
+ */
+ uint32_t nr_vcpus;
+ uint32_t padding;
+ } v;
} u;
};
typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v7 for Xen 4.7 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 [PATCH v7 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-03-16 16:47 ` Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-17 2:28 ` Dario Faggioli
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 4/4] xl: " Chong Li
3 siblings, 2 replies; 20+ messages in thread
From: Chong Li @ 2016-03-16 16:47 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 v6:
1) Resolve some coding sytle issues
Changes on PATCH v5:
1) In xc_sched_rtds_vcpu_get/set, re-issueing the hypercall
if it is preempted.
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 | 68 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 01a6dda..4567585 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..d1d1aa5 100644
--- a/tools/libxc/xc_rt.c
+++ b/tools/libxc/xc_rt.c
@@ -62,3 +62,71 @@ 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 = 0;
+ unsigned processed = 0;
+ 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;
+
+ while ( processed < num_vcpus )
+ {
+ domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus - processed;
+ set_xen_guest_handle_offset(domctl.u.scheduler_op.u.v.vcpus, vcpus,
+ processed);
+ if ( (rc = do_domctl(xch, &domctl)) != 0 )
+ break;
+ processed += domctl.u.scheduler_op.u.v.nr_vcpus;
+ }
+
+ 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;
+ unsigned processed = 0;
+ 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;
+
+ while ( processed < num_vcpus )
+ {
+ domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus - processed;
+ set_xen_guest_handle_offset(domctl.u.scheduler_op.u.v.vcpus, vcpus,
+ processed);
+ if ( (rc = do_domctl(xch, &domctl)) != 0 )
+ break;
+ processed += domctl.u.scheduler_op.u.v.nr_vcpus;
+ }
+
+ xc_hypercall_bounce_post(xch, vcpus);
+
+ return rc;
+}
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 [PATCH v7 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 1/4] xen: enable " Chong Li
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 2/4] libxc: " Chong Li
@ 2016-03-16 16:47 ` Chong Li
2016-03-16 19:24 ` Wei Liu
` (2 more replies)
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 4/4] xl: " Chong Li
3 siblings, 3 replies; 20+ messages in thread
From: Chong Li @ 2016-03-16 16:47 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 v6:
1) Resolve some coding style issues
2) Change sched_rtds_validate_params()
3) Small changes for sched_rtds_vcpus_params_set(all) functions
Changes on PATCH v5:
1) Add a seperate function, sched_rtds_vcpus_params_set_all(), to set
the parameters of all vcpus of a domain.
2) Add libxl_vcpu_sched_params_set_all() to invoke the above function.
3) Coding style changes. (I didn't find the indentation rules for function
calls with long parameters (still 4 spaces?), so I just imitated the
indentation style of some existing functions)
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 | 321 ++++++++++++++++++++++++++++++++++++++++----
tools/libxl/libxl.h | 37 +++++
tools/libxl/libxl_types.idl | 14 ++
3 files changed, 349 insertions(+), 23 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bd3aac8..0f9fb7e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5770,6 +5770,201 @@ 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)
+{
+ int rc;
+
+ if (period < 1) {
+ LOG(ERROR, "VCPU period is out of range, "
+ "valid values are larger than or equal to 1");
+ rc = ERROR_INVAL; /* error scheduling parameter */
+ goto out;
+ }
+
+ if (budget < 1) {
+ LOG(ERROR, "VCPU budget is not set or out of range, "
+ "valid values are larger than or equal to 1");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ if (budget > period) {
+ LOG(ERROR, "VCPU budget must be smaller than "
+ "or equal to VCPU period");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+ rc = 0;
+out:
+ return rc;
+}
+
+/* Get the RTDS scheduling parameters of vcpu(s) */
+static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
+ libxl_vcpu_sched_params *scinfo)
+{
+ uint32_t num_vcpus;
+ int i, r, rc;
+ xc_dominfo_t info;
+ struct xen_domctl_schedparam_vcpu *vcpus;
+
+ r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (r < 0) {
+ LOGE(ERROR, "getting domain info");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ 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);
+ rc = ERROR_INVAL;
+ goto out;
+ }
+ vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
+ }
+ } else
+ for (i = 0; i < num_vcpus; i++)
+ vcpus[i].vcpuid = i;
+
+ r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus);
+ if (r != 0) {
+ LOGE(ERROR, "getting vcpu sched rtds");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ 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].u.rtds.period;
+ scinfo->vcpus[i].budget = vcpus[i].u.rtds.budget;
+ scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
+ }
+ rc = 0;
+out:
+ return rc;
+}
+
+/* Set the RTDS scheduling parameters of vcpu(s) */
+static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid,
+ const libxl_vcpu_sched_params *scinfo)
+{
+ int r, rc;
+ int i;
+ uint16_t max_vcpuid;
+ xc_dominfo_t info;
+ struct xen_domctl_schedparam_vcpu *vcpus;
+
+ r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (r < 0) {
+ LOGE(ERROR, "getting domain info");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ max_vcpuid = info.max_vcpu_id;
+
+ if (scinfo->num_vcpus <= 0) {
+ rc = ERROR_INVAL;
+ goto out;
+ } else {
+ for (i = 0; i < scinfo->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);
+ rc = ERROR_INVAL;
+ goto out;
+ }
+ rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period,
+ scinfo->vcpus[i].budget);
+ if (rc) {
+ rc = ERROR_INVAL;
+ goto out;
+ }
+ }
+ GCNEW_ARRAY(vcpus, scinfo->num_vcpus);
+ for (i = 0; i < scinfo->num_vcpus; i++) {
+ vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
+ vcpus[i].u.rtds.period = scinfo->vcpus[i].period;
+ vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget;
+ }
+ }
+
+ r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
+ vcpus, scinfo->num_vcpus);
+ if (r != 0) {
+ LOGE(ERROR, "setting vcpu sched rtds");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ rc = 0;
+out:
+ return rc;
+}
+
+/* Set the RTDS scheduling parameters of all vcpus of a domain */
+static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid,
+ const libxl_vcpu_sched_params *scinfo)
+{
+ int r, rc;
+ int i;
+ uint16_t max_vcpuid;
+ xc_dominfo_t info;
+ struct xen_domctl_schedparam_vcpu *vcpus;
+ uint32_t num_vcpus;
+
+ r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+ if (r < 0) {
+ LOGE(ERROR, "getting domain info");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ max_vcpuid = info.max_vcpu_id;
+
+ if (scinfo->num_vcpus != 1) {
+ rc = ERROR_INVAL;
+ goto out;
+ } else {
+ if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
+ scinfo->vcpus[0].budget)) {
+ rc = ERROR_INVAL;
+ goto out;
+ }
+ num_vcpus = max_vcpuid + 1;
+ GCNEW_ARRAY(vcpus, num_vcpus);
+ for (i = 0; i < num_vcpus; i++) {
+ vcpus[i].vcpuid = i;
+ vcpus[i].u.rtds.period = scinfo->vcpus[0].period;
+ vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget;
+ }
+ }
+
+ r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
+ vcpus, num_vcpus);
+ if (r != 0) {
+ LOGE(ERROR, "setting vcpu sched rtds");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ rc = 0;
+out:
+ return rc;
+}
+
static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
@@ -5802,30 +5997,10 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
LOGE(ERROR, "getting domain sched rtds");
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");
+ if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT &&
+ scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
+ if (sched_rtds_validate_params(gc, scinfo->period, scinfo->budget))
return ERROR_INVAL;
- }
- sdom.budget = scinfo->budget;
- }
-
- if (sdom.budget > sdom.period) {
- LOG(ERROR, "VCPU budget is larger than VCPU period, "
- "VCPU budget should be no larger than VCPU period");
- return ERROR_INVAL;
- }
rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
if (rc < 0) {
@@ -5873,6 +6048,74 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
return ret;
}
+int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
+ const libxl_vcpu_sched_params *scinfo)
+{
+ GC_INIT(ctx);
+ libxl_scheduler sched = scinfo->sched;
+ int rc;
+
+ if (sched == LIBXL_SCHEDULER_UNKNOWN)
+ sched = libxl__domain_scheduler(gc, domid);
+
+ switch (sched) {
+ case LIBXL_SCHEDULER_SEDF:
+ LOG(ERROR, "SEDF scheduler no longer available");
+ rc = 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");
+ rc = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_RTDS:
+ rc = sched_rtds_vcpus_params_set(gc, domid, scinfo);
+ break;
+ default:
+ LOG(ERROR, "Unknown scheduler");
+ rc = ERROR_INVAL;
+ break;
+ }
+
+ GC_FREE;
+ return rc;
+}
+
+int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid,
+ const libxl_vcpu_sched_params *scinfo)
+{
+ GC_INIT(ctx);
+ libxl_scheduler sched = scinfo->sched;
+ int rc;
+
+ if (sched == LIBXL_SCHEDULER_UNKNOWN)
+ sched = libxl__domain_scheduler(gc, domid);
+
+ switch (sched) {
+ case LIBXL_SCHEDULER_SEDF:
+ LOG(ERROR, "SEDF scheduler no longer available");
+ rc = 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");
+ rc = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_RTDS:
+ rc = sched_rtds_vcpus_params_set_all(gc, domid, scinfo);
+ break;
+ default:
+ LOG(ERROR, "Unknown scheduler");
+ rc = ERROR_INVAL;
+ break;
+ }
+
+ GC_FREE;
+ return rc;
+}
+
int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
@@ -5907,6 +6150,38 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
return ret;
}
+int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
+ libxl_vcpu_sched_params *scinfo)
+{
+ GC_INIT(ctx);
+ int rc;
+
+ scinfo->sched = libxl__domain_scheduler(gc, domid);
+
+ switch (scinfo->sched) {
+ case LIBXL_SCHEDULER_SEDF:
+ LOG(ERROR, "SEDF scheduler is no longer available");
+ rc = 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");
+ rc = ERROR_INVAL;
+ break;
+ case LIBXL_SCHEDULER_RTDS:
+ rc = sched_rtds_vcpu_get(gc, domid, scinfo);
+ break;
+ default:
+ LOG(ERROR, "Unknown scheduler");
+ rc = ERROR_INVAL;
+ break;
+ }
+
+ GC_FREE;
+ return rc;
+}
+
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..df35d09 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -206,6 +206,17 @@
#define LIBXL_HAVE_DEVICE_MODEL_USER 1
/*
+ * libxl_vcpu_sched_params is used to store per-vcpu params.
+ */
+#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,11 +1658,37 @@ 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
+
+/* 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 *params);
+
+/* 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 *params);
+/* Get the per-vcpu scheduling parameters */
+int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
+ libxl_vcpu_sched_params *params);
+
+/* Set the per-vcpu scheduling parameters */
+int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
+ const libxl_vcpu_sched_params *params);
+
+/* Set the per-vcpu scheduling parameters of all vcpus of a domain */
+int libxl_vcpu_sched_params_set_all(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);
int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index cf3730f..7487fc9 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -378,6 +378,20 @@ 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'}),
+ ("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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v7 for Xen 4.7 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 [PATCH v7 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
` (2 preceding siblings ...)
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-03-16 16:47 ` Chong Li
2016-03-16 19:24 ` Wei Liu
3 siblings, 1 reply; 20+ messages in thread
From: Chong Li @ 2016-03-16 16:47 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 v6:
1) More explain in xl.pod.1 and cmdtable.c
2) Resolve some coding sytle issues
Changes on PATCH v5:
1) Add sched_vcpu_set_all() for the cases that all vcpus of a
domain need to be changed together.
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 | 38 ++++++
tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++++++++-----
tools/libxl/xl_cmdtable.c | 16 ++-
3 files changed, 320 insertions(+), 35 deletions(-)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 4279c7c..1a8aacd 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.
@@ -1066,6 +1070,40 @@ Restrict output to domains in the specified cpupool.
=back
+B<EXAMPLES>
+
+=over 4
+
+=item B<-d [domid]>
+
+List default per-domain params for a domain
+
+=item B<-d [domid] [params (period and budget)]>
+
+Set per-domain params for a domain
+
+=item B<-d [domid] -v [vcpuid] -v [vcpuid] ...>
+
+List per-VCPU params for a domain
+
+=item B<-d [domid] -v all>
+
+List all per-VCPU params for a domain
+
+=item B<-v all>
+
+List all per-VCPU params for all domains
+
+=item B<-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...>
+
+Set per-VCPU params for a domain
+
+=item B<-d [domid] -v all [params]>
+
+Set all per-VCPU params for a domain
+
+=back
+
=back
=head1 CPUPOOLS COMMANDS
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2b6371d..131541f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5823,6 +5823,52 @@ 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_vcpu_set_all(int domid, const libxl_vcpu_sched_params *scinfo)
+{
+ int rc;
+
+ rc = libxl_vcpu_sched_params_set_all(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 +5988,37 @@ 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 +6092,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
@@ -6215,84 +6351,183 @@ int main_sched_credit2(int argc, char **argv)
/*
* <nothing> : List all domain paramters and sched params
- * -d [domid] : List domain params for domain
+ * -d [domid] : List default domain params for domain
* -d [domid] [params] : Set domain params for domain
+ * -d [domid] -v [vcpuid 1] -v [vcpuid 2] ... :
+ * List per-VCPU params for domain
+ * -d [domid] -v all : List all per-VCPU params for domain
+ * -v all : List all per-VCPU params for all domains
+ * -d [domid] -v [vcpuid 1] [params] -v [vcpuid 2] [params] ... :
+ * Set per-VCPU params for domain
+ * -d [domid] -v all [params] : Set all per-VCPU params for domain
*/
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, rc;
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
};
- 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 >= p_size) {
+ /* periods array is full
+ * double the array size for new elements
+ */
+ p_size *= 2;
+ periods = xrealloc(periods, p_size);
+ }
+ periods[p_index++] = strtol(optarg, NULL, 10);
opt_p = 1;
break;
case 'b':
- budget = strtol(optarg, NULL, 10);
+ if (b_index >= b_size) { /* budgets array is full */
+ b_size *= 2;
+ budgets = xrealloc(budgets, b_size);
+ }
+ budgets[b_index++] = strtol(optarg, NULL, 10);
opt_b = 1;
break;
+ case 'v':
+ if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */
+ opt_all = 1;
+ break;
+ }
+ if (v_index >= v_size) { /* vcpus array is full */
+ v_size *= 2;
+ vcpus = xrealloc(vcpus, v_size);
+ }
+ 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 = EXIT_FAILURE;
+ 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 = EXIT_FAILURE;
+ 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 = EXIT_FAILURE;
+ goto out;
+ }
+ if (opt_v && opt_all) {
+ fprintf(stderr, "Incorrect VCPU IDs.\n");
+ rc = EXIT_FAILURE;
+ 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 = EXIT_FAILURE;
+ goto out;
}
- if (!dom) { /* list all domain's rt scheduler info */
- if (sched_domain_output(LIBXL_SCHEDULER_RTDS,
- sched_rtds_domain_output,
+ if ((!dom) && opt_all) {
+ /* get all domain's per-vcpu rtds scheduler parameters */
+ rc = -sched_vcpu_output(LIBXL_SCHEDULER_RTDS,
+ sched_rtds_vcpu_output,
sched_rtds_pool_output,
- cpupool))
- return EXIT_FAILURE;
+ 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) {
+ /* get per-vcpu rtds scheduling parameters */
+ 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 scheduling parameters */
+ libxl_vcpu_sched_params scinfo;
+ libxl_vcpu_sched_params_init(&scinfo);
scinfo.sched = LIBXL_SCHEDULER_RTDS;
- scinfo.period = period;
- scinfo.budget = budget;
+ if (v_index > 0) {
+ scinfo.num_vcpus = v_index;
+ 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];
+ }
+ rc = sched_vcpu_set(domid, &scinfo);
+ } else { /* set params for all vcpus */
+ scinfo.num_vcpus = 1;
+ scinfo.vcpus = (libxl_sched_params *)
+ xmalloc(sizeof(libxl_sched_params));
+ scinfo.vcpus[0].period = periods[0];
+ scinfo.vcpus[0].budget = budgets[0];
+ rc = sched_vcpu_set_all(domid, &scinfo);
+ }
- rc = sched_domain_set(domid, &scinfo);
- libxl_domain_sched_params_dispose(&scinfo);
- if (rc)
- return EXIT_FAILURE;
+ libxl_vcpu_sched_params_dispose(&scinfo);
+ if (rc) {
+ rc = -rc;
+ goto out;
+ }
}
}
- return EXIT_SUCCESS;
+ rc = 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..34f5262 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -268,10 +268,22 @@ 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> [-v[=VCPUID/all]] [-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"
+ "-b BUDGET, --budget=BUDGET Budget (us)\n\n"
+ "Examples:\n\n"
+ "-d [domid] : List default per-domain params for a domain\n"
+ "-d [domid] [params (period and budget)] : Set per-domain params for a domain\n"
+ "-d [domid] -v [vcpuid] -v [vcpuid] ... : "
+ "List per-VCPU params for a domain\n"
+ "-d [domid] -v all : List all per-VCPU params for a domain\n"
+ "-v all : List all per-VCPU params for all domains\n"
+ "-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ... : "
+ "Set per-VCPU params for a domain\n"
+ "-d [domid] -v all [params] : Set all per-VCPU params for a domain\n"
},
{ "domid",
&main_domid, 0, 0,
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 2/4] libxc: " Chong Li
@ 2016-03-16 19:24 ` Wei Liu
2016-03-17 2:28 ` Dario Faggioli
1 sibling, 0 replies; 20+ messages in thread
From: Wei Liu @ 2016-03-16 19:24 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
xen-devel, Meng Xu, dgolomb
On Wed, Mar 16, 2016 at 11:47:49AM -0500, 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>
Acked-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-03-16 19:24 ` Wei Liu
2016-03-17 4:05 ` Dario Faggioli
2016-03-17 4:29 ` Dario Faggioli
2 siblings, 0 replies; 20+ messages in thread
From: Wei Liu @ 2016-03-16 19:24 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 Wed, Mar 16, 2016 at 11:47:50AM -0500, Chong Li wrote:
> 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>
>
Good work fixing all issues.
Acked-by: Wei Liu <wei.liu2@citrix.com>
And I have some nit-picking below.
> +
> +/* Set the RTDS scheduling parameters of vcpu(s) */
> +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + int r, rc;
> + int i;
> + uint16_t max_vcpuid;
> + xc_dominfo_t info;
> + struct xen_domctl_schedparam_vcpu *vcpus;
> +
> + r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (r < 0) {
> + LOGE(ERROR, "getting domain info");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + max_vcpuid = info.max_vcpu_id;
> +
> + if (scinfo->num_vcpus <= 0) {
> + rc = ERROR_INVAL;
> + goto out;
> + } else {
> + for (i = 0; i < scinfo->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);
> + rc = ERROR_INVAL;
> + goto out;
> + }
> + rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period,
> + scinfo->vcpus[i].budget);
> + if (rc) {
> + rc = ERROR_INVAL;
> + goto out;
> + }
> + }
> + GCNEW_ARRAY(vcpus, scinfo->num_vcpus);
> + for (i = 0; i < scinfo->num_vcpus; i++) {
> + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
> + vcpus[i].u.rtds.period = scinfo->vcpus[i].period;
> + vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget;
> + }
> + }
> +
You could have written this hunk like this:
if (scinfo->num_vcpus <= 0) {
rc = ERROR_INVAL;
goto out;
}
for (i = 0; i < scinfo->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);
rc = ERROR_INVAL;
goto out;
}
rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period,
scinfo->vcpus[i].budget);
if (rc) {
rc = ERROR_INVAL;
goto out;
}
}
GCNEW_ARRAY(vcpus, scinfo->num_vcpus);
for (i = 0; i < scinfo->num_vcpus; i++) {
vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
vcpus[i].u.rtds.period = scinfo->vcpus[i].period;
vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget;
}
But, you original code is still OK. This is just FYI. No need to resend
just because of this.
> + r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
> + vcpus, scinfo->num_vcpus);
> + if (r != 0) {
> + LOGE(ERROR, "setting vcpu sched rtds");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + rc = 0;
> +out:
> + return rc;
> +}
> +
> +/* Set the RTDS scheduling parameters of all vcpus of a domain */
> +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + int r, rc;
> + int i;
> + uint16_t max_vcpuid;
> + xc_dominfo_t info;
> + struct xen_domctl_schedparam_vcpu *vcpus;
> + uint32_t num_vcpus;
> +
> + r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (r < 0) {
> + LOGE(ERROR, "getting domain info");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + max_vcpuid = info.max_vcpu_id;
> +
> + if (scinfo->num_vcpus != 1) {
> + rc = ERROR_INVAL;
> + goto out;
> + } else {
> + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> + scinfo->vcpus[0].budget)) {
> + rc = ERROR_INVAL;
> + goto out;
> + }
> + num_vcpus = max_vcpuid + 1;
> + GCNEW_ARRAY(vcpus, num_vcpus);
> + for (i = 0; i < num_vcpus; i++) {
> + vcpus[i].vcpuid = i;
> + vcpus[i].u.rtds.period = scinfo->vcpus[0].period;
> + vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget;
> + }
> + }
Same here, the else branch can be simplified. Again, it's just FYI. No
need to resend.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 4/4] xl: " Chong Li
@ 2016-03-16 19:24 ` Wei Liu
2016-03-16 19:29 ` Wei Liu
0 siblings, 1 reply; 20+ messages in thread
From: Wei Liu @ 2016-03-16 19:24 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
xen-devel, Meng Xu, dgolomb
On Wed, Mar 16, 2016 at 11:47:51AM -0500, Chong Li wrote:
> ---
> docs/man/xl.pod.1 | 38 ++++++
> tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++++++++-----
> tools/libxl/xl_cmdtable.c | 16 ++-
> 3 files changed, 320 insertions(+), 35 deletions(-)
>
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 4279c7c..1a8aacd 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.
> @@ -1066,6 +1070,40 @@ Restrict output to domains in the specified cpupool.
>
> =back
>
> +B<EXAMPLES>
> +
> +=over 4
> +
> +=item B<-d [domid]>
> +
> +List default per-domain params for a domain
> +
> +=item B<-d [domid] [params (period and budget)]>
> +
> +Set per-domain params for a domain
> +
> +=item B<-d [domid] -v [vcpuid] -v [vcpuid] ...>
> +
> +List per-VCPU params for a domain
> +
> +=item B<-d [domid] -v all>
> +
> +List all per-VCPU params for a domain
> +
> +=item B<-v all>
> +
> +List all per-VCPU params for all domains
> +
> +=item B<-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...>
> +
> +Set per-VCPU params for a domain
> +
> +=item B<-d [domid] -v all [params]>
> +
> +Set all per-VCPU params for a domain
> +
> +=back
> +
Urgh, there might be some misunderstanding here. These are explanations
to specific commands, while I asked for some concrete examples.
Search for "example" in xl manpage.
> =back
>
> =head1 CPUPOOLS COMMANDS
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..131541f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5823,6 +5823,52 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
> return 0;
> }
>
[...]
> static int sched_rtds_pool_output(uint32_t poolid)
> {
> char *poolname;
> @@ -6015,6 +6092,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;
Normally xl shouldn't use libxl's error code. And for your convenience
you can just return -1 or 1.
> + }
> + }
> +
> + 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;
Same here.
> + }
> +
> + 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
> @@ -6215,84 +6351,183 @@ int main_sched_credit2(int argc, char **argv)
>
> /*
> * <nothing> : List all domain paramters and sched params
> - * -d [domid] : List domain params for domain
> + * -d [domid] : List default domain params for domain
> * -d [domid] [params] : Set domain params for domain
> + * -d [domid] -v [vcpuid 1] -v [vcpuid 2] ... :
> + * List per-VCPU params for domain
> + * -d [domid] -v all : List all per-VCPU params for domain
> + * -v all : List all per-VCPU params for all domains
> + * -d [domid] -v [vcpuid 1] [params] -v [vcpuid 2] [params] ... :
> + * Set per-VCPU params for domain
> + * -d [domid] -v all [params] : Set all per-VCPU params for domain
> */
> 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, rc;
> 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
> };
>
> - 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 >= p_size) {
> + /* periods array is full
> + * double the array size for new elements
> + */
> + p_size *= 2;
> + periods = xrealloc(periods, p_size);
> + }
> + periods[p_index++] = strtol(optarg, NULL, 10);
> opt_p = 1;
> break;
> case 'b':
> - budget = strtol(optarg, NULL, 10);
> + if (b_index >= b_size) { /* budgets array is full */
> + b_size *= 2;
> + budgets = xrealloc(budgets, b_size);
> + }
> + budgets[b_index++] = strtol(optarg, NULL, 10);
> opt_b = 1;
> break;
> + case 'v':
> + if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */
> + opt_all = 1;
> + break;
> + }
> + if (v_index >= v_size) { /* vcpus array is full */
> + v_size *= 2;
> + vcpus = xrealloc(vcpus, v_size);
> + }
> + vcpus[v_index++] = strtol(optarg, NULL, 10);
> + opt_v = 1;
> + break;
You seemed to miss my request for factoring out a function. But now I
think about it, that probably won't buy us much good. So I'm fine with
code like this now.
> case 'c':
> cpupool = optarg;
> break;
> }
>
[...]
> int main_domid(int argc, char **argv)
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index fdc1ac6..34f5262 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -268,10 +268,22 @@ 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> [-v[=VCPUID/all]] [-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"
> + "-b BUDGET, --budget=BUDGET Budget (us)\n\n"
> + "Examples:\n\n"
> + "-d [domid] : List default per-domain params for a domain\n"
> + "-d [domid] [params (period and budget)] : Set per-domain params for a domain\n"
> + "-d [domid] -v [vcpuid] -v [vcpuid] ... : "
> + "List per-VCPU params for a domain\n"
> + "-d [domid] -v all : List all per-VCPU params for a domain\n"
> + "-v all : List all per-VCPU params for all domains\n"
> + "-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ... : "
> + "Set per-VCPU params for a domain\n"
> + "-d [domid] -v all [params] : Set all per-VCPU params for a domain\n"
No need to have example here.
> },
> { "domid",
> &main_domid, 0, 0,
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 4/4] xl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 19:24 ` Wei Liu
@ 2016-03-16 19:29 ` Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Wei Liu @ 2016-03-16 19:29 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
xen-devel, Meng Xu, dgolomb
On Wed, Mar 16, 2016 at 07:24:35PM +0000, Wei Liu wrote:
> On Wed, Mar 16, 2016 at 11:47:51AM -0500, Chong Li wrote:
> > ---
> > docs/man/xl.pod.1 | 38 ++++++
> > tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++++++++-----
> > tools/libxl/xl_cmdtable.c | 16 ++-
> > 3 files changed, 320 insertions(+), 35 deletions(-)
> >
> > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> > index 4279c7c..1a8aacd 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.
> > @@ -1066,6 +1070,40 @@ Restrict output to domains in the specified cpupool.
> >
> > =back
> >
> > +B<EXAMPLES>
> > +
> > +=over 4
> > +
> > +=item B<-d [domid]>
> > +
> > +List default per-domain params for a domain
> > +
> > +=item B<-d [domid] [params (period and budget)]>
> > +
> > +Set per-domain params for a domain
> > +
> > +=item B<-d [domid] -v [vcpuid] -v [vcpuid] ...>
> > +
> > +List per-VCPU params for a domain
> > +
> > +=item B<-d [domid] -v all>
> > +
> > +List all per-VCPU params for a domain
> > +
> > +=item B<-v all>
> > +
> > +List all per-VCPU params for all domains
> > +
> > +=item B<-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...>
> > +
> > +Set per-VCPU params for a domain
> > +
> > +=item B<-d [domid] -v all [params]>
> > +
> > +Set all per-VCPU params for a domain
> > +
> > +=back
> > +
>
> Urgh, there might be some misunderstanding here. These are explanations
> to specific commands, while I asked for some concrete examples.
>
> Search for "example" in xl manpage.
>
BTW I think you just need to move some of the examples in your cover
letter to manpage.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 2/4] libxc: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 2/4] libxc: " Chong Li
2016-03-16 19:24 ` Wei Liu
@ 2016-03-17 2:28 ` Dario Faggioli
1 sibling, 0 replies; 20+ messages in thread
From: Dario Faggioli @ 2016-03-17 2:28 UTC (permalink / raw)
To: Chong Li, xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, Meng Xu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 1475 bytes --]
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> Add xc_sched_rtds_vcpu_get/set functions to interact with
> Xen to get/set a domain's per-VCPU parameters.
>
Patch is ok, IMO, with only two minor comments.
First, the subject is a bit long. I'd go for:
"libxc: enable per-VCPU parameters for RTDS"
Second, here...
> 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>
> --- a/tools/libxc/xc_rt.c
> +++ b/tools/libxc/xc_rt.c
> @@ -62,3 +62,71 @@ 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 = 0;
>
rc does not need to be initialized to 0 (and hence it shouldn't be).
Even with only this fixed (i.e., even if the subject stays as it is):
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-16 19:24 ` Wei Liu
@ 2016-03-17 4:05 ` Dario Faggioli
2016-03-17 19:50 ` Chong Li
2016-03-17 4:29 ` Dario Faggioli
2 siblings, 1 reply; 20+ messages in thread
From: Dario Faggioli @ 2016-03-17 4:05 UTC (permalink / raw)
To: Chong Li, xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, Ian Jackson, Meng Xu,
dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 6987 bytes --]
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
>
Hey,
Good job indeed, Chong, this is much better.
Now, I appreciate that Wei already Acked this, but nevertheless, I have
some comments.
I'll put them down here, and then it will be up to maintainers and
committers to figure out whether they need to be addressed or not (Wei?
Ian?)
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5770,6 +5770,201 @@ 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)
> +{
> + int rc;
> +
> + if (period < 1) {
> + LOG(ERROR, "VCPU period is out of range, "
> + "valid values are larger than or equal to 1");
>
These strings are really better if kept on one line, and it does not
look impossible in this case:
LOG(ERROR, "Invalid VCPU period of %d (it should be >= 1)", period);
> + rc = ERROR_INVAL; /* error scheduling parameter */
>
The comment should go away.
> + goto out;
> + }
> +
> + if (budget < 1) {
> + LOG(ERROR, "VCPU budget is not set or out of range, "
> + "valid values are larger than or equal to 1");
>
LOG(ERROR, "Invalid VCPU budget of %d (it should be >= 1)", budget);
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + if (budget > period) {
> + LOG(ERROR, "VCPU budget must be smaller than "
> + "or equal to VCPU period");
>
LOG(ERROR, "VCPU budget must be smaller than period, but %d > %d",
budget, period);
> +/* Set the RTDS scheduling parameters of vcpu(s) */
> +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t
> domid,
> + const libxl_vcpu_sched_params
> *scinfo)
> +{
> + int r, rc;
> + int i;
> + uint16_t max_vcpuid;
> + xc_dominfo_t info;
> + struct xen_domctl_schedparam_vcpu *vcpus;
> +
> + r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (r < 0) {
> + LOGE(ERROR, "getting domain info");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + max_vcpuid = info.max_vcpu_id;
> +
> + if (scinfo->num_vcpus <= 0) {
> + rc = ERROR_INVAL;
> + goto out;
> + } else {
>
Personally, I consider what Wei suggested for avoiding, at once, one
more level of indentation, as well as this really really ugly
if {
goto
} else {
}
rather important to do.
But then again, I'm not the maintainer of this code, so if it's fine
for them, then fine. :-)
> + for (i = 0; i < scinfo->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);
>
LOG(ERROR, "Invalid VCPU %d: valid range is [0, %d]",
scinfo->vcpus[i].vcpuid, info.max_vcpu_id);
> +/* Set the RTDS scheduling parameters of all vcpus of a domain */
> +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t
> domid,
> + const libxl_vcpu_sched_params
> *scinfo)
>
Indentation?
It seems to me that it just fits, even if done properly:
static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid,
const libxl_vcpu_sched_params *scinfo)
> +{
> + int r, rc;
> + int i;
> + uint16_t max_vcpuid;
> + xc_dominfo_t info;
> + struct xen_domctl_schedparam_vcpu *vcpus;
> + uint32_t num_vcpus;
> +
> + r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (r < 0) {
> + LOGE(ERROR, "getting domain info");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + max_vcpuid = info.max_vcpu_id;
> +
> + if (scinfo->num_vcpus != 1) {
> + rc = ERROR_INVAL;
> + goto out;
> + } else {
>
Same as above, of course.
> @@ -5802,30 +5997,10 @@ static int sched_rtds_domain_set(libxl__gc
> *gc, uint32_t domid,
> LOGE(ERROR, "getting domain sched rtds");
> 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");
> + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT &&
> + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
> + if (sched_rtds_validate_params(gc, scinfo->period, scinfo-
> >budget))
> return ERROR_INVAL;
>
I'm not sure I understand. What's happening in this function?
As it stands after this patch, it looks to me that:
- we read the default scheduling parameter from Xen,
via xc_sched_rtds_domain_get()
- we (possibly, if both are non-default) validate a new period and
budget couple of values
- we don't use such values for anything, and set back what we got
from Xen, via xc_sched_rtds_domain_set()
Either I'm missing something very basic, or this is not what Wei said
when reviewing v6:
"Then at callsites you set those values with two direct assignment:
if (validate(period_value, budget_value) != 0) {
error;
}
period = period_value;
budget = budget_value;"
Is it?
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-17 4:05 ` Dario Faggioli
@ 2016-03-17 4:29 ` Dario Faggioli
2 siblings, 0 replies; 20+ messages in thread
From: Dario Faggioli @ 2016-03-17 4:29 UTC (permalink / raw)
To: Chong Li, xen-devel
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, ian.jackson, Meng Xu,
dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 2542 bytes --]
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
>
And a couple more things that I forgot.
I'd shorten the subject line, as already suggested for libxc.
> +/* Get the RTDS scheduling parameters of vcpu(s) */
> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
I think this should be called
static int sched_rtds_vcpu_params_get(libxl__gc *gc, uint32_t domid,
libxl_vcpu_sched_params *scinfo)
for homogeneity with the _set counterpart here below:
> @@ -5873,6 +6048,74 @@ int libxl_domain_sched_params_set(libxl_ctx
> *ctx, uint32_t domid,
> return ret;
> }
>
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> + const libxl_vcpu_sched_params
> *scinfo)
> +{
So, this function takes a libxl_vcpu_sched_params*, which is basically
an array of vcpus' parameters, one element per vcpu (and it can be
sparse).
Are there any restrictions on how such array should be constructed, for
calling this function? Are they any special meaning of particular
configurations of the array?
I think at least the latter is true, in fact:
- this calls sched_rtds_vcpus_params_set();
- in there, if the array is empty, we fail;
- if the array has one or more elements, we deal with the vcpus
(and just them) specified in the elements of the array itself.
Then there is libxl_vcpu_sched_params_set_all(). That one:
- calls sched_rtds_vcpus_params_set_all();
- in there, if the array has more than just one element, we fail.
And then the get side, with libxl_vcpu_sched_params_get(). That one:
- calls sched_rtds_vcpu_get();
- in there, if the array is empty, we create one, and return info for
all the vcpus;
- if the array has one or more elements, we deal with them (and just
them).
I think this should be documented some (unless it's already there and I
missed it).
Thanks and Regards again,
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-03-17 10:03 ` Dario Faggioli
2016-03-17 20:42 ` Chong Li
2016-03-18 7:48 ` Jan Beulich
0 siblings, 2 replies; 20+ messages in thread
From: Dario Faggioli @ 2016-03-17 10:03 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: 23793 bytes --]
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1054,15 +1054,16 @@ csched_dom_cntl(
> * lock. Runq lock not needed anywhere in here. */
> spin_lock_irqsave(&prv->lock, flags);
>
> - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
> + switch ( op->cmd )
> {
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + return -EINVAL;
> + case XEN_DOMCTL_SCHEDOP_getinfo:
> op->u.credit.weight = sdom->weight;
> op->u.credit.cap = sdom->cap;
>
Not feeling to strong about it, but I think
switch ( op->cmd )
{
case XEN_DOMCTL_SCHEDOP_getinfo:
op->u.credit.weight = sdom->weight;
op->u.credit.cap = sdom->cap;
break;
case XEN_DOMCTL_SCHEDOP_putinfo:
if ( op->u.credit.weight != 0 )
{
if ( !list_empty(&sdom->active_sdom_elem) )
{
prv->weight -= sdom->weight * sdom->active_vcpu_count;
prv->weight += op->u.credit.weight * sdom->active_vcpu_count;
}
sdom->weight = op->u.credit.weight;
}
if ( op->u.credit.cap != (uint16_t)~0U )
sdom->cap = op->u.credit.cap;
break;
case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
default:
return -EINVAL;
}
(i.e., grouping the cases that needs only returning -EINVAL) is better,
the final result, more than the patch itself.
And the same for Credit2, of course.
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1129,24 +1145,22 @@ 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 = 0;
>
> 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;
> - }
> + /* Return the default parameters.
> + * A previous bug fixed here:
> + * The default PERIOD or BUDGET should be divided by
> MICROSECS(1),
> + * before returned to upper caller.
> + */
Comment style (missing opening '/*').
Also, putting this kind of things in comments is not at all ideal. If
doing this in this patch, you should mention it in the changelog. Or
you do it in a separate patch (that you put before this one in the
series).
I'd say that, in this specific case, is not a big deal which one of the
two approaches you take (mentioning or separate patch), but the having
a separate one is indeed almost always preferable (e.g., the fix can be
backported).
> + spin_lock_irqsave(&prv->lock, flags);
> + op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1);
> + op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
> + spin_unlock_irqrestore(&prv->lock, flags);
>
I don't think we need to take the lock here... RTDS_DEFAULT_PERIOD is
not going to change under our feet. :-)
> @@ -1163,6 +1177,78 @@ rt_dom_cntl(
> }
> spin_unlock_irqrestore(&prv->lock, flags);
> break;
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + while ( index < op->u.v.nr_vcpus )
> + {
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + break;
> + }
> +
> + spin_lock_irqsave(&prv->lock, flags);
> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> + local_sched.u.rtds.budget = svc->budget / MICROSECS(1);
> + local_sched.u.rtds.period = svc->period / MICROSECS(1);
> + spin_unlock_irqrestore(&prv->lock, flags);
> +
> + if ( copy_to_guest_offset(op->u.v.vcpus, index,
> + &local_sched, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( (++index > 0x3f) && hypercall_preempt_check() )
> + break;
>
So, I know it's 0x3f in XEN_SYSCTL_pcitopoinfo, but unless I'm missing
something, I don't see why this can't be "63".
I'd also put a short comment right above, something like:
/* Process a most 64 vCPUs without checking for preemptions. */
> + }
> + if ( !rc ) /* notify upper caller how many vcpus have been
> processed */
>
Move the comment to the line above (and terminate it with a full stop).
And by the way, there's a lot of code repetition here. What about
handling get and set together, sort of like this:
case XEN_DOMCTL_SCHEDOP_getvcpuinfo: |
case XEN_DOMCTL_SCHEDOP_putvcpuinfo: |#ifdef CONFIG_HAS_PCI
while ( index < op->u.v.nr_vcpus ) | case XEN_SYSCTL_pcitopoinfo:
{ | {
if ( copy_from_guest_offset(&local_sched, | xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
op->u.v.vcpus, index, 1) ) | unsigned int i = 0;
{ |
rc = -EFAULT; | if ( guest_handle_is_null(ti->devs) ||
break; | guest_handle_is_null(ti->nodes) )
} | {
| ret = -EINVAL;
if ( local_sched.vcpuid >= d->max_vcpus || | break;
d->vcpu[local_sched.vcpuid] == NULL ) | }
{ |
rc = -EINVAL; | while ( i < ti->num_devs )
break; | {
} | physdev_pci_device_t dev;
| uint32_t node;
if ( op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo ) | const struct pci_dev *pdev;
{ |
spin_lock_irqsave(&prv->lock, flags); | if ( copy_from_guest_offset(&dev, ti->devs, i, 1) )
svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | {
local_sched.u.rtds.budget = svc->budget / MICROSECS(1); | ret = -EFAULT;
local_sched.u.rtds.period = svc->period / MICROSECS(1); | break;
spin_unlock_irqrestore(&prv->lock, flags); | }
|
if ( copy_to_guest_offset(op->u.v.vcpus, index, | pcidevs_lock();
&local_sched, 1) ) | pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
{ | if ( !pdev )
rc = -EFAULT; | node = XEN_INVALID_DEV;
break; | else if ( pdev->node == NUMA_NO_NODE )
} | node = XEN_INVALID_NODE_ID;
} | else
else | node = pdev->node;
{ | pcidevs_unlock();
period = MICROSECS(local_sched.u.rtds.period); |
budget = MICROSECS(local_sched.u.rtds.budget); | if ( copy_to_guest_offset(ti->nodes, i, &node, 1) )
if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET || | {
budget > period || period < RTDS_MIN_PERIOD ) | ret = -EFAULT;
{ | break;
rc = -EINVAL; | }
break; |
} | /* Process a most 64 vCPUs without checking for preemptions. */
| if ( (++i > 0x3f) && hypercall_preempt_check() )
spin_lock_irqsave(&prv->lock, flags); | break;
svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | }
svc->period = period; |
svc->budget = budget; | if ( !ret && (ti->num_devs != i) )
spin_unlock_irqrestore(&prv->lock, flags); | {
| ti->num_devs = i;
} | if ( __copy_field_to_guest(u_sysctl, op, u.pcitopoinfo.num_devs) )
/* Process a most 64 vCPUs without checking for preemptions. */ | ret = -EFAULT;
if ( (++index > 63) && hypercall_preempt_check() ) | }
break; | break;
} | }
|#endif
/* Notify upper caller how many vcpus have been processed. */ |
if ( !rc ) | case XEN_SYSCTL_tmem_op:
op->u.v.nr_vcpus = index; | ret = tmem_control(&op->u.tmem_op);
break;
I have only compile tested this, but it looks to me that it can fly...
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> + * Set or get info?
> + * For schedulers supporting per-vcpu settings (e.g., RTDS):
> + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
> + * XEN_DOMCTL_SCHEDOP_getinfo gets default params;
> + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus;
> + *
> + * For schedulers not supporting per-vcpu settings:
> + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
> + * XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params;
> + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo 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_* */
/* IN/OUT */
> 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;
> + /*
> + * IN: Number of elements in vcpus array.
> + * OUT: Number of processed elements of vcpus array.
> + */
> + uint32_t nr_vcpus;
> + uint32_t padding;
> + } v;
> } u;
> };
>
That is: make it even more clear that the whole union is used as
IN/OUT.
Then, indeed, inside v, what is the meaning of the nr_vcpus field in
each direction, as you're doing already.
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-17 4:05 ` Dario Faggioli
@ 2016-03-17 19:50 ` Chong Li
2016-03-18 9:17 ` Dario Faggioli
0 siblings, 1 reply; 20+ messages in thread
From: Chong Li @ 2016-03-17 19:50 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
xen-devel, Meng Xu, Dagaen Golomb
On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
>> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
>> functions to support per-VCPU settings.
>>
>> +/* Set the RTDS scheduling parameters of all vcpus of a domain */
>> +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t
>> domid,
>> + const libxl_vcpu_sched_params
>> *scinfo)
>>
> Indentation?
If I follow the indentation rule, the second line would be longer than
80 characters.
The function name is just too long.
>
>> @@ -5802,30 +5997,10 @@ static int sched_rtds_domain_set(libxl__gc
>> *gc, uint32_t domid,
>> LOGE(ERROR, "getting domain sched rtds");
>> 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");
>> + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT &&
>> + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
>> + if (sched_rtds_validate_params(gc, scinfo->period, scinfo-
>> >budget))
>> return ERROR_INVAL;
>>
> I'm not sure I understand. What's happening in this function?
>
> As it stands after this patch, it looks to me that:
> - we read the default scheduling parameter from Xen,
> via xc_sched_rtds_domain_get()
> - we (possibly, if both are non-default) validate a new period and
> budget couple of values
> - we don't use such values for anything, and set back what we got
> from Xen, via xc_sched_rtds_domain_set()
>
> Either I'm missing something very basic, or this is not what Wei said
> when reviewing v6:
>
> "Then at callsites you set those values with two direct assignment:
>
> if (validate(period_value, budget_value) != 0) {
> error;
> }
> period = period_value;
> budget = budget_value;"
>
> Is it?
The current RTDS (xen 4.6) is:
1) Read the (per-domain) scheduling params from Xen, and store them to sdom
2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
-1), use sdom.period;
else sdom.period = new period (if new period is valid);
If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is
-1), use sdom.budget;
else sdom.budget = new budget;
3) xc_sched_rtds_domain_set (sdom);
In our patch, my plan is:
1) Read the default params from Xen, and store them to sdom
2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
-1), use sdom.period;
else sdom.period = new period (if new period is valid);
If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is
-1), use sdom.budget;
else sdom.budget = new budget;
3) xc_sched_rtds_domain_set (sdom);
Even though I made some mistakes in this post (forgot the two "else"s
in my plan), is this plan ok?
Chong
--
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-17 10:03 ` Dario Faggioli
@ 2016-03-17 20:42 ` Chong Li
2016-03-18 7:39 ` Jan Beulich
2016-03-18 7:48 ` Jan Beulich
1 sibling, 1 reply; 20+ messages in thread
From: Chong Li @ 2016-03-17 20:42 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Jan Beulich, Dagaen Golomb
On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
>
>> --- a/xen/common/sched_rt.c
>> +++ b/xen/common/sched_rt.c
>> @@ -1129,24 +1145,22 @@ 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 = 0;
>>
>> 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;
>> - }
>> + /* Return the default parameters.
>> + * A previous bug fixed here:
>> + * The default PERIOD or BUDGET should be divided by
>> MICROSECS(1),
>> + * before returned to upper caller.
>> + */
> Comment style (missing opening '/*').
>
> Also, putting this kind of things in comments is not at all ideal. If
> doing this in this patch, you should mention it in the changelog. Or
> you do it in a separate patch (that you put before this one in the
> series).
>
> I'd say that, in this specific case, is not a big deal which one of the
> two approaches you take (mentioning or separate patch), but the having
> a separate one is indeed almost always preferable (e.g., the fix can be
> backported).
If I choose mentioning, do I move the comment to the changelog? Or do I keep
it here and say it again in the changelog?
Chong
--
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-17 20:42 ` Chong Li
@ 2016-03-18 7:39 ` Jan Beulich
2016-03-18 10:47 ` Dario Faggioli
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2016-03-18 7:39 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, Dario Faggioli, xen-devel,
Meng Xu, Dagaen Golomb
>>> On 17.03.16 at 21:42, <lichong659@gmail.com> wrote:
> On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
>> On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
>
>>
>>> --- a/xen/common/sched_rt.c
>>> +++ b/xen/common/sched_rt.c
>>> @@ -1129,24 +1145,22 @@ 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 = 0;
>>>
>>> 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;
>>> - }
>>> + /* Return the default parameters.
>>> + * A previous bug fixed here:
>>> + * The default PERIOD or BUDGET should be divided by
>>> MICROSECS(1),
>>> + * before returned to upper caller.
>>> + */
>> Comment style (missing opening '/*').
>>
>> Also, putting this kind of things in comments is not at all ideal. If
>> doing this in this patch, you should mention it in the changelog. Or
>> you do it in a separate patch (that you put before this one in the
>> series).
>>
>> I'd say that, in this specific case, is not a big deal which one of the
>> two approaches you take (mentioning or separate patch), but the having
>> a separate one is indeed almost always preferable (e.g., the fix can be
>> backported).
>
> If I choose mentioning, do I move the comment to the changelog? Or do I keep
> it here and say it again in the changelog?
Just consider what would happen if everyone mentioned in
comments the bugs they fixed. I think the answer to you question
is obvious then...
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-17 10:03 ` Dario Faggioli
2016-03-17 20:42 ` Chong Li
@ 2016-03-18 7:48 ` Jan Beulich
1 sibling, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2016-03-18 7:48 UTC (permalink / raw)
To: Dario Faggioli, Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, xen-devel, Meng Xu, dgolomb
>>> On 17.03.16 at 11:03, <dario.faggioli@citrix.com> wrote:
> On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>> @@ -1054,15 +1054,16 @@ csched_dom_cntl(
>> * lock. Runq lock not needed anywhere in here. */
>> spin_lock_irqsave(&prv->lock, flags);
>>
>> - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
>> + switch ( op->cmd )
>> {
>> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
>> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> + return -EINVAL;
>> + case XEN_DOMCTL_SCHEDOP_getinfo:
>> op->u.credit.weight = sdom->weight;
>> op->u.credit.cap = sdom->cap;
>>
> Not feeling to strong about it, but I think
>
> switch ( op->cmd )
> {
> case XEN_DOMCTL_SCHEDOP_getinfo:
> op->u.credit.weight = sdom->weight;
> op->u.credit.cap = sdom->cap;
> break;
> case XEN_DOMCTL_SCHEDOP_putinfo:
> if ( op->u.credit.weight != 0 )
> {
> if ( !list_empty(&sdom->active_sdom_elem) )
> {
> prv->weight -= sdom->weight * sdom->active_vcpu_count;
> prv->weight += op->u.credit.weight * sdom->active_vcpu_count;
> }
> sdom->weight = op->u.credit.weight;
> }
>
> if ( op->u.credit.cap != (uint16_t)~0U )
> sdom->cap = op->u.credit.cap;
> break;
> case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> default:
> return -EINVAL;
> }
>
> (i.e., grouping the cases that needs only returning -EINVAL) is better,
> the final result, more than the patch itself.
In fact there's no point in explicitly naming the unhandled values
as long as they're matching what "default:" does.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
2016-03-17 19:50 ` Chong Li
@ 2016-03-18 9:17 ` Dario Faggioli
0 siblings, 0 replies; 20+ messages in thread
From: Dario Faggioli @ 2016-03-18 9:17 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
xen-devel, Meng Xu, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 5212 bytes --]
On Thu, 2016-03-17 at 14:50 -0500, Chong Li wrote:
> On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> >
> > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> > >
> > > +/* Set the RTDS scheduling parameters of all vcpus of a domain
> > > */
> > > +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc,
> > > uint32_t
> > > domid,
> > > + const libxl_vcpu_sched_params
> > > *scinfo)
> > >
> > Indentation?
> If I follow the indentation rule, the second line would be longer
> than
> 80 characters.
> The function name is just too long.
>
static
int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid,
const libxl_vcpu_sched_params *scinfo)
or
static int
sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid,
const libxl_vcpu_sched_params *scinfo)
or shorten the name.
From quickly looking at libxl, neither of the first two proposed
solutions seems to happen much, so I recommend shortening the name a
bit. It's an internal function, so we can do that pretty freely.
> > > 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");
> > > + if (scinfo->period !=
> > > LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT &&
> > > + scinfo->budget !=
> > > LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
> > > + if (sched_rtds_validate_params(gc, scinfo->period,
> > > scinfo-
> > > >
> > > > budget))
> > > return ERROR_INVAL;
> > >
> > I'm not sure I understand. What's happening in this function?
> >
> > As it stands after this patch, it looks to me that:
> > - we read the default scheduling parameter from Xen,
> > via xc_sched_rtds_domain_get()
> > - we (possibly, if both are non-default) validate a new period and
> > budget couple of values
> > - we don't use such values for anything, and set back what we got
> > from Xen, via xc_sched_rtds_domain_set()
> >
> > Either I'm missing something very basic, or this is not what Wei
> > said
> > when reviewing v6:
> >
> > "Then at callsites you set those values with two direct assignment:
> >
> > if (validate(period_value, budget_value) != 0) {
> > error;
> > }
> > period = period_value;
> > budget = budget_value;"
> >
> > Is it?
> The current RTDS (xen 4.6) is:
>
> 1) Read the (per-domain) scheduling params from Xen, and store them
> to sdom
> 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
> -1), use sdom.period;
> else sdom.period = new period (if new period is valid);
> If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which
> is
> -1), use sdom.budget;
> else sdom.budget = new budget;
> 3) xc_sched_rtds_domain_set (sdom);
>
Sort of, let me restate it more generally:
1) get currently set period and budget;
2) if scinfo->period is not PERIOD_DEFAULT and is valid, update
the period;
3) if scinfo->budget is not BUDGET_DEFAULT and is valid, update
the budget
However that happens in terms of where the values of budget and period
are stashed, what call to xc_sched_rtds_domain_{get,set}() are made,
etc.
So, basically, when you say "if period equals PERIOD_DEFAULT", you mean
scinfo->period, i.e., the period being set is just the default libxl
value for it, which in turns means (most likely) one want to only alter
the budget.
> In our patch, my plan is:
> 1) Read the default params from Xen, and store them to sdom
> 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
> -1), use sdom.period;
> else sdom.period = new period (if new period is valid);
> If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which
> is
> -1), use sdom.budget;
> else sdom.budget = new budget;
> 3) xc_sched_rtds_domain_set (sdom);
>
> Even though I made some mistakes in this post (forgot the two "else"s
> in my plan), is this plan ok?
>
Your plan should be to leve things exactly as they are, from a
functional/logical perspective.
The difference between "(per-domain) scheduling params from Xen" of
right now, and "the default params from Xen" of after the patch, is of
no concern here, as it's all done by Xen.
So, really, this should be _all_ about taking the chance of refactoring
the code such as the validating function does not have weird side
effects, not about changing the logic, which looks fine to me.
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-18 7:39 ` Jan Beulich
@ 2016-03-18 10:47 ` Dario Faggioli
2016-03-18 20:22 ` Chong Li
0 siblings, 1 reply; 20+ messages in thread
From: Dario Faggioli @ 2016-03-18 10:47 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Jan Beulich, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --]
On Fri, 2016-03-18 at 01:39 -0600, Jan Beulich wrote:
> >
> > > > On 17.03.16 at 21:42, <lichong659@gmail.com> wrote:
> > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli
> > <dario.faggioli@citrix.com> wrote:
> > >
> > > I'd say that, in this specific case, is not a big deal which one
> > > of the
> > > two approaches you take (mentioning or separate patch), but the
> > > having
> > > a separate one is indeed almost always preferable (e.g., the fix
> > > can be
> > > backported).
> > If I choose mentioning, do I move the comment to the changelog? Or
> > do I keep
> > it here and say it again in the changelog?
> Just consider what would happen if everyone mentioned in
> comments the bugs they fixed. I think the answer to you question
> is obvious then...
>
Exactly! :-)
And, please (Chong), let me restate this: "having a separate one is
indeed almost always preferable (e.g., the fix can be backported)"
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] 20+ messages in thread
* Re: [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
2016-03-18 10:47 ` Dario Faggioli
@ 2016-03-18 20:22 ` Chong Li
0 siblings, 0 replies; 20+ messages in thread
From: Chong Li @ 2016-03-18 20:22 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Jan Beulich, Dagaen Golomb
On Fri, Mar 18, 2016 at 5:47 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2016-03-18 at 01:39 -0600, Jan Beulich wrote:
>> >
>> > > > On 17.03.16 at 21:42, <lichong659@gmail.com> wrote:
>> > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli
>> > <dario.faggioli@citrix.com> wrote:
>> > >
>> > > I'd say that, in this specific case, is not a big deal which one
>> > > of the
>> > > two approaches you take (mentioning or separate patch), but the
>> > > having
>> > > a separate one is indeed almost always preferable (e.g., the fix
>> > > can be
>> > > backported).
>> > If I choose mentioning, do I move the comment to the changelog? Or
>> > do I keep
>> > it here and say it again in the changelog?
>> Just consider what would happen if everyone mentioned in
>> comments the bugs they fixed. I think the answer to you question
>> is obvious then...
>>
> Exactly! :-)
>
> And, please (Chong), let me restate this: "having a separate one is
> indeed almost always preferable (e.g., the fix can be backported)"
>
Considering the complexity of making a new patch and
today's the last day for posting, I'll mention this bug fix in the changlog.
Chong
--
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] 20+ messages in thread
end of thread, other threads:[~2016-03-18 20:22 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-16 16:47 [PATCH v7 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 1/4] xen: enable " Chong Li
2016-03-17 10:03 ` Dario Faggioli
2016-03-17 20:42 ` Chong Li
2016-03-18 7:39 ` Jan Beulich
2016-03-18 10:47 ` Dario Faggioli
2016-03-18 20:22 ` Chong Li
2016-03-18 7:48 ` Jan Beulich
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 2/4] libxc: " Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-17 2:28 ` Dario Faggioli
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-17 4:05 ` Dario Faggioli
2016-03-17 19:50 ` Chong Li
2016-03-18 9:17 ` Dario Faggioli
2016-03-17 4:29 ` Dario Faggioli
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 4/4] xl: " Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-16 19:29 ` Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).