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 -- <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)