From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v1 1/3] xen:rtds: towards work conserving RTDS Date: Tue, 8 Aug 2017 16:57:13 +0200 Message-ID: <1502204233.18446.12.camel@citrix.com> References: <1502036563-4275-1-git-send-email-mengxu@cis.upenn.edu> <1502036563-4275-2-git-send-email-mengxu@cis.upenn.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7275099789659538380==" Return-path: In-Reply-To: <1502036563-4275-2-git-send-email-mengxu@cis.upenn.edu> 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: Stefano Stabellini , Wei Liu , george.dunlap@eu.citrix.com, Andrew Cooper , ian.jackson@eu.citrix.com, TimDeegan , xumengpanda@gmail.com, Jan Beulich , wei.liu@citrix.com List-Id: xen-devel@lists.xenproject.org --===============7275099789659538380== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-DauTnmeAuRQok2v8gZGQ" --=-DauTnmeAuRQok2v8gZGQ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sun, 2017-08-06 at 12:22 -0400, Meng Xu wrote: > Make RTDS scheduler work conserving without breaking the real-time > guarantees. >=20 > VCPU model: > Each real-time VCPU is extended to have an extratime flag > and a priority_level field. > When a VCPU's budget is depleted in the current period, > if it has extratime flag set, > its priority_level will increase by 1 and its budget will be > refilled; > othewrise, the VCPU will be moved to the depletedq. >=20 > Scheduling policy is modified global EDF: > A VCPU v1 has higher priority than another VCPU v2 if > (i) v1 has smaller priority_leve; or > (ii) v1 has the same priority_level but has a smaller deadline >=20 > Queue management: > Run queue holds VCPUs with extratime flag set and VCPUs with > remaining budget. Run queue is sorted in increasing order of VCPUs > priorities. > Depleted queue holds VCPUs which have extratime flag cleared and > depleted budget. > Replenished queue is not modified. >=20 > Signed-off-by: Meng Xu >=20 This looks mostly good to me. There are only a couple of things left, in addition to the changlog+comment mention to how the 'spare bandwidth' distribution works, that we agreed upon in the other thread. > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c=C2=A0 > @@ -245,6 +258,11 @@ static inline struct list_head *rt_replq(const > struct scheduler *ops) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return &rt_priv(ops)->replq; > =C2=A0} > =C2=A0 > +static inline bool has_extratime(const struct rt_vcpu *svc) > +{ > +=C2=A0=C2=A0=C2=A0=C2=A0return (svc->flags & RTDS_extratime) ? 1 : 0; > +} > + > Cool... I like 'has_extratime()' soo much better as a name than what it was before! Thanks. :-) > =C2=A0/* > =C2=A0 * Helper functions for manipulating the runqueue, the depleted > queue, > =C2=A0 * and the replenishment events queue. > @@ -274,6 +292,21 @@ vcpu_on_replq(const struct rt_vcpu *svc) > =C2=A0} > =C2=A0 > =C2=A0/* > + * If v1 priority >=3D v2 priority, return value > 0 > + * Otherwise, return value < 0 > + */ > +static s_time_t > +compare_vcpu_priority(const struct rt_vcpu *v1, const struct rt_vcpu > *v2) > +{ > +=C2=A0=C2=A0=C2=A0=C2=A0int prio =3D v2->priority_level - v1->priority_l= evel; > + > +=C2=A0=C2=A0=C2=A0=C2=A0if ( prio =3D=3D 0 ) > +=C2=A0=C2=A0=C2=A0=C2=A0return v2->cur_deadline - v1->cur_deadline; > + Indentation. > @@ -423,15 +459,18 @@ rt_update_deadline(s_time_t now, struct rt_vcpu > *svc) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0svc->last_start =3D now; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0svc->cur_budget =3D svc->budget; > +=C2=A0=C2=A0=C2=A0=C2=A0svc->priority_level =3D 0; > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* TRACE */ > =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=A0struct __packed { > =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=A0unsigned vcpu:16, dom:16; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= unsigned priority_level; > =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=A0uint64_t cur_deadline, cur_budget; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} d; > Can you please, and in this very comment, update tools/xentrace/xenalyze.c and tools/xentrace/formats as well, to take into account this new field? > diff --git a/xen/include/public/domctl.h > b/xen/include/public/domctl.h > index 0669c31..ba5daa9 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -360,6 +360,9 @@ typedef struct xen_domctl_sched_credit2 { > =C2=A0typedef struct xen_domctl_sched_rtds { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint32_t period; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint32_t budget; > +#define _XEN_DOMCTL_SCHED_RTDS_extratime 0 > +#define > XEN_DOMCTL_SCHED_RTDS_extratime=C2=A0=C2=A0(1U<<_XEN_DOMCTL_SCHED_RTDS_ex= tratim > e) > +=C2=A0=C2=A0=C2=A0=C2=A0uint32_t flags; > I'd add a one liner comment above the flag definition, as, for instance, how things are done in createdomain: struct xen_domctl_createdomain { =C2=A0=C2=A0=C2=A0=C2=A0/* IN parameters */ =C2=A0=C2=A0=C2=A0=C2=A0uint32_t ssidref; =C2=A0=C2=A0=C2=A0=C2=A0xen_domain_handle_t handle; =C2=A0/* Is this an HVM guest (as opposed to a PVH or PV guest)? */ #define _XEN_DOMCTL_CDF_hvm_guest=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00 #define XEN_DOMCTL_CDF_hvm_guest=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(1U<<_X= EN_DOMCTL_CDF_hvm_guest) =C2=A0/* Use hardware-assisted paging if available? */ #define _XEN_DOMCTL_CDF_hap=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A01 #define XEN_DOMCTL_CDF_hap=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0(1U<<_XEN_DOMCTL_CDF_hap) Also, consider shortening the name (e.g., by contracting the SCHED_RTDS part; it does not matter if it's not 100% equal to what's in sched_rt.c, I think). This, of course, is just my opinion, and final say belongs to maintainers of this public interface, which I think means 'THE REST', and most of them are not Cc-ed. Let me do that... Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-DauTnmeAuRQok2v8gZGQ 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 iQIcBAABCAAGBQJZidFJAAoJEBZCeImluHPuwqkP/0XhkGKMv23arKU+usClgYyY s6mb7ep2c7Zf9S+JHfTI83JTAEiXsegk9ALd/lknf32RrxvyDzx2PO0r/ST7haKS eCzxLA2zwCdNT4y6oj2jqwWP8zPr7X8//Akmi+9zodfKeSIwvJqJskKgahWc0cao Oe8rosMZiLZ4YB/4uEXAptWo6ofQ2OwJMKKK0Ib/hbRqn3IIdEL1M5Fb/1x0O5xO WcthdPUrvaXhGYqIwaxDYKYt5GwvAt2p8qYeIfoVUyXrqQJYHsirMFA5zJKanqxz Y+rAR1GIFQLUt5sCQ2YA9Gau18YErlV/Y3zDvOFI5RwRoS12aMvuQtsNCuyFBOhz UkdEFgcBW2q6Fd4nJcso/pA2jssiYwYZJvh6cfmY2C/v+DLf8oOErTCllUs8/24b 9MfCuXsHBgN+HBI9c2cpwC4YJyELafhm70jBlFP5JSrdpjFMComHFuWDDUTpGBbW 3BzjZG0tXELrDXBS4fIODzVEjC6xflicQDA2V4a/rjGEPi4sZXUFTnLI33da8zA+ L55xhNDFgbrkuaLtq8K9x4Y15LhFYVxWo42HwLxIY/xY8iTCBRkK6VdQdPo6jv0p IUkrTeKqwAbGCtMgBQSRTg3NMdvKK1x3X2LDk1H9qOTW084KMB4rUP12bZG8DKCL q0zRw0iFM0pkKHsjbpDP =HvgJ -----END PGP SIGNATURE----- --=-DauTnmeAuRQok2v8gZGQ-- --===============7275099789659538380== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============7275099789659538380==--