Re: [Xen-devel] [PATCH v2] xen:rtds:Fix bug in budget accounting

[Meng Xu <xumengpanda@xxxxxxxxx>]

>> I don't really like updating the last_start in burn_budget().
>>
>> last_start is the recent starting time of the VCPU. It is to record
>> the starting time of a currently running VCPU. So it should be
>> updated
>> only when the VCPU is scheduled.
>>
> Yes, the name suggest it is what you write. But how is it used? It's
> used to make sure that we burn the proper amount of budget.
>
> And it looks to me that, to serve this purpose, it needs to be updated
> to NOW() (or something equivalent to that), when we touch cur_budget,
> or we risk inconsistencies like, well, like the one we're discussing
> here! :-)
>
> OTOH, is there a reason for which we need to track the time at which a
> vcpu started to execute? If yes, I don't see that.
>
> So, I agree that last_start may look a bit misleading, but that's what
> it is... let's change its name if you think it's worth. Note, however,
> that apart from this case when the budget is actually updated within
> the timer handler, because of a deadline miss, the value contained in
> that variable would actually _really_ be the time at which it started
> executing, even if you update it in burn_budget() and in
> rt_update_deadline().
>
> Perhaps, add a comment about this whole thing, in rt_update_deadline().
>
>> When burn_budget() is called, the VCPU can be scheduled or
>> de-scheduled. When the VCPU is de-scheduled, we don't have to update
>> the last_start, since it will be updated in the next time when the
>> VCPU is scheduled.
>>
>> If we update last_start in burn_budget(), it won't cause incorrect
>> functionality. It just wastes time on one assignment operation.
>>
> Well, let's not bother about one assignment, ok? :-P
>
> When you deschedule a vcpu, whatever it is in last_start is inaccurate
> and useless anyway.
>
> That being said, until burn_budget() is called only from 1 place, it's
> also true that we don't need to bother updating last_start inside it.
> But as soon as we'll add a call to burn_budget() somewhere else, if we
> forgot to update last_start, we'll have problems.
>
> For now, I don't foresee us adding such calls, but doing things in the
> way that leaves the least possibilities to cause bugs is in general a
> good thing. :-)
>

Sure. Then I will send a separate patch to update last_start when we
update cur_budget. It is just to avoid subtle bug in the future and
won't fix the bug we discussed here.
So I think it may be better to be a separate patch.

>
>> >  - Why cur_deadline-period, and not NOW() (well, actually, now)?
>> >    Well, in the scenario above, and in all the others I can think
>> > of,
>> >    it would be almost the same. And yet, I'd prefer using now, for
>> >    clarity and consistency.
>>
>> Hmm, it depends on how we account for the budget.
>> Suppose the VCPU keeps running.
>> the start of next period is t = 10 and the next period ends at t =
>> 20.
>> rt_update_deadline is invoked at t = 10.1.
>>
> Yes, I know...
>
>> If last_start = 10, which is cur_deadline - period, we will account
>> for the 0.1 time unit as the budget consumed by the VCPU.
>> If last_start = 10.1, which is now, we will not account for the 0.1
>> time unit.
>>
> ...but by using 10, you're saying that you are sure that the vcpu has
> been running since 10? Because, if you do that, that's what you are
> charging the vcpu for.
>
> Note that this is a genuine question that I'm _deliberately_ dumping on
> you! :-P :-P

Ah-ha. The rt_update_deadline is called at the current deadline, which
is also the start of next period. So "now" is actually a value close
to 10. Either last_start = now, or last_start = cur_deadline - period
does not make much difference. now - (cur_deadline - period) is the
delay of the repl_timer_handler(), which should not be much.

 If the vcpu is not running at 10, the last_start = now still has no
real meaning. However, last_start will be updated later when it is
scheduled. So the VCPU's parameters will still be correctly updated.

I'm using last_start = now then to make code more consistent.

>
>> Since we always account for the time spent in hypervisor into the
>> budget of currently running VCPU, I chose last_start = cur_deadline -
>> period.
>> I'm ok for last_start = now, considering the consistency. So your
>> call. :-)
>>
> If the answer to the above question is 'yes', I guess I'm fine with
> that too.
>
> Actually, you can use cur_deadline, if you put the assignment _before_
> updating the deadline (together with a comment about why we use that
> instead of NOW()).

Ah-ha. Yes. That is better than cur_deadline - period. However, I
chose to use last_start = now for the next version. ;-)

Meng
-- 
-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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