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

[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 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!

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)

Attachment: signature.asc
Description: This is a digitally signed message part

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