Re: [PATCH] xen/sched_credit: Use delay to control scheduling frequency

[George Dunlap <george.dunlap@citrix.com>]

On Tue, 2011-12-20 at 09:07 +0000, Ian Campbell wrote:
>  
> > +/* Scheduler generic parameters
> > +*/
> > +extern int sched_ratelimit_us;
> 
> If this is generic then it seems like xen/sched.h is the right place for
> it.

Yes, this should be declared in xen/include/xen/sched.h.

> Is it really generic though if only the credit scheduler applies it?

The concept is generic enough that I think saying, "This is the control,
not all schedulers implement it" makes sense.  credit2 *may* in the
future have use for such a control; at which point it would be a bit
asinine to have two switches, named "credit_sched_ratelimit_us" and
"credit2_sched_ratelimit_us".

> > +    /* If we have schedule rate limiting enabled, check to see
> > +     * how long we've run for. */
> > +    if ( !tasklet_work_scheduled
> > +         && sched_ratelimit_us
> > +         && vcpu_runnable(current)
> > +         && !is_idle_vcpu(current)
> > +         && runtime < MICROSECS(sched_ratelimit_us) )
> > +    {
> > +        snext = scurr;
> > +        snext->start_time += now;
> > +        perfc_incr(delay_ms);
> > +        tslice = MICROSECS(sched_ratelimit_us);
> > +        ret.migrated = 0;
> > +        goto out;
> > +    }
> > +    else
> > +    {
> 
> This is the same comment as the next block below, does it really apply
> here too?
> 
> Since the previous if block ends with a goto the else clause is a bit
> redundant.

Yeah, probably better w/o the else.

> > +/* Default scheduling rate limit: 1ms 
> > + * It's not recommended to set this value bigger than "sched_credit_tslice_ms" 
> > + * otherwise, something weired may happen

Hui, "something weird may happen" is fine for informal speech, but for
comments we tend to use more formal language.  I might say something
like this:

"The behavior when sched_ratelimit_us is greater than
sched_credit_tslice_ms is undefined."

 -George