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)