Re: [Xen-devel] [PATCH 2/2] credit2: Reset until the front of the runqueue is positive

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.03.13 at 15:14, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> Under normal circumstances, snext->credit should never be less than
> -CSCHED_MIN_TIMER.  However, under some circumstances, a vcpu with low
> credits may be allowed to run long enough that its credits are
> actually less than -CSCHED_CREDIT_INIT.
> 
> (Instances have been observed, for example, where a vcpu with 200us of
> credit was allowed to run for 11ms, giving it -10.8ms of credit.  Thus
> it was still negative even after the reset.)
> 
> If this is the case for snext, we simply want to keep moving everyone
> up until it is in the black again.  This fair because none of the
> other vcpus want to run at the moment.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> ---
>  xen/common/sched_credit2.c |   81 
> +++++++++++++++++++++++++++-----------------
>  1 file changed, 49 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 5bf5ebc..7265d5b 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -588,41 +588,58 @@ no_tickle:
>  /*
>   * Credit-related code
>   */
> -static void reset_credit(const struct scheduler *ops, int cpu, s_time_t now)
> +static void reset_credit(const struct scheduler *ops, int cpu, s_time_t 
> now,
> +                         struct csched_vcpu *snext)
>  {
>      struct csched_runqueue_data *rqd = RQD(ops, cpu);
>      struct list_head *iter;
>  
> -    list_for_each( iter, &rqd->svc )
> -    {
> -        struct csched_vcpu * svc = list_entry(iter, struct csched_vcpu, 
> rqd_elem);
> -
> -        int start_credit;
> -
> -        BUG_ON( is_idle_vcpu(svc->vcpu) );
> -        BUG_ON( svc->rqd != rqd );
> -
> -        start_credit = svc->credit;
> -
> -        /* "Clip" credits to max carryover */
> -        if ( svc->credit > CSCHED_CARRYOVER_MAX )
> -            svc->credit = CSCHED_CARRYOVER_MAX;
> -        /* And add INIT */
> -        svc->credit += CSCHED_CREDIT_INIT;
> -        svc->start_time = now;
> -
> -        /* TRACE */ {
> -            struct {
> -                unsigned dom:16,vcpu:16;
> -                unsigned credit_start, credit_end;
> -            } d;
> -            d.dom = svc->vcpu->domain->domain_id;
> -            d.vcpu = svc->vcpu->vcpu_id;
> -            d.credit_start = start_credit;
> -            d.credit_end = svc->credit;
> -            trace_var(TRC_CSCHED2_CREDIT_RESET, 1,
> -                      sizeof(d),
> -                      (unsigned char *)&d);
> +    /*
> +     * Under normal circumstances, snext->credit should never be less
> +     * than -CSCHED_MIN_TIMER.  However, under some circumstances, a
> +     * vcpu with low credits may be allowed to run long enough that
> +     * its credits are actually less than -CSCHED_CREDIT_INIT.
> +     * (Instances have been observed, for example, where a vcpu with
> +     * 200us of credit was allowed to run for 11ms, giving it -10.8ms
> +     * of credit.  Thus it was still negative even after the reset.)
> +     *
> +     * If this is the case for snext, we simply want to keep moving
> +     * everyone up until it is in the black again.  This fair because
> +     * none of the other vcpus want to run at the moment.
> +     */
> +    while (snext->credit <= CSCHED_CREDIT_RESET ) {

So how long can this loop last? Can't you get away with a loop
altogether, considering that you only add CSCHED_CREDIT_INIT
inside the loop?

Also, I hope there is some sort of guarantee that snext gets
updated by the loop in the first place.

Jan

> +        list_for_each( iter, &rqd->svc )
> +        {
> +            struct csched_vcpu * svc;
> +            int start_credit;
> +
> +            svc = list_entry(iter, struct csched_vcpu, rqd_elem);
> +
> +            BUG_ON( is_idle_vcpu(svc->vcpu) );
> +            BUG_ON( svc->rqd != rqd );
> +
> +            start_credit = svc->credit;
> +
> +            /* "Clip" credits to max carryover */
> +            if ( svc->credit > CSCHED_CARRYOVER_MAX )
> +                svc->credit = CSCHED_CARRYOVER_MAX;
> +            /* And add INIT */
> +            svc->credit += CSCHED_CREDIT_INIT;
> +            svc->start_time = now;
> +
> +            /* TRACE */ {
> +                struct {
> +                    unsigned dom:16,vcpu:16;
> +                    unsigned credit_start, credit_end;
> +                } d;
> +                d.dom = svc->vcpu->domain->domain_id;
> +                d.vcpu = svc->vcpu->vcpu_id;
> +                d.credit_start = start_credit;
> +                d.credit_end = svc->credit;
> +                trace_var(TRC_CSCHED2_CREDIT_RESET, 1,
> +                          sizeof(d),
> +                          (unsigned char *)&d);
> +            }
>          }
>      }
>  
> @@ -1731,7 +1748,7 @@ csched_schedule(
>          /* Check for the reset condition */
>          if ( snext->credit <= CSCHED_CREDIT_RESET )
>          {
> -            reset_credit(ops, cpu, now);
> +            reset_credit(ops, cpu, now, snext);
>              balance_load(ops, cpu, now);
>          }
>  
> -- 
> 1.7.9.5



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