* [PATCH v8 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS
@ 2016-03-18 21:26 Chong Li
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 1/4] xen: enable " Chong Li
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Chong Li @ 2016-03-18 21:26 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 for RTDS
libxc: enable per-VCPU parameter for RTDS
libxl: enable per-VCPU parameter for RTDS
xl: enable per-VCPU parameter for RTDS
docs/man/xl.pod.1 | 77 +++++++++++
tools/libxc/include/xenctrl.h | 8 ++
tools/libxc/xc_rt.c | 68 +++++++++
tools/libxl/libxl.c | 314 +++++++++++++++++++++++++++++++++++++++---
tools/libxl/libxl.h | 37 +++++
tools/libxl/libxl_types.idl | 14 ++
tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++-----
tools/libxl/xl_cmdtable.c | 4 +-
xen/common/sched_credit.c | 14 +-
xen/common/sched_credit2.c | 13 +-
xen/common/sched_rt.c | 93 +++++++++++--
xen/common/schedule.c | 15 +-
xen/include/public/domctl.h | 64 +++++++--
13 files changed, 925 insertions(+), 97 deletions(-)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-18 21:26 [PATCH v8 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
@ 2016-03-18 21:26 ` Chong Li
2016-03-21 13:35 ` Jan Beulich
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 2/4] libxc: " Chong Li
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-03-18 21:26 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 v7:
1) A bug in case XEN_DOMCTL_SCHEDOP_getinfo (in Xen 4.6) is fixed:
The default PERIOD or BUDGET should be divided by MICROSECS(1),
before returned to upper caller.
2) In the *cntl() functions in sched_credit.c and sched_credit2.c, remove
two unhandled cases in "switch".
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 | 14 +++----
xen/common/sched_credit2.c | 13 ++++---
xen/common/sched_rt.c | 93 ++++++++++++++++++++++++++++++++++++++-------
xen/common/schedule.c | 15 ++++++--
xen/include/public/domctl.h | 64 +++++++++++++++++++++++++------
5 files changed, 157 insertions(+), 42 deletions(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 0dce790..b504702 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1054,15 +1054,13 @@ 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_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 +1073,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..c2dcf0f 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1421,14 +1421,12 @@ 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_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 +1455,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..b640e59 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,16 @@ 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. */
+ op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1);
+ op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
break;
case XEN_DOMCTL_SCHEDOP_putinfo:
if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
@@ -1163,6 +1171,63 @@ rt_dom_cntl(
}
spin_unlock_irqrestore(&prv->lock, flags);
break;
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ 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;
+ }
+
+ if ( op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
+ {
+ 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;
+ }
+ }
+ else
+ {
+ 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);
+ }
+ /* Process a most 64 vCPUs without checking for preemptions. */
+ if ( (++index > 63) && 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..24675f5 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -338,24 +338,64 @@ 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_* */
+ /* 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;
};
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] 24+ messages in thread
* [PATCH v8 for Xen 4.7 2/4] libxc: enable per-VCPU parameter for RTDS
2016-03-18 21:26 [PATCH v8 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-03-18 21:26 ` Chong Li
2016-03-29 17:08 ` Dario Faggioli
2016-03-31 16:59 ` Wei Liu
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 4/4] xl: " Chong Li
3 siblings, 2 replies; 24+ messages in thread
From: Chong Li @ 2016-03-18 21:26 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..221d17f 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;
+ 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] 24+ messages in thread
* [PATCH v8 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
2016-03-18 21:26 [PATCH v8 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 1/4] xen: enable " Chong Li
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 2/4] libxc: " Chong Li
@ 2016-03-18 21:26 ` Chong Li
2016-03-30 17:04 ` Dario Faggioli
2016-03-31 16:59 ` Wei Liu
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 4/4] xl: " Chong Li
3 siblings, 2 replies; 24+ messages in thread
From: Chong Li @ 2016-03-18 21:26 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 v7:
1) Fix a mistake in sched_rtds_domain_set()
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 | 314 +++++++++++++++++++++++++++++++++++++++++---
tools/libxl/libxl.h | 37 ++++++
tools/libxl/libxl_types.idl | 14 ++
3 files changed, 344 insertions(+), 21 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bd3aac8..b8d7f07 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5770,6 +5770,196 @@ 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, "Invalid VCPU period of %d (it should be >= 1)", period);
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ if (budget < 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 period, "
+ "but %d > %d", budget, 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_vcpu_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;
+ }
+ for (i = 0; i < scinfo->num_vcpus; i++) {
+ if (scinfo->vcpus[i].vcpuid < 0 ||
+ scinfo->vcpus[i].vcpuid > max_vcpuid) {
+ LOG(ERROR, "Invalid VCPU %d: valid range is [0, %d]",
+ scinfo->vcpus[i].vcpuid, 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_vcpu_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;
+ }
+ 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 +5992,12 @@ 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;
- }
+ if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT)
sdom.period = scinfo->period;
- }
-
- if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
- if (scinfo->budget < 1) {
- LOG(ERROR, "VCPU budget is not set or out of range, "
- "valid values are larger than 1");
- return ERROR_INVAL;
- }
+ if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
sdom.budget = scinfo->budget;
- }
-
- if (sdom.budget > sdom.period) {
- LOG(ERROR, "VCPU budget is larger than VCPU period, "
- "VCPU budget should be no larger than VCPU period");
+ if (sched_rtds_validate_params(gc, sdom.period, sdom.budget))
return ERROR_INVAL;
- }
rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
if (rc < 0) {
@@ -5873,6 +6045,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_vcpu_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_vcpu_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 +6147,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] 24+ messages in thread
* [PATCH v8 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
2016-03-18 21:26 [PATCH v8 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
` (2 preceding siblings ...)
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-03-18 21:26 ` Chong Li
2016-03-31 17:00 ` Wei Liu
2016-03-31 17:17 ` Dario Faggioli
3 siblings, 2 replies; 24+ messages in thread
From: Chong Li @ 2016-03-18 21:26 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 v7:
1) Add example to xl.pod.1
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 | 77 ++++++++++++
tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++++++++-----
tools/libxl/xl_cmdtable.c | 4 +-
3 files changed, 348 insertions(+), 34 deletions(-)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 4279c7c..0350baa 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,79 @@ Restrict output to domains in the specified cpupool.
=back
+B<EXAMPLE>
+
+=over 4
+
+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".
+
+=back
+
=back
=head1 CPUPOOLS COMMANDS
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2b6371d..97e8b33 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 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 1;
+ }
+
+ 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..9662bda 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -268,8 +268,10 @@ 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"
},
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-03-21 13:35 ` Jan Beulich
2016-03-21 15:18 ` Chong Li
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2016-03-21 13:35 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, xen-devel,
Meng Xu, dgolomb
>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
> to independently get and set the scheduling parameters of each
> vCPU of a domain
>
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>
> ---
> Changes on PATCH v7:
> 1) A bug in case XEN_DOMCTL_SCHEDOP_getinfo (in Xen 4.6) is fixed:
> The default PERIOD or BUDGET should be divided by MICROSECS(1),
> before returned to upper caller.
Seems like there's still some misunderstanding here: Anything
past the first --- won't make it into the repo, yet the description
of what bug you fix should end up there.
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -338,24 +338,64 @@ 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];
So why uint16_t[2] instead of just uint32_t? And what's the
padding needed for in the first place? Also, while for a domctl it's
not as strictly needed as for other hypercalls, checking that all
padding fields are zero would still seem to be rather desirable.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-21 13:35 ` Jan Beulich
@ 2016-03-21 15:18 ` Chong Li
2016-03-21 15:39 ` George Dunlap
2016-03-21 15:49 ` Jan Beulich
0 siblings, 2 replies; 24+ messages in thread
From: Chong Li @ 2016-03-21 15:18 UTC (permalink / raw)
To: Jan Beulich
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
On Mon, Mar 21, 2016 at 8:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
>> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
>> to independently get and set the scheduling parameters of each
>> vCPU of a domain
>>
>> Signed-off-by: Chong Li <chong.li@wustl.edu>
>> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>>
>> ---
>> Changes on PATCH v7:
>> 1) A bug in case XEN_DOMCTL_SCHEDOP_getinfo (in Xen 4.6) is fixed:
>> The default PERIOD or BUDGET should be divided by MICROSECS(1),
>> before returned to upper caller.
>
> Seems like there's still some misunderstanding here: Anything
> past the first --- won't make it into the repo, yet the description
> of what bug you fix should end up there.
I see. Do I put the "bug fix" before "Signed-off-by ..." or after it?
>
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -338,24 +338,64 @@ 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];
>
> So why uint16_t[2] instead of just uint32_t? And what's the
> padding needed for in the first place?
You're right. It's better to use uint32_t, which pads (the struct) to
the 64-bit boundary.
> Also, while for a domctl it's
> not as strictly needed as for other hypercalls, checking that all
> padding fields are zero would still seem to be rather desirable.
>
Do you mean:
+ case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+ case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+ if ( op->u.v.padding !=0 )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ 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.padding != 0 )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ if ( local_sched.vcpuid >= d->max_vcpus ||
+ d->vcpu[local_sched.vcpuid] == NULL )
+ {
+ rc = -EINVAL;
+ break;
+ }
?
Thanks,
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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-21 15:18 ` Chong Li
@ 2016-03-21 15:39 ` George Dunlap
2016-03-21 15:49 ` Jan Beulich
1 sibling, 0 replies; 24+ messages in thread
From: George Dunlap @ 2016-03-21 15:39 UTC (permalink / raw)
To: Chong Li, Jan Beulich
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
On 21/03/16 15:18, Chong Li wrote:
> On Mon, Mar 21, 2016 at 8:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
>>> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
>>> to independently get and set the scheduling parameters of each
>>> vCPU of a domain
>>>
>>> Signed-off-by: Chong Li <chong.li@wustl.edu>
>>> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>>> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>>>
>>> ---
>>> Changes on PATCH v7:
>>> 1) A bug in case XEN_DOMCTL_SCHEDOP_getinfo (in Xen 4.6) is fixed:
>>> The default PERIOD or BUDGET should be divided by MICROSECS(1),
>>> before returned to upper caller.
>>
>> Seems like there's still some misunderstanding here: Anything
>> past the first --- won't make it into the repo, yet the description
>> of what bug you fix should end up there.
>
> I see. Do I put the "bug fix" before "Signed-off-by ..." or after it?
You need to put everything important for understanding this changeset in
the changset description. So in this case, you'd probably add a
paragraph like this:
"Also fix a bug in XEN_DOMCTL_SCHEDOP_getinfo, where PERIOD and BUDGET
are not divided by MICROSECS(1) before being returned to the caller."
(And that would be before the SoB.)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-21 15:18 ` Chong Li
2016-03-21 15:39 ` George Dunlap
@ 2016-03-21 15:49 ` Jan Beulich
2016-03-21 16:03 ` Chong Li
1 sibling, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2016-03-21 15:49 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
>>> On 21.03.16 at 16:18, <lichong659@gmail.com> wrote:
> On Mon, Mar 21, 2016 at 8:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -338,24 +338,64 @@ 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];
>>
>> So why uint16_t[2] instead of just uint32_t? And what's the
>> padding needed for in the first place?
>
> You're right. It's better to use uint32_t, which pads (the struct) to
> the 64-bit boundary.
Which doesn't answer the "why in the first place" part - I don't
see why structure size needs to be a multiple of 64 bits.
>> Also, while for a domctl it's
>> not as strictly needed as for other hypercalls, checking that all
>> padding fields are zero would still seem to be rather desirable.
>>
>
> Do you mean:
>
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + if ( op->u.v.padding !=0 )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + 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.padding != 0 )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + break;
> + }
>
> ?
Yes, something along those lines.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-21 15:49 ` Jan Beulich
@ 2016-03-21 16:03 ` Chong Li
2016-03-21 16:25 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Chong Li @ 2016-03-21 16:03 UTC (permalink / raw)
To: Jan Beulich
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
On Mon, Mar 21, 2016 at 10:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 21.03.16 at 16:18, <lichong659@gmail.com> wrote:
>> On Mon, Mar 21, 2016 at 8:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
>>>> --- a/xen/include/public/domctl.h
>>>> +++ b/xen/include/public/domctl.h
>>>> @@ -338,24 +338,64 @@ 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];
>>>
>>> So why uint16_t[2] instead of just uint32_t? And what's the
>>> padding needed for in the first place?
>>
>> You're right. It's better to use uint32_t, which pads (the struct) to
>> the 64-bit boundary.
>
> Which doesn't answer the "why in the first place" part - I don't
> see why structure size needs to be a multiple of 64 bits.
>
In this patch post,
http://lists.xen.org/archives/html/xen-devel/2015-07/msg02334.html
you had a comment about the structure size, which I think you mean
the struct size should be a multiple of 64 bits.
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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-21 16:03 ` Chong Li
@ 2016-03-21 16:25 ` Jan Beulich
2016-03-25 18:07 ` Chong Li
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2016-03-21 16:25 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
>>> On 21.03.16 at 17:03, <lichong659@gmail.com> wrote:
> On Mon, Mar 21, 2016 at 10:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 21.03.16 at 16:18, <lichong659@gmail.com> wrote:
>>> On Mon, Mar 21, 2016 at 8:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
>>>>> --- a/xen/include/public/domctl.h
>>>>> +++ b/xen/include/public/domctl.h
>>>>> @@ -338,24 +338,64 @@ 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];
>>>>
>>>> So why uint16_t[2] instead of just uint32_t? And what's the
>>>> padding needed for in the first place?
>>>
>>> You're right. It's better to use uint32_t, which pads (the struct) to
>>> the 64-bit boundary.
>>
>> Which doesn't answer the "why in the first place" part - I don't
>> see why structure size needs to be a multiple of 64 bits.
>>
> In this patch post,
>
> http://lists.xen.org/archives/html/xen-devel/2015-07/msg02334.html
>
> you had a comment about the structure size, which I think you mean
> the struct size should be a multiple of 64 bits.
Looks like I had got mislead by there being struct
xen_domctl_sched_sedf, but it not being part of the union. I'm
sorry for that.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-21 16:25 ` Jan Beulich
@ 2016-03-25 18:07 ` Chong Li
2016-03-29 10:58 ` Dario Faggioli
2016-03-29 16:59 ` Dario Faggioli
0 siblings, 2 replies; 24+ messages in thread
From: Chong Li @ 2016-03-25 18:07 UTC (permalink / raw)
To: Jan Beulich
Cc: Chong Li, Sisu Xi, George Dunlap, dario.faggioli, xen-devel,
Meng Xu, Dagaen Golomb
On Mon, Mar 21, 2016 at 11:25 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 21.03.16 at 17:03, <lichong659@gmail.com> wrote:
>> On Mon, Mar 21, 2016 at 10:49 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 21.03.16 at 16:18, <lichong659@gmail.com> wrote:
>>>> On Mon, Mar 21, 2016 at 8:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
>>>>>> --- a/xen/include/public/domctl.h
>>>>>> +++ b/xen/include/public/domctl.h
>>>>>> @@ -338,24 +338,64 @@ 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];
>>>>>
>>>>> So why uint16_t[2] instead of just uint32_t? And what's the
>>>>> padding needed for in the first place?
>>>>
>>>> You're right. It's better to use uint32_t, which pads (the struct) to
>>>> the 64-bit boundary.
>>>
>>> Which doesn't answer the "why in the first place" part - I don't
>>> see why structure size needs to be a multiple of 64 bits.
>>>
>> In this patch post,
>>
>> http://lists.xen.org/archives/html/xen-devel/2015-07/msg02334.html
>>
>> you had a comment about the structure size, which I think you mean
>> the struct size should be a multiple of 64 bits.
>
> Looks like I had got mislead by there being struct
> xen_domctl_sched_sedf, but it not being part of the union. I'm
> sorry for that.
>
Ok. I've already fixed all problems pointed out by George and Jan.
Dario and Meng, do you have any other comments on this patch?
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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-25 18:07 ` Chong Li
@ 2016-03-29 10:58 ` Dario Faggioli
2016-03-29 16:59 ` Dario Faggioli
1 sibling, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-03-29 10:58 UTC (permalink / raw)
To: Chong Li, Jan Beulich
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 581 bytes --]
On Fri, 2016-03-25 at 13:07 -0500, Chong Li wrote:
> Ok. I've already fixed all problems pointed out by George and Jan.
>
> Dario and Meng, do you have any other comments on this patch?
>
Hey,
Sorry for the delay... busy, and then public holidays... I'll look at
this series later today.
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-25 18:07 ` Chong Li
2016-03-29 10:58 ` Dario Faggioli
@ 2016-03-29 16:59 ` Dario Faggioli
2016-03-29 17:13 ` Konrad Rzeszutek Wilk
1 sibling, 1 reply; 24+ messages in thread
From: Dario Faggioli @ 2016-03-29 16:59 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: 710 bytes --]
On Fri, 2016-03-25 at 13:07 -0500, Chong Li wrote:
> Ok. I've already fixed all problems pointed out by George and Jan.
>
> Dario and Meng, do you have any other comments on this patch?
>
Ok, I've looked at the patch and no, I don't have any further comments
on this patch (and it looks to me you addressed all the comments I made
on v7). So this one looks fine, with others' comments addressed, of
course.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 2/4] libxc: enable per-VCPU parameter for RTDS
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 2/4] libxc: " Chong Li
@ 2016-03-29 17:08 ` Dario Faggioli
2016-03-31 16:59 ` Wei Liu
1 sibling, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-03-29 17:08 UTC (permalink / raw)
To: Chong Li, xen-devel, Wei Liu
Cc: george.dunlap, dgolomb, Chong Li, Meng Xu, Sisu Xi
[-- Attachment #1.1: Type: text/plain, Size: 1614 bytes --]
On Fri, 2016-03-18 at 16:26 -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>
>
> ---
> Changes on PATCH v6:
> 1) Resolve some coding sytle issues
>
You did not mention any chage since v7, but it looks like you did the
changes I suggested.
That should have been mentioned, if only, to help people figuring out
that it's the only thing that actually changed.
Also, Wei said:
Acked-by: Wei Liu <wei.liu2@citrix.com>
And I said that, if you fixed the pointless initialization of rc, you
could add:
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
I think both still applies. Maybe it was ok to neglect Wei's Ack, since
you changed the subject line, which is important. But you should say
so, and catch his attention on this, so he can re-Ack the patch (if he
wants).
My tag, for sure, still applies, as I explicitly said so, and so you
should have put it in this version, to speedup review/commit.
In any case, I'm re-issueing it:
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Please, stick it below the S-o-b tags in next version.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-29 16:59 ` Dario Faggioli
@ 2016-03-29 17:13 ` Konrad Rzeszutek Wilk
2016-03-29 17:36 ` Dario Faggioli
0 siblings, 1 reply; 24+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-29 17:13 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Jan Beulich, Chong Li, Dagaen Golomb
On Tue, Mar 29, 2016 at 06:59:15PM +0200, Dario Faggioli wrote:
> On Fri, 2016-03-25 at 13:07 -0500, Chong Li wrote:
> > Ok. I've already fixed all problems pointed out by George and Jan.
> >
> > Dario and Meng, do you have any other comments on this patch?
> >
> Ok, I've looked at the patch and no, I don't have any further comments
> on this patch (and it looks to me you addressed all the comments I made
> on v7). So this one looks fine, with others' comments addressed, of
> course.
Sounds like an Reviewed-by?
>
> 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)
>
> _______________________________________________
> 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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
2016-03-29 17:13 ` Konrad Rzeszutek Wilk
@ 2016-03-29 17:36 ` Dario Faggioli
0 siblings, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-03-29 17:36 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Chong Li, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
Jan Beulich, Chong Li, Dagaen Golomb
[-- Attachment #1.1: Type: text/plain, Size: 1112 bytes --]
On Tue, 2016-03-29 at 13:13 -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 29, 2016 at 06:59:15PM +0200, Dario Faggioli wrote:
> > On Fri, 2016-03-25 at 13:07 -0500, Chong Li wrote:
> > > Ok. I've already fixed all problems pointed out by George and
> > > Jan.
> > >
> > > Dario and Meng, do you have any other comments on this patch?
> > >
> > Ok, I've looked at the patch and no, I don't have any further
> > comments
> > on this patch (and it looks to me you addressed all the comments I
> > made
> > on v7). So this one looks fine, with others' comments addressed, of
> > course.
> Sounds like an Reviewed-by?
>
It does sounds *like* it, but since this is being re-sent anyway, I'll
take the chance to check the new version --just to be sure-- and
provide it at that point/time, in all its glory. :-D
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-03-30 17:04 ` Dario Faggioli
2016-03-31 16:59 ` Wei Liu
1 sibling, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-03-30 17:04 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: 1038 bytes --]
On Fri, 2016-03-18 at 16:26 -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>
>
> ---
> Changes on PATCH v7:
> 1) Fix a mistake in sched_rtds_domain_set()
>
You happen to have done more than "just" this. In fact, I made some
comments unrelated to this issue, and you addressed them (which is a
good thing :-)).
It is ok for this to be a very very very quick summary, but it's only
useful if it is also complete.
In any case, this patch 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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 2/4] libxc: enable per-VCPU parameter for RTDS
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 2/4] libxc: " Chong Li
2016-03-29 17:08 ` Dario Faggioli
@ 2016-03-31 16:59 ` Wei Liu
1 sibling, 0 replies; 24+ messages in thread
From: Wei Liu @ 2016-03-31 16:59 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
xen-devel, Meng Xu, dgolomb
On Fri, Mar 18, 2016 at 04:26:23PM -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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-30 17:04 ` Dario Faggioli
@ 2016-03-31 16:59 ` Wei Liu
1 sibling, 0 replies; 24+ messages in thread
From: Wei Liu @ 2016-03-31 16:59 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 Fri, Mar 18, 2016 at 04:26:24PM -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>
>
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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 4/4] xl: " Chong Li
@ 2016-03-31 17:00 ` Wei Liu
2016-03-31 17:22 ` Dario Faggioli
2016-03-31 17:17 ` Dario Faggioli
1 sibling, 1 reply; 24+ messages in thread
From: Wei Liu @ 2016-03-31 17:00 UTC (permalink / raw)
To: Chong Li
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
xen-devel, Meng Xu, dgolomb
On Fri, Mar 18, 2016 at 04:26:25PM -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU settings.
>
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>
> ---
> Changes on PATCH v7:
> 1) Add example to xl.pod.1
>
So you've added what I asked. I'm satisfied with this patch.
Subject to ack or review by Dario:
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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 4/4] xl: " Chong Li
2016-03-31 17:00 ` Wei Liu
@ 2016-03-31 17:17 ` Dario Faggioli
2016-03-31 20:16 ` Wei Liu
1 sibling, 1 reply; 24+ messages in thread
From: Dario Faggioli @ 2016-03-31 17:17 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: 24684 bytes --]
On Fri, 2016-03-18 at 16:26 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU settings.
>
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>
Ok, so, importing this patch tells me this:
$ stg import -M /home/dario/[Xen-devel]_[PATCH_v8_for_Xen_4.7_4_4]_xl:_enable_per-VCPU_parameter_for_RTDS.mbox
Checking for changes in the working directory ... done
Importing patch "xl-enable-per-vcpu-parameter" ... <stdin>:67: trailing whitespace.
1) show the budget and period of each VCPU of each domain,
<stdin>:96: trailing whitespace.
2) show the budget and period of each VCPU of a specific domain,
<stdin>:97: trailing whitespace.
by using, e.g., "xl sched-rtds -d vm1 -v all" command. The output
<stdin>:108: trailing whitespace.
To show a subset of the parameters of the VCPUs of a specific domain,
<stdin>:109: trailing whitespace.
please use, e.g.,"xl sched-rtds -d vm1 -v 0 -v 3" command.
error: patch failed: tools/libxl/xl_cmdimpl.c:6215
error: tools/libxl/xl_cmdimpl.c: patch does not apply
stg import: Diff does not apply cleanly
While just applying it, tells me this:
$ patch -p1 < /home/dario/\[Xen-devel\]_\[PATCH_v8_for_Xen_4.7_4_4\]_xl\:_enable_per-VCPU_parameter_for_RTDS.mbox
patching file docs/man/xl.pod.1
patching file tools/libxl/xl_cmdimpl.c
Hunk #1 succeeded at 6137 (offset 314 lines).
Hunk #2 succeeded at 6302 (offset 314 lines).
Hunk #3 succeeded at 6406 (offset 314 lines).
Hunk #4 FAILED at 6351.
1 out of 4 hunks FAILED -- saving rejects to file tools/libxl/xl_cmdimpl.c.rej
patching file tools/libxl/xl_cmdtable.c
So, still some trailing white spaces, and some rebasing issue.
Actually, the failed hunk is possibly caused by delay in reviewing, and
I'm sorry for this. I've had a look at the .rej, and I'm not sure I see
what is really going wrong... Can you have a look yourself?
About the trailings, their in examples, not in code. Still, I'd ask you
to fix them.
> --- 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,79 @@ Restrict output to domains in the specified
> cpupool.
>
> =back
>
> +B<EXAMPLE>
> +
> +=over 4
> +
> +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:
> +
Use B<-v all> to see the budget and period of all the VCPUs of
all the domains:
> +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
>
Command and command output should be indented. Looking at the rest of
the file, 2 spaces looks what is mostly in other places.
It looks better and, I think, it would have an actual effect in the
rendered manpage.
In order to prevent the line for getting to long, feel free to mangle
the output a little bit, and lose some of the spaces between the domain
names and their IDs.
> +Using "xl sched-rtds" will output the default scheduling parameters
> +for each domain. An example would be like:
> +
Without any arguments, it will output the default scheduling
parameters for each domain:
> +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
>
Same here about indentation (and everywhere else below, so I'll avoid
repeating this)
> +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:
> +
Use, for instance B<-d vm1 -v all> to see the budget and period of all
VCPUs of a specific domain (B<vm1>):
> +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:
> +
To see the parameters of a subset of the VCPUs of a domain, use:
> +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:
> +
If no B<-v> is specified, the default scheduling parameter for the
domain are shown:
> +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,
It is possible to change the budget and the period of multiple VCPUs
of a domain with just one command, for instance:
> +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.
To change the parameters of all the VCPUs of a domain, use B<-v all>:
> --- 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);
> + }
For new code, let's use EXIT_SUCCESS and EXIT_FAILURE.
Oh, just in case, that applies to when exit() is used, so...
> + 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;
> +}
> +
... these two returns are ok to be 1 and 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;
> +}
> +
These two functions either terminate the program, or return 0. That
means they can be void.
> @@ -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;
> +
I don't see the need for propagating rc... This should just return 0 or
1, like, for instance, sched_credit_domain_output() is doing, doesn't
it?
> @@ -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 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 1;
> + }
> +
> + 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;
>
Variables should be declared at the beginning of the block, i.e., just
below for(){, in this case.
> + libxl_vcpu_sched_params_init(&scinfo_out);
> + output(-1, &scinfo_out);
> + libxl_vcpu_sched_params_dispose(&scinfo_out);
>
And by the way, when calling output() with -1, which I think means
"just print the table header, do we really need a valid and inited
libxl_vcpu_sched_params? Can't it be NULL (sched_rtds_vcpu_output()
looks already able to work well in this case to me).
> + for (i = 0; i < nb_domain; i++) {
> + if (info[i].cpupool != poolinfo[p].poolid)
> + continue;
> + libxl_vcpu_sched_params scinfo;
>
Ditto about variable declaration.
> + libxl_vcpu_sched_params_init(&scinfo);
> + scinfo.num_vcpus = 0;
> + rc = output(info[i].domid, &scinfo);
>
Mm.. so 0 because we want to all vcpus. Ugly. As it was ugly in the set
case, and that is why we added libxl_*_set_all(). I'm sorry I'm
spotting this only now, but I think we need
a libxl_vcpu_sched_params_get_all(). :-/
> @@ -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
> + */
Comment style.
> + 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;
> }
>
So, the logic here is really complex, but I think it's correct. The
only problem I spot is the function return value (which is an actual
exit code for xl, since this is a main_bla() kind of function).
The function should return either EXIT_SUCCESS or EXIT_FAILURE.
With your changes, I think there are chances for this to not be the
case, e.g.:
+ } 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;
Am I right?
IMO, this should be fixed, to prevent even more inconsistency in xl
than what we have already on this front
A minor thing, while you're there, don't use a variable called rc for
this ('ret' or 'r' would be a better fit with this component's coding
style).
Sorry again for the delay replying to this.
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
2016-03-31 17:00 ` Wei Liu
@ 2016-03-31 17:22 ` Dario Faggioli
0 siblings, 0 replies; 24+ messages in thread
From: Dario Faggioli @ 2016-03-31 17:22 UTC (permalink / raw)
To: Wei Liu, Chong Li
Cc: Chong Li, Sisu Xi, george.dunlap, xen-devel, Meng Xu, dgolomb
[-- Attachment #1.1: Type: text/plain, Size: 1495 bytes --]
On Thu, 2016-03-31 at 18:00 +0100, Wei Liu wrote:
> On Fri, Mar 18, 2016 at 04:26:25PM -0500, Chong Li wrote:
> >
> > Change main_sched_rtds and related output functions to support
> > per-VCPU settings.
> >
> > Signed-off-by: Chong Li <chong.li@wustl.edu>
> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> > Signed-off-by: Sisu Xi <xisisu@gmail.com>
> >
> > ---
> > Changes on PATCH v7:
> > 1) Add example to xl.pod.1
> >
> So you've added what I asked. I'm satisfied with this patch.
>
> Subject to ack or review by Dario:
>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
>
Here I am. I had a look (and sorry if it took a bit), and I indeed
found what I believe are a few issues.
The re-wording of the examples in the manual page are improvements IMO
(although I'm not a native speaker myself), but, much more important,
I've seen things in the code that I think need changing.
Nothing too complicated to do, I think, but still something.
So, Wei, perhaps you can give a quick look at my comments and say
whether you think my observations make any sense? If they do, and if
Chong respins the series, I guarantee I'll be much quicker in re-
reviewing 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] 24+ messages in thread
* Re: [PATCH v8 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
2016-03-31 17:17 ` Dario Faggioli
@ 2016-03-31 20:16 ` Wei Liu
0 siblings, 0 replies; 24+ messages in thread
From: Wei Liu @ 2016-03-31 20:16 UTC (permalink / raw)
To: Dario Faggioli
Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, xen-devel, Meng Xu,
Chong Li, dgolomb
On Thu, Mar 31, 2016 at 07:17:57PM +0200, Dario Faggioli wrote:
[...]
> > +3) Users can set the budget and period of multiple VCPUs of a
> > +specific domain with only one command,
> It is possible to change the budget and the period of multiple VCPUs
> of a domain with just one command, for instance:
>
> > +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.
> To change the parameters of all the VCPUs of a domain, use B<-v all>:
>
Documentations are always open to improvement even after freeze, so I
think we're mostly fine here. Whether Chong fixes that in the next
version or submit subsequent patch for docs is fine.
Let's focus on the code itself now, that's something we need to get
right.
> > --- 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);
> > + }
> For new code, let's use EXIT_SUCCESS and EXIT_FAILURE.
>
> Oh, just in case, that applies to when exit() is used, so...
>
This needs fixing.
> > + 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;
> > +}
> > +
> ... these two returns are ok to be 1 and 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;
> > +}
> > +
> These two functions either terminate the program, or return 0. That
> means they can be void.
>
Nice to have, but not strictly required. It's not a bug per se. I won't
block this series just because of this.
> > @@ -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;
> > +
> I don't see the need for propagating rc... This should just return 0 or
> 1, like, for instance, sched_credit_domain_output() is doing, doesn't
> it?
>
Again, nice to have. Good to be consistent. Not a bug per se. Not a
blocker.
> > @@ -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 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 1;
> > + }
> > +
> > + 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;
> >
> Variables should be declared at the beginning of the block, i.e., just
> below for(){, in this case.
>
This needs fixing. It might trip over strict compiler setting.
> > + libxl_vcpu_sched_params_init(&scinfo_out);
> > + output(-1, &scinfo_out);
> > + libxl_vcpu_sched_params_dispose(&scinfo_out);
> >
> And by the way, when calling output() with -1, which I think means
> "just print the table header, do we really need a valid and inited
> libxl_vcpu_sched_params? Can't it be NULL (sched_rtds_vcpu_output()
> looks already able to work well in this case to me).
>
Right, this looks simple enough to fix.
> > + for (i = 0; i < nb_domain; i++) {
> > + if (info[i].cpupool != poolinfo[p].poolid)
> > + continue;
> > + libxl_vcpu_sched_params scinfo;
> >
> Ditto about variable declaration.
>
> > + libxl_vcpu_sched_params_init(&scinfo);
> > + scinfo.num_vcpus = 0;
> > + rc = output(info[i].domid, &scinfo);
> >
> Mm.. so 0 because we want to all vcpus. Ugly. As it was ugly in the set
> case, and that is why we added libxl_*_set_all(). I'm sorry I'm
> spotting this only now, but I think we need
> a libxl_vcpu_sched_params_get_all(). :-/
>
Yes, this makes sense.
> > @@ -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
> > + */
> Comment style.
>
This needs fixing.
> > + 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;
> > }
> >
> So, the logic here is really complex, but I think it's correct. The
> only problem I spot is the function return value (which is an actual
> exit code for xl, since this is a main_bla() kind of function).
>
> The function should return either EXIT_SUCCESS or EXIT_FAILURE.
>
> With your changes, I think there are chances for this to not be the
> case, e.g.:
>
> + } 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;
>
> Am I right?
>
> IMO, this should be fixed, to prevent even more inconsistency in xl
> than what we have already on this front
>
Yes, this needs fixing, too.
Chong, when you finish your new series, can you also provide a git
branch for it. Thanks.
Wei.
> A minor thing, while you're there, don't use a variable called rc for
> this ('ret' or 'r' would be a better fit with this component's coding
> style).
>
> Sorry again for the delay replying to this.
>
> 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)
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2016-03-31 20:16 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-18 21:26 [PATCH v8 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 1/4] xen: enable " Chong Li
2016-03-21 13:35 ` Jan Beulich
2016-03-21 15:18 ` Chong Li
2016-03-21 15:39 ` George Dunlap
2016-03-21 15:49 ` Jan Beulich
2016-03-21 16:03 ` Chong Li
2016-03-21 16:25 ` Jan Beulich
2016-03-25 18:07 ` Chong Li
2016-03-29 10:58 ` Dario Faggioli
2016-03-29 16:59 ` Dario Faggioli
2016-03-29 17:13 ` Konrad Rzeszutek Wilk
2016-03-29 17:36 ` Dario Faggioli
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 2/4] libxc: " Chong Li
2016-03-29 17:08 ` Dario Faggioli
2016-03-31 16:59 ` Wei Liu
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-30 17:04 ` Dario Faggioli
2016-03-31 16:59 ` Wei Liu
2016-03-18 21:26 ` [PATCH v8 for Xen 4.7 4/4] xl: " Chong Li
2016-03-31 17:00 ` Wei Liu
2016-03-31 17:22 ` Dario Faggioli
2016-03-31 17:17 ` Dario Faggioli
2016-03-31 20:16 ` 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).