From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v1 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler Date: Mon, 11 May 2015 16:06:33 +0200 Message-ID: <1431353193.8979.85.camel@citrix.com> References: <1431018326-3239-1-git-send-email-chong.li@wustl.edu> <1431018326-3239-4-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: <1431018326-3239-4-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, xen-devel@lists.xen.org, mengxu@cis.upenn.edu, jbeulich@suse.com, dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org On Thu, 2015-05-07 at 12:05 -0500, Chong Li wrote: > Change sched_rtds_domain_get/set functions to support per-VCPU settings for RTDS scheduler. > > Signed-off-by: Chong Li > Signed-off-by: Meng Xu > Signed-off-by: Sisu Xi > --- > tools/libxl/libxl.c | 143 +++++++++++++++++++++++++++++++++----------- > tools/libxl/libxl.h | 6 ++ > tools/libxl/libxl_types.idl | 11 ++++ > 3 files changed, 126 insertions(+), 34 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index feb3aa9..5f66753 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5800,10 +5800,17 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid, > static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, > libxl_domain_sched_params *scinfo) > { > - struct xen_domctl_sched_rtds sdom; > - int rc; > - > - rc = xc_sched_rtds_domain_get(CTX->xch, domid, &sdom); > + uint16_t num_vcpus; > + int rc, i; > + xc_dominfo_t info; > Coding style: we want a blank line between variable definitions and actual code. > + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info); > + if (rc < 0) { > + LOGE(ERROR, "getting domain info"); > + return ERROR_FAIL; > + } > + num_vcpus = info.nr_online_vcpus; > It looks like the most of other places in libxl where this is necessary use libxl_list_vcpu(), which, if you want to open code it, uses info.max_vcpu_id. I'd do the same. > + struct xen_domctl_sched_rtds_params sdom[num_vcpus]; > And, please, all the variable definitions go together, at the top of the function. Avoid having them scattered around the body. I see why this is here, but no, that's not a good reason... For allocating arrays in libxl, have a look at GCNEW_ARRAY, or libxl__calloc, libxl__malloc, libxl__realloc, etc. > + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus); > if (rc != 0) { > LOGE(ERROR, "getting domain sched rtds"); > return ERROR_FAIL; > @@ -5821,43 +5835,104 @@ static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, > static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, > const libxl_domain_sched_params *scinfo) > { > - struct xen_domctl_sched_rtds sdom; > int rc; > + int i; > > - rc = xc_sched_rtds_domain_get(CTX->xch, domid, &sdom); > - if (rc != 0) { > - LOGE(ERROR, "getting domain sched rtds"); > - return ERROR_FAIL; > - } > + if(scinfo->rtds.num_vcpus <= 0) {/*set per-dom rtds parameters*/ > The comment is better before or after, rather than inline. But, really, can you define another helper function and call it, instead than re-indenting everything and making this one much more long and complex? Oh, and the error checking and error reporting code should be factored and shared, not duplicated. I mean this: > + 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; > + } And this: > + if (period < 1) { > + LOG(ERROR, "VCPU period is out of range, " > + "valid values are larger than 1"); > + return ERROR_INVAL; > + } > And the same for every other one. Regards, Dario