Re: [Xen-devel] [PATCH] credi2-ratelimit: Implement rate limit for credit2 scheduler

[anshul makkar <anshul.makkar@xxxxxxxxxx>]

On 13/07/16 09:53, Dario Faggioli wrote:
> On Tue, 2016-07-12 at 17:16 +0100, George Dunlap wrote:
> > On Wed, Jul 6, 2016 at 6:33 PM, Makkar anshul.makkar@xxxxxxxxxx
> > <Anshul> wrote:
> > > --- a/xen/common/sched_credit2.c
> > > +++ b/xen/common/sched_credit2.c
> > > +#define MAX_TSLICE(t1, t2)  \
> > > +                   ({ typeof (t1) _t1 = (t1); \
> > > +                      typeof (t1) _t2 = (t2); \
> > > +                      _t1 > _t2 ? _t1 < 0 ? 0 : _t1 : _t2 < 0 ? 0
> > > : _t2; })
> > Hiding the zero-checking behind this seems a bit strange to me.  I
> > think if the algorithm is properly laid out, we probably don't need
> > this at all (see my proposed alternate below).
> I think it's more "overflow-avoidance" than "zero-checking", and as I
> said, that's probably better done by means of subtraction(s).
>
> In any case, I agree that, if we don't need it, that's even better. :-)
Agreed. I am going to remove it. Its not needed with modified algo.
>
> > This is all really confusing.  What about something like this (also
> > attached)?  The basic idea is to calculate min_time, then go through
> > the normal algorithm, then clip it once at the end.
> Yes, this matches my idea and my comment, and I think the code provided
> by George makes sense. Only one thing:
>
> > @@ -1675,9 +1711,19 @@ csched2_runtime(const struct scheduler *ops,
> > int cpu, struct csched2_vcpu *snext
> > +     * or your the ratelimit time.
> >        */
> > +    /* Calculate mintime */
> > +    min_time = CSCHED2_MIN_TIMER;
> > +    if ( prv->ratelimit_us ) {
> > +        s_time_t ratelimit_min = snext->vcpu-
> > > runstate.state_entry_time +
> > +            MICROSECS(prv->ratelimit_us) - now;
> Here snext can indeed be someone which was running already (e.g., we're
> choosing current again), in which case runstate.state_entry-time-now
> would indeed tell us how long it's actually been running, and the
> formula (coupled with the if below) is correct.
>
> But it also can be someone which is runnable (e.g., we're choosing
> someone from the runqueue and preempting current), in which case
> runstate.state_entry_time tells when it became runnable, and
> state_entry_time-now is how long it's been runnable, which is not what
> we want here.
>
> In think, in such a case, we want ratelimit_min to just be equal to
> prv->ratelimit_us. So, maybe, something like this:
>
>   /* Caluclate mintime */
>   min_time = CSCHED2_MIN_TIMER;
>   if ( prv->ratelimit_us )
>   {
>       s_time_t ratelimit_min = prv->ratelimit_us;
>       if ( snext->vcpu->is_running )     // XXX or is it better snext == 
> curr_on_cpu(cpu)
>           ratelimit_min = snext->vcpu->runstate.state_entry_time +
>                           MICROSECS(prv->ratelimit_us) - now;
>       if ( ratelimit_min > min_time )
>           min_time = ratelimit_min;
>   }
Agreed.
> > > @@ -2353,6 +2431,8 @@ csched2_init(struct scheduler *ops)
> > >           prv->runq_map[i] = -1;
> > >           prv->rqd[i].id = -1;
> > >       }
> > > +    /* initialize ratelimit */
> > > +    prv->ratelimit_us = 1000 * CSCHED2_MIN_TIMER;
> > Is there any reason this isn't using sched_ratelimit_us (the
> > hypervisor command-line option, which the credit scheduler is using)?
> Yeah, I guess just using that is the best thing to start with.
Agreed.
> Regards,
> Dario
Will post the reworked patch.

Anshul


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