From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v1 3/4] libxl: add rt scheduler Date: Fri, 5 Sep 2014 12:21:07 +0200 Message-ID: <1409912467.2673.288.camel@Solace.lan> References: <1408921125-21470-1-git-send-email-mengxu@cis.upenn.edu> <1408921125-21470-4-git-send-email-mengxu@cis.upenn.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1398428212303525519==" Return-path: In-Reply-To: <1408921125-21470-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 Cc: ian.campbell@citrix.com, xisisu@gmail.com, stefano.stabellini@eu.citrix.com, george.dunlap@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, xumengpanda@gmail.com, JBeulich@suse.com, chaowang@wustl.edu, lichong659@gmail.com, dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org --===============1398428212303525519== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-YV/O/GqpixIhmG1E99+p" --=-YV/O/GqpixIhmG1E99+p Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On dom, 2014-08-24 at 18:58 -0400, Meng Xu wrote: > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5154,6 +5154,139 @@ static int sched_sedf_domain_set(libxl__gc *gc, u= int32_t domid, > return 0; > } > =20 > +static int sched_rt_domain_get(libxl__gc *gc, uint32_t domid, > + libxl_domain_sched_params *scinfo) > +{ > + struct xen_domctl_sched_rt_params* sdom; > + uint16_t num_vcpus; > + int rc, i; > + > + rc =3D xc_sched_rt_domain_get_num_vcpus(CTX->xch, domid, &num_vcpus)= ; > + if (rc !=3D 0) { > + LOGE(ERROR, "getting num_vcpus of domain sched rt"); > + return ERROR_FAIL; > + } > As George pointed out already, you can get the num_vcpus via xc_domain_getinfo(). I agree with George's review about the rest of this function (appropriately updated to take what we decided about the interface into account :-) ). > +#define SCHED_RT_VCPU_PERIOD_MAX 31536000000000 /* one year in micros= econd*/ > +#define SCHED_RT_VCPU_BUDGET_MAX SCHED_RT_VCPU_PERIOD_MAX > + This may be me not remembering correctly the outcome of a preceding discussion... did we say we were going with _RT_ or with something like _RTDS_ ? ISTR the latter... Also, macros like INT_MAX, UINT_MAX, etc., or << and ~ "tricks" are, IMO, preferrable to the open coding of the value. Finally, I wonder whether these would better live in some headers, closer to the declaration of period and budget (where their type is also visible) and, as a nice side effect of that, available to libxl callers as well. > +/* > + * Sanity check of the scinfo parameters > + * return 0 if all values are valid > + * return 1 if one param is default value > + * return 2 if the target vcpu's index, period or budget is out of range > + */ > +static int sched_rt_domain_set_validate_params(libxl__gc *gc, > + const libxl_domain_sched_= params *scinfo, > + const uint16_t num_vcpus) > +{ > + int vcpu_index =3D scinfo->rt.vcpu_index; > + As per the low level interface (Xen and libxc) there should be no need for any vcpu_index anymore, right? I'm just double checking, as the discussion was --as it should, on these things-- long and involved :-D > + if (vcpu_index < 0 || vcpu_index > num_vcpus) > + { > + LOG(ERROR, "VCPU index is not set or out of range, " > + "valid values are within range from 0 to %d", num_vc= pus); > + return 2; > + } > + > + if (scinfo->rt.period < 1 || > + scinfo->rt.period > SCHED_RT_VCPU_PERIOD_MAX) > + { > + LOG(ERROR, "VCPU period is not set or out of range, " > + "valid values are within range from 0 to %lu", SCHED= _RT_VCPU_PERIOD_MAX); > + return 2; > + } > + > + if (scinfo->rt.budget < 1 || > + scinfo->rt.budget > SCHED_RT_VCPU_BUDGET_MAX) > + { > + LOG(ERROR, "VCPU budget is not set or out of range, " > + "valid values are within range from 0 to %lu", SCHED= _RT_VCPU_BUDGET_MAX); > + return 2; > + } > + I think some basics sanity checking are fine done here. E.g., you may also add a (budget <=3D period) check. However, as George said already: - make sure you limit these to the kind of checks based on info the=20 toolstack should have, and defer the rest to the hypervisor - if something goes wrong, make sure to always return libxl error codes (typically, ERROR_INVAL), as sched_credit_domain_set() does. Oh and, just double checking again, I think we decided to handle (budget =3D=3D0 && period =3D=3D 0) in a special way, so remember that! :-P It's personal taste, I guess, but I think you don't really need an helper function for this, and it can leave in sched_rt_domain_set. > +static int sched_rt_domain_set(libxl__gc *gc, uint32_t domid, > + const libxl_domain_sched_params *scinfo) > +{ > + struct xen_domctl_sched_rt_params sdom; > + uint16_t num_vcpus; > + int rc; > +=20 > + rc =3D xc_sched_rt_domain_get_num_vcpus(CTX->xch, domid, &num_vcpus)= ; > + if (rc !=3D 0) { > + LOGE(ERROR, "getting domain sched rt"); > + return ERROR_FAIL; > + } > + =20 > + rc =3D sched_rt_domain_set_validate_params(gc, scinfo, num_vcpus); > + if (rc =3D=3D 2) > + return ERROR_INVAL; > + if (rc =3D=3D 1) > + return 0; > + if (rc =3D=3D 0) > + { > + sdom.index =3D scinfo->rt.vcpu_index; > + sdom.period =3D scinfo->rt.period; > + sdom.budget =3D scinfo->rt.budget; > + } > + So, I see that you are actually returning libxl error codes, good. Well, again, just put the code above directly here, instead of having to define and then interpret an ad-hoc error handling logic. :-) > @@ -5177,6 +5310,9 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, u= int32_t domid, > case LIBXL_SCHEDULER_ARINC653: > ret=3Dsched_arinc653_domain_set(gc, domid, scinfo); > break; > + case LIBXL_SCHEDULER_RT: > + ret=3Dsched_rt_domain_set(gc, domid, scinfo); > + break; Again, I seriously think I remember we agreed upon _SCHEDULER_RTDS (or _RT_DS) as thee name of this thing. Am I wrong? > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -153,6 +153,7 @@ libxl_scheduler =3D Enumeration("scheduler", [ > (5, "credit"), > (6, "credit2"), > (7, "arinc653"), > + (8, "rt"), rtds? rt_ds? > @@ -303,6 +304,19 @@ libxl_domain_restore_params =3D Struct("domain_resto= re_params", [ > ("checkpointed_stream", integer), > ]) > =20 > +libxl_rt_vcpu =3D Struct("vcpu",[ > + ("period", uint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERI= OD_DEFAULT'}), > + ("budget", uint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDG= ET_DEFAULT'}), > + ("index", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCP= U_INDEX_DEFAULT'}), > + ]) > + > +libxl_domain_sched_rt_params =3D Struct("domain_sched_rt_params",[ > + ("vcpus", Array(libxl_rt_vcpu, "num_vcpus")), > + ("period", uint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERI= OD_DEFAULT'}), > + ("budget", uint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDG= ET_DEFAULT'}), > + ("vcpu_index", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCP= U_INDEX_DEFAULT'}), > + ]) > + > libxl_domain_sched_params =3D Struct("domain_sched_params",[ > ("sched", libxl_scheduler), > ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEI= GHT_DEFAULT'}), > @@ -311,6 +325,7 @@ libxl_domain_sched_params =3D Struct("domain_sched_pa= rams",[ > ("slice", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLI= CE_DEFAULT'}), > ("latency", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LAT= ENCY_DEFAULT'}), > ("extratime", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXT= RATIME_DEFAULT'}), > + ("rt", libxl_domain_sched_rt_params), > ]) And about this part, we discussed and agreed already in the other thread. :-) Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-YV/O/GqpixIhmG1E99+p Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlQJjpMACgkQk4XaBE3IOsQ4PgCggkvnzRwwLDazoayeXN5ICgdO sX4AniykL9+VgvLJdEbV4qHujs5agWTg =9O3E -----END PGP SIGNATURE----- --=-YV/O/GqpixIhmG1E99+p-- --===============1398428212303525519== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============1398428212303525519==--