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

[Meng Xu <xumengpanda@xxxxxxxxx>]

Hi George and Dario,

2014-09-09 5:42 GMT-04:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
> 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.
>
> That is not an hot path anyway, so either approach would be fine in that
> case. For all the other occurrences, the while{} approach is an absolute
> win-win, IMO.
>
>> If so, since division and multiplication are so
>> expensive, it might make more sense to make this a while() loop:
>>
>>   while (now - svc_cur_deadline > 0 )
>>   {
>>    svc->cur_deadline += svc->period;
>>    count++;
>>   }
>>   if ( count ) {
>>    svc->cur_budget = svc->budget;
>>    [tracing code]
>>   }
>>
>> 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
>
> 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]
>   }
>
> Or even just the do {} while (and below), and have the check at the call
> sites (as George is also saying below).

I see the point and will change them for the next version. Thank you
very much! :-)

>
>> I probably wouldn't  make this a precondition of going in.
>>
> No, I'm not strict about this either, we can do it later. I don't think
> it's a big or a too disruptive change, though. :-)
>
>> > +
>> > +        /* TRACE */
>> > +        {
>> > +            struct {
>> > +                unsigned dom:16,vcpu:16;
>> > +                unsigned cur_budget_lo, cur_budget_hi;
>> > +            } d;
>> > +            d.dom = svc->vcpu->domain->domain_id;
>> > +            d.vcpu = svc->vcpu->vcpu_id;
>> > +            d.cur_budget_lo = (unsigned) svc->cur_budget;
>> > +            d.cur_budget_hi = (unsigned) (svc->cur_budget >> 32);
>> > +            trace_var(TRC_RT_BUDGET_REPLENISH, 1,
>> > +                      sizeof(d),
>> > +                      (unsigned char *) &d);
>> > +        }
>> > +
>> > +        return;
>> > +    }
>> > +}
>> > +
>> > +static inline void
>> > +__runq_remove(struct rt_vcpu *svc)
>> > +{
>> > +    if ( __vcpu_on_runq(svc) )
>> > +        list_del_init(&svc->runq_elem);
>> > +}
>> > +
>> > +/*
>> > + * Insert svc in the RunQ according to EDF: vcpus with smaller deadlines
>> > + * goes first.
>> > + */
>> > +static void
>> > +__runq_insert(const struct scheduler *ops, struct rt_vcpu *svc)
>> > +{
>> > +    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... :-)
>
>> Overall, the code was pretty clean, and easy for me to read -- very much
>> like credit1 and credit2, so thanks. :-)
>>
> Yep, indeed!

Yes, it is. :-)


Thank you very much for your helpful comments and advice! I will
incorporate them in the next version.

Best,

Meng
-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

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