[Goal] The current xl sched-rtds tool can only set the VCPUs of a domain to the same parameter although the scheduler supports VCPUs with different parameters. This patchset is to enable xl sched-rtds tool to configure the VCPUs of a domain with different parameters. This per-VCPU settings can be used in many scenarios. For example, based on Dario's statement in our pervious discussion (http://lists.xen.org/archives/html/xen-devel/2014-09/msg00423.html), if there are two real-time applications, which have different timing requirements, running in a multi-VCPU guest domain, it is beneficial to pin these two applications to two seperate VCPUs with different scheduling parameters. What this patchset includes is a wanted and planned feature for RTDS scheudler(http://wiki.xenproject.org/wiki/RTDS-Based-Scheduler) in Xen 4.7. The interface design of the xl sched-rtds tool is based on Meng's previous discussion with Dario, George and Wei (http://lists.xen.org/archives/html/xen-devel/2015-02/msg02606.html). Basically, there are two main changes: 1) in xl, we create an array that records all VCPUs whose parameters are about to modify or output. 2) in libxl, we receive the array and call different xc functions to handle it. 3) in xen and libxc, we use XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo(introduced by this patchset) as the hypercall for per-VCPU operations(get/set method). [Usage] With this patchset in use, xl sched-rtds tool can: 1) show the budget and period of each VCPU of each domain, by using "xl sched-rtds -v all" command. An example would be like: # xl sched-rtds -v all Cpupool Pool-0: sched=RTDS Name ID VCPU Period Budget Domain-0 0 0 10000 4000 vm1 1 0 300 150 vm1 1 1 400 200 vm1 1 2 10000 4000 vm1 1 3 1000 500 vm2 2 0 10000 4000 vm2 2 1 10000 4000 Using "xl sched-rtds" will output the default scheduling parameters for each domain. An example would be like: # xl sched-rtds Cpupool Pool-0: sched=RTDS Name ID Period Budget Domain-0 0 10000 4000 vm1 1 10000 4000 vm2 2 10000 4000 2) show the budget and period of each VCPU of a specific domain, by using, e.g., "xl sched-rtds -d vm1 -v all" command. The output would be like: # xl sched-rtds -d vm1 -v all Name ID VCPU Period Budget vm1 1 0 300 150 vm1 1 1 400 200 vm1 1 2 10000 4000 vm1 1 3 1000 500 To show a subset of the parameters of the VCPUs of a specific domain, please use, e.g.,"xl sched-rtds -d vm1 -v 0 -v 3" command. The output would be: # xl sched-rtds -d vm1 -v 0 -v 3 Name ID VCPU Period Budget vm1 1 0 300 150 vm1 1 3 1000 500 Using command, e.g., "xl sched-rtds -d vm1" will output the default scheduling parameters of vm1. An example would be like: # xl sched-rtds -d vm1 Name ID Period Budget vm1 1 10000 4000 3) Users can set the budget and period of multiple VCPUs of a specific domain with only one command, e.g., "xl sched-rtds -d vm1 -v 0 -p 100 -b 50 -v 3 -p 300 -b 150". Users can set all VCPUs with the same parameters, by one command. e.g., "xl sched-rtds -d vm1 -v all -p 500 -b 250". --- CC: <dario.faggioli@citrix.com> CC: <george.dunlap@eu.citrix.com> CC: <dgolomb@seas.upenn.edu> CC: <mengxu@cis.upenn.edu> CC: <jbeulich@suse.com> CC: <wei.liu2@citrix.com> CC: <lichong659@gmail.com> CC: <ian.jackson@eu.citrix.com> CC: <ian.campbell@eu.citrix.com> Chong Li (4): xen: enable per-VCPU parameter settings for RTDS scheduler libxc: enable per-VCPU parameter settings for RTDS scheduler libxl: enable per-VCPU parameter settings for RTDS scheduler xl: enable per-VCPU parameter settings for RTDS scheduler docs/man/xl.pod.1 | 38 +++++ tools/libxc/include/xenctrl.h | 8 ++ tools/libxc/xc_rt.c | 68 +++++++++ tools/libxl/libxl.c | 321 +++++++++++++++++++++++++++++++++++++++--- tools/libxl/libxl.h | 37 +++++ tools/libxl/libxl_types.idl | 14 ++ tools/libxl/xl_cmdimpl.c | 301 ++++++++++++++++++++++++++++++++++----- tools/libxl/xl_cmdtable.c | 16 ++- xen/common/sched_credit.c | 17 ++- xen/common/sched_credit2.c | 16 ++- xen/common/sched_rt.c | 114 +++++++++++++-- xen/common/schedule.c | 15 +- xen/include/public/domctl.h | 63 +++++++-- 13 files changed, 928 insertions(+), 100 deletions(-) -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls to independently get and set the scheduling parameters of each vCPU of a domain Signed-off-by: Chong Li <chong.li@wustl.edu> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> Signed-off-by: Sisu Xi <xisisu@gmail.com> --- Changes on PATCH v6: 1) Add explain for nr_vcpus in struct xen_domctl_scheduler_op, because it is used in both IN and OUT ways. 2) Remove the check and warning for vcpu settings with low budget or budget. Making this feature "per-domain" or "per-operation" is one of the future work. 3) In the *cntl() functions in sched_credit.c and sched_credit2.c, change the "if-else" structure to "switch" structure. 4) In rt_dom_cntl(), use copy_to_guest* instead of __copy_to_guest*, because the latter one requires lock protection. Changes on PATCH v5: 1) When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we do preemption check in a similar way to XEN_SYSCTL_pcitopoinfo Changes on PATCH v4: 1) Add uint32_t vcpu_index to struct xen_domctl_scheduler_op. When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we call hypercall_preemption_check in case the current hypercall lasts too long. If we decide to preempt the current hypercall, we record the index of the most-recent finished vcpu into the vcpu_index of struct xen_domctl_scheduler_op. So when we resume the hypercall after preemption, we start processing from the posion specified by vcpu_index, and don't need to repeat the work that has already been done in the hypercall before the preemption. (This design is based on the do_grant_table_op() in grant_table.c) 2) Coding style changes Changes on PATCH v3: 1) Remove struct xen_domctl_schedparam_t. 2) Change struct xen_domctl_scheduler_op. 3) Check if period/budget is within a validated range Changes on PATCH v2: 1) Change struct xen_domctl_scheduler_op, for transferring per-vcpu parameters between libxc and hypervisor. 2) Handler of XEN_DOMCTL_SCHEDOP_getinfo now just returns the default budget and period values of RTDS scheduler. 3) Handler of XEN_DOMCTL_SCHEDOP_getvcpuinfo now can return a random subset of the parameters of the VCPUs of a specific domain CC: <dario.faggioli@citrix.com> CC: <george.dunlap@eu.citrix.com> CC: <dgolomb@seas.upenn.edu> CC: <mengxu@cis.upenn.edu> CC: <jbeulich@suse.com> CC: <lichong659@gmail.com> --- xen/common/sched_credit.c | 17 ++++--- xen/common/sched_credit2.c | 16 ++++--- xen/common/sched_rt.c | 114 ++++++++++++++++++++++++++++++++++++++------ xen/common/schedule.c | 15 ++++-- xen/include/public/domctl.h | 63 +++++++++++++++++++----- 5 files changed, 183 insertions(+), 42 deletions(-) diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index 0dce790..82b0d14 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -1054,15 +1054,16 @@ csched_dom_cntl( * lock. Runq lock not needed anywhere in here. */ spin_lock_irqsave(&prv->lock, flags); - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) + switch ( op->cmd ) { + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + return -EINVAL; + case XEN_DOMCTL_SCHEDOP_getinfo: op->u.credit.weight = sdom->weight; op->u.credit.cap = sdom->cap; - } - else - { - ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo); - + break; + case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.credit.weight != 0 ) { if ( !list_empty(&sdom->active_sdom_elem) ) @@ -1075,7 +1076,9 @@ csched_dom_cntl( if ( op->u.credit.cap != (uint16_t)~0U ) sdom->cap = op->u.credit.cap; - + break; + default: + return -EINVAL; } spin_unlock_irqrestore(&prv->lock, flags); diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index 3c49ffa..46d54bc 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -1421,14 +1421,15 @@ csched2_dom_cntl( * runq lock to update csvcs. */ spin_lock_irqsave(&prv->lock, flags); - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) + switch ( op->cmd ) { + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + return -EINVAL; + case XEN_DOMCTL_SCHEDOP_getinfo: op->u.credit2.weight = sdom->weight; - } - else - { - ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo); - + break; + case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.credit2.weight != 0 ) { struct vcpu *v; @@ -1457,6 +1458,9 @@ csched2_dom_cntl( vcpu_schedule_unlock(lock, svc->vcpu); } } + break; + default: + return -EINVAL; } spin_unlock_irqrestore(&prv->lock, flags); diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c index 3f1d047..359c2db 100644 --- a/xen/common/sched_rt.c +++ b/xen/common/sched_rt.c @@ -86,6 +86,22 @@ #define RTDS_DEFAULT_PERIOD (MICROSECS(10000)) #define RTDS_DEFAULT_BUDGET (MICROSECS(4000)) +/* + * Max period: max delta of time type, because period is added to the time + * a vcpu activates, so this must not overflow. + * Min period: 10 us, considering the scheduling overhead (when period is + * too low, scheduling is invoked too frequently, causing high overhead). + */ +#define RTDS_MAX_PERIOD (STIME_DELTA_MAX) +#define RTDS_MIN_PERIOD (MICROSECS(10)) + +/* + * Min budget: 10 us, considering the scheduling overhead (when budget is + * consumed too fast, scheduling is invoked too frequently, causing + * high overhead). + */ +#define RTDS_MIN_BUDGET (MICROSECS(10)) + #define UPDATE_LIMIT_SHIFT 10 #define MAX_SCHEDULE (MILLISECS(1)) /* @@ -1129,24 +1145,22 @@ rt_dom_cntl( struct vcpu *v; unsigned long flags; int rc = 0; + xen_domctl_schedparam_vcpu_t local_sched; + s_time_t period, budget; + uint32_t index = 0; switch ( op->cmd ) { case XEN_DOMCTL_SCHEDOP_getinfo: - if ( d->max_vcpus > 0 ) - { - spin_lock_irqsave(&prv->lock, flags); - svc = rt_vcpu(d->vcpu[0]); - op->u.rtds.period = svc->period / MICROSECS(1); - op->u.rtds.budget = svc->budget / MICROSECS(1); - spin_unlock_irqrestore(&prv->lock, flags); - } - else - { - /* If we don't have vcpus yet, let's just return the defaults. */ - op->u.rtds.period = RTDS_DEFAULT_PERIOD; - op->u.rtds.budget = RTDS_DEFAULT_BUDGET; - } + /* Return the default parameters. + * A previous bug fixed here: + * The default PERIOD or BUDGET should be divided by MICROSECS(1), + * before returned to upper caller. + */ + spin_lock_irqsave(&prv->lock, flags); + op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1); + op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1); + spin_unlock_irqrestore(&prv->lock, flags); break; case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 ) @@ -1163,6 +1177,78 @@ rt_dom_cntl( } spin_unlock_irqrestore(&prv->lock, flags); break; + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + while ( index < op->u.v.nr_vcpus ) + { + if ( copy_from_guest_offset(&local_sched, + op->u.v.vcpus, index, 1) ) + { + rc = -EFAULT; + break; + } + if ( local_sched.vcpuid >= d->max_vcpus || + d->vcpu[local_sched.vcpuid] == NULL ) + { + rc = -EINVAL; + break; + } + + spin_lock_irqsave(&prv->lock, flags); + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); + local_sched.u.rtds.budget = svc->budget / MICROSECS(1); + local_sched.u.rtds.period = svc->period / MICROSECS(1); + spin_unlock_irqrestore(&prv->lock, flags); + + if ( copy_to_guest_offset(op->u.v.vcpus, index, + &local_sched, 1) ) + { + rc = -EFAULT; + break; + } + if ( (++index > 0x3f) && hypercall_preempt_check() ) + break; + } + if ( !rc ) /* notify upper caller how many vcpus have been processed */ + op->u.v.nr_vcpus = index; + break; + + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + while ( index < op->u.v.nr_vcpus ) + { + if ( copy_from_guest_offset(&local_sched, + op->u.v.vcpus, index, 1) ) + { + rc = -EFAULT; + break; + } + if ( local_sched.vcpuid >= d->max_vcpus || + d->vcpu[local_sched.vcpuid] == NULL ) + { + rc = -EINVAL; + break; + } + + period = MICROSECS(local_sched.u.rtds.period); + budget = MICROSECS(local_sched.u.rtds.budget); + if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET || + budget > period || period < RTDS_MIN_PERIOD ) + { + rc = -EINVAL; + break; + } + + spin_lock_irqsave(&prv->lock, flags); + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); + svc->period = period; + svc->budget = budget; + spin_unlock_irqrestore(&prv->lock, flags); + + if ( (++index > 0x3f) && hypercall_preempt_check() ) + break; + } + if ( !rc ) /* notify upper caller how many vcpus have been processed */ + op->u.v.nr_vcpus = index; + break; } return rc; diff --git a/xen/common/schedule.c b/xen/common/schedule.c index c195129..cd25e08 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -1148,11 +1148,20 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op) if ( ret ) return ret; - if ( (op->sched_id != DOM2OP(d)->sched_id) || - ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) && - (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) ) + if ( op->sched_id != DOM2OP(d)->sched_id ) return -EINVAL; + switch ( op->cmd ) + { + case XEN_DOMCTL_SCHEDOP_putinfo: + case XEN_DOMCTL_SCHEDOP_getinfo: + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + break; + default: + return -EINVAL; + } + /* NB: the pluggable scheduler code needs to take care * of locking by itself. */ if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 ) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 7a56b3f..9297c01 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -338,24 +338,63 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t); #define XEN_SCHEDULER_ARINC653 7 #define XEN_SCHEDULER_RTDS 8 -/* Set or get info? */ +typedef struct xen_domctl_sched_credit { + uint16_t weight; + uint16_t cap; +} xen_domctl_sched_credit_t; + +typedef struct xen_domctl_sched_credit2 { + uint16_t weight; +} xen_domctl_sched_credit2_t; + +typedef struct xen_domctl_sched_rtds { + uint32_t period; + uint32_t budget; +} xen_domctl_sched_rtds_t; + +typedef struct xen_domctl_schedparam_vcpu { + union { + xen_domctl_sched_credit_t credit; + xen_domctl_sched_credit2_t credit2; + xen_domctl_sched_rtds_t rtds; + } u; + uint32_t vcpuid; + uint16_t padding[2]; +} xen_domctl_schedparam_vcpu_t; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t); + +/* + * Set or get info? + * For schedulers supporting per-vcpu settings (e.g., RTDS): + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; + * XEN_DOMCTL_SCHEDOP_getinfo gets default params; + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus; + * + * For schedulers not supporting per-vcpu settings: + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; + * XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params; + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error; + */ #define XEN_DOMCTL_SCHEDOP_putinfo 0 #define XEN_DOMCTL_SCHEDOP_getinfo 1 +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2 +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3 struct xen_domctl_scheduler_op { uint32_t sched_id; /* XEN_SCHEDULER_* */ uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */ union { - struct xen_domctl_sched_credit { - uint16_t weight; - uint16_t cap; - } credit; - struct xen_domctl_sched_credit2 { - uint16_t weight; - } credit2; - struct xen_domctl_sched_rtds { - uint32_t period; - uint32_t budget; - } rtds; + xen_domctl_sched_credit_t credit; + xen_domctl_sched_credit2_t credit2; + xen_domctl_sched_rtds_t rtds; + struct { + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus; + /* + * IN: Number of elements in vcpus array. + * OUT: Number of processed elements of vcpus array. + */ + uint32_t nr_vcpus; + uint32_t padding; + } v; } u; }; typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t; -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Add xc_sched_rtds_vcpu_get/set functions to interact with Xen to get/set a domain's per-VCPU parameters. Signed-off-by: Chong Li <chong.li@wustl.edu> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> Signed-off-by: Sisu Xi <xisisu@gmail.com> --- Changes on PATCH v6: 1) Resolve some coding sytle issues Changes on PATCH v5: 1) In xc_sched_rtds_vcpu_get/set, re-issueing the hypercall if it is preempted. Changes on PATCH v4: 1) Minor modifications on the function parameters. Changes on PATCH v2: 1) Minor modifications due to the change of struct xen_domctl_scheduler_op. CC: <dario.faggioli@citrix.com> CC: <george.dunlap@eu.citrix.com> CC: <dgolomb@seas.upenn.edu> CC: <mengxu@cis.upenn.edu> CC: <wei.liu2@citrix.com> CC: <lichong659@gmail.com> --- tools/libxc/include/xenctrl.h | 8 +++++ tools/libxc/xc_rt.c | 68 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 01a6dda..4567585 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -893,6 +893,14 @@ int xc_sched_rtds_domain_set(xc_interface *xch, int xc_sched_rtds_domain_get(xc_interface *xch, uint32_t domid, struct xen_domctl_sched_rtds *sdom); +int xc_sched_rtds_vcpu_set(xc_interface *xch, + uint32_t domid, + struct xen_domctl_schedparam_vcpu *vcpus, + uint32_t num_vcpus); +int xc_sched_rtds_vcpu_get(xc_interface *xch, + uint32_t domid, + struct xen_domctl_schedparam_vcpu *vcpus, + uint32_t num_vcpus); int xc_sched_arinc653_schedule_set( diff --git a/tools/libxc/xc_rt.c b/tools/libxc/xc_rt.c index d59e5ce..d1d1aa5 100644 --- a/tools/libxc/xc_rt.c +++ b/tools/libxc/xc_rt.c @@ -62,3 +62,71 @@ int xc_sched_rtds_domain_get(xc_interface *xch, return rc; } + +int xc_sched_rtds_vcpu_set(xc_interface *xch, + uint32_t domid, + struct xen_domctl_schedparam_vcpu *vcpus, + uint32_t num_vcpus) +{ + int rc = 0; + unsigned processed = 0; + DECLARE_DOMCTL; + DECLARE_HYPERCALL_BOUNCE(vcpus, sizeof(*vcpus) * num_vcpus, + XC_HYPERCALL_BUFFER_BOUNCE_IN); + + if ( xc_hypercall_bounce_pre(xch, vcpus) ) + return -1; + + domctl.cmd = XEN_DOMCTL_scheduler_op; + domctl.domain = (domid_t) domid; + domctl.u.scheduler_op.sched_id = XEN_SCHEDULER_RTDS; + domctl.u.scheduler_op.cmd = XEN_DOMCTL_SCHEDOP_putvcpuinfo; + + while ( processed < num_vcpus ) + { + domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus - processed; + set_xen_guest_handle_offset(domctl.u.scheduler_op.u.v.vcpus, vcpus, + processed); + if ( (rc = do_domctl(xch, &domctl)) != 0 ) + break; + processed += domctl.u.scheduler_op.u.v.nr_vcpus; + } + + xc_hypercall_bounce_post(xch, vcpus); + + return rc; +} + +int xc_sched_rtds_vcpu_get(xc_interface *xch, + uint32_t domid, + struct xen_domctl_schedparam_vcpu *vcpus, + uint32_t num_vcpus) +{ + int rc; + unsigned processed = 0; + DECLARE_DOMCTL; + DECLARE_HYPERCALL_BOUNCE(vcpus, sizeof(*vcpus) * num_vcpus, + XC_HYPERCALL_BUFFER_BOUNCE_BOTH); + + if ( xc_hypercall_bounce_pre(xch, vcpus) ) + return -1; + + domctl.cmd = XEN_DOMCTL_scheduler_op; + domctl.domain = (domid_t) domid; + domctl.u.scheduler_op.sched_id = XEN_SCHEDULER_RTDS; + domctl.u.scheduler_op.cmd = XEN_DOMCTL_SCHEDOP_getvcpuinfo; + + while ( processed < num_vcpus ) + { + domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus - processed; + set_xen_guest_handle_offset(domctl.u.scheduler_op.u.v.vcpus, vcpus, + processed); + if ( (rc = do_domctl(xch, &domctl)) != 0 ) + break; + processed += domctl.u.scheduler_op.u.v.nr_vcpus; + } + + xc_hypercall_bounce_post(xch, vcpus); + + return rc; +} -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support per-VCPU settings. Signed-off-by: Chong Li <chong.li@wustl.edu> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> Signed-off-by: Sisu Xi <xisisu@gmail.com> --- Changes on PATCH v6: 1) Resolve some coding style issues 2) Change sched_rtds_validate_params() 3) Small changes for sched_rtds_vcpus_params_set(all) functions Changes on PATCH v5: 1) Add a seperate function, sched_rtds_vcpus_params_set_all(), to set the parameters of all vcpus of a domain. 2) Add libxl_vcpu_sched_params_set_all() to invoke the above function. 3) Coding style changes. (I didn't find the indentation rules for function calls with long parameters (still 4 spaces?), so I just imitated the indentation style of some existing functions) Changes on PATCH v4: 1) Coding style changes Changes on PATCH v3: 1) Add sanity check on vcpuid 2) Add comments on per-domain and per-vcpu functions for libxl users Changes on PATCH v2: 1) New data structure (libxl_vcpu_sched_params and libxl_sched_params) to help per-VCPU settings. 2) sched_rtds_vcpu_get now can return a random subset of the parameters of the VCPUs of a specific domain. CC: <dario.faggioli@citrix.com> CC: <george.dunlap@eu.citrix.com> CC: <dgolomb@seas.upenn.edu> CC: <mengxu@cis.upenn.edu> CC: <wei.liu2@citrix.com> CC: <lichong659@gmail.com> CC: <ian.jackson@eu.citrix.com> CC: <ian.campbell@eu.citrix.com> --- tools/libxl/libxl.c | 321 ++++++++++++++++++++++++++++++++++++++++---- tools/libxl/libxl.h | 37 +++++ tools/libxl/libxl_types.idl | 14 ++ 3 files changed, 349 insertions(+), 23 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index bd3aac8..0f9fb7e 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5770,6 +5770,201 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid, return 0; } +static int sched_rtds_validate_params(libxl__gc *gc, int period, int budget) +{ + int rc; + + if (period < 1) { + LOG(ERROR, "VCPU period is out of range, " + "valid values are larger than or equal to 1"); + rc = ERROR_INVAL; /* error scheduling parameter */ + goto out; + } + + if (budget < 1) { + LOG(ERROR, "VCPU budget is not set or out of range, " + "valid values are larger than or equal to 1"); + rc = ERROR_INVAL; + goto out; + } + + if (budget > period) { + LOG(ERROR, "VCPU budget must be smaller than " + "or equal to VCPU period"); + rc = ERROR_INVAL; + goto out; + } + rc = 0; +out: + return rc; +} + +/* Get the RTDS scheduling parameters of vcpu(s) */ +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + uint32_t num_vcpus; + int i, r, rc; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + + num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus : + info.max_vcpu_id + 1; + + GCNEW_ARRAY(vcpus, num_vcpus); + + if (scinfo->num_vcpus > 0) { + for (i = 0; i < num_vcpus; i++) { + if (scinfo->vcpus[i].vcpuid < 0 || + scinfo->vcpus[i].vcpuid > info.max_vcpu_id) { + LOG(ERROR, "VCPU index is out of range, " + "valid values are within range from 0 to %d", + info.max_vcpu_id); + rc = ERROR_INVAL; + goto out; + } + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + } + } else + for (i = 0; i < num_vcpus; i++) + vcpus[i].vcpuid = i; + + r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "getting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + scinfo->sched = LIBXL_SCHEDULER_RTDS; + if (scinfo->num_vcpus == 0) { + scinfo->num_vcpus = num_vcpus; + scinfo->vcpus = libxl__calloc(NOGC, num_vcpus, + sizeof(libxl_sched_params)); + } + for (i = 0; i < num_vcpus; i++) { + scinfo->vcpus[i].period = vcpus[i].u.rtds.period; + scinfo->vcpus[i].budget = vcpus[i].u.rtds.budget; + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; + } + rc = 0; +out: + return rc; +} + +/* Set the RTDS scheduling parameters of vcpu(s) */ +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + int r, rc; + int i; + uint16_t max_vcpuid; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + max_vcpuid = info.max_vcpu_id; + + if (scinfo->num_vcpus <= 0) { + rc = ERROR_INVAL; + goto out; + } else { + for (i = 0; i < scinfo->num_vcpus; i++) { + if (scinfo->vcpus[i].vcpuid < 0 || + scinfo->vcpus[i].vcpuid > max_vcpuid) { + LOG(ERROR, "VCPU index is out of range, " + "valid values are within range from 0 to %d", + max_vcpuid); + rc = ERROR_INVAL; + goto out; + } + rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period, + scinfo->vcpus[i].budget); + if (rc) { + rc = ERROR_INVAL; + goto out; + } + } + GCNEW_ARRAY(vcpus, scinfo->num_vcpus); + for (i = 0; i < scinfo->num_vcpus; i++) { + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + vcpus[i].u.rtds.period = scinfo->vcpus[i].period; + vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget; + } + } + + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, scinfo->num_vcpus); + if (r != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + rc = 0; +out: + return rc; +} + +/* Set the RTDS scheduling parameters of all vcpus of a domain */ +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + int r, rc; + int i; + uint16_t max_vcpuid; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + uint32_t num_vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + max_vcpuid = info.max_vcpu_id; + + if (scinfo->num_vcpus != 1) { + rc = ERROR_INVAL; + goto out; + } else { + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, + scinfo->vcpus[0].budget)) { + rc = ERROR_INVAL; + goto out; + } + num_vcpus = max_vcpuid + 1; + GCNEW_ARRAY(vcpus, num_vcpus); + for (i = 0; i < num_vcpus; i++) { + vcpus[i].vcpuid = i; + vcpus[i].u.rtds.period = scinfo->vcpus[0].period; + vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget; + } + } + + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + rc = 0; +out: + return rc; +} + static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5802,30 +5997,10 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, LOGE(ERROR, "getting domain sched rtds"); return ERROR_FAIL; } - - if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { - if (scinfo->period < 1) { - LOG(ERROR, "VCPU period is not set or out of range, " - "valid values are larger than 1"); - return ERROR_INVAL; - } - sdom.period = scinfo->period; - } - - if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { - if (scinfo->budget < 1) { - LOG(ERROR, "VCPU budget is not set or out of range, " - "valid values are larger than 1"); + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) + if (sched_rtds_validate_params(gc, scinfo->period, scinfo->budget)) return ERROR_INVAL; - } - sdom.budget = scinfo->budget; - } - - if (sdom.budget > sdom.period) { - LOG(ERROR, "VCPU budget is larger than VCPU period, " - "VCPU budget should be no larger than VCPU period"); - return ERROR_INVAL; - } rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom); if (rc < 0) { @@ -5873,6 +6048,74 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, return ret; } +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + libxl_scheduler sched = scinfo->sched; + int rc; + + if (sched == LIBXL_SCHEDULER_UNKNOWN) + sched = libxl__domain_scheduler(gc, domid); + + switch (sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpus_params_set(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + +int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + libxl_scheduler sched = scinfo->sched; + int rc; + + if (sched == LIBXL_SCHEDULER_UNKNOWN) + sched = libxl__domain_scheduler(gc, domid); + + switch (sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpus_params_set_all(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5907,6 +6150,38 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, return ret; } +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + int rc; + + scinfo->sched = libxl__domain_scheduler(gc, domid); + + switch (scinfo->sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler is no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpu_get(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + static int libxl__domain_s3_resume(libxl__gc *gc, int domid) { int rc = 0; diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 6b73848..df35d09 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -206,6 +206,17 @@ #define LIBXL_HAVE_DEVICE_MODEL_USER 1 /* + * libxl_vcpu_sched_params is used to store per-vcpu params. + */ +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1 + +/* + * LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS indicates RTDS scheduler + * now supports per-vcpu settings. + */ +#define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1 + +/* * libxl_domain_build_info has the arm.gic_version field. */ #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 @@ -1647,11 +1658,37 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid, #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1 #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1 +/* Per-VCPU parameters */ +#define LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT -1 + +/* Get the per-domain scheduling parameters. + * For schedulers that support per-vcpu settings (e.g., RTDS), + * calling *_domain_get functions will get default scheduling + * parameters. + */ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, libxl_domain_sched_params *params); + +/* Set the per-domain scheduling parameters. + * For schedulers that support per-vcpu settings (e.g., RTDS), + * calling *_domain_set functions will set all vcpus with the same + * scheduling parameters. + */ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, const libxl_domain_sched_params *params); +/* Get the per-vcpu scheduling parameters */ +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *params); + +/* Set the per-vcpu scheduling parameters */ +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *params); + +/* Set the per-vcpu scheduling parameters of all vcpus of a domain */ +int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *params); + int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid, libxl_trigger trigger, uint32_t vcpuid); int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index cf3730f..7487fc9 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -378,6 +378,20 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ ("stream_version", uint32, {'init_val': '1'}), ]) +libxl_sched_params = Struct("sched_params",[ + ("vcpuid", integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}), + ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}), + ("cap", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}), + ("period", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}), + ("extratime", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}), + ("budget", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}), + ]) + +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[ + ("sched", libxl_scheduler), + ("vcpus", Array(libxl_sched_params, "num_vcpus")), + ]) + libxl_domain_sched_params = Struct("domain_sched_params",[ ("sched", libxl_scheduler), ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}), -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Change main_sched_rtds and related output functions to support per-VCPU settings. Signed-off-by: Chong Li <chong.li@wustl.edu> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> Signed-off-by: Sisu Xi <xisisu@gmail.com> --- Changes on PATCH v6: 1) More explain in xl.pod.1 and cmdtable.c 2) Resolve some coding sytle issues Changes on PATCH v5: 1) Add sched_vcpu_set_all() for the cases that all vcpus of a domain need to be changed together. Changes on PATCH v4: 1) Coding style changes Changes on PATCH v3: 1) Support commands, e.g., "xl sched-rtds -d vm1" to output the default scheduling parameters Changes on PATCH v2: 1) Remove per-domain output functions for RTDS scheduler. 2) Users now use '-v all' to specify all VCPUs. 3) Support outputting a subset of the parameters of the VCPUs of a specific domain. 4) When setting all VCPUs with the same parameters (by only one command), no per-domain function is invoked. CC: <dario.faggioli@citrix.com> CC: <george.dunlap@eu.citrix.com> CC: <dgolomb@seas.upenn.edu> CC: <mengxu@cis.upenn.edu> CC: <wei.liu2@citrix.com> CC: <lichong659@gmail.com> --- docs/man/xl.pod.1 | 38 ++++++ tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++++++++----- tools/libxl/xl_cmdtable.c | 16 ++- 3 files changed, 320 insertions(+), 35 deletions(-) diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 index 4279c7c..1a8aacd 100644 --- a/docs/man/xl.pod.1 +++ b/docs/man/xl.pod.1 @@ -1051,6 +1051,10 @@ B<OPTIONS> Specify domain for which scheduler parameters are to be modified or retrieved. Mandatory for modifying scheduler parameters. +=item B<-v VCPUID/all>, B<--vcpuid=VCPUID/all> + +Specify vcpu for which scheduler parameters are to be modified or retrieved. + =item B<-p PERIOD>, B<--period=PERIOD> Period of time, in microseconds, over which to replenish the budget. @@ -1066,6 +1070,40 @@ Restrict output to domains in the specified cpupool. =back +B<EXAMPLES> + +=over 4 + +=item B<-d [domid]> + +List default per-domain params for a domain + +=item B<-d [domid] [params (period and budget)]> + +Set per-domain params for a domain + +=item B<-d [domid] -v [vcpuid] -v [vcpuid] ...> + +List per-VCPU params for a domain + +=item B<-d [domid] -v all> + +List all per-VCPU params for a domain + +=item B<-v all> + +List all per-VCPU params for all domains + +=item B<-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...> + +Set per-VCPU params for a domain + +=item B<-d [domid] -v all [params]> + +Set all per-VCPU params for a domain + +=back + =back =head1 CPUPOOLS COMMANDS diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 2b6371d..131541f 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -5823,6 +5823,52 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo) return 0; } +static int sched_vcpu_get(libxl_scheduler sched, int domid, + libxl_vcpu_sched_params *scinfo) +{ + int rc; + + rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo); + if (rc) { + fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n"); + exit(-1); + } + if (scinfo->sched != sched) { + fprintf(stderr, "libxl_vcpu_sched_params_get returned %s not %s.\n", + libxl_scheduler_to_string(scinfo->sched), + libxl_scheduler_to_string(sched)); + return 1; + } + + return 0; +} + +static int sched_vcpu_set(int domid, const libxl_vcpu_sched_params *scinfo) +{ + int rc; + + rc = libxl_vcpu_sched_params_set(ctx, domid, scinfo); + if (rc) { + fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n"); + exit(-1); + } + + return rc; +} + +static int sched_vcpu_set_all(int domid, const libxl_vcpu_sched_params *scinfo) +{ + int rc; + + rc = libxl_vcpu_sched_params_set_all(ctx, domid, scinfo); + if (rc) { + fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n"); + exit(-1); + } + + return rc; +} + static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo) { if (libxl_sched_credit_params_set(ctx, poolid, scinfo)) { @@ -5942,6 +5988,37 @@ static int sched_rtds_domain_output( return 0; } +static int sched_rtds_vcpu_output(int domid, libxl_vcpu_sched_params *scinfo) +{ + char *domname; + int rc = 0; + int i; + + if (domid < 0) { + printf("%-33s %4s %4s %9s %9s\n", "Name", "ID", + "VCPU", "Period", "Budget"); + return 0; + } + + rc = sched_vcpu_get(LIBXL_SCHEDULER_RTDS, domid, scinfo); + if (rc) + goto out; + + domname = libxl_domid_to_name(ctx, domid); + for ( i = 0; i < scinfo->num_vcpus; i++ ) { + printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n", + domname, + domid, + scinfo->vcpus[i].vcpuid, + scinfo->vcpus[i].period, + scinfo->vcpus[i].budget); + } + free(domname); + +out: + return rc; +} + static int sched_rtds_pool_output(uint32_t poolid) { char *poolname; @@ -6015,6 +6092,65 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int), return 0; } +static int sched_vcpu_output(libxl_scheduler sched, + int (*output)(int, libxl_vcpu_sched_params *), + int (*pooloutput)(uint32_t), const char *cpupool) +{ + libxl_dominfo *info; + libxl_cpupoolinfo *poolinfo = NULL; + uint32_t poolid; + int nb_domain, n_pools = 0, i, p; + int rc = 0; + + if (cpupool) { + if (libxl_cpupool_qualifier_to_cpupoolid(ctx, cpupool, &poolid, NULL) + || !libxl_cpupoolid_is_valid(ctx, poolid)) { + fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool); + return -ERROR_FAIL; + } + } + + info = libxl_list_domain(ctx, &nb_domain); + if (!info) { + fprintf(stderr, "libxl_list_domain failed.\n"); + return 1; + } + poolinfo = libxl_list_cpupool(ctx, &n_pools); + if (!poolinfo) { + fprintf(stderr, "error getting cpupool info\n"); + libxl_dominfo_list_free(info, nb_domain); + return -ERROR_NOMEM; + } + + for (p = 0; !rc && (p < n_pools); p++) { + if ((poolinfo[p].sched != sched) || + (cpupool && (poolid != poolinfo[p].poolid))) + continue; + + pooloutput(poolinfo[p].poolid); + + libxl_vcpu_sched_params scinfo_out; + libxl_vcpu_sched_params_init(&scinfo_out); + output(-1, &scinfo_out); + libxl_vcpu_sched_params_dispose(&scinfo_out); + for (i = 0; i < nb_domain; i++) { + if (info[i].cpupool != poolinfo[p].poolid) + continue; + libxl_vcpu_sched_params scinfo; + libxl_vcpu_sched_params_init(&scinfo); + scinfo.num_vcpus = 0; + rc = output(info[i].domid, &scinfo); + libxl_vcpu_sched_params_dispose(&scinfo); + if (rc) + break; + } + } + + libxl_cpupoolinfo_list_free(poolinfo, n_pools); + libxl_dominfo_list_free(info, nb_domain); + return 0; +} + /* * <nothing> : List all domain params and sched params from all pools * -d [domid] : List domain params for domain @@ -6215,84 +6351,183 @@ int main_sched_credit2(int argc, char **argv) /* * <nothing> : List all domain paramters and sched params - * -d [domid] : List domain params for domain + * -d [domid] : List default domain params for domain * -d [domid] [params] : Set domain params for domain + * -d [domid] -v [vcpuid 1] -v [vcpuid 2] ... : + * List per-VCPU params for domain + * -d [domid] -v all : List all per-VCPU params for domain + * -v all : List all per-VCPU params for all domains + * -d [domid] -v [vcpuid 1] [params] -v [vcpuid 2] [params] ... : + * Set per-VCPU params for domain + * -d [domid] -v all [params] : Set all per-VCPU params for domain */ int main_sched_rtds(int argc, char **argv) { const char *dom = NULL; const char *cpupool = NULL; - int period = 0; /* period is in microsecond */ - int budget = 0; /* budget is in microsecond */ + int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs that change */ + int *periods = (int *)xmalloc(sizeof(int)); /* period is in microsecond */ + int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in microsecond */ + int v_size = 1; /* size of vcpus array */ + int p_size = 1; /* size of periods array */ + int b_size = 1; /* size of budgets array */ + int v_index = 0; /* index in vcpus array */ + int p_index =0; /* index in periods array */ + int b_index =0; /* index for in budgets array */ bool opt_p = false; bool opt_b = false; - int opt, rc; + bool opt_v = false; + bool opt_all = false; /* output per-dom parameters */ + int opt, i, rc; static struct option opts[] = { {"domain", 1, 0, 'd'}, {"period", 1, 0, 'p'}, {"budget", 1, 0, 'b'}, + {"vcpuid",1, 0, 'v'}, {"cpupool", 1, 0, 'c'}, COMMON_LONG_OPTS }; - SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) { + SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) { case 'd': dom = optarg; break; case 'p': - period = strtol(optarg, NULL, 10); + if (p_index >= p_size) { + /* periods array is full + * double the array size for new elements + */ + p_size *= 2; + periods = xrealloc(periods, p_size); + } + periods[p_index++] = strtol(optarg, NULL, 10); opt_p = 1; break; case 'b': - budget = strtol(optarg, NULL, 10); + if (b_index >= b_size) { /* budgets array is full */ + b_size *= 2; + budgets = xrealloc(budgets, b_size); + } + budgets[b_index++] = strtol(optarg, NULL, 10); opt_b = 1; break; + case 'v': + if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */ + opt_all = 1; + break; + } + if (v_index >= v_size) { /* vcpus array is full */ + v_size *= 2; + vcpus = xrealloc(vcpus, v_size); + } + vcpus[v_index++] = strtol(optarg, NULL, 10); + opt_v = 1; + break; case 'c': cpupool = optarg; break; } - if (cpupool && (dom || opt_p || opt_b)) { + if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) { fprintf(stderr, "Specifying a cpupool is not allowed with " "other options.\n"); - return EXIT_FAILURE; + rc = EXIT_FAILURE; + goto out; } - if (!dom && (opt_p || opt_b)) { - fprintf(stderr, "Must specify a domain.\n"); - return EXIT_FAILURE; + if (!dom && (opt_p || opt_b || opt_v)) { + fprintf(stderr, "Missing parameters.\n"); + rc = EXIT_FAILURE; + goto out; } - if (opt_p != opt_b) { - fprintf(stderr, "Must specify period and budget\n"); - return EXIT_FAILURE; + if (dom && !opt_v && !opt_all && (opt_p || opt_b)) { + fprintf(stderr, "Must specify VCPU.\n"); + rc = EXIT_FAILURE; + goto out; + } + if (opt_v && opt_all) { + fprintf(stderr, "Incorrect VCPU IDs.\n"); + rc = EXIT_FAILURE; + goto out; + } + if (((v_index > b_index) && opt_b) || ((v_index > p_index) && opt_p) + || p_index != b_index) { + fprintf(stderr, "Incorrect number of period and budget\n"); + rc = EXIT_FAILURE; + goto out; } - if (!dom) { /* list all domain's rt scheduler info */ - if (sched_domain_output(LIBXL_SCHEDULER_RTDS, - sched_rtds_domain_output, + if ((!dom) && opt_all) { + /* get all domain's per-vcpu rtds scheduler parameters */ + rc = -sched_vcpu_output(LIBXL_SCHEDULER_RTDS, + sched_rtds_vcpu_output, sched_rtds_pool_output, - cpupool)) - return EXIT_FAILURE; + cpupool); + goto out; + } else if (!dom && !opt_all) { + /* list all domain's default scheduling parameters */ + rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS, + sched_rtds_domain_output, + sched_rtds_pool_output, + cpupool); + goto out; } else { uint32_t domid = find_domain(dom); - if (!opt_p && !opt_b) { /* output rt scheduler info */ + if (!opt_v && !opt_all) { /* output default scheduling parameters */ sched_rtds_domain_output(-1); - if (sched_rtds_domain_output(domid)) - return EXIT_FAILURE; - } else { /* set rt scheduler paramaters */ - libxl_domain_sched_params scinfo; - libxl_domain_sched_params_init(&scinfo); + rc = -sched_rtds_domain_output(domid); + goto out; + } else if (!opt_p && !opt_b) { + /* get per-vcpu rtds scheduling parameters */ + libxl_vcpu_sched_params scinfo; + libxl_vcpu_sched_params_init(&scinfo); + sched_rtds_vcpu_output(-1, &scinfo); + scinfo.num_vcpus = v_index; + if (v_index > 0) + scinfo.vcpus = (libxl_sched_params *) + xmalloc(sizeof(libxl_sched_params) * (v_index)); + for (i = 0; i < v_index; i++) + scinfo.vcpus[i].vcpuid = vcpus[i]; + rc = -sched_rtds_vcpu_output(domid, &scinfo); + libxl_vcpu_sched_params_dispose(&scinfo); + goto out; + } else if (opt_v || opt_all) { + /* set per-vcpu rtds scheduling parameters */ + libxl_vcpu_sched_params scinfo; + libxl_vcpu_sched_params_init(&scinfo); scinfo.sched = LIBXL_SCHEDULER_RTDS; - scinfo.period = period; - scinfo.budget = budget; + if (v_index > 0) { + scinfo.num_vcpus = v_index; + scinfo.vcpus = (libxl_sched_params *) + xmalloc(sizeof(libxl_sched_params) * (v_index)); + for (i = 0; i < v_index; i++) { + scinfo.vcpus[i].vcpuid = vcpus[i]; + scinfo.vcpus[i].period = periods[i]; + scinfo.vcpus[i].budget = budgets[i]; + } + rc = sched_vcpu_set(domid, &scinfo); + } else { /* set params for all vcpus */ + scinfo.num_vcpus = 1; + scinfo.vcpus = (libxl_sched_params *) + xmalloc(sizeof(libxl_sched_params)); + scinfo.vcpus[0].period = periods[0]; + scinfo.vcpus[0].budget = budgets[0]; + rc = sched_vcpu_set_all(domid, &scinfo); + } - rc = sched_domain_set(domid, &scinfo); - libxl_domain_sched_params_dispose(&scinfo); - if (rc) - return EXIT_FAILURE; + libxl_vcpu_sched_params_dispose(&scinfo); + if (rc) { + rc = -rc; + goto out; + } } } - return EXIT_SUCCESS; + rc = EXIT_SUCCESS; +out: + free(vcpus); + free(periods); + free(budgets); + return rc; } int main_domid(int argc, char **argv) diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c index fdc1ac6..34f5262 100644 --- a/tools/libxl/xl_cmdtable.c +++ b/tools/libxl/xl_cmdtable.c @@ -268,10 +268,22 @@ struct cmd_spec cmd_table[] = { { "sched-rtds", &main_sched_rtds, 0, 1, "Get/set rtds scheduler parameters", - "[-d <Domain> [-p[=PERIOD]] [-b[=BUDGET]]]", + "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]", "-d DOMAIN, --domain=DOMAIN Domain to modify\n" + "-v VCPUID/all, --vcpuid=VCPUID/all VCPU to modify or output;\n" + " Using '-v all' to modify/output all vcpus\n" "-p PERIOD, --period=PERIOD Period (us)\n" - "-b BUDGET, --budget=BUDGET Budget (us)\n" + "-b BUDGET, --budget=BUDGET Budget (us)\n\n" + "Examples:\n\n" + "-d [domid] : List default per-domain params for a domain\n" + "-d [domid] [params (period and budget)] : Set per-domain params for a domain\n" + "-d [domid] -v [vcpuid] -v [vcpuid] ... : " + "List per-VCPU params for a domain\n" + "-d [domid] -v all : List all per-VCPU params for a domain\n" + "-v all : List all per-VCPU params for all domains\n" + "-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ... : " + "Set per-VCPU params for a domain\n" + "-d [domid] -v all [params] : Set all per-VCPU params for a domain\n" }, { "domid", &main_domid, 0, 0, -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, Mar 16, 2016 at 11:47:49AM -0500, Chong Li wrote: > Add xc_sched_rtds_vcpu_get/set functions to interact with > Xen to get/set a domain's per-VCPU parameters. > > Signed-off-by: Chong Li <chong.li@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> Acked-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, Mar 16, 2016 at 11:47:50AM -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > > Signed-off-by: Chong Li <chong.li@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> > Good work fixing all issues. Acked-by: Wei Liu <wei.liu2@citrix.com> And I have some nit-picking below. > + > +/* Set the RTDS scheduling parameters of vcpu(s) */ > +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) > +{ > + int r, rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + > + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (r < 0) { > + LOGE(ERROR, "getting domain info"); > + rc = ERROR_FAIL; > + goto out; > + } > + max_vcpuid = info.max_vcpu_id; > + > + if (scinfo->num_vcpus <= 0) { > + rc = ERROR_INVAL; > + goto out; > + } else { > + for (i = 0; i < scinfo->num_vcpus; i++) { > + if (scinfo->vcpus[i].vcpuid < 0 || > + scinfo->vcpus[i].vcpuid > max_vcpuid) { > + LOG(ERROR, "VCPU index is out of range, " > + "valid values are within range from 0 to %d", > + max_vcpuid); > + rc = ERROR_INVAL; > + goto out; > + } > + rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period, > + scinfo->vcpus[i].budget); > + if (rc) { > + rc = ERROR_INVAL; > + goto out; > + } > + } > + GCNEW_ARRAY(vcpus, scinfo->num_vcpus); > + for (i = 0; i < scinfo->num_vcpus; i++) { > + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; > + vcpus[i].u.rtds.period = scinfo->vcpus[i].period; > + vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget; > + } > + } > + You could have written this hunk like this: if (scinfo->num_vcpus <= 0) { rc = ERROR_INVAL; goto out; } for (i = 0; i < scinfo->num_vcpus; i++) { if (scinfo->vcpus[i].vcpuid < 0 || scinfo->vcpus[i].vcpuid > max_vcpuid) { LOG(ERROR, "VCPU index is out of range, " "valid values are within range from 0 to %d", max_vcpuid); rc = ERROR_INVAL; goto out; } rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period, scinfo->vcpus[i].budget); if (rc) { rc = ERROR_INVAL; goto out; } } GCNEW_ARRAY(vcpus, scinfo->num_vcpus); for (i = 0; i < scinfo->num_vcpus; i++) { vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; vcpus[i].u.rtds.period = scinfo->vcpus[i].period; vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget; } But, you original code is still OK. This is just FYI. No need to resend just because of this. > + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, > + vcpus, scinfo->num_vcpus); > + if (r != 0) { > + LOGE(ERROR, "setting vcpu sched rtds"); > + rc = ERROR_FAIL; > + goto out; > + } > + rc = 0; > +out: > + return rc; > +} > + > +/* Set the RTDS scheduling parameters of all vcpus of a domain */ > +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) > +{ > + int r, rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + uint32_t num_vcpus; > + > + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (r < 0) { > + LOGE(ERROR, "getting domain info"); > + rc = ERROR_FAIL; > + goto out; > + } > + max_vcpuid = info.max_vcpu_id; > + > + if (scinfo->num_vcpus != 1) { > + rc = ERROR_INVAL; > + goto out; > + } else { > + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, > + scinfo->vcpus[0].budget)) { > + rc = ERROR_INVAL; > + goto out; > + } > + num_vcpus = max_vcpuid + 1; > + GCNEW_ARRAY(vcpus, num_vcpus); > + for (i = 0; i < num_vcpus; i++) { > + vcpus[i].vcpuid = i; > + vcpus[i].u.rtds.period = scinfo->vcpus[0].period; > + vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget; > + } > + } Same here, the else branch can be simplified. Again, it's just FYI. No need to resend. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, Mar 16, 2016 at 11:47:51AM -0500, Chong Li wrote: > --- > docs/man/xl.pod.1 | 38 ++++++ > tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++++++++----- > tools/libxl/xl_cmdtable.c | 16 ++- > 3 files changed, 320 insertions(+), 35 deletions(-) > > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 > index 4279c7c..1a8aacd 100644 > --- a/docs/man/xl.pod.1 > +++ b/docs/man/xl.pod.1 > @@ -1051,6 +1051,10 @@ B<OPTIONS> > Specify domain for which scheduler parameters are to be modified or retrieved. > Mandatory for modifying scheduler parameters. > > +=item B<-v VCPUID/all>, B<--vcpuid=VCPUID/all> > + > +Specify vcpu for which scheduler parameters are to be modified or retrieved. > + > =item B<-p PERIOD>, B<--period=PERIOD> > > Period of time, in microseconds, over which to replenish the budget. > @@ -1066,6 +1070,40 @@ Restrict output to domains in the specified cpupool. > > =back > > +B<EXAMPLES> > + > +=over 4 > + > +=item B<-d [domid]> > + > +List default per-domain params for a domain > + > +=item B<-d [domid] [params (period and budget)]> > + > +Set per-domain params for a domain > + > +=item B<-d [domid] -v [vcpuid] -v [vcpuid] ...> > + > +List per-VCPU params for a domain > + > +=item B<-d [domid] -v all> > + > +List all per-VCPU params for a domain > + > +=item B<-v all> > + > +List all per-VCPU params for all domains > + > +=item B<-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...> > + > +Set per-VCPU params for a domain > + > +=item B<-d [domid] -v all [params]> > + > +Set all per-VCPU params for a domain > + > +=back > + Urgh, there might be some misunderstanding here. These are explanations to specific commands, while I asked for some concrete examples. Search for "example" in xl manpage. > =back > > =head1 CPUPOOLS COMMANDS > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 2b6371d..131541f 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -5823,6 +5823,52 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo) > return 0; > } > [...] > static int sched_rtds_pool_output(uint32_t poolid) > { > char *poolname; > @@ -6015,6 +6092,65 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int), > return 0; > } > > +static int sched_vcpu_output(libxl_scheduler sched, > + int (*output)(int, libxl_vcpu_sched_params *), > + int (*pooloutput)(uint32_t), const char *cpupool) > +{ > + libxl_dominfo *info; > + libxl_cpupoolinfo *poolinfo = NULL; > + uint32_t poolid; > + int nb_domain, n_pools = 0, i, p; > + int rc = 0; > + > + if (cpupool) { > + if (libxl_cpupool_qualifier_to_cpupoolid(ctx, cpupool, &poolid, NULL) > + || !libxl_cpupoolid_is_valid(ctx, poolid)) { > + fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool); > + return -ERROR_FAIL; Normally xl shouldn't use libxl's error code. And for your convenience you can just return -1 or 1. > + } > + } > + > + info = libxl_list_domain(ctx, &nb_domain); > + if (!info) { > + fprintf(stderr, "libxl_list_domain failed.\n"); > + return 1; > + } > + poolinfo = libxl_list_cpupool(ctx, &n_pools); > + if (!poolinfo) { > + fprintf(stderr, "error getting cpupool info\n"); > + libxl_dominfo_list_free(info, nb_domain); > + return -ERROR_NOMEM; Same here. > + } > + > + for (p = 0; !rc && (p < n_pools); p++) { > + if ((poolinfo[p].sched != sched) || > + (cpupool && (poolid != poolinfo[p].poolid))) > + continue; > + > + pooloutput(poolinfo[p].poolid); > + > + libxl_vcpu_sched_params scinfo_out; > + libxl_vcpu_sched_params_init(&scinfo_out); > + output(-1, &scinfo_out); > + libxl_vcpu_sched_params_dispose(&scinfo_out); > + for (i = 0; i < nb_domain; i++) { > + if (info[i].cpupool != poolinfo[p].poolid) > + continue; > + libxl_vcpu_sched_params scinfo; > + libxl_vcpu_sched_params_init(&scinfo); > + scinfo.num_vcpus = 0; > + rc = output(info[i].domid, &scinfo); > + libxl_vcpu_sched_params_dispose(&scinfo); > + if (rc) > + break; > + } > + } > + > + libxl_cpupoolinfo_list_free(poolinfo, n_pools); > + libxl_dominfo_list_free(info, nb_domain); > + return 0; > +} > + > /* > * <nothing> : List all domain params and sched params from all pools > * -d [domid] : List domain params for domain > @@ -6215,84 +6351,183 @@ int main_sched_credit2(int argc, char **argv) > > /* > * <nothing> : List all domain paramters and sched params > - * -d [domid] : List domain params for domain > + * -d [domid] : List default domain params for domain > * -d [domid] [params] : Set domain params for domain > + * -d [domid] -v [vcpuid 1] -v [vcpuid 2] ... : > + * List per-VCPU params for domain > + * -d [domid] -v all : List all per-VCPU params for domain > + * -v all : List all per-VCPU params for all domains > + * -d [domid] -v [vcpuid 1] [params] -v [vcpuid 2] [params] ... : > + * Set per-VCPU params for domain > + * -d [domid] -v all [params] : Set all per-VCPU params for domain > */ > int main_sched_rtds(int argc, char **argv) > { > const char *dom = NULL; > const char *cpupool = NULL; > - int period = 0; /* period is in microsecond */ > - int budget = 0; /* budget is in microsecond */ > + int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs that change */ > + int *periods = (int *)xmalloc(sizeof(int)); /* period is in microsecond */ > + int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in microsecond */ > + int v_size = 1; /* size of vcpus array */ > + int p_size = 1; /* size of periods array */ > + int b_size = 1; /* size of budgets array */ > + int v_index = 0; /* index in vcpus array */ > + int p_index =0; /* index in periods array */ > + int b_index =0; /* index for in budgets array */ > bool opt_p = false; > bool opt_b = false; > - int opt, rc; > + bool opt_v = false; > + bool opt_all = false; /* output per-dom parameters */ > + int opt, i, rc; > static struct option opts[] = { > {"domain", 1, 0, 'd'}, > {"period", 1, 0, 'p'}, > {"budget", 1, 0, 'b'}, > + {"vcpuid",1, 0, 'v'}, > {"cpupool", 1, 0, 'c'}, > COMMON_LONG_OPTS > }; > > - SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) { > + SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) { > case 'd': > dom = optarg; > break; > case 'p': > - period = strtol(optarg, NULL, 10); > + if (p_index >= p_size) { > + /* periods array is full > + * double the array size for new elements > + */ > + p_size *= 2; > + periods = xrealloc(periods, p_size); > + } > + periods[p_index++] = strtol(optarg, NULL, 10); > opt_p = 1; > break; > case 'b': > - budget = strtol(optarg, NULL, 10); > + if (b_index >= b_size) { /* budgets array is full */ > + b_size *= 2; > + budgets = xrealloc(budgets, b_size); > + } > + budgets[b_index++] = strtol(optarg, NULL, 10); > opt_b = 1; > break; > + case 'v': > + if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */ > + opt_all = 1; > + break; > + } > + if (v_index >= v_size) { /* vcpus array is full */ > + v_size *= 2; > + vcpus = xrealloc(vcpus, v_size); > + } > + vcpus[v_index++] = strtol(optarg, NULL, 10); > + opt_v = 1; > + break; You seemed to miss my request for factoring out a function. But now I think about it, that probably won't buy us much good. So I'm fine with code like this now. > case 'c': > cpupool = optarg; > break; > } > [...] > int main_domid(int argc, char **argv) > diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c > index fdc1ac6..34f5262 100644 > --- a/tools/libxl/xl_cmdtable.c > +++ b/tools/libxl/xl_cmdtable.c > @@ -268,10 +268,22 @@ struct cmd_spec cmd_table[] = { > { "sched-rtds", > &main_sched_rtds, 0, 1, > "Get/set rtds scheduler parameters", > - "[-d <Domain> [-p[=PERIOD]] [-b[=BUDGET]]]", > + "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]", > "-d DOMAIN, --domain=DOMAIN Domain to modify\n" > + "-v VCPUID/all, --vcpuid=VCPUID/all VCPU to modify or output;\n" > + " Using '-v all' to modify/output all vcpus\n" > "-p PERIOD, --period=PERIOD Period (us)\n" > - "-b BUDGET, --budget=BUDGET Budget (us)\n" > + "-b BUDGET, --budget=BUDGET Budget (us)\n\n" > + "Examples:\n\n" > + "-d [domid] : List default per-domain params for a domain\n" > + "-d [domid] [params (period and budget)] : Set per-domain params for a domain\n" > + "-d [domid] -v [vcpuid] -v [vcpuid] ... : " > + "List per-VCPU params for a domain\n" > + "-d [domid] -v all : List all per-VCPU params for a domain\n" > + "-v all : List all per-VCPU params for all domains\n" > + "-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ... : " > + "Set per-VCPU params for a domain\n" > + "-d [domid] -v all [params] : Set all per-VCPU params for a domain\n" No need to have example here. > }, > { "domid", > &main_domid, 0, 0, > -- > 1.9.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, Mar 16, 2016 at 07:24:35PM +0000, Wei Liu wrote: > On Wed, Mar 16, 2016 at 11:47:51AM -0500, Chong Li wrote: > > --- > > docs/man/xl.pod.1 | 38 ++++++ > > tools/libxl/xl_cmdimpl.c | 301 +++++++++++++++++++++++++++++++++++++++++----- > > tools/libxl/xl_cmdtable.c | 16 ++- > > 3 files changed, 320 insertions(+), 35 deletions(-) > > > > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 > > index 4279c7c..1a8aacd 100644 > > --- a/docs/man/xl.pod.1 > > +++ b/docs/man/xl.pod.1 > > @@ -1051,6 +1051,10 @@ B<OPTIONS> > > Specify domain for which scheduler parameters are to be modified or retrieved. > > Mandatory for modifying scheduler parameters. > > > > +=item B<-v VCPUID/all>, B<--vcpuid=VCPUID/all> > > + > > +Specify vcpu for which scheduler parameters are to be modified or retrieved. > > + > > =item B<-p PERIOD>, B<--period=PERIOD> > > > > Period of time, in microseconds, over which to replenish the budget. > > @@ -1066,6 +1070,40 @@ Restrict output to domains in the specified cpupool. > > > > =back > > > > +B<EXAMPLES> > > + > > +=over 4 > > + > > +=item B<-d [domid]> > > + > > +List default per-domain params for a domain > > + > > +=item B<-d [domid] [params (period and budget)]> > > + > > +Set per-domain params for a domain > > + > > +=item B<-d [domid] -v [vcpuid] -v [vcpuid] ...> > > + > > +List per-VCPU params for a domain > > + > > +=item B<-d [domid] -v all> > > + > > +List all per-VCPU params for a domain > > + > > +=item B<-v all> > > + > > +List all per-VCPU params for all domains > > + > > +=item B<-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...> > > + > > +Set per-VCPU params for a domain > > + > > +=item B<-d [domid] -v all [params]> > > + > > +Set all per-VCPU params for a domain > > + > > +=back > > + > > Urgh, there might be some misunderstanding here. These are explanations > to specific commands, while I asked for some concrete examples. > > Search for "example" in xl manpage. > BTW I think you just need to move some of the examples in your cover letter to manpage. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1475 bytes --] On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > Add xc_sched_rtds_vcpu_get/set functions to interact with > Xen to get/set a domain's per-VCPU parameters. > Patch is ok, IMO, with only two minor comments. First, the subject is a bit long. I'd go for: "libxc: enable per-VCPU parameters for RTDS" Second, here... > Signed-off-by: Chong Li <chong.li@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> > --- a/tools/libxc/xc_rt.c > +++ b/tools/libxc/xc_rt.c > @@ -62,3 +62,71 @@ int xc_sched_rtds_domain_get(xc_interface *xch, > > return rc; > } > + > +int xc_sched_rtds_vcpu_set(xc_interface *xch, > + uint32_t domid, > + struct xen_domctl_schedparam_vcpu *vcpus, > + uint32_t num_vcpus) > +{ > + int rc = 0; > rc does not need to be initialized to 0 (and hence it shouldn't be). Even with only this fixed (i.e., even if the subject stays as it is): Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 6987 bytes --] On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > Hey, Good job indeed, Chong, this is much better. Now, I appreciate that Wei already Acked this, but nevertheless, I have some comments. I'll put them down here, and then it will be up to maintainers and committers to figure out whether they need to be addressed or not (Wei? Ian?) > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5770,6 +5770,201 @@ static int sched_credit2_domain_set(libxl__gc > *gc, uint32_t domid, > return 0; > } > > +static int sched_rtds_validate_params(libxl__gc *gc, int period, int > budget) > +{ > + int rc; > + > + if (period < 1) { > + LOG(ERROR, "VCPU period is out of range, " > + "valid values are larger than or equal to 1"); > These strings are really better if kept on one line, and it does not look impossible in this case: LOG(ERROR, "Invalid VCPU period of %d (it should be >= 1)", period); > + rc = ERROR_INVAL; /* error scheduling parameter */ > The comment should go away. > + goto out; > + } > + > + if (budget < 1) { > + LOG(ERROR, "VCPU budget is not set or out of range, " > + "valid values are larger than or equal to 1"); > LOG(ERROR, "Invalid VCPU budget of %d (it should be >= 1)", budget); > + rc = ERROR_INVAL; > + goto out; > + } > + > + if (budget > period) { > + LOG(ERROR, "VCPU budget must be smaller than " > + "or equal to VCPU period"); > LOG(ERROR, "VCPU budget must be smaller than period, but %d > %d", budget, period); > +/* Set the RTDS scheduling parameters of vcpu(s) */ > +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t > domid, > + const libxl_vcpu_sched_params > *scinfo) > +{ > + int r, rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + > + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (r < 0) { > + LOGE(ERROR, "getting domain info"); > + rc = ERROR_FAIL; > + goto out; > + } > + max_vcpuid = info.max_vcpu_id; > + > + if (scinfo->num_vcpus <= 0) { > + rc = ERROR_INVAL; > + goto out; > + } else { > Personally, I consider what Wei suggested for avoiding, at once, one more level of indentation, as well as this really really ugly if { goto } else { } rather important to do. But then again, I'm not the maintainer of this code, so if it's fine for them, then fine. :-) > + for (i = 0; i < scinfo->num_vcpus; i++) { > + if (scinfo->vcpus[i].vcpuid < 0 || > + scinfo->vcpus[i].vcpuid > max_vcpuid) { > + LOG(ERROR, "VCPU index is out of range, " > + "valid values are within range from 0 to > %d", > + max_vcpuid); > LOG(ERROR, "Invalid VCPU %d: valid range is [0, %d]", scinfo->vcpus[i].vcpuid, info.max_vcpu_id); > +/* Set the RTDS scheduling parameters of all vcpus of a domain */ > +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t > domid, > + const libxl_vcpu_sched_params > *scinfo) > Indentation? It seems to me that it just fits, even if done properly: static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid, const libxl_vcpu_sched_params *scinfo) > +{ > + int r, rc; > + int i; > + uint16_t max_vcpuid; > + xc_dominfo_t info; > + struct xen_domctl_schedparam_vcpu *vcpus; > + uint32_t num_vcpus; > + > + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (r < 0) { > + LOGE(ERROR, "getting domain info"); > + rc = ERROR_FAIL; > + goto out; > + } > + max_vcpuid = info.max_vcpu_id; > + > + if (scinfo->num_vcpus != 1) { > + rc = ERROR_INVAL; > + goto out; > + } else { > Same as above, of course. > @@ -5802,30 +5997,10 @@ static int sched_rtds_domain_set(libxl__gc > *gc, uint32_t domid, > LOGE(ERROR, "getting domain sched rtds"); > return ERROR_FAIL; > } > - > - if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { > - if (scinfo->period < 1) { > - LOG(ERROR, "VCPU period is not set or out of range, " > - "valid values are larger than 1"); > - return ERROR_INVAL; > - } > - sdom.period = scinfo->period; > - } > - > - if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { > - if (scinfo->budget < 1) { > - LOG(ERROR, "VCPU budget is not set or out of range, " > - "valid values are larger than 1"); > + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && > + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) > + if (sched_rtds_validate_params(gc, scinfo->period, scinfo- > >budget)) > return ERROR_INVAL; > I'm not sure I understand. What's happening in this function? As it stands after this patch, it looks to me that: - we read the default scheduling parameter from Xen, via xc_sched_rtds_domain_get() - we (possibly, if both are non-default) validate a new period and budget couple of values - we don't use such values for anything, and set back what we got from Xen, via xc_sched_rtds_domain_set() Either I'm missing something very basic, or this is not what Wei said when reviewing v6: "Then at callsites you set those values with two direct assignment: if (validate(period_value, budget_value) != 0) { error; } period = period_value; budget = budget_value;" Is it? Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 2542 bytes --] On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > And a couple more things that I forgot. I'd shorten the subject line, as already suggested for libxc. > +/* Get the RTDS scheduling parameters of vcpu(s) */ > +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ I think this should be called static int sched_rtds_vcpu_params_get(libxl__gc *gc, uint32_t domid, libxl_vcpu_sched_params *scinfo) for homogeneity with the _set counterpart here below: > @@ -5873,6 +6048,74 @@ int libxl_domain_sched_params_set(libxl_ctx > *ctx, uint32_t domid, > return ret; > } > > +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, > + const libxl_vcpu_sched_params > *scinfo) > +{ So, this function takes a libxl_vcpu_sched_params*, which is basically an array of vcpus' parameters, one element per vcpu (and it can be sparse). Are there any restrictions on how such array should be constructed, for calling this function? Are they any special meaning of particular configurations of the array? I think at least the latter is true, in fact: - this calls sched_rtds_vcpus_params_set(); - in there, if the array is empty, we fail; - if the array has one or more elements, we deal with the vcpus (and just them) specified in the elements of the array itself. Then there is libxl_vcpu_sched_params_set_all(). That one: - calls sched_rtds_vcpus_params_set_all(); - in there, if the array has more than just one element, we fail. And then the get side, with libxl_vcpu_sched_params_get(). That one: - calls sched_rtds_vcpu_get(); - in there, if the array is empty, we create one, and return info for all the vcpus; - if the array has one or more elements, we deal with them (and just them). I think this should be documented some (unless it's already there and I missed it). Thanks and Regards again, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 23793 bytes --] On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -1054,15 +1054,16 @@ csched_dom_cntl( > * lock. Runq lock not needed anywhere in here. */ > spin_lock_irqsave(&prv->lock, flags); > > - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) > + switch ( op->cmd ) > { > + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: > + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > + return -EINVAL; > + case XEN_DOMCTL_SCHEDOP_getinfo: > op->u.credit.weight = sdom->weight; > op->u.credit.cap = sdom->cap; > Not feeling to strong about it, but I think switch ( op->cmd ) { case XEN_DOMCTL_SCHEDOP_getinfo: op->u.credit.weight = sdom->weight; op->u.credit.cap = sdom->cap; break; case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.credit.weight != 0 ) { if ( !list_empty(&sdom->active_sdom_elem) ) { prv->weight -= sdom->weight * sdom->active_vcpu_count; prv->weight += op->u.credit.weight * sdom->active_vcpu_count; } sdom->weight = op->u.credit.weight; } if ( op->u.credit.cap != (uint16_t)~0U ) sdom->cap = op->u.credit.cap; break; case XEN_DOMCTL_SCHEDOP_putvcpuinfo: case XEN_DOMCTL_SCHEDOP_getvcpuinfo: default: return -EINVAL; } (i.e., grouping the cases that needs only returning -EINVAL) is better, the final result, more than the patch itself. And the same for Credit2, of course. > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -1129,24 +1145,22 @@ rt_dom_cntl( > struct vcpu *v; > unsigned long flags; > int rc = 0; > + xen_domctl_schedparam_vcpu_t local_sched; > + s_time_t period, budget; > + uint32_t index = 0; > > switch ( op->cmd ) > { > case XEN_DOMCTL_SCHEDOP_getinfo: > - if ( d->max_vcpus > 0 ) > - { > - spin_lock_irqsave(&prv->lock, flags); > - svc = rt_vcpu(d->vcpu[0]); > - op->u.rtds.period = svc->period / MICROSECS(1); > - op->u.rtds.budget = svc->budget / MICROSECS(1); > - spin_unlock_irqrestore(&prv->lock, flags); > - } > - else > - { > - /* If we don't have vcpus yet, let's just return the > defaults. */ > - op->u.rtds.period = RTDS_DEFAULT_PERIOD; > - op->u.rtds.budget = RTDS_DEFAULT_BUDGET; > - } > + /* Return the default parameters. > + * A previous bug fixed here: > + * The default PERIOD or BUDGET should be divided by > MICROSECS(1), > + * before returned to upper caller. > + */ Comment style (missing opening '/*'). Also, putting this kind of things in comments is not at all ideal. If doing this in this patch, you should mention it in the changelog. Or you do it in a separate patch (that you put before this one in the series). I'd say that, in this specific case, is not a big deal which one of the two approaches you take (mentioning or separate patch), but the having a separate one is indeed almost always preferable (e.g., the fix can be backported). > + spin_lock_irqsave(&prv->lock, flags); > + op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1); > + op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1); > + spin_unlock_irqrestore(&prv->lock, flags); > I don't think we need to take the lock here... RTDS_DEFAULT_PERIOD is not going to change under our feet. :-) > @@ -1163,6 +1177,78 @@ rt_dom_cntl( > } > spin_unlock_irqrestore(&prv->lock, flags); > break; > + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > + while ( index < op->u.v.nr_vcpus ) > + { > + if ( copy_from_guest_offset(&local_sched, > + op->u.v.vcpus, index, 1) ) > + { > + rc = -EFAULT; > + break; > + } > + if ( local_sched.vcpuid >= d->max_vcpus || > + d->vcpu[local_sched.vcpuid] == NULL ) > + { > + rc = -EINVAL; > + break; > + } > + > + spin_lock_irqsave(&prv->lock, flags); > + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); > + local_sched.u.rtds.budget = svc->budget / MICROSECS(1); > + local_sched.u.rtds.period = svc->period / MICROSECS(1); > + spin_unlock_irqrestore(&prv->lock, flags); > + > + if ( copy_to_guest_offset(op->u.v.vcpus, index, > + &local_sched, 1) ) > + { > + rc = -EFAULT; > + break; > + } > + if ( (++index > 0x3f) && hypercall_preempt_check() ) > + break; > So, I know it's 0x3f in XEN_SYSCTL_pcitopoinfo, but unless I'm missing something, I don't see why this can't be "63". I'd also put a short comment right above, something like: /* Process a most 64 vCPUs without checking for preemptions. */ > + } > + if ( !rc ) /* notify upper caller how many vcpus have been > processed */ > Move the comment to the line above (and terminate it with a full stop). And by the way, there's a lot of code repetition here. What about handling get and set together, sort of like this: case XEN_DOMCTL_SCHEDOP_getvcpuinfo: | case XEN_DOMCTL_SCHEDOP_putvcpuinfo: |#ifdef CONFIG_HAS_PCI while ( index < op->u.v.nr_vcpus ) | case XEN_SYSCTL_pcitopoinfo: { | { if ( copy_from_guest_offset(&local_sched, | xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo; op->u.v.vcpus, index, 1) ) | unsigned int i = 0; { | rc = -EFAULT; | if ( guest_handle_is_null(ti->devs) || break; | guest_handle_is_null(ti->nodes) ) } | { | ret = -EINVAL; if ( local_sched.vcpuid >= d->max_vcpus || | break; d->vcpu[local_sched.vcpuid] == NULL ) | } { | rc = -EINVAL; | while ( i < ti->num_devs ) break; | { } | physdev_pci_device_t dev; | uint32_t node; if ( op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo ) | const struct pci_dev *pdev; { | spin_lock_irqsave(&prv->lock, flags); | if ( copy_from_guest_offset(&dev, ti->devs, i, 1) ) svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | { local_sched.u.rtds.budget = svc->budget / MICROSECS(1); | ret = -EFAULT; local_sched.u.rtds.period = svc->period / MICROSECS(1); | break; spin_unlock_irqrestore(&prv->lock, flags); | } | if ( copy_to_guest_offset(op->u.v.vcpus, index, | pcidevs_lock(); &local_sched, 1) ) | pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn); { | if ( !pdev ) rc = -EFAULT; | node = XEN_INVALID_DEV; break; | else if ( pdev->node == NUMA_NO_NODE ) } | node = XEN_INVALID_NODE_ID; } | else else | node = pdev->node; { | pcidevs_unlock(); period = MICROSECS(local_sched.u.rtds.period); | budget = MICROSECS(local_sched.u.rtds.budget); | if ( copy_to_guest_offset(ti->nodes, i, &node, 1) ) if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET || | { budget > period || period < RTDS_MIN_PERIOD ) | ret = -EFAULT; { | break; rc = -EINVAL; | } break; | } | /* Process a most 64 vCPUs without checking for preemptions. */ | if ( (++i > 0x3f) && hypercall_preempt_check() ) spin_lock_irqsave(&prv->lock, flags); | break; svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | } svc->period = period; | svc->budget = budget; | if ( !ret && (ti->num_devs != i) ) spin_unlock_irqrestore(&prv->lock, flags); | { | ti->num_devs = i; } | if ( __copy_field_to_guest(u_sysctl, op, u.pcitopoinfo.num_devs) ) /* Process a most 64 vCPUs without checking for preemptions. */ | ret = -EFAULT; if ( (++index > 63) && hypercall_preempt_check() ) | } break; | break; } | } |#endif /* Notify upper caller how many vcpus have been processed. */ | if ( !rc ) | case XEN_SYSCTL_tmem_op: op->u.v.nr_vcpus = index; | ret = tmem_control(&op->u.tmem_op); break; I have only compile tested this, but it looks to me that it can fly... > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > + * Set or get info? > + * For schedulers supporting per-vcpu settings (e.g., RTDS): > + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; > + * XEN_DOMCTL_SCHEDOP_getinfo gets default params; > + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus; > + * > + * For schedulers not supporting per-vcpu settings: > + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; > + * XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params; > + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error; > + */ > #define XEN_DOMCTL_SCHEDOP_putinfo 0 > #define XEN_DOMCTL_SCHEDOP_getinfo 1 > +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2 > +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3 > struct xen_domctl_scheduler_op { > uint32_t sched_id; /* XEN_SCHEDULER_* */ > uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */ /* IN/OUT */ > union { > - struct xen_domctl_sched_credit { > - uint16_t weight; > - uint16_t cap; > - } credit; > - struct xen_domctl_sched_credit2 { > - uint16_t weight; > - } credit2; > - struct xen_domctl_sched_rtds { > - uint32_t period; > - uint32_t budget; > - } rtds; > + xen_domctl_sched_credit_t credit; > + xen_domctl_sched_credit2_t credit2; > + xen_domctl_sched_rtds_t rtds; > + struct { > + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus; > + /* > + * IN: Number of elements in vcpus array. > + * OUT: Number of processed elements of vcpus array. > + */ > + uint32_t nr_vcpus; > + uint32_t padding; > + } v; > } u; > }; > That is: make it even more clear that the whole union is used as IN/OUT. Then, indeed, inside v, what is the meaning of the nr_vcpus field in each direction, as you're doing already. Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: >> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set >> functions to support per-VCPU settings. >> >> +/* Set the RTDS scheduling parameters of all vcpus of a domain */ >> +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t >> domid, >> + const libxl_vcpu_sched_params >> *scinfo) >> > Indentation? If I follow the indentation rule, the second line would be longer than 80 characters. The function name is just too long. > >> @@ -5802,30 +5997,10 @@ static int sched_rtds_domain_set(libxl__gc >> *gc, uint32_t domid, >> LOGE(ERROR, "getting domain sched rtds"); >> return ERROR_FAIL; >> } >> - >> - if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { >> - if (scinfo->period < 1) { >> - LOG(ERROR, "VCPU period is not set or out of range, " >> - "valid values are larger than 1"); >> - return ERROR_INVAL; >> - } >> - sdom.period = scinfo->period; >> - } >> - >> - if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { >> - if (scinfo->budget < 1) { >> - LOG(ERROR, "VCPU budget is not set or out of range, " >> - "valid values are larger than 1"); >> + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && >> + scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) >> + if (sched_rtds_validate_params(gc, scinfo->period, scinfo- >> >budget)) >> return ERROR_INVAL; >> > I'm not sure I understand. What's happening in this function? > > As it stands after this patch, it looks to me that: > - we read the default scheduling parameter from Xen, > via xc_sched_rtds_domain_get() > - we (possibly, if both are non-default) validate a new period and > budget couple of values > - we don't use such values for anything, and set back what we got > from Xen, via xc_sched_rtds_domain_set() > > Either I'm missing something very basic, or this is not what Wei said > when reviewing v6: > > "Then at callsites you set those values with two direct assignment: > > if (validate(period_value, budget_value) != 0) { > error; > } > period = period_value; > budget = budget_value;" > > Is it? The current RTDS (xen 4.6) is: 1) Read the (per-domain) scheduling params from Xen, and store them to sdom 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is -1), use sdom.period; else sdom.period = new period (if new period is valid); If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is -1), use sdom.budget; else sdom.budget = new budget; 3) xc_sched_rtds_domain_set (sdom); In our patch, my plan is: 1) Read the default params from Xen, and store them to sdom 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is -1), use sdom.period; else sdom.period = new period (if new period is valid); If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which is -1), use sdom.budget; else sdom.budget = new budget; 3) xc_sched_rtds_domain_set (sdom); Even though I made some mistakes in this post (forgot the two "else"s in my plan), is this plan ok? Chong -- Chong Li Department of Computer Science and Engineering Washington University in St.louis _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > >> --- a/xen/common/sched_rt.c >> +++ b/xen/common/sched_rt.c >> @@ -1129,24 +1145,22 @@ rt_dom_cntl( >> struct vcpu *v; >> unsigned long flags; >> int rc = 0; >> + xen_domctl_schedparam_vcpu_t local_sched; >> + s_time_t period, budget; >> + uint32_t index = 0; >> >> switch ( op->cmd ) >> { >> case XEN_DOMCTL_SCHEDOP_getinfo: >> - if ( d->max_vcpus > 0 ) >> - { >> - spin_lock_irqsave(&prv->lock, flags); >> - svc = rt_vcpu(d->vcpu[0]); >> - op->u.rtds.period = svc->period / MICROSECS(1); >> - op->u.rtds.budget = svc->budget / MICROSECS(1); >> - spin_unlock_irqrestore(&prv->lock, flags); >> - } >> - else >> - { >> - /* If we don't have vcpus yet, let's just return the >> defaults. */ >> - op->u.rtds.period = RTDS_DEFAULT_PERIOD; >> - op->u.rtds.budget = RTDS_DEFAULT_BUDGET; >> - } >> + /* Return the default parameters. >> + * A previous bug fixed here: >> + * The default PERIOD or BUDGET should be divided by >> MICROSECS(1), >> + * before returned to upper caller. >> + */ > Comment style (missing opening '/*'). > > Also, putting this kind of things in comments is not at all ideal. If > doing this in this patch, you should mention it in the changelog. Or > you do it in a separate patch (that you put before this one in the > series). > > I'd say that, in this specific case, is not a big deal which one of the > two approaches you take (mentioning or separate patch), but the having > a separate one is indeed almost always preferable (e.g., the fix can be > backported). If I choose mentioning, do I move the comment to the changelog? Or do I keep it here and say it again in the changelog? Chong -- Chong Li Department of Computer Science and Engineering Washington University in St.louis _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 17.03.16 at 21:42, <lichong659@gmail.com> wrote: > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: >> On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > >> >>> --- a/xen/common/sched_rt.c >>> +++ b/xen/common/sched_rt.c >>> @@ -1129,24 +1145,22 @@ rt_dom_cntl( >>> struct vcpu *v; >>> unsigned long flags; >>> int rc = 0; >>> + xen_domctl_schedparam_vcpu_t local_sched; >>> + s_time_t period, budget; >>> + uint32_t index = 0; >>> >>> switch ( op->cmd ) >>> { >>> case XEN_DOMCTL_SCHEDOP_getinfo: >>> - if ( d->max_vcpus > 0 ) >>> - { >>> - spin_lock_irqsave(&prv->lock, flags); >>> - svc = rt_vcpu(d->vcpu[0]); >>> - op->u.rtds.period = svc->period / MICROSECS(1); >>> - op->u.rtds.budget = svc->budget / MICROSECS(1); >>> - spin_unlock_irqrestore(&prv->lock, flags); >>> - } >>> - else >>> - { >>> - /* If we don't have vcpus yet, let's just return the >>> defaults. */ >>> - op->u.rtds.period = RTDS_DEFAULT_PERIOD; >>> - op->u.rtds.budget = RTDS_DEFAULT_BUDGET; >>> - } >>> + /* Return the default parameters. >>> + * A previous bug fixed here: >>> + * The default PERIOD or BUDGET should be divided by >>> MICROSECS(1), >>> + * before returned to upper caller. >>> + */ >> Comment style (missing opening '/*'). >> >> Also, putting this kind of things in comments is not at all ideal. If >> doing this in this patch, you should mention it in the changelog. Or >> you do it in a separate patch (that you put before this one in the >> series). >> >> I'd say that, in this specific case, is not a big deal which one of the >> two approaches you take (mentioning or separate patch), but the having >> a separate one is indeed almost always preferable (e.g., the fix can be >> backported). > > If I choose mentioning, do I move the comment to the changelog? Or do I keep > it here and say it again in the changelog? Just consider what would happen if everyone mentioned in comments the bugs they fixed. I think the answer to you question is obvious then... Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 17.03.16 at 11:03, <dario.faggioli@citrix.com> wrote: > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: >> --- a/xen/common/sched_credit.c >> +++ b/xen/common/sched_credit.c >> @@ -1054,15 +1054,16 @@ csched_dom_cntl( >> * lock. Runq lock not needed anywhere in here. */ >> spin_lock_irqsave(&prv->lock, flags); >> >> - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) >> + switch ( op->cmd ) >> { >> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: >> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: >> + return -EINVAL; >> + case XEN_DOMCTL_SCHEDOP_getinfo: >> op->u.credit.weight = sdom->weight; >> op->u.credit.cap = sdom->cap; >> > Not feeling to strong about it, but I think > > switch ( op->cmd ) > { > case XEN_DOMCTL_SCHEDOP_getinfo: > op->u.credit.weight = sdom->weight; > op->u.credit.cap = sdom->cap; > break; > case XEN_DOMCTL_SCHEDOP_putinfo: > if ( op->u.credit.weight != 0 ) > { > if ( !list_empty(&sdom->active_sdom_elem) ) > { > prv->weight -= sdom->weight * sdom->active_vcpu_count; > prv->weight += op->u.credit.weight * sdom->active_vcpu_count; > } > sdom->weight = op->u.credit.weight; > } > > if ( op->u.credit.cap != (uint16_t)~0U ) > sdom->cap = op->u.credit.cap; > break; > case XEN_DOMCTL_SCHEDOP_putvcpuinfo: > case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > default: > return -EINVAL; > } > > (i.e., grouping the cases that needs only returning -EINVAL) is better, > the final result, more than the patch itself. In fact there's no point in explicitly naming the unhandled values as long as they're matching what "default:" does. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 5212 bytes --] On Thu, 2016-03-17 at 14:50 -0500, Chong Li wrote: > On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > > > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > > > > > > +/* Set the RTDS scheduling parameters of all vcpus of a domain > > > */ > > > +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, > > > uint32_t > > > domid, > > > + const libxl_vcpu_sched_params > > > *scinfo) > > > > > Indentation? > If I follow the indentation rule, the second line would be longer > than > 80 characters. > The function name is just too long. > static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, const libxl_vcpu_sched_params *scinfo) or static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, const libxl_vcpu_sched_params *scinfo) or shorten the name. From quickly looking at libxl, neither of the first two proposed solutions seems to happen much, so I recommend shortening the name a bit. It's an internal function, so we can do that pretty freely. > > > LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { > > > - if (scinfo->budget < 1) { > > > - LOG(ERROR, "VCPU budget is not set or out of range, > > > " > > > - "valid values are larger than 1"); > > > + if (scinfo->period != > > > LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && > > > + scinfo->budget != > > > LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) > > > + if (sched_rtds_validate_params(gc, scinfo->period, > > > scinfo- > > > > > > > > budget)) > > > return ERROR_INVAL; > > > > > I'm not sure I understand. What's happening in this function? > > > > As it stands after this patch, it looks to me that: > > - we read the default scheduling parameter from Xen, > > via xc_sched_rtds_domain_get() > > - we (possibly, if both are non-default) validate a new period and > > budget couple of values > > - we don't use such values for anything, and set back what we got > > from Xen, via xc_sched_rtds_domain_set() > > > > Either I'm missing something very basic, or this is not what Wei > > said > > when reviewing v6: > > > > "Then at callsites you set those values with two direct assignment: > > > > if (validate(period_value, budget_value) != 0) { > > error; > > } > > period = period_value; > > budget = budget_value;" > > > > Is it? > The current RTDS (xen 4.6) is: > > 1) Read the (per-domain) scheduling params from Xen, and store them > to sdom > 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is > -1), use sdom.period; > else sdom.period = new period (if new period is valid); > If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which > is > -1), use sdom.budget; > else sdom.budget = new budget; > 3) xc_sched_rtds_domain_set (sdom); > Sort of, let me restate it more generally: 1) get currently set period and budget; 2) if scinfo->period is not PERIOD_DEFAULT and is valid, update the period; 3) if scinfo->budget is not BUDGET_DEFAULT and is valid, update the budget However that happens in terms of where the values of budget and period are stashed, what call to xc_sched_rtds_domain_{get,set}() are made, etc. So, basically, when you say "if period equals PERIOD_DEFAULT", you mean scinfo->period, i.e., the period being set is just the default libxl value for it, which in turns means (most likely) one want to only alter the budget. > In our patch, my plan is: > 1) Read the default params from Xen, and store them to sdom > 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is > -1), use sdom.period; > else sdom.period = new period (if new period is valid); > If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which > is > -1), use sdom.budget; > else sdom.budget = new budget; > 3) xc_sched_rtds_domain_set (sdom); > > Even though I made some mistakes in this post (forgot the two "else"s > in my plan), is this plan ok? > Your plan should be to leve things exactly as they are, from a functional/logical perspective. The difference between "(per-domain) scheduling params from Xen" of right now, and "the default params from Xen" of after the patch, is of no concern here, as it's all done by Xen. So, really, this should be _all_ about taking the chance of refactoring the code such as the validating function does not have weird side effects, not about changing the logic, which looks fine to me. Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --] On Fri, 2016-03-18 at 01:39 -0600, Jan Beulich wrote: > > > > > > On 17.03.16 at 21:42, <lichong659@gmail.com> wrote: > > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli > > <dario.faggioli@citrix.com> wrote: > > > > > > I'd say that, in this specific case, is not a big deal which one > > > of the > > > two approaches you take (mentioning or separate patch), but the > > > having > > > a separate one is indeed almost always preferable (e.g., the fix > > > can be > > > backported). > > If I choose mentioning, do I move the comment to the changelog? Or > > do I keep > > it here and say it again in the changelog? > Just consider what would happen if everyone mentioned in > comments the bugs they fixed. I think the answer to you question > is obvious then... > Exactly! :-) And, please (Chong), let me restate this: "having a separate one is indeed almost always preferable (e.g., the fix can be backported)" Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Fri, Mar 18, 2016 at 5:47 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Fri, 2016-03-18 at 01:39 -0600, Jan Beulich wrote: >> > >> > > > On 17.03.16 at 21:42, <lichong659@gmail.com> wrote: >> > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli >> > <dario.faggioli@citrix.com> wrote: >> > > >> > > I'd say that, in this specific case, is not a big deal which one >> > > of the >> > > two approaches you take (mentioning or separate patch), but the >> > > having >> > > a separate one is indeed almost always preferable (e.g., the fix >> > > can be >> > > backported). >> > If I choose mentioning, do I move the comment to the changelog? Or >> > do I keep >> > it here and say it again in the changelog? >> Just consider what would happen if everyone mentioned in >> comments the bugs they fixed. I think the answer to you question >> is obvious then... >> > Exactly! :-) > > And, please (Chong), let me restate this: "having a separate one is > indeed almost always preferable (e.g., the fix can be backported)" > Considering the complexity of making a new patch and today's the last day for posting, I'll mention this bug fix in the changlog. Chong -- Chong Li Department of Computer Science and Engineering Washington University in St.louis _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel