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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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
> > @@ -171,6 +171,11 @@ integer_param("sched_credit2_migrate_resist",
> > opt_migrate_resist);
> >  #define c2r(_ops, _cpu)     (CSCHED2_PRIV(_ops)->runq_map[(_cpu)])
> >  /* CPU to runqueue struct macro */
> >  #define RQD(_ops, _cpu)     (&CSCHED2_PRIV(_ops)->rqd[c2r(_ops,
> > _cpu)])
> > +/* Find the max of time slice */
> > +#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. :-)

> > @@ -1709,17 +1767,22 @@ csched2_runtime(const struct scheduler
> > *ops, int cpu, struct csched2_vcpu *snext
> >           * at a different rate. */
> >          time = c2t(rqd, rt_credit, snext);
> > 
> > -        /* Check limits */
> > -        if ( time < CSCHED2_MIN_TIMER )
> > -        {
> > -            time = CSCHED2_MIN_TIMER;
> > -            SCHED_STAT_CRANK(runtime_min_timer);
> > -        }
> > -        else if ( time > CSCHED2_MAX_TIMER )
> > +        /* Check limits.
> > +         * time > ratelimit --> time > MIN.
> > +         */
> > +        if ( time > CSCHED2_MAX_TIMER )
> >          {
> > +
> >              time = CSCHED2_MAX_TIMER;
> >              SCHED_STAT_CRANK(runtime_max_timer);
> >          }
> > +        else
> > +        {
> > +            time = MAX_TSLICE(MAX_TSLICE(CSCHED2_MIN_TIMER,
> > +                                          MICROSECS(prv-
> > >ratelimit_us)), time);
> > +            SCHED_STAT_CRANK(time);
> > +        }
> > +
> >      }
> 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
>       * 1) Run until snext's credit will be 0
>       * 2) But if someone is waiting, run until snext's credit is
> equal
>       * to his
> -     * 3) But never run longer than MAX_TIMER or shorter than
> MIN_TIMER.
> +     * 3) But never run longer than MAX_TIMER or shorter than
> MIN_TIMER
> +     * 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;
 }

> > @@ -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.

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: signature.asc
Description: This is a digitally signed message part

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