On Tue, 2016-09-20 at 14:32 +0100, George Dunlap wrote:
> On 17/08/16 18:18, Dario Faggioli wrote:
> > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> > @@ -1771,9 +1771,18 @@ csched_schedule(
> >       *   cpu and steal it.
> >       */
> >  
> > -    /* If we have schedule rate limiting enabled, check to see
> > -     * how long we've run for. */
> > -    if ( !tasklet_work_scheduled
> > +    /*
> > +     * If we have schedule rate limiting enabled, check to see
> > +     * how long we've run for.
> > +     *
> > +     * If scurr is yielding, however, we don't let rate limiting
> > kick in.
> > +     * In fact, it may be the case that scurr is about to spin,
> > and there's
> > +     * no point forcing it to do so until rate limiting expires.
> > +     *
> > +     * While there, take the chance for clearing the yield flag at
> > once.
> > +     */
> > +    if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_YIELD, &scurr-
> > >flags)
> 
> It looks like YIELD is implemented by putting it lower in the
> runqueue
> in __runqueue_insert().  But here you're clearing the flag before the
> insert happens -- won't this effectively disable yield() for credit1?
> 
Yes, I think you're right... I'm not sure how I thought it would work.
:-O

Thanks for pointing this out, will fix in v2.

> > diff --git a/xen/common/sched_credit2.c
> > b/xen/common/sched_credit2.c
> > index 569174b..c8e0ee7 100644
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -2267,36 +2267,40 @@ runq_candidate(struct csched2_runqueue_data
> > *rqd,
> > [...]
> This looks good, but the code re-organization probably goes better in
> the previous patch.  Since you're re-sending anyway, would you mind
> moving it there?
> 
> I'm not sure the credit2 yield-ratelimit needs to be in a separate
> patch; since you're implementing yield in credit2 from scratch you
> could
> just implement it all in one go.  But since you have a patch for
> credit1
> anyway, I think whichever way is fine.
> 
Ok, yes, I'm moving all the Credit2 bits from this patch to the one
that is actually implementing yield in Credit2 from scratch, and
leaving this as the patch that makes Credit1 stop ratelimiting upon
yield.

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)