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

["Lv, Hui" <hui.lv@xxxxxxxxx>]

George, 

Any comments for this? 

Is it the right "warning" way?


> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, January 02, 2012 4:19 PM
> To: Lv, Hui
> Cc: Ian.Campbell@xxxxxxxxxx; George.Dunlap@xxxxxxxxxxxxx;
> raistlin@xxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] xen/sched_credit: Use delay to control scheduling
> frequency
> 
> >>> On 26.12.11 at 09:46, Hui Lv <hui.lv@xxxxxxxxx> wrote:
> > Some modifications for this patch.
> > 1.Based on George's proposal, added a ratelimit_us element to
> csched_priv to
> > constrain prv->ratelimit_us <= 1000* prv->tslice_ms in csched_init.
> 
> I do not see why you need a per-scheduler-instance variable for
> issuing the warning and correcting the value - one warning (during
> the first initialization) is entirely sufficient. (Otoh the already
> existing
> tslice_ms field is pointless currently too, as it never gets changed
> after being initialized from sched_credit_tslice_ms - George?)
> 
> Jan
> 
> > 2.Move the definition of sched_ratelimit_us to xen/sched.h
> > 3.Remove the redundant "else" structure
> >
> > See if you have any comment for such changes.
> >
> >
> > This patch can improve Xen performance:
> > 1. Basically, the "delay method" can achieve 11% overall performance
> boost
> > for SPECvirt than original credit scheduler.
> > 2. We have tried 1ms delay and 10ms delay, there is no big difference
> > between these two configurations. (1ms is enough to achieve a good
> > performance)
> > 3. We have compared different load level response time/latency (low,
> high,
> > peak), "delay method" didn't bring very much response time increase.
> > 4. 1ms delay can reduce 30% context switch at peak performance, where
> > produces the benefits. (int sched_ratelimit_us = 1000 is the
> recommended
> > setting)
> >
> > Signed-off-by: Hui Lv <hui.lv@xxxxxxxxx>
> > Signed-off-by: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> >
> > diff -r a4bff36780a3 -r 18f40e1d0274 xen/common/sched_credit.c
> > --- a/xen/common/sched_credit.c     Fri Dec 16 18:46:27 2011 +0000
> > +++ b/xen/common/sched_credit.c     Mon Dec 26 03:44:39 2011 -0500
> > @@ -172,6 +172,7 @@ struct csched_private {
> >      uint32_t credit;
> >      int credit_balance;
> >      uint32_t runq_sort;
> > +    unsigned ratelimit_us;
> >      /* Period of master and tick in milliseconds */
> >      unsigned tslice_ms, tick_period_us, ticks_per_tslice;
> >      unsigned credits_per_tslice;
> > @@ -1297,10 +1298,15 @@ csched_schedule(
> >      struct csched_private *prv = CSCHED_PRIV(ops);
> >      struct csched_vcpu *snext;
> >      struct task_slice ret;
> > +    s_time_t runtime, tslice;
> >
> >      CSCHED_STAT_CRANK(schedule);
> >      CSCHED_VCPU_CHECK(current);
> >
> > +    runtime = now - current->runstate.state_entry_time;
> > +    if ( runtime < 0 ) /* Does this ever happen? */
> > +        runtime = 0;
> > +
> >      if ( !is_idle_vcpu(scurr->vcpu) )
> >      {
> >          /* Update credits of a non-idle VCPU. */
> > @@ -1313,6 +1319,35 @@ csched_schedule(
> >          scurr->pri = CSCHED_PRI_IDLE;
> >      }
> >
> > +    /* Choices, choices:
> > +     * - If we have a tasklet, we need to run the idle vcpu no
> matter what.
> > +     * - If sched rate limiting is in effect, and the current vcpu
> has
> > +     *   run for less than that amount of time, continue the current
> one,
> > +     *   but with a shorter timeslice and return it immediately
> > +     * - Otherwise, chose the one with the highest priority (which
> may
> > +     *   be the one currently running)
> > +     * - If the currently running one is TS_OVER, see if there
> > +     *   is a higher priority one waiting on the runqueue of another
> > +     *   cpu and steal it.
> > +     */
> > +
> > +    /* If we have schedule rate limiting enabled, check to see
> > +     * how long we've run for. */
> > +    if ( !tasklet_work_scheduled
> > +         && prv->ratelimit_us
> > +         && vcpu_runnable(current)
> > +         && !is_idle_vcpu(current)
> > +         && runtime < MICROSECS(prv->ratelimit_us) )
> > +    {
> > +        snext = scurr;
> > +        snext->start_time += now;
> > +        perfc_incr(delay_ms);
> > +        tslice = MICROSECS(prv->ratelimit_us);
> > +        ret.migrated = 0;
> > +        goto out;
> > +    }
> > +    tslice = MILLISECS(prv->tslice_ms);
> > +
> >      /*
> >       * Select next runnable local VCPU (ie top of local runq)
> >       */
> > @@ -1367,11 +1402,12 @@ csched_schedule(
> >      if ( !is_idle_vcpu(snext->vcpu) )
> >          snext->start_time += now;
> >
> > +out:
> >      /*
> >       * Return task to run next...
> >       */
> >      ret.time = (is_idle_vcpu(snext->vcpu) ?
> > -                -1 : MILLISECS(prv->tslice_ms));
> > +                -1 : tslice);
> >      ret.task = snext->vcpu;
> >
> >      CSCHED_VCPU_CHECK(ret.task);
> > @@ -1533,6 +1569,15 @@ csched_init(struct scheduler *ops)
> >      prv->tick_period_us = prv->tslice_ms * 1000 / prv-
> >ticks_per_tslice;
> >      prv->credits_per_tslice = CSCHED_CREDITS_PER_MSEC * prv-
> >tslice_ms;
> >
> > +    if ( MICROSECS(sched_ratelimit_us) >
> MILLISECS(sched_credit_tslice_ms) )
> > +    {
> > +        printk("WARNING: sched_ratelimit_us >"
> > +               "sched_credit_tslice_ms is undefined\n"
> > +               "ratelimit_us is set to 1000 * tslice_ms forcely\n");
> > +        prv->ratelimit_us = 1000 * prv->tslice_ms;
> > +    }
> > +    else
> > +        prv->ratelimit_us = sched_ratelimit_us;
> >      return 0;
> >  }
> >
> > diff -r a4bff36780a3 -r 18f40e1d0274 xen/common/schedule.c
> > --- a/xen/common/schedule.c Fri Dec 16 18:46:27 2011 +0000
> > +++ b/xen/common/schedule.c Mon Dec 26 03:44:39 2011 -0500
> > @@ -47,6 +47,11 @@ string_param("sched", opt_sched);
> >  bool_t sched_smt_power_savings = 0;
> >  boolean_param("sched_smt_power_savings", sched_smt_power_savings);
> >
> > +/* Default scheduling rate limit: 1ms
> > + * The behavior when sched_ratelimit_us is greater than
> > sched_credit_tslice_ms is undefined
> > + * */
> > +int sched_ratelimit_us = 1000;
> > +integer_param("sched_ratelimit_us", sched_ratelimit_us);
> >  /* Various timer handlers. */
> >  static void s_timer_fn(void *unused);
> >  static void vcpu_periodic_timer_fn(void *data);
> > diff -r a4bff36780a3 -r 18f40e1d0274 xen/include/xen/perfc_defn.h
> > --- a/xen/include/xen/perfc_defn.h  Fri Dec 16 18:46:27 2011
> +0000
> > +++ b/xen/include/xen/perfc_defn.h  Mon Dec 26 03:44:39 2011 -
> 0500
> > @@ -16,6 +16,7 @@ PERFCOUNTER(sched_irq,              "sch
> >  PERFCOUNTER(sched_run,              "sched: runs through scheduler")
> >  PERFCOUNTER(sched_ctx,              "sched: context switches")
> >
> > +PERFCOUNTER(delay_ms,               "csched: delay")
> >  PERFCOUNTER(vcpu_check,             "csched: vcpu_check")
> >  PERFCOUNTER(schedule,               "csched: schedule")
> >  PERFCOUNTER(acct_run,               "csched: acct_run")
> > diff -r a4bff36780a3 -r 18f40e1d0274 xen/include/xen/sched-if.h
> > --- a/xen/include/xen/sched-if.h    Fri Dec 16 18:46:27 2011 +0000
> > +++ b/xen/include/xen/sched-if.h    Mon Dec 26 03:44:39 2011 -0500
> > @@ -16,6 +16,11 @@ extern struct cpupool *cpupool0;
> >  /* cpus currently in no cpupool */
> >  extern cpumask_t cpupool_free_cpus;
> >
> > +/* Scheduler generic parameters
> > + * */
> > +extern int sched_ratelimit_us;
> > +
> > +
> >  /*
> >   * In order to allow a scheduler to remap the lock->cpu mapping,
> >   * we have a per-cpu pointer, along with a pre-allocated set of
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel