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

[Tim Deegan <tim@xxxxxxx>]

At 11:46 +0000 on 26 Feb (1361879193), Jan Beulich wrote:
> >>> On 26.02.13 at 12:26, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> > On 02/25/2013 11:30 AM, Jan Beulich wrote:
> >>>>> On 25.02.13 at 12:12, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> >>> On 25/02/13 09:29, Jan Beulich wrote:
> >>>>>>> On 22.02.13 at 18:26, Dario Faggioli <dario.faggioli@xxxxxxxxxx> 
> >>>>>>> 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.

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