Re: [Xen-devel] [RFC PATCH v2 2/7] Fixed formatting and misleading comments/variables. Added comments and renamed variables to accurately reflect modern terminology

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mer, 2014-07-09 at 16:55 -0400, Josh Whitehead wrote:
> Due to the age of the scheduler there were many incorrect/misleading comments
> and variable names, the bulk of which centered around the fact that "VCPU" and
> "Domain" used to be synonymous.  Therefore a large portion of these 
> modifcations
> involve simply changing a variable "d" to a "v" or a "dom" to "vcpu" so that 
> the
> comments and variable names are accurate to what's being used.
> 
Indeed!

> A few other name changes were also made, the most significant being the change
> from "slice" to "budget" to better reflect modern terminology used in 
> real-time
> algorithms such as CBS and deferrable server.
> 
BTW, I said that, at libxl and libxc level (but I mostly was referring
to libxl) it's fine to keep slice as the name of this param, at least
for now.

I confirm that that's what I think, for libxl. Right here, deep down
inside the hypervisor, I agree about the slice-->budget transition.

> --- a/xen/common/sched_sedf.c
> +++ b/xen/common/sched_sedf.c

> @@ -28,14 +49,13 @@
>  #define SEDF_ASLEEP (16)
>  
This one here alone, and define to 16, is kind of ugly, but I shall say
that when reviewing the previous patch...

>  #define DEFAULT_PERIOD (MILLISECS(20))
> -#define DEFAULT_SLICE (MILLISECS(10))
> +#define DEFAULT_BUDGET (MILLISECS(10))
>  
>  #define PERIOD_MAX MILLISECS(10000) /* 10s  */
>  #define PERIOD_MIN (MICROSECS(10))  /* 10us */
> -#define SLICE_MIN (MICROSECS(5))    /*  5us */
> +#define BUDGET_MIN (MICROSECS(5))    /*  5us */
>
How about we make the name of the scheduler part of the name of these
macros. I.e., SEDF_DEFAULT_BUDGET and SEDF_MIN_BUDGET (or
SEDF_BUDGET_MIN, but I like it worse) ?

It's certainly not necessary, but it's what other schedulers do, and it
may come handy when grep-ping, etc.

> -#define IMPLY(a, b) (!(a) || (b))
> -#define EQ(a, b) ((!!(a)) == (!!(b)))
> +#define EQ(_A, _B) ((!!(_A)) == (!!(_B)))
>  
Do we really need to keep this? Not such a big deal I guess, but I truly
tryly truly dislike it! :-(

>  struct sedf_dom_info {
> @@ -52,16 +72,16 @@ struct sedf_vcpu_info {
>      struct list_head list;
>   
>      /* Parameters for EDF */
> -    s_time_t  period;  /* = relative deadline */
> -    s_time_t  slice;   /* = worst case execution time */
> +    s_time_t  period;  /* = Server scheduling period */
>
I'd say "VCPUs scheduling period". I know 'Server' is the correct term,
but I suspect it will just be confusing people...
 
>  #define SEDF_PRIV(_ops) \
>      ((struct sedf_priv_info *)((_ops)->sched_data))
> -#define EDOM_INFO(d)   ((struct sedf_vcpu_info *)((d)->sched_priv))
> -#define CPU_INFO(cpu)  \
> -    ((struct sedf_cpu_info *)per_cpu(schedule_data, cpu).sched_priv)
> -#define LIST(d)        (&EDOM_INFO(d)->list)
> -#define RUNQ(cpu)      (&CPU_INFO(cpu)->runnableq)
> -#define WAITQ(cpu)     (&CPU_INFO(cpu)->waitq)
> -#define IDLETASK(cpu)  (idle_vcpu[cpu])
> +#define SEDF_VCPU(_vcpu)   ((struct sedf_vcpu_info *)((_vcpu)->sched_priv))
> +#define SEDF_PCPU(_cpu)  \
> +    ((struct sedf_cpu_info *)per_cpu(schedule_data, _cpu).sched_priv)
> +#define LIST(_vcpu)        (&SEDF_VCPU(_vcpu)->list)
> +#define RUNQ(_cpu)      (&SEDF_PCPU(_cpu)->runnableq)
> +#define WAITQ(_cpu)     (&SEDF_PCPU(_cpu)->waitq)
> +#define IDLETASK(_cpu)  (idle_vcpu[_cpu])
>  
While at this, can you put down a comment on what LIST, RUNQ and WAITQ
are? RUNQ is pretty obvious, the others, not so much.

>  #define PERIOD_BEGIN(inf) ((inf)->deadl_abs - (inf)->period)
>  
> -#define DIV_UP(x,y) (((x) + (y) - 1) / y)
> +#define DIV_UP(_X, _Y) (((_X) + (_Y) - 1) / _Y)
>  
Does this makes much difference?
 
>  /*
> - * Adds a domain to the queue of processes which wait for the beginning of 
> the
> - * next period; this list is therefore sortet by this time, which is simply
> + * Adds a vcpu to the queue of processes which wait for the beginning of the
                               of vcpus    ?

> + * next period; this list is therefore sorted by this time, which is simply
>   * absol. deadline - period.
>   */ 
> -DOMAIN_COMPARER(waitq, list, PERIOD_BEGIN(d1), PERIOD_BEGIN(d2));
> +VCPU_COMPARER(waitq, list, PERIOD_BEGIN(v1), PERIOD_BEGIN(v2));
>  static inline void __add_to_waitqueue_sort(struct vcpu *v)
>  {
>      ASSERT(!__task_on_queue(v));
> @@ -156,12 +176,12 @@ static inline void __add_to_waitqueue_sort(struct vcpu 
> *v)
>  }
>  
>  /*
> - * Adds a domain to the queue of processes which have started their current
> + * Adds a vcpu to the queue of processes which have started their current
>
same here.

>   * period and are runnable (i.e. not blocked, dieing,...). The first element
>   * on this list is running on the processor, if the list is empty the idle
>   * task will run. As we are implementing EDF, this list is sorted by 
> deadlines.
>   */ 

> @@ -187,19 +207,19 @@ static void *sedf_alloc_vdata(const struct scheduler 
> *ops, struct vcpu *v, void
>  
>      inf->vcpu = v;
>  
> -    inf->deadl_abs   = 0;
> -    inf->status      = SEDF_ASLEEP;
> +    inf->deadl_abs  = 0;
> +    inf->status     = SEDF_ASLEEP;
>  
>      if (v->domain->domain_id == 0)
>      {
> -        /* Domain 0, needs a slice to boot the machine */
> -        inf->period      = DEFAULT_PERIOD;
> -        inf->slice       = DEFAULT_SLICE;
> +        /* Domain 0, needs a budget to boot the machine */
> +        inf->period = DEFAULT_PERIOD;
> +        inf->budget = DEFAULT_BUDGET;
>      }
>      else
>      {
> -        inf->period      = DEFAULT_PERIOD;
> -        inf->slice       = 0;
> +        inf->period = DEFAULT_PERIOD;
> +        inf->budget = 0;
>      }
>
So, by default, Dom0 gets DEFAULT_BUDGET, other domains get 0. Does this
mean they can't even start executing until someone changes that
manually, by modifying the parameters and giving them some budget?


>  static struct task_slice sedf_do_schedule(
>      const struct scheduler *ops, s_time_t now, bool_t tasklet_work_scheduled)
> @@ -401,18 +421,18 @@ static struct task_slice sedf_do_schedule(
>      int                   cpu      = smp_processor_id();
>      struct list_head     *runq     = RUNQ(cpu);
>      struct list_head     *waitq    = WAITQ(cpu);
> -    struct sedf_vcpu_info *inf     = EDOM_INFO(current);
> +    struct sedf_vcpu_info *inf     = SEDF_VCPU(current);
>      struct sedf_vcpu_info *runinf, *waitinf;
>      struct task_slice      ret;
>  
>      SCHED_STAT_CRANK(schedule);
>  
> -    /* Idle tasks don't need any of the following stuf */
> +    /* Idle tasks don't need any of the following stuff */
          Idle vcpu

It's again not a big deal, but it's more consistent.

>  /*
> - * This function wakes up a domain, i.e. moves them into the waitqueue
> - * things to mention are: admission control is taking place nowhere at
> - * the moment, so we can't be sure, whether it is safe to wake the domain
> - * up at all. 
>
I am all in favor of the removal of this complex and broken logic for
handling wake-ups.

However, the chunk of commit above --which is pretty much unrelated to
wake-ups-- seems something useful to have somewhere, perhaps at the
beginning of the file.

Anyway, whether you want to use this chunk or not, what I have in mind
is it'd be worth mentioning that the scheduler does not (by design)
perform any admission control at all.

What do you think?
 
> -static void sedf_wake(const struct scheduler *ops, struct vcpu *d)
> +/*
> + * This function wakes up a vcpu, i.e. moves them into the appropriate queue
> + *
> + * When a blocked vcpu unblocks, it is allowed to start execution at
> + * the beginning of the next complete period
> + * (D..deadline, R..running, B..blocking/sleeping, U..unblocking/waking up
> + *
> + * DRRB_____D__U_____DRRRRR___D________ ... 
> + *
> + * - This causes the vcpu to miss a period (and a deadlline)
> + * - Doesn't disturb the schedule at all
> + * - Deadlines keep occuring isochronous
> + */
>
Oh, ok. So, between the various special cases, you decided to pick
number 1, calle "Very conservative". If it were me, I would probably
have picked the one called "Part 2b" (i.e., the one that resets the
deadline).

Well, I don't think this matter much anyway, since you're changing it in
the next patch, I'm sure. :-)

> +static void sedf_wake(const struct scheduler *ops, struct vcpu *v)
>  {
>      s_time_t              now = NOW();
> -    struct sedf_vcpu_info* inf = EDOM_INFO(d);
> +    struct sedf_vcpu_info* inf = SEDF_VCPU(v);
>  
> -    if ( unlikely(is_idle_vcpu(d)) )
> +    if ( unlikely(is_idle_vcpu(v)) )
>          return;
>     
> -    if ( unlikely(__task_on_queue(d)) )
> +    if ( unlikely(__task_on_queue(v)) )
>          return;
>  
> -    ASSERT(!sedf_runnable(d));
> +    ASSERT(!sedf_runnable(v));
>      inf->status &= ~SEDF_ASLEEP;
>   
>      if ( unlikely(inf->deadl_abs == 0) )
>      {
>          /* Initial setup of the deadline */
> -        inf->deadl_abs = now + inf->slice;
> +        inf->deadl_abs = now + inf->budget;
>      }
>    
>  #ifdef SEDF_STATS 
> @@ -627,14 +595,14 @@ static void sedf_wake(const struct scheduler *ops, 
> struct vcpu *d)
>      }
>      else
>      {
> -            /* Long unblocking */
> +        /* Long unblocking, someone is going to miss their deadline. */
>
Sorry? Someone who?

>          inf->long_block_tot++;
>      }

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