Re: [Xen-devel] [PATCH v2 1/4] xen: add real time scheduler rt

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Tue, Sep 9, 2014 at 10:42 AM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> On Mon, 2014-09-08 at 19:44 +0100, George Dunlap wrote:
>> On 09/07/2014 08:40 PM, Meng Xu wrote:
>
>> > +/*
>> > + * update deadline and budget when deadline is in the past,
>> > + * it need to be updated to the deadline of the current period
>> > + */
>> > +static void
>> > +rt_update_helper(s_time_t now, struct rt_vcpu *svc)
>> > +{
>> > +    s_time_t diff = now - svc->cur_deadline;
>> > +
>> > +    if ( diff >= 0 )
>> > +    {
>> > +        /* now can be later for several periods */
>> > +        long count = ( diff/svc->period ) + 1;
>> > +        svc->cur_deadline += count * svc->period;
>> > +        svc->cur_budget = svc->budget;
>>
>> In the common case, don't we expect diff/svc->period to be a small
>> number, like 0 or 1?
>>
> In general, yes. The only exception is when cur_deadline is set for the
> first time. In that case, now can be arbitrary large and cur_deadline
> will always be 0, so quite a few iterations may be required, possibly
> taking longer than the div and the mult.

Right, well we should be able to special-case zero.  Is there any
reason, if cur_deadline == 0, not to just set cur_deadline=now +
svc->period?  I can see a reason why after skipping several periods
you'd want the future periods "lined up with" previous periods.  But
is there a need to have all the periods lined up from the beginning of
time?

>> And similarly for the other 64-bit division Dario was asking about below?
>>
> Hehe, this is, I think, the third or fourth time I say I'd like this to
> be turned into a while! :-D

Well, if you've asked for it several times, we should probably make it
a precondition of going in then.

> If it were me doing this, I'd go for something like this:
>
>   static void
>   rt_update_helper(s_time_t now, struct rt_vcpu *svc)
>   {
>       if ( svc->cur_deadline > now )
>           return;
>
>       do
>           svc->cur_deadline += svc->period;
>       while ( svc->cur_deadline <= now );
>       svc->cur_budget = svc->budget;
>
>       [tracing]
>   }

Yes, that looks even cleaner. :-)

>> > +{
>> > +    struct rt_private *prv = RT_PRIV(ops);
>> > +    struct list_head *runq = RUNQ(ops);
>>
> Oh, BTW, George, what do you think about these? The case, I mean. Since
> now they're  static inlines, I've been telling Meng to turn the function
> names lower case.
>
> This is, of course, a minor thing, but since we're saying the are not
> major issues... :-)

Yes, static inlines need to be lower case.

 -George

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