From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [RFC PATCH v2 2/7] Fixed formatting and misleading comments/variables. Added comments and renamed variables to accurately reflect modern terminology Date: Fri, 11 Jul 2014 05:59:40 +0200 Message-ID: <1405051180.29306.208.camel@Solace> References: <1404939348-4926-1-git-send-email-josh.whitehead@dornerworks.com> <1404939348-4926-3-git-send-email-josh.whitehead@dornerworks.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5298132645800369456==" Return-path: In-Reply-To: <1404939348-4926-3-git-send-email-josh.whitehead@dornerworks.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Josh Whitehead Cc: Ian Campbell , Stefano Stabellini , George Dunlap , Ian Jackson , Robert VanVossen , Xen-devel , Nathan Studer List-Id: xen-devel@lists.xenproject.org --===============5298132645800369456== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-cIQ7jJvmPBRt6dB1FGHH" --=-cIQ7jJvmPBRt6dB1FGHH Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On mer, 2014-07-09 at 16:55 -0400, Josh Whitehead wrote: > Due to the age of the scheduler there were many incorrect/misleading comm= ents > 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 modif= cations > involve simply changing a variable "d" to a "v" or a "dom" to "vcpu" so t= hat the > comments and variable names are accurate to what's being used. >=20 Indeed! > A few other name changes were also made, the most significant being the c= hange > from "slice" to "budget" to better reflect modern terminology used in rea= l-time > algorithms such as CBS and deferrable server. >=20 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) > =20 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)) > =20 > #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)) =3D=3D (!!(b))) > +#define EQ(_A, _B) ((!!(_A)) =3D=3D (!!(_B))) > =20 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; > =20 > /* Parameters for EDF */ > - s_time_t period; /* =3D relative deadline */ > - s_time_t slice; /* =3D worst case execution time */ > + s_time_t period; /* =3D 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... =20 > #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_pri= v)) > +#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]) > =20 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) > =20 > -#define DIV_UP(x,y) (((x) + (y) - 1) / y) > +#define DIV_UP(_X, _Y) (((_X) + (_Y) - 1) / _Y) > =20 Does this makes much difference? =20 > /* > - * 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 sim= ply > + * 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 sim= ply > * absol. deadline - period. > */=20 > -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 v= cpu *v) > } > =20 > /* > - * Adds a domain to the queue of processes which have started their curr= ent > + * Adds a vcpu to the queue of processes which have started their curren= t > same here. > * period and are runnable (i.e. not blocked, dieing,...). The first ele= ment > * on this list is running on the processor, if the list is empty the id= le > * task will run. As we are implementing EDF, this list is sorted by dea= dlines. > */=20 > @@ -187,19 +207,19 @@ static void *sedf_alloc_vdata(const struct schedule= r *ops, struct vcpu *v, void > =20 > inf->vcpu =3D v; > =20 > - inf->deadl_abs =3D 0; > - inf->status =3D SEDF_ASLEEP; > + inf->deadl_abs =3D 0; > + inf->status =3D SEDF_ASLEEP; > =20 > if (v->domain->domain_id =3D=3D 0) > { > - /* Domain 0, needs a slice to boot the machine */ > - inf->period =3D DEFAULT_PERIOD; > - inf->slice =3D DEFAULT_SLICE; > + /* Domain 0, needs a budget to boot the machine */ > + inf->period =3D DEFAULT_PERIOD; > + inf->budget =3D DEFAULT_BUDGET; > } > else > { > - inf->period =3D DEFAULT_PERIOD; > - inf->slice =3D 0; > + inf->period =3D DEFAULT_PERIOD; > + inf->budget =3D 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_sched= uled) > @@ -401,18 +421,18 @@ static struct task_slice sedf_do_schedule( > int cpu =3D smp_processor_id(); > struct list_head *runq =3D RUNQ(cpu); > struct list_head *waitq =3D WAITQ(cpu); > - struct sedf_vcpu_info *inf =3D EDOM_INFO(current); > + struct sedf_vcpu_info *inf =3D SEDF_VCPU(current); > struct sedf_vcpu_info *runinf, *waitinf; > struct task_slice ret; > =20 > SCHED_STAT_CRANK(schedule); > =20 > - /* 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 domai= n > - * up at all.=20 > 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? =20 > -static void sedf_wake(const struct scheduler *ops, struct vcpu *d) > +/* > + * This function wakes up a vcpu, i.e. moves them into the appropriate q= ueue > + * > + * 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________ ...=20 > + * > + * - 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 =3D NOW(); > - struct sedf_vcpu_info* inf =3D EDOM_INFO(d); > + struct sedf_vcpu_info* inf =3D SEDF_VCPU(v); > =20 > - if ( unlikely(is_idle_vcpu(d)) ) > + if ( unlikely(is_idle_vcpu(v)) ) > return; > =20 > - if ( unlikely(__task_on_queue(d)) ) > + if ( unlikely(__task_on_queue(v)) ) > return; > =20 > - ASSERT(!sedf_runnable(d)); > + ASSERT(!sedf_runnable(v)); > inf->status &=3D ~SEDF_ASLEEP; > =20 > if ( unlikely(inf->deadl_abs =3D=3D 0) ) > { > /* Initial setup of the deadline */ > - inf->deadl_abs =3D now + inf->slice; > + inf->deadl_abs =3D now + inf->budget; > } > =20 > #ifdef SEDF_STATS=20 > @@ -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 --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-cIQ7jJvmPBRt6dB1FGHH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlO/YSwACgkQk4XaBE3IOsQqjwCgqbsYhV0hh5aXoE0bpAN5QUfR fbUAnRPC4LeN1NZ/FaR+QVKtv3BqVAWR =BV4/ -----END PGP SIGNATURE----- --=-cIQ7jJvmPBRt6dB1FGHH-- --===============5298132645800369456== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5298132645800369456==--