From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v2 3/4] libxl: add rt scheduler Date: Mon, 8 Sep 2014 16:19:06 +0100 Message-ID: <540DC8EA.2020607@eu.citrix.com> References: <1410118861-2671-1-git-send-email-mengxu@cis.upenn.edu> <1410118861-2671-4-git-send-email-mengxu@cis.upenn.edu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1410118861-2671-4-git-send-email-mengxu@cis.upenn.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: Meng Xu , xen-devel@lists.xen.org Cc: ian.campbell@citrix.com, xisisu@gmail.com, stefano.stabellini@eu.citrix.com, lu@cse.wustl.edu, dario.faggioli@citrix.com, ian.jackson@eu.citrix.com, ptxlinh@gmail.com, xumengpanda@gmail.com, JBeulich@suse.com, chaowang@wustl.edu, lichong659@gmail.com, dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org On 09/07/2014 08:41 PM, Meng Xu wrote: > Add libxl functions to set/get domain's parameters for rt scheduler > Note: VCPU's information (period, budget) is in microsecond (us). > > Signed-off-by: Meng Xu > Signed-off-by: Sisu Xi > --- > tools/libxl/libxl.c | 75 +++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl.h | 1 + > tools/libxl/libxl_types.idl | 2 ++ > 3 files changed, 78 insertions(+) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 2ae5fca..6840c92 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5155,6 +5155,75 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid, > return 0; > } > > +static int sched_rt_domain_get(libxl__gc *gc, uint32_t domid, > + libxl_domain_sched_params *scinfo) > +{ > + struct xen_domctl_sched_rt sdom; > + int rc; > + > + rc = xc_sched_rt_domain_get(CTX->xch, domid, &sdom); > + if (rc != 0) { > + LOGE(ERROR, "getting domain sched rt"); > + return ERROR_FAIL; > + } > + > + libxl_domain_sched_params_init(scinfo); > + > + scinfo->sched = LIBXL_SCHEDULER_RT_DS; > + scinfo->period = sdom.period; > + scinfo->budget = sdom.budget; > + > + return 0; > +} > + > +#define SCHED_RT_DS_VCPU_PERIOD_UINT_MAX 4294967295U /* 2^32 - 1 us */ > +#define SCHED_RT_DS_VCPU_BUDGET_UINT_MAX SCHED_RT_DS_VCPU_PERIOD_UINT_MAX I think what Dario was looking for was this: #define SCHED_RT_DS_VCPU_PERIOD_MAX UINT_MAX I.e., use the already-defined #defines with meaningful names (line UINT_MAX), and avoid open-coding (i.e., typing out a "magic" number, like 429....U). > + > +static int sched_rt_domain_set(libxl__gc *gc, uint32_t domid, > + const libxl_domain_sched_params *scinfo) > +{ > + struct xen_domctl_sched_rt sdom; > + int rc; > + > + rc = xc_sched_rt_domain_get(CTX->xch, domid, &sdom); You need to check the return value here and bail out on an error. > + > + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { > + if (scinfo->period < 1 || > + scinfo->period > SCHED_RT_DS_VCPU_PERIOD_UINT_MAX) { ...but this isn't right anyway, right? scinfo->period is a signed integer. You shouldn't be comparing it to an unsigned int; and this can never be false anyway, because even if it's automatically cast to be unsigned, the type isn't big enough to be bigger than UINT_MAX anyway. If period is allowed to be anything up to INT_MAX, then there's no need to check the upper bound. Checking to make sure it's >= 1 should be sufficient. Then you can just get rid of the #defines above. > + LOG(ERROR, "VCPU period is not set or out of range, " > + "valid values are within range from 0 to %u", > + SCHED_RT_DS_VCPU_PERIOD_UINT_MAX); > + return ERROR_INVAL; > + } > + sdom.period = scinfo->period; > + } > + > + if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { > + if (scinfo->budget < 1 || > + scinfo->budget > SCHED_RT_DS_VCPU_BUDGET_UINT_MAX) { Same here. > + LOG(ERROR, "VCPU budget is not set or out of range, " > + "valid values are within range from 0 to %u", > + SCHED_RT_DS_VCPU_BUDGET_UINT_MAX); > + 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_rt_domain_set(CTX->xch, domid, &sdom); > + if (rc < 0) { > + LOGE(ERROR, "setting domain sched rt"); > + return ERROR_FAIL; > + } > + > + return 0; > +} > + > int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, > const libxl_domain_sched_params *scinfo) > { > @@ -5178,6 +5247,9 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_ARINC653: > ret=sched_arinc653_domain_set(gc, domid, scinfo); > break; > + case LIBXL_SCHEDULER_RT_DS: > + ret=sched_rt_domain_set(gc, domid, scinfo); > + break; > default: > LOG(ERROR, "Unknown scheduler"); > ret=ERROR_INVAL; > @@ -5208,6 +5280,9 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_CREDIT2: > ret=sched_credit2_domain_get(gc, domid, scinfo); > break; > + case LIBXL_SCHEDULER_RT_DS: > + ret=sched_rt_domain_get(gc, domid, scinfo); > + break; > default: > LOG(ERROR, "Unknown scheduler"); > ret=ERROR_INVAL; > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 460207b..dbe736c 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -1280,6 +1280,7 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid, > #define LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT -1 > #define LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT -1 > #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1 > +#define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1 > > int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, > libxl_domain_sched_params *params); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 931c9e9..72f24fe 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -153,6 +153,7 @@ libxl_scheduler = Enumeration("scheduler", [ > (5, "credit"), > (6, "credit2"), > (7, "arinc653"), > + (8, "rt_ds"), rtds Other than that, looks good. -George