From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 1/1] ratelimit: Implement rate limit for credit2 scheduler Rate limit assures that a vcpu will execute for a minimum amount of time before being put at the back of a queue or being preempted by higher priority thread. Date: Tue, 26 Jul 2016 12:50:48 +0200 Message-ID: <1469530248.32102.37.camel@citrix.com> References: <1468844544-2229-1-git-send-email-anshul.makkar@citrix.com> <1469183791.13039.288.camel@citrix.com> <579623E8.50100@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5782170485309365306==" Return-path: In-Reply-To: <579623E8.50100@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: anshul makkar , xen-devel@lists.xen.org Cc: george.dunlap@eu.citrix.com List-Id: xen-devel@lists.xenproject.org --===============5782170485309365306== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-wdKOQ1pX4DAQ/94zHnPT" --=-wdKOQ1pX4DAQ/94zHnPT Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2016-07-25 at 15:36 +0100, anshul makkar wrote: > On 22/07/16 11:36, Dario Faggioli wrote: > >=C2=A0 > > This version is an improvement, but it looks to me that you've > > missed a > > few of the review comments to v1. > > Sorry about that. !! > NP. It's indeed something quite important... but it happens from time to time. :-) > @@ -1775,9 +1829,13 @@ runq_candidate(struct > > > csched2_runqueue_data > > > *rqd, > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > >=20 > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* If the= next one on the list has more credit than > > > current > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* (or idle, if= current is not runnable), choose it. */ > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* (or idle, if= current is not runnable) and current one > > > has > > > already > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* executed for= more than ratelimit. choose it. > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Control has = reached here means that current vcpu has > > > executed > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* ratelimit_us= or ratelimit is off, so chose the next > > > one. > > > +=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=A0if ( svc-= >credit > snext->credit ) > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0snext =3D svc; > > > +=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=A0snext =3D svc; > > >=20 > > Both me and George agreed that changing the comment like this is > > not > > helping much and should not be done. > Though, I find the extended comment useful, but if you don't agree I=C2= =A0 > will remove it v3. >=20 So, I just wanted to reply to this, then I'll go looking at v3. Things like this are, up to a rather high extent, a matter of taste. And I see why you think you're adding useful information, and I'm not saying that is completely untrue. So, I'm explaining why I'm asking you to remove this, and then I'll go looking at v3. :-) =C2=A0As said, it's not that what you add is completely useless. But that particular piece of code the comment is referring to has nothing to do with ratelimiting, and I thus just don't think it is useful to have its comment talking about ratelimit. Actually, it may be confusing. In fact, if I'm reading the code trying to focus on something that is not ratelimit, a comment like this risks driving my focus away, and forcing me to think what ratelimiting has to do with this. In fact, there are two other possible reasons why we may not get to this point of the loop, and each of them: =C2=A0- has its own comment where it is actually checked for/enforced; =C2=A0- does not have its own comment repeated here (e.g., like, for =C2=A0 =C2=A0affinity, "Control has reached here means that svc can run on = this=C2=A0 =C2=A0 =C2=A0cpu."). So, the ratelimit related aspects should be put in a comment close to the code that checks and enforces ratelimiting, which is there already, in your patch, where they will be: =C2=A01. helpful to people trying to understand rateliminting; =C2=A02. out of the way of people reasoning on anything else than=C2=A0 =C2=A0 =C2=A0 ratelimiting. Thanks and 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) --=-wdKOQ1pX4DAQ/94zHnPT 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 iQIcBAABCAAGBQJXl0CKAAoJEBZCeImluHPuxxkP/A+Wi+1UOEQBuAJqFO9oVNEE 4CEVRoCJ18GcBPbm3yAmDEskC1215k/kTT3VQK3O+PgRjbvRax9l+rcuQ/qO8mdr VxWM2N6I3KAA6VdMkosrM1g09nyT57KJEPC15X4mL+0gM+rINuKauJ8lQd3IWz6g N0qkNvei0c4CnFIvuk46+I6Gg7hC4PkZrpk053zVpJBzQ7gx8hbzX3w1qeQ1HiLl kGh5alsMdgmOqlKRPRP85eyLPR7oZ7Ui7To01L+McN3DseS40dPHY2jrhVcZX+mX fpIOJ04TzFaDiFnaXwRGue6tnAsuKDnuEZ8DxnUkLb8G3V+qi32E71KllYLLUT57 aLfy1/mQLMYf0RhR/SukBP7DCG9Lv8cZfY+bE4Tyr1uMCoDfqpdXBGmrEP1Yu2hs 6sJmBKoFKklYJR4W0k6g1PMn4xFbXix1/C/9flJXcCQo0VDXK1BcWpGNvuqrinx+ N2UJseGXvg7op7EPRFgdLBUrepiyR2XHq0pFBynoCzNbLWzaSV6dSh0vKjGQIZSz tURM1W43rmDxKOjWW3HUodoeYk6FLaZZnhT2ZkLuluQ3X94e+vhLBMQe2u3b1OB/ hRj/H/QM4lMyNDqmTXrl5W7rEzWCpdlI/MsAdHgcaa1FNTx5qr9YgjrWCxTPxAHg RY7yzsR6yC3PBsmm47ny =u7gT -----END PGP SIGNATURE----- --=-wdKOQ1pX4DAQ/94zHnPT-- --===============5782170485309365306== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============5782170485309365306==--