From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 1/4] xen: add real time scheduler rt Date: Tue, 9 Sep 2014 11:42:01 +0200 Message-ID: <1410255721.5651.32.camel@Abyss> References: <1410118861-2671-1-git-send-email-mengxu@cis.upenn.edu> <1410118861-2671-2-git-send-email-mengxu@cis.upenn.edu> <540DF920.9090802@eu.citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3780750344285410455==" Return-path: In-Reply-To: <540DF920.9090802@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: ian.campbell@citrix.com, xisisu@gmail.com, stefano.stabellini@eu.citrix.com, lu@cse.wustl.edu, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, ptxlinh@gmail.com, xumengpanda@gmail.com, Meng Xu , JBeulich@suse.com, chaowang@wustl.edu, lichong659@gmail.com, dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org --===============3780750344285410455== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-uP7YMLFxbI0WjhkXHkRx" --=-uP7YMLFxbI0WjhkXHkRx Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2014-09-08 at 19:44 +0100, George Dunlap wrote: > On 09/07/2014 08:40 PM, Meng Xu wrote: > > +/* > > + * update deadline and budget when deadline is in the past, > > + * it need to be updated to the deadline of the current period > > + */ > > +static void > > +rt_update_helper(s_time_t now, struct rt_vcpu *svc) > > +{ > > + s_time_t diff =3D now - svc->cur_deadline; > > + > > + if ( diff >=3D 0 ) > > + { > > + /* now can be later for several periods */ > > + long count =3D ( diff/svc->period ) + 1; > > + svc->cur_deadline +=3D count * svc->period; > > + svc->cur_budget =3D svc->budget; >=20 > In the common case, don't we expect diff/svc->period to be a small=20 > number, like 0 or 1? =20 > In general, yes. The only exception is when cur_deadline is set for the first time. In that case, now can be arbitrary large and cur_deadline will always be 0, so quite a few iterations may be required, possibly taking longer than the div and the mult. That is not an hot path anyway, so either approach would be fine in that case. For all the other occurrences, the while{} approach is an absolute win-win, IMO. > If so, since division and multiplication are so=20 > expensive, it might make more sense to make this a while() loop: >=20 > while (now - svc_cur_deadline > 0 ) > { > svc->cur_deadline +=3D svc->period; > count++; > } > if ( count ) { > svc->cur_budget =3D svc->budget; > [tracing code] > } >=20 > And similarly for the other 64-bit division Dario was asking about below? >=20 Hehe, this is, I think, the third or fourth time I say I'd like this to be turned into a while! :-D If it were me doing this, I'd go for something like this: static void rt_update_helper(s_time_t now, struct rt_vcpu *svc) { if ( svc->cur_deadline > now ) return; do svc->cur_deadline +=3D svc->period; while ( svc->cur_deadline <=3D now ); svc->cur_budget =3D svc->budget; [tracing] } Or even just the do {} while (and below), and have the check at the call sites (as George is also saying below). > I probably wouldn't make this a precondition of going in. >=20 No, I'm not strict about this either, we can do it later. I don't think it's a big or a too disruptive change, though. :-) > > + > > + /* TRACE */ > > + { > > + struct { > > + unsigned dom:16,vcpu:16; > > + unsigned cur_budget_lo, cur_budget_hi; > > + } d; > > + d.dom =3D svc->vcpu->domain->domain_id; > > + d.vcpu =3D svc->vcpu->vcpu_id; > > + d.cur_budget_lo =3D (unsigned) svc->cur_budget; > > + d.cur_budget_hi =3D (unsigned) (svc->cur_budget >> 32); > > + trace_var(TRC_RT_BUDGET_REPLENISH, 1, > > + sizeof(d), > > + (unsigned char *) &d); > > + } > > + > > + return; > > + } > > +} > > + > > +static inline void > > +__runq_remove(struct rt_vcpu *svc) > > +{ > > + if ( __vcpu_on_runq(svc) ) > > + list_del_init(&svc->runq_elem); > > +} > > + > > +/* > > + * Insert svc in the RunQ according to EDF: vcpus with smaller deadlin= es > > + * goes first. > > + */ > > +static void > > +__runq_insert(const struct scheduler *ops, struct rt_vcpu *svc) > > +{ > > + struct rt_private *prv =3D RT_PRIV(ops); > > + struct list_head *runq =3D RUNQ(ops); > Oh, BTW, George, what do you think about these? The case, I mean. Since now they're static inlines, I've been telling Meng to turn the function names lower case. This is, of course, a minor thing, but since we're saying the are not major issues... :-) > Overall, the code was pretty clean, and easy for me to read -- very much= =20 > like credit1 and credit2, so thanks. :-) >=20 Yep, indeed! Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-uP7YMLFxbI0WjhkXHkRx 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 iEYEABECAAYFAlQOy2kACgkQk4XaBE3IOsRiYQCcCdLujDUEP2szTIAkU5qyHgX6 528An1oX0VOTnK/p6HuLcq8EmdM2iJ9G =AfcX -----END PGP SIGNATURE----- --=-uP7YMLFxbI0WjhkXHkRx-- --===============3780750344285410455== 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 --===============3780750344285410455==--