Re: [Xen-devel] [PATCH v3 1/2] xen: credit2: implement yield()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 30/09/16 15:21, Dario Faggioli wrote:
> When a vcpu explicitly yields it is usually giving
> us an advice of "let someone else run and come back
> to me in a bit."
> 
> Credit2 isn't, so far, doing anything when a vcpu
> yields, which means an yield is basically a NOP (well,
> actually, it's pure overhead, as it causes the scheduler
> kick in, but the result is --at least 99% of the time--
> that the very same vcpu that yielded continues to run).
> 
> With this patch, when a vcpu yields, we go and try
> picking the next vcpu on the runqueue that can run on
> the pcpu where the yielding vcpu is running. Of course,
> if we don't find any other vcpu that wants and can run
> there, the yielding vcpu will continue.
> 
> Also, add an yield performance counter, and fix the
> style of a couple of comments.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

Thanks!

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Anshul Makkar <anshul.makkar@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> Changes from v2:
>  * implement yield as a "switch", rather than as a "knob", as suggested
>    during review.
> 
> Changes from v1:
>  * add _us to the parameter name, as suggested during review;
>  * get rid of the minimum value for the yield bias;
>  * apply the idle bias via subtraction of credits to the yielding vcpu, rather
>    than via addition to all the others;
>  * merge the Credit2 bits of what was patch 7 here, as suggested during 
> review.
> ---
>  xen/common/sched_credit2.c   |   54 
> +++++++++++++++++++++++++++++++-----------
>  xen/common/schedule.c        |    2 ++
>  xen/include/xen/perfc_defn.h |    1 +
>  3 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 72e31b5..c0646e9 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -181,7 +181,13 @@
>   */
>  #define __CSFLAG_runq_migrate_request 3
>  #define CSFLAG_runq_migrate_request (1<<__CSFLAG_runq_migrate_request)
> -
> +/*
> + * CSFLAG_vcpu_yield: this vcpu was running, and has called vcpu_yield(). The
> + * scheduler is invoked to see if we can give the cpu to someone else, and
> + * get back to the yielding vcpu in a while.
> + */
> +#define __CSFLAG_vcpu_yield 4
> +#define CSFLAG_vcpu_yield (1<<__CSFLAG_vcpu_yield)
>  
>  static unsigned int __read_mostly opt_migrate_resist = 500;
>  integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> @@ -1431,6 +1437,14 @@ out:
>  }
>  
>  static void
> +csched2_vcpu_yield(const struct scheduler *ops, struct vcpu *v)
> +{
> +    struct csched2_vcpu * const svc = CSCHED2_VCPU(v);
> +
> +    __set_bit(__CSFLAG_vcpu_yield, &svc->flags);
> +}
> +
> +static void
>  csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
>  {
>      struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
> @@ -2250,26 +2264,31 @@ runq_candidate(struct csched2_runqueue_data *rqd,
>      struct list_head *iter;
>      struct csched2_vcpu *snext = NULL;
>      struct csched2_private *prv = CSCHED2_PRIV(per_cpu(scheduler, cpu));
> +    bool yield = __test_and_clear_bit(__CSFLAG_vcpu_yield, &scurr->flags);
>  
>      *skipped = 0;
>  
> -    /* Default to current if runnable, idle otherwise */
> -    if ( vcpu_runnable(scurr->vcpu) )
> -        snext = scurr;
> -    else
> -        snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> -
>      /*
>       * Return the current vcpu if it has executed for less than ratelimit.
>       * Adjuststment for the selected vcpu's credit and decision
>       * for how long it will run will be taken in csched2_runtime.
> +     *
> +     * Note that, if scurr is yielding, we don't let rate limiting kick in.
> +     * In fact, it may be the case that scurr is about to spin, and there's
> +     * no point forcing it to do so until rate limiting expires.
>       */
> -    if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
> +    if ( !yield && prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
>           vcpu_runnable(scurr->vcpu) &&
>           (now - scurr->vcpu->runstate.state_entry_time) <
>            MICROSECS(prv->ratelimit_us) )
>          return scurr;
>  
> +    /* Default to current if runnable, idle otherwise */
> +    if ( vcpu_runnable(scurr->vcpu) )
> +        snext = scurr;
> +    else
> +        snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> +
>      list_for_each( iter, &rqd->runq )
>      {
>          struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, 
> runq_elem);
> @@ -2293,8 +2312,10 @@ runq_candidate(struct csched2_runqueue_data *rqd,
>              continue;
>          }
>  
> -        /* If this is on a different processor, don't pull it unless
> -         * its credit is at least CSCHED2_MIGRATE_RESIST higher. */
> +        /*
> +         * If this is on a different processor, don't pull it unless
> +         * its credit is at least CSCHED2_MIGRATE_RESIST higher.
> +         */
>          if ( svc->vcpu->processor != cpu
>               && snext->credit + CSCHED2_MIGRATE_RESIST > svc->credit )
>          {
> @@ -2303,9 +2324,12 @@ runq_candidate(struct csched2_runqueue_data *rqd,
>              continue;
>          }
>  
> -        /* If the next one on the list has more credit than current
> -         * (or idle, if current is not runnable), choose it. */
> -        if ( svc->credit > snext->credit )
> +        /*
> +         * If the next one on the list has more credit than current
> +         * (or idle, if current is not runnable), or if current is
> +         * yielding, choose it.
> +         */
> +        if ( yield || svc->credit > snext->credit )
>              snext = svc;
>  
>          /* In any case, if we got this far, break. */
> @@ -2391,7 +2415,8 @@ csched2_schedule(
>       */
>      if ( tasklet_work_scheduled )
>      {
> -        trace_var(TRC_CSCHED2_SCHED_TASKLET, 1, 0,  NULL);
> +        __clear_bit(__CSFLAG_vcpu_yield, &scurr->flags);
> +        trace_var(TRC_CSCHED2_SCHED_TASKLET, 1, 0, NULL);
>          snext = CSCHED2_VCPU(idle_vcpu[cpu]);
>      }
>      else
> @@ -2975,6 +3000,7 @@ static const struct scheduler sched_credit2_def = {
>  
>      .sleep          = csched2_vcpu_sleep,
>      .wake           = csched2_vcpu_wake,
> +    .yield          = csched2_vcpu_yield,
>  
>      .adjust         = csched2_dom_cntl,
>      .adjust_global  = csched2_sys_cntl,
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 104d203..5b444c4 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -947,6 +947,8 @@ long vcpu_yield(void)
>      SCHED_OP(VCPU2OP(v), yield, v);
>      vcpu_schedule_unlock_irq(lock, v);
>  
> +    SCHED_STAT_CRANK(vcpu_yield);
> +
>      TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>      raise_softirq(SCHEDULE_SOFTIRQ);
>      return 0;
> diff --git a/xen/include/xen/perfc_defn.h b/xen/include/xen/perfc_defn.h
> index 4a835b8..900fddd 100644
> --- a/xen/include/xen/perfc_defn.h
> +++ b/xen/include/xen/perfc_defn.h
> @@ -23,6 +23,7 @@ PERFCOUNTER(vcpu_alloc,             "sched: vcpu_alloc")
>  PERFCOUNTER(vcpu_insert,            "sched: vcpu_insert")
>  PERFCOUNTER(vcpu_remove,            "sched: vcpu_remove")
>  PERFCOUNTER(vcpu_sleep,             "sched: vcpu_sleep")
> +PERFCOUNTER(vcpu_yield,             "sched: vcpu_yield")
>  PERFCOUNTER(vcpu_wake_running,      "sched: vcpu_wake_running")
>  PERFCOUNTER(vcpu_wake_onrunq,       "sched: vcpu_wake_onrunq")
>  PERFCOUNTER(vcpu_wake_runnable,     "sched: vcpu_wake_runnable")
> 


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