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.

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3216 bytes --]

On Mon, 2016-07-25 at 15:36 +0100, anshul makkar wrote:
> On 22/07/16 11:36, Dario Faggioli wrote:
> > 
> > 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,
> > >           }
> > > 
> > >           /* If the next one on the list has more credit than
> > > current
> > > -         * (or idle, if current is not runnable), choose it. */
> > > +         * (or idle, if current is not runnable) and current one
> > > has
> > > already
> > > +         * executed for more than ratelimit. choose it.
> > > +         * Control has reached here means that current vcpu has
> > > executed >
> > > +         * ratelimit_us or ratelimit is off, so chose the next
> > > one.
> > > +         */
> > >           if ( svc->credit > snext->credit )
> > > -            snext = svc;
> > > +                snext = svc;
> > > 
> > 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 
> will remove it v3.
> 
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. :-)  As 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:
 - has its own comment where it is actually checked for/enforced;
 - does not have its own comment repeated here (e.g., like, for
   affinity, "Control has reached here means that svc can run on this 
   cpu.").

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:
 1. helpful to people trying to understand rateliminting;
 2. out of the way of people reasoning on anything else than 
    ratelimiting.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel