Re: [PATCH] credit: track residual from divisions done during accounting

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

On 02/18/2013 12:37 PM, Jan Beulich wrote:
> This should help with under-accounting of vCPU-s running for extremly
> short periods of time, but becoming runnable again at a high frequency.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

The changes to credit1 look good, and I'm fine with a patch having those 
(and a commented ASSERT) go in.

Credit2 I'm not so happy with, because the names "t2c" and "c2t" imply 
(at least to me) that they are only converting, not changing anything; 
particularly in the way that t2c is called.  At the moment everything 
will work fine, but it's just laying a trap for someone in the future. :-)

I've got a patch in my queue dealing with this section already -- why 
don't you apply just the sched_credit.c part of the patch, and I'll take 
the credit2 part of your patch and rework it so it satisfies me.

  -George



>
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -19,6 +19,7 @@
>   #include <xen/sched-if.h>
>   #include <xen/softirq.h>
>   #include <asm/atomic.h>
> +#include <asm/div64.h>
>   #include <xen/errno.h>
>   #include <xen/keyhandler.h>
>   #include <xen/trace.h>
> @@ -136,6 +137,7 @@ struct csched_vcpu {
>       struct csched_dom *sdom;
>       struct vcpu *vcpu;
>       atomic_t credit;
> +    unsigned int residual;
>       s_time_t start_time;   /* When we were scheduled (used for credit) */
>       uint16_t flags;
>       int16_t pri;
> @@ -242,6 +244,7 @@ __runq_remove(struct csched_vcpu *svc)
>   static void burn_credits(struct csched_vcpu *svc, s_time_t now)
>   {
>       s_time_t delta;
> +    uint64_t val;
>       unsigned int credits;
>
>       /* Assert svc is current */
> @@ -250,7 +253,10 @@ static void burn_credits(struct csched_v
>       if ( (delta = now - svc->start_time) <= 0 )
>           return;
>
> -    credits = (delta*CSCHED_CREDITS_PER_MSEC + MILLISECS(1)/2) / MILLISECS(1);
> +    val = delta * CSCHED_CREDITS_PER_MSEC + svc->residual;
> +    svc->residual = do_div(val, MILLISECS(1));
> +    credits = val;
> +    ASSERT(credits == val);
>       atomic_sub(credits, &svc->credit);
>       svc->start_time += (credits * MILLISECS(1)) / CSCHED_CREDITS_PER_MSEC;
>   }
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -21,7 +21,7 @@
>   #include <xen/perfc.h>
>   #include <xen/sched-if.h>
>   #include <xen/softirq.h>
> -#include <asm/atomic.h>
> +#include <asm/div64.h>
>   #include <xen/errno.h>
>   #include <xen/trace.h>
>   #include <xen/cpu.h>
> @@ -205,7 +205,7 @@ struct csched_runqueue_data {
>
>       struct list_head runq; /* Ordered list of runnable vms */
>       struct list_head svc;  /* List of all vcpus assigned to this runqueue */
> -    int max_weight;
> +    unsigned int max_weight;
>
>       cpumask_t idle,        /* Currently idle */
>           tickled;           /* Another cpu in the queue is already targeted for this one */
> @@ -244,7 +244,8 @@ struct csched_vcpu {
>       struct csched_dom *sdom;
>       struct vcpu *vcpu;
>
> -    int weight;
> +    unsigned int weight;
> +    unsigned int residual;
>
>       int credit;
>       s_time_t start_time; /* When we were scheduled (used for credit) */
> @@ -275,12 +276,15 @@ struct csched_dom {
>    */
>   static s_time_t t2c(struct csched_runqueue_data *rqd, s_time_t time, struct csched_vcpu *svc)
>   {
> -    return time * rqd->max_weight / svc->weight;
> +    uint64_t val = time * rqd->max_weight + svc->residual;
> +
> +    svc->residual = do_div(val, svc->weight);
> +    return val;
>   }
>
>   static s_time_t c2t(struct csched_runqueue_data *rqd, s_time_t credit, struct csched_vcpu *svc)
>   {
> -    return credit * svc->weight / rqd->max_weight;
> +    return (credit * svc->weight - svc->residual) / rqd->max_weight;
>   }
>
>   /*
>
>
>