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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3007 bytes --]

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>

--- 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;
 }
 
 /*




[-- Attachment #2: sched-credit-residual.patch --]
[-- Type: text/plain, Size: 3065 bytes --]

credit: track residual from divisions done during accounting

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>

--- 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;
 }
 
 /*

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1166 bytes --]

On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
>
> @@ -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);

I may be missing something, but how can the assert ever be false, given
the assignment right before it?

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>>
>> @@ -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);
> 
> I may be missing something, but how can the assert ever be false, given
> the assignment right before it?

val being wider than credit, this checks that there was no truncation.

Jan

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1650 bytes --]

On Mon, 2013-02-25 at 09:29 +0000, Jan Beulich wrote:
> >>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> > On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
> >> --- a/xen/common/sched_credit.c
> >> +++ b/xen/common/sched_credit.c
> >>
> >> @@ -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);
> > 
> > I may be missing something, but how can the assert ever be false, given
> > the assignment right before it?
> 
> val being wider than credit, this checks that there was no truncation.
> 
Ok. At least I was right in the "missing something" part. :-) TBH, I
figured it had to be something related to that, but was failing to spot
it... Sorry for the noise and thanks for the explanation!

Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[David Vrabel]

On 25/02/13 09:29, Jan Beulich wrote:
>>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
>> On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
>>> --- a/xen/common/sched_credit.c
>>> +++ b/xen/common/sched_credit.c
>>>
>>> @@ -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);
>>
>> I may be missing something, but how can the assert ever be false, given
>> the assignment right before it?
> 
> val being wider than credit, this checks that there was no truncation.

ASSERT(val <= UINT_MAX);

Would be clearer.

David

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

[Jan Beulich]

>>> On 25.02.13 at 12:12, David Vrabel <david.vrabel@citrix.com> wrote:
> On 25/02/13 09:29, Jan Beulich wrote:
>>>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
>>> On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
>>>> --- a/xen/common/sched_credit.c
>>>> +++ b/xen/common/sched_credit.c
>>>>
>>>> @@ -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);
>>>
>>> I may be missing something, but how can the assert ever be false, given
>>> the assignment right before it?
>> 
>> val being wider than credit, this checks that there was no truncation.
> 
> ASSERT(val <= UINT_MAX);
> 
> Would be clearer.

A matter of taste perhaps...

Jan

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

[George Dunlap]

On 02/25/2013 11:30 AM, Jan Beulich wrote:
>>>> On 25.02.13 at 12:12, David Vrabel <david.vrabel@citrix.com> wrote:
>> On 25/02/13 09:29, Jan Beulich wrote:
>>>>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
>>>> On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
>>>>> --- a/xen/common/sched_credit.c
>>>>> +++ b/xen/common/sched_credit.c
>>>>>
>>>>> @@ -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);
>>>>
>>>> I may be missing something, but how can the assert ever be false, given
>>>> the assignment right before it?
>>>
>>> val being wider than credit, this checks that there was no truncation.
>>
>> ASSERT(val <= UINT_MAX);
>>
>> Would be clearer.
>
> A matter of taste perhaps...

I have a taste for coders having to keep as little state in their head 
as possible. :-)  Comparing to UINT_MAX prompts the coder specifically 
to think about the size of the variables.

  -George

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

[Jan Beulich]

>>> On 26.02.13 at 12:26, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/25/2013 11:30 AM, Jan Beulich wrote:
>>>>> On 25.02.13 at 12:12, David Vrabel <david.vrabel@citrix.com> wrote:
>>> On 25/02/13 09:29, Jan Beulich wrote:
>>>>>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
>>>>> On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
>>>>>> --- a/xen/common/sched_credit.c
>>>>>> +++ b/xen/common/sched_credit.c
>>>>>>
>>>>>> @@ -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);
>>>>>
>>>>> I may be missing something, but how can the assert ever be false, given
>>>>> the assignment right before it?
>>>>
>>>> val being wider than credit, this checks that there was no truncation.
>>>
>>> ASSERT(val <= UINT_MAX);
>>>
>>> Would be clearer.
>>
>> A matter of taste perhaps...
> 
> I have a taste for coders having to keep as little state in their head 
> as possible. :-)  Comparing to UINT_MAX prompts the coder specifically 
> to think about the size of the variables.

Okay, assuming this is the only thing you dislike, I'll change it then
and re-submit.

But for the record - using UINT_MAX here will get things out of
sync the moment the type of "credits" changes, whereas with
the way I had coded it this would be taken care of implicitly.

Jan

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

[Tim Deegan]

At 11:46 +0000 on 26 Feb (1361879193), Jan Beulich wrote:
> >>> On 26.02.13 at 12:26, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> > On 02/25/2013 11:30 AM, Jan Beulich wrote:
> >>>>> On 25.02.13 at 12:12, David Vrabel <david.vrabel@citrix.com> wrote:
> >>> On 25/02/13 09:29, Jan Beulich wrote:
> >>>>>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
> >>>>> On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
> >>>>>> --- a/xen/common/sched_credit.c
> >>>>>> +++ b/xen/common/sched_credit.c
> >>>>>>
> >>>>>> @@ -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);
> >>>>>
> >>>>> I may be missing something, but how can the assert ever be false, given
> >>>>> the assignment right before it?
> >>>>
> >>>> val being wider than credit, this checks that there was no truncation.
> >>>
> >>> ASSERT(val <= UINT_MAX);
> >>>
> >>> Would be clearer.
> >>
> >> A matter of taste perhaps...
> > 
> > I have a taste for coders having to keep as little state in their head 
> > as possible. :-)  Comparing to UINT_MAX prompts the coder specifically 
> > to think about the size of the variables.
> 
> Okay, assuming this is the only thing you dislike, I'll change it then
> and re-submit.
> 
> But for the record - using UINT_MAX here will get things out of
> sync the moment the type of "credits" changes, whereas with
> the way I had coded it this would be taken care of implicitly.

How about ASSERT(((typeof credits) val) == val) before the assignment?

Tim.

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

[David Vrabel]

On 26/02/13 11:52, Tim Deegan wrote:
> At 11:46 +0000 on 26 Feb (1361879193), Jan Beulich wrote:
>>>>> On 26.02.13 at 12:26, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> On 02/25/2013 11:30 AM, Jan Beulich wrote:
>>>>>>> On 25.02.13 at 12:12, David Vrabel <david.vrabel@citrix.com> wrote:
>>>>> On 25/02/13 09:29, Jan Beulich wrote:
>>>>>>>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@citrix.com> wrote:
>>>>>>> On Mon, 2013-02-18 at 12:37 +0000, Jan Beulich wrote:
>>>>>>>> +    ASSERT(credits == val);
>>>>>>>
>>>>>>> I may be missing something, but how can the assert ever be false, given
>>>>>>> the assignment right before it?
>>>>>>
>>>>>> val being wider than credit, this checks that there was no truncation.
>>>>>
>>>>> ASSERT(val <= UINT_MAX);
>>>>>
>>>>> Would be clearer.
>>>>
>>>> A matter of taste perhaps...
>>>
>>> I have a taste for coders having to keep as little state in their head 
>>> as possible. :-)  Comparing to UINT_MAX prompts the coder specifically 
>>> to think about the size of the variables.
>>
>> Okay, assuming this is the only thing you dislike, I'll change it then
>> and re-submit.
>>
>> But for the record - using UINT_MAX here will get things out of
>> sync the moment the type of "credits" changes, whereas with
>> the way I had coded it this would be taken care of implicitly.
> 
> How about ASSERT(((typeof credits) val) == val) before the assignment?

FWIW, this works for me.

David

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

[Ian Campbell]

On Tue, 2013-02-26 at 12:00 +0000, David Vrabel wrote:
> On 26/02/13 11:52, Tim Deegan wrote:
> > How about ASSERT(((typeof credits) val) == val) before the assignment?

Why not just
	ASSERT(credits == val); /* Ensure we haven't truncated val */

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

[Jan Beulich]

>>> On 26.02.13 at 13:21, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2013-02-26 at 12:00 +0000, David Vrabel wrote:
>> On 26/02/13 11:52, Tim Deegan wrote:
>> > How about ASSERT(((typeof credits) val) == val) before the assignment?
> 
> Why not just
> 	ASSERT(credits == val); /* Ensure we haven't truncated val */

I.e. just want em to add a comment? That's certainly fine with me.

Jan

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

[George Dunlap]

On 02/26/2013 12:21 PM, Ian Campbell wrote:
> On Tue, 2013-02-26 at 12:00 +0000, David Vrabel wrote:
>> On 26/02/13 11:52, Tim Deegan wrote:
>>> How about ASSERT(((typeof credits) val) == val) before the assignment?
>
> Why not just
> 	ASSERT(credits == val); /* Ensure we haven't truncated val */

I prefer this one to having "typeof credits" in the ASSERT.  The main 
thing is just to minimize the amount of effort a programmer has to 
expend trying to figure out what's going on, so he can spend it on 
something else. :-)

  -George

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

[Ian Campbell]

On Tue, 2013-02-26 at 12:51 +0000, Jan Beulich wrote:
> >>> On 26.02.13 at 13:21, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Tue, 2013-02-26 at 12:00 +0000, David Vrabel wrote:
> >> On 26/02/13 11:52, Tim Deegan wrote:
> >> > How about ASSERT(((typeof credits) val) == val) before the assignment?
> > 
> > Why not just
> > 	ASSERT(credits == val); /* Ensure we haven't truncated val */
> 
> I.e. just want em to add a comment?

That's right s/val/credits/ depending on what the ASSERT is really doing
(I didn't go back and check)

Ian.

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

[Tim Deegan]

At 12:54 +0000 on 26 Feb (1361883253), George Dunlap wrote:
> On 02/26/2013 12:21 PM, Ian Campbell wrote:
> >On Tue, 2013-02-26 at 12:00 +0000, David Vrabel wrote:
> >>On 26/02/13 11:52, Tim Deegan wrote:
> >>>How about ASSERT(((typeof credits) val) == val) before the assignment?
> >
> >Why not just
> >	ASSERT(credits == val); /* Ensure we haven't truncated val */
> 
> I prefer this one to having "typeof credits" in the ASSERT.  The main 
> thing is just to minimize the amount of effort a programmer has to 
> expend trying to figure out what's going on, so he can spend it on 
> something else. :-)

Quite.  It occurred to me afterwards that we could have a FITS_IN()
macro for that purpose.  Or, as mentioned, a comment. :)

Tim.

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

[George Dunlap]

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;
>   }
>
>   /*
>
>
>

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

[Jan Beulich]

>>> On 26.02.13 at 16:00, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> 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.

That's fine with me of course.

May I take the above as a pre-ack to a patch modified accordingly?

Jan

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

[George Dunlap]

On 02/26/2013 03:07 PM, Jan Beulich wrote:
>>>> On 26.02.13 at 16:00, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> 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.
>
> That's fine with me of course.
>
> May I take the above as a pre-ack to a patch modified accordingly?

Yes, I suppose that's fine. :-)

  -George