From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler Date: Fri, 18 Mar 2016 10:17:31 +0100 Message-ID: <1458292651.15374.82.camel@citrix.com> References: <1458146871-2813-1-git-send-email-lichong659@gmail.com> <1458146871-2813-4-git-send-email-lichong659@gmail.com> <1458187536.3102.980.camel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4430522055275632977==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Chong Li Cc: Chong Li , Wei Liu , Sisu Xi , George Dunlap , Ian Jackson , xen-devel , Meng Xu , Dagaen Golomb List-Id: xen-devel@lists.xenproject.org --===============4430522055275632977== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-alEZhfq5LE1rH1Wl5eyy" --=-alEZhfq5LE1rH1Wl5eyy Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2016-03-17 at 14:50 -0500, Chong Li wrote: > On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli > wrote: > >=20 > > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > > >=C2=A0 > > > +/* 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, > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const libxl_vcpu_sched_params > > > *scinfo) > > >=20 > > Indentation? > If I follow the indentation rule, the second line would be longer > than > 80 characters. > The function name is just too long. >=20 static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const lib= xl_vcpu_sched_params *scinfo) or=C2=A0 static int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const libxl_vcpu_sched_params *sc= info) or shorten the name. =46rom 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) { > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (scinfo->budget <= 1) { > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0LOG(ERROR, "VCPU budget is not set or out of range, > > > " > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"va= lid values are larger than 1"); > > > +=C2=A0=C2=A0=C2=A0=C2=A0if (scinfo->period !=3D > > > LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT && > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0scinfo->budget !=3D > > > LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (sched_rtds_valid= ate_params(gc, scinfo->period, > > > scinfo- > > > >=20 > > > > budget)) > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0return ERROR_INVAL; > > >=20 > > I'm not sure I understand. What's happening in this function? > >=20 > > As it stands after this patch, it looks to me that: > > =C2=A0- we read the default scheduling parameter from Xen, > > =C2=A0=C2=A0=C2=A0via xc_sched_rtds_domain_get() > > =C2=A0- we (possibly, if both are non-default) validate a new period an= d > > =C2=A0=C2=A0=C2=A0budget couple of values > > =C2=A0- we don't use such values for anything, and set back what we got > > =C2=A0=C2=A0=C2=A0from Xen, via xc_sched_rtds_domain_set() > >=20 > > Either I'm missing something very basic, or this is not what Wei > > said > > when reviewing v6: > >=20 > > "Then at callsites you set those values with two direct assignment: > >=20 > > =C2=A0=C2=A0=C2=A0if (validate(period_value, budget_value) !=3D 0) { > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0error; > > =C2=A0=C2=A0=C2=A0} > > =C2=A0=C2=A0=C2=A0period =3D period_value; > > =C2=A0=C2=A0=C2=A0budget =3D budget_value;" > >=20 > > Is it? > The current RTDS (xen 4.6) is: >=20 > 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; > =C2=A0=C2=A0=C2=A0=C2=A0else sdom.period =3D new period (if new period is= valid); > =C2=A0=C2=A0=C2=A0=C2=A0If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_= DEFAULT (which > is > -1), use sdom.budget; > =C2=A0=C2=A0=C2=A0=C2=A0else sdom.budget =3D new budget; > 3) xc_sched_rtds_domain_set (sdom); >=20 Sort of, let me restate it more generally: =C2=A01) get currently set period and budget; =C2=A02) if scinfo->period is not PERIOD_DEFAULT and is valid, update =C2=A0 =C2=A0 the period; =C2=A03) if scinfo->budget is not BUDGET_DEFAULT and is valid, update =C2=A0 =C2=A0 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; > =C2=A0=C2=A0=C2=A0=C2=A0else sdom.period =3D new period (if new period is= valid); > =C2=A0=C2=A0=C2=A0=C2=A0If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_= DEFAULT (which > is > -1), use sdom.budget; > =C2=A0=C2=A0=C2=A0=C2=A0else sdom.budget =3D new budget; > 3) xc_sched_rtds_domain_set (sdom); >=20 > Even though I made some mistakes in this post (forgot the two "else"s > in my plan), is this plan ok? >=20 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 --=C2=A0 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-alEZhfq5LE1rH1Wl5eyy 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 iEYEABECAAYFAlbrx6wACgkQk4XaBE3IOsTqIwCeIE1ztfm1ZCJrHTqiT4sMiaCg dpsAoJ0SxaXTB4HzNeJc0YJApMIbDWWy =Wdd+ -----END PGP SIGNATURE----- --=-alEZhfq5LE1rH1Wl5eyy-- --===============4430522055275632977== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============4430522055275632977==--