From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS Date: Tue, 8 Aug 2017 18:09:36 +0200 Message-ID: <1502208576.18446.17.camel@citrix.com> References: <1502036563-4275-1-git-send-email-mengxu@cis.upenn.edu> <1502036563-4275-4-git-send-email-mengxu@cis.upenn.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8701562371696853392==" 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: Meng Xu , "xen-devel@lists.xen.org" Cc: George Dunlap , Ian Jackson , Wei Liu , Meng Xu List-Id: xen-devel@lists.xenproject.org --===============8701562371696853392== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-kwpz9SCzHKOMqvZMoPw2" --=-kwpz9SCzHKOMqvZMoPw2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sun, 2017-08-06 at 22:43 -0400, Meng Xu wrote: > On Sun, Aug 6, 2017 at 12:22 PM, Meng Xu > wrote: > >=20 > > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c > > index 2c71a9f..88933a4 100644 > > --- a/tools/xl/xl_cmdtable.c > > +++ b/tools/xl/xl_cmdtable.c > > @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] =3D { > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ "sched-rtds", > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0&main_sched_rtds, 0, 1, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"Get/set rtds scheduler param= eters", > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"[-d [-v[=3DVCPUID/all]] = [-p[=3DPERIOD]] [- > > b[=3DBUDGET]]]", > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"[-d [-v[=3DVCPUID/all]] = [-p[=3DPERIOD]] [-b[=3DBUDGET]] > > [-e[=3DEXTRATIME]]]", > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"-d DOMAIN, --domain=3DDOMAIN= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Domain to modify\n" > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"-v VCPUID/all, --vcpuid=3DVC= PUID/all=C2=A0=C2=A0=C2=A0=C2=A0VCPU to modify or > > output;\n" > > =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=A0Using '-v al= l' to modify/output all vcpus\n" > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"-p PERIOD, --period=3DPERIOD= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Period (us)\n" > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"-b BUDGET, --budget=3DBUDGET= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Budget (us)\n" > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"-e EXTRATIME, --extratime=3DEXTRA= TIME EXTRATIME (1=3Dyes, > > 0=3Dno)\n" >=20 > Hi Dario, >=20 > I kept the EXTRATIME value for -e option because: (1) it may be more > intuitive for users; (2) it needs much less code change than the > input > style that does not need EXTRATIME value. >=20 I'm of the opposite view. But again, it's tools' people views' that count. :-D > As to (1), if users want to set some VCPUs with extratime flag set > and > some with extratime flag clear, there are two types of input: > (a) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -e 0 -v 2 -p 10000 -b > 4000 -e 1 -v 5 -p 10000 -b 4000 -e 0 > (b) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -v 2 -p 10000 -b 4000 -e > 1 -v 5 -p 10000 -b 4000 > I felt that the style (a) is more intuitive and the input commands > have very static pattern, i.e., each vcpu must have -v -p -b -e > options set. > Exactly, I do think that (b) is indeed a better user interface. For instance, what if I want to change period and budget of vcpu 1 of domain 3, _without_ changing whether or not it can use the extra time.=C2= =A0 With (a), I don't think I can do that. Or at least, I'd have to either remember or check what extratime is right now, and pass that same value explicitly to `xl sched-rtds -d 3 -v 1 ...'. That does not look super user-friendly to me. > As to (2), if we go with -e without EXTRATIME, we will have to keep > track of the vcpu that has no -e option. I thought about this option, > we can pre-set the extratime value to false when -v option is > assigned: > =C2=A0=C2=A0=C2=A0=C2=A0case 'v': > =C2=A0=C2=A0=C2=A0=C2=A0... > =C2=A0=C2=A0=C2=A0=C2=A0extratimes[v_index]=C2=A0=C2=A0=3D 0; >=20 > and set the extratimes[v_index] =3D 0 when -e is set. >=20 Yes, something like that. Or, even better, use its current value. That would require calling libxl_vcpu_sched_params_get() (or, at times, libxl_vcpu_sched_params_get_all()), which I assumed you were doing already, while you apparently don't. Mmm... > This approach is not very neat in the code: we have to reallocate > memory for extratimes array when its size is not enough; we also have > to deal with the special case when -e is set before -v, such as the > command "xl sched-rtds -p 10000 -b 4000 -e -v 0" >=20 Err... sorry, there's code for reallocations in this patch already, isn't this the case? Also, it may be me, but I don't understand how this is different from how -b and -p are dealt with. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-kwpz9SCzHKOMqvZMoPw2 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 iQIcBAABCAAGBQJZieJAAAoJEBZCeImluHPu+tEP/0mF9oGqs8ZA1wUSm6nvxwwd aIe4FV9ii6yvutQzkYaau1moRLk/I6NuaXIQ0BDVX8Z9vOocypCYrjTP487KcoUd 7iMilKXN/lPgNGniOZ/TU53ybgfTn09UYh8/JDZIQXEsCmrnL65NNncrrz84hvpL 6RvYnrAJdMbZglEvoT+EUON91UfTbqQYaUsngxvNIP+lTdL/H2ti59I1kU2JLc2X qPKMThQw2xc8UmI0mIYOEBYr5WzCSr5Gllr3nGJH5J/7bG0+RaTnOyueu6wccQBv xWozsf81jOgL3s7f2/wWWjSALv8lUbWohnI2c80tZKn4A3u9xldZAITXWD9YI3mg 3MXH9bPUqLB2lOdAfEqmrgt2Ux57nuAIfr+AKLQu2Nu9DSY8ypDaCfttKJJaf49a PhHPIsj+SYPTaiSnsRUwpkDV5tqnXw+ml1jIcoA4gvlz19Wj3vXawCS0VK1Puzb2 23R5TWs+VVTbp10HWj4KmpNVjLPk8qrmA5tqeRHvJN84ipSldf4vGB0REWHlvs/e zLQzykMqXwBC7Gisyqhg/hLzEEvUDd6EjC3fKbXC84MQerQeLjlEKbIQYIe7NXzs b7Y9v6iadxcSvd2AhRHm3Fpkl3f8D6dRPnF/0XSAybKDSy6t1N/3owpsPvO1Uy95 iEwvytxywZZa8TEaGUEp =uxEn -----END PGP SIGNATURE----- --=-kwpz9SCzHKOMqvZMoPw2-- --===============8701562371696853392== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============8701562371696853392==--