From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler Date: Fri, 5 Jun 2015 12:37:33 +0100 Message-ID: <1433504253.7108.231.camel@citrix.com> References: <1432598984-20914-1-git-send-email-chong.li@wustl.edu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1432598984-20914-1-git-send-email-chong.li@wustl.edu> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chong Li Cc: Chong Li , wei.liu2@citrix.com, Sisu Xi , george.dunlap@eu.citrix.com, dario.faggioli@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, mengxu@cis.upenn.edu, dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org On Mon, 2015-05-25 at 19:09 -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support > per-VCPU settings for RTDS scheduler. > > Add a new data structure (libxl_vcpu_sched_params) to help per-VCPU settings. > > Signed-off-by: Chong Li > Signed-off-by: Meng Xu > Signed-off-by: Sisu Xi > --- > tools/libxl/libxl.c | 189 ++++++++++++++++++++++++++++++++++++++------ > tools/libxl/libxl.h | 19 +++++ > tools/libxl/libxl_types.idl | 11 +++ > 3 files changed, 196 insertions(+), 23 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index feb3aa9..169901a 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5797,6 +5797,120 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid, > return 0; > } > > +static int sched_rtds_validate_params(libxl__gc *gc, uint32_t period, > + uint32_t budget, uint32_t *sdom_period, > + uint32_t *sdom_budget) > +{ > + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT is -1, while period appears to be unsigned, so I think think this will ever be false. This means you will be setting the period to 2^32 if the default is requested. > + if (period < 1) { > + LOG(ERROR, "VCPU period is not set or out of range, " > + "valid values are larger than 1"); According to the check they are 1 or greater i.e. 1 is considered valid while the message suggests that 2 is the first valid value. > + return ERROR_INVAL; > + } > + *sdom_period = period; > + } > + > + if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { Likewise. > + if (budget < 1) { > + LOG(ERROR, "VCPU budget is not set or out of range, " > + "valid values are larger than 1"); Likewise. > + return ERROR_INVAL; > + } > + *sdom_budget = budget; > + } > + > + if (budget > period) { > + LOG(ERROR, "VCPU budget is larger than VCPU period, " > + "VCPU budget should be no larger than VCPU period"); The second half of this statement is redundant I think. > + return ERROR_INVAL; > + } > + > + return 0; > +} > + > +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ > + uint16_t num_vcpus; > + int rc, i; > + xc_dominfo_t info; > + > + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; > + } > + num_vcpus = info.max_vcpu_id + 1; > + > + struct xen_domctl_sched_rtds_params *sdom = libxl__malloc(NOGC, ^ one space only please. > + sizeof(struct xen_domctl_sched_rtds_params) * num_vcpus); GCNEW_ARRAY, I think. Please define sdom at the top (but init it here) Also, I think you leak sdom here, since it is not returned to the user it should be gc'd. > + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus); > + if (rc != 0) { > + LOGE(ERROR, "getting vcpu sched rtds"); > + return ERROR_FAIL; > + } > + > + libxl_vcpu_sched_params_init(scinfo); > + > + scinfo->sched = LIBXL_SCHEDULER_RTDS; > + scinfo->num_vcpus = num_vcpus; > + scinfo->vcpus = (libxl_rtds_vcpu *) > + libxl__malloc(NOGC, sizeof(libxl_rtds_vcpu) * num_vcpus); You don't need to cast the result of malloc (in C at least) Also libxl__calloc is better for allocating arrays, but as above you want GCNEW_ARRAY anyway. NOGC is correct for this allocation. > + for(i = 0; i < num_vcpus; i++) { > + scinfo->vcpus[i].period = sdom[i].period; > + scinfo->vcpus[i].budget = sdom[i].budget; > + } > + > + return 0; > +} > + > +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid, > + const libxl_vcpu_sched_params *scinfo) > +{ > + int rc; > + int i; > + uint16_t num_vcpus; > + int vcpuid; > + uint32_t budget, period; > + xc_dominfo_t info; > + > + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; > + } > + num_vcpus = info.max_vcpu_id + 1; > + > + struct xen_domctl_sched_rtds_params *sdom = > + libxl__malloc(NOGC, scinfo->num_vcpus); GCNEW_ARRAY, and not NOGC, define sdom at the top please and the same double space issue in the type definition as before too. > + for (i = 0; i < scinfo->num_vcpus; i++) { > + vcpuid = scinfo->vcpus[i].vcpuid; Isn't scinfo->vcpus[i].vcpuid unsigned, while the local var is an int. > + rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget, > + &sdom.period, &sdom.budget); While this is ok, we normally align the parameters under each other, i.e. align the second line with "gc" in this case. There are a few of these here. > + if (rc == ERROR_INVAL) > + return rc; > > rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom); > if (rc < 0) { > @@ -5899,6 +5994,30 @@ 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 ret; > + > + if (sched == LIBXL_SCHEDULER_UNKNOWN) > + sched = libxl__domain_scheduler(gc, domid); > + > + switch (sched) { > + case LIBXL_SCHEDULER_RTDS: > + ret=sched_rtds_vcpu_set(gc, domid, scinfo); Spaces around = please (in several places) > + break; > + default: > + LOG(ERROR, "Unknown scheduler"); > + ret=ERROR_INVAL; I think unknown scheduler is distinct from a scheduler having no per-vcpu parameters. > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 117b61d..d28d274 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ > ("checkpointed_stream", integer), > ]) > > +libxl_rtds_vcpu = Struct("rtds_vcpu",[ > + ("period", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}), > + ("budget", uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}), > + ("vcpuid", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}), > + ]) > + > +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[ > + ("sched", libxl_scheduler), > + ("vcpus", Array(libxl_rtds_vcpu, "num_vcpus")), How will we handle a second scheduler with per-vcpu parameters being added in the future without breaking the API? > + ]) > + > libxl_domain_sched_params = Struct("domain_sched_params",[ > ("sched", libxl_scheduler), > ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),