Jan Beulich
2013-Feb-18 12:37 UTC
[PATCH] 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;
}
/*
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Dario Faggioli
2013-Feb-22 17:26 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Feb-25 09:29 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
>>> 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
Dario Faggioli
2013-Feb-25 10:45 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
David Vrabel
2013-Feb-25 11:12 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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
Jan Beulich
2013-Feb-25 11:30 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
>>> 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
George Dunlap
2013-Feb-26 11:26 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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
Jan Beulich
2013-Feb-26 11:46 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
>>> 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
Tim Deegan
2013-Feb-26 11:52 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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.
David Vrabel
2013-Feb-26 12:00 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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
Ian Campbell
2013-Feb-26 12:21 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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 */
Jan Beulich
2013-Feb-26 12:51 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
>>> 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
George Dunlap
2013-Feb-26 12:54 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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
Ian Campbell
2013-Feb-26 13:10 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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.
Tim Deegan
2013-Feb-26 13:46 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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.
George Dunlap
2013-Feb-26 15:00 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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; > } > > /* > > >
Jan Beulich
2013-Feb-26 15:07 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
>>> 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
George Dunlap
2013-Feb-26 15:11 UTC
Re: [PATCH] credit: track residual from divisions done during accounting
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