From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <george.dunlap@citrix.com>
Subject: Re: [PATCH] xen/sched_credit: Use delay to control
 scheduling frequency
Date: Tue, 20 Dec 2011 10:21:08 +0000
Message-ID: <1324376468.2143.148.camel@elijah>
References: <114495d1ff947737518b.1324332781@wsm-ep-n0>
	<1324372056.23729.8.camel@zakaz.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <1324372056.23729.8.camel@zakaz.uk.xensource.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>, "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Hui Lv <hui.lv@intel.com>
List-Id: xen-devel@lists.xenproject.org

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