| Message ID | 1450385974-12732-2-git-send-email-jonathan.creekmore@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote: > Allow the schedulers to be independently enabled or disabled at > compile-time instead of just allowing the scheduler to be selected on > the command line. > Reading this quickly, that "instead" gave me a bit of an hard time. I'm not a native English speaker, and I'm sure it's me that am wrong, but for some reason the sentence made me think that the patch would somehow disallow specifying a scheduler during boot, in Xen's command line. Fact is, I don't think the phrase "instead of just allowing the scheduler to be selected on the command line." adds much information, and I'd just remove it. > To match existing behavior, all four schedulers are > compiled in by default, although the RTDS and ARINC653 are marked > EXPERIMENTAL to match their not currently supported status. > This may change shortly, but as of now, Credit2 is still experimental too. I think I'd still recommend enabling it in the help file (i.e., I'm ok with "If unsure, say Y"), but we certainly should mark it with "(EPERIMENTAL)". I don't know much on how kconfig works, so all the changes to the makefile, etc, I'm not able to review them properly. On the other hand, the code here below... > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -38,8 +38,8 @@ > #include <public/sched.h> > #include <xsm/xsm.h> > > -/* opt_sched: scheduler - default to credit */ > -static char __initdata opt_sched[10] = "credit"; > +/* opt_sched: scheduler - default to configured value */ > +static char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT; > string_param("sched", opt_sched); > > /* if sched_smt_power_savings is set, > @@ -65,10 +65,18 @@ DEFINE_PER_CPU(struct schedule_data, > schedule_data); > DEFINE_PER_CPU(struct scheduler *, scheduler); > > static const struct scheduler *schedulers[] = { > +#ifdef CONFIG_SCHED_CREDIT > &sched_credit_def, > +#endif > +#ifdef CONFIG_SCHED_CREDIT2 > &sched_credit2_def, > +#endif > +#ifdef CONFIG_SCHED_ARINC653 > &sched_arinc653_def, > +#endif > +#ifdef CONFIG_SCHED_RTDS > &sched_rtds_def, > +#endif > }; > ... can have, with the changelog changed as shown, my: Acked-by: Dario Faggioli <dario.faggioli@citrix.com> Regards, Daio
> On Dec 17, 2015, at 7:35 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > > On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote: >> Allow the schedulers to be independently enabled or disabled at >> compile-time instead of just allowing the scheduler to be selected on >> the command line. >> > Reading this quickly, that "instead" gave me a bit of an hard time. I'm > not a native English speaker, and I'm sure it's me that am wrong, but > for some reason the sentence made me think that the patch would somehow > disallow specifying a scheduler during boot, in Xen's command line. > > Fact is, I don't think the phrase "instead of just allowing the > scheduler to be selected on the command line." adds much information, > and I'd just remove it. Sorry for the confusion — I did not want to give the impression that this patch would remove that functionality. I can definitely remove that language from the patch. > >> To match existing behavior, all four schedulers are >> compiled in by default, although the RTDS and ARINC653 are marked >> EXPERIMENTAL to match their not currently supported status. >> > This may change shortly, but as of now, Credit2 is still experimental > too. I think I'd still recommend enabling it in the help file (i.e., > I'm ok with "If unsure, say Y"), but we certainly should mark it with > "(EPERIMENTAL)”. OK, I can mark that EXPERIMENTAL as well. I was under the impression that 4.7 was going to migrate Credit2 to being SUPPORTED, but I may have jumped the gun in the Kconfig. > I don't know much on how kconfig works, so all the changes to the > makefile, etc, I'm not able to review them properly. > > On the other hand, the code here below... > >> --- a/xen/common/schedule.c >> +++ b/xen/common/schedule.c >> @@ -38,8 +38,8 @@ >> #include <public/sched.h> >> #include <xsm/xsm.h> >> >> -/* opt_sched: scheduler - default to credit */ >> -static char __initdata opt_sched[10] = "credit"; >> +/* opt_sched: scheduler - default to configured value */ >> +static char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT; >> string_param("sched", opt_sched); >> >> /* if sched_smt_power_savings is set, >> @@ -65,10 +65,18 @@ DEFINE_PER_CPU(struct schedule_data, >> schedule_data); >> DEFINE_PER_CPU(struct scheduler *, scheduler); >> >> static const struct scheduler *schedulers[] = { >> +#ifdef CONFIG_SCHED_CREDIT >> &sched_credit_def, >> +#endif >> +#ifdef CONFIG_SCHED_CREDIT2 >> &sched_credit2_def, >> +#endif >> +#ifdef CONFIG_SCHED_ARINC653 >> &sched_arinc653_def, >> +#endif >> +#ifdef CONFIG_SCHED_RTDS >> &sched_rtds_def, >> +#endif >> }; >> > ... can have, with the changelog changed as shown, my: > > Acked-by: Dario Faggioli <dario.faggioli@citrix.com> > > Regards, > Daio > -- > <<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) >
On 17/12/2015 20:59, Jonathan Creekmore wrote: > Allow the schedulers to be independently enabled or disabled at > compile-time instead of just allowing the scheduler to be selected on > the command line. To match existing behavior, all four schedulers are > compiled in by default, although the RTDS and ARINC653 are marked > EXPERIMENTAL to match their not currently supported status. > > CC: George Dunlap <george.dunlap@eu.citrix.com> > CC: Dario Faggioli <dario.faggioli@citrix.com> > Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> With the suggestions Dario made, and one minor nit below, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > diff --git a/xen/common/Makefile b/xen/common/Makefile > index 8ab15ba..3a2026a 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -30,10 +30,10 @@ obj-y += rangeset.o > obj-y += radix-tree.o > obj-y += rbtree.o > obj-y += rcupdate.o > -obj-y += sched_credit.o > -obj-y += sched_credit2.o > -obj-y += sched_arinc653.o > -obj-y += sched_rt.o > +obj-$(CONFIG_SCHED_CREDIT) += sched_credit.o > +obj-$(CONFIG_SCHED_CREDIT2) += sched_credit2.o > +obj-$(CONFIG_SCHED_ARINC653) += sched_arinc653.o > +obj-$(CONFIG_SCHED_RTDS) += sched_rt.o I know it was wrong before, but please reorder
On Thu, 2015-12-17 at 20:20 -0600, Jonathan Creekmore wrote: > > On Dec 17, 2015, at 7:35 PM, Dario Faggioli <dario.faggioli@citrix. > > com> wrote: > > > > Fact is, I don't think the phrase "instead of just allowing the > > scheduler to be selected on the command line." adds much > > information, > > and I'd just remove it. > > Sorry for the confusion — I did not want to give the impression that > this patch would remove that functionality. I can definitely remove > that language from the patch. > Thanks. :-) > > > > > To match existing behavior, all four schedulers are > > > compiled in by default, although the RTDS and ARINC653 are marked > > > EXPERIMENTAL to match their not currently supported status. > > > > > This may change shortly, but as of now, Credit2 is still > > experimental > > too. I think I'd still recommend enabling it in the help file > > (i.e., > > I'm ok with "If unsure, say Y"), but we certainly should mark it > > with > > "(EPERIMENTAL)”. > > OK, I can mark that EXPERIMENTAL as well. I was under the impression > that 4.7 was going to migrate Credit2 to being SUPPORTED, but I may > have jumped the gun in the Kconfig. > That is very much my plan, and I still think it will be possible to accomplish it. However, right now, it is experimental. So, I think the proper course of action would be to mark it as experimental right now. Then, as soon as we 'un-experimental' it (no matter whether within or after 4.7), we'll amend Kconfig as well. Regards, Dario
On 18/12/15 01:35, Dario Faggioli wrote: > On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote: >> Allow the schedulers to be independently enabled or disabled at >> compile-time instead of just allowing the scheduler to be selected on >> the command line. >> > Reading this quickly, that "instead" gave me a bit of an hard time. I'm > not a native English speaker, and I'm sure it's me that am wrong, but > for some reason the sentence made me think that the patch would somehow > disallow specifying a scheduler during boot, in Xen's command line. I think the "just" in the sentence is an attempt to disambiguate (i.e., instead of *only* allowing the scheduler to be chosen on the command-line, *also* allow it to be chosen at compile-time). But I agree that the whole clause isn't really necessary and it would be clearer without it. -George > > Fact is, I don't think the phrase "instead of just allowing the > scheduler to be selected on the command line." adds much information, > and I'd just remove it. > >> To match existing behavior, all four schedulers are >> compiled in by default, although the RTDS and ARINC653 are marked >> EXPERIMENTAL to match their not currently supported status. >> > This may change shortly, but as of now, Credit2 is still experimental > too. I think I'd still recommend enabling it in the help file (i.e., > I'm ok with "If unsure, say Y"), but we certainly should mark it with > "(EPERIMENTAL)". > > I don't know much on how kconfig works, so all the changes to the > makefile, etc, I'm not able to review them properly. > > On the other hand, the code here below... > >> --- a/xen/common/schedule.c >> +++ b/xen/common/schedule.c >> @@ -38,8 +38,8 @@ >> #include <public/sched.h> >> #include <xsm/xsm.h> >> >> -/* opt_sched: scheduler - default to credit */ >> -static char __initdata opt_sched[10] = "credit"; >> +/* opt_sched: scheduler - default to configured value */ >> +static char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT; >> string_param("sched", opt_sched); >> >> /* if sched_smt_power_savings is set, >> @@ -65,10 +65,18 @@ DEFINE_PER_CPU(struct schedule_data, >> schedule_data); >> DEFINE_PER_CPU(struct scheduler *, scheduler); >> >> static const struct scheduler *schedulers[] = { >> +#ifdef CONFIG_SCHED_CREDIT >> &sched_credit_def, >> +#endif >> +#ifdef CONFIG_SCHED_CREDIT2 >> &sched_credit2_def, >> +#endif >> +#ifdef CONFIG_SCHED_ARINC653 >> &sched_arinc653_def, >> +#endif >> +#ifdef CONFIG_SCHED_RTDS >> &sched_rtds_def, >> +#endif >> }; >> > ... can have, with the changelog changed as shown, my: > > Acked-by: Dario Faggioli <dario.faggioli@citrix.com> > > Regards, > Daio >
On Fri, 2015-12-18 at 10:52 +0000, George Dunlap wrote: > On 18/12/15 01:35, Dario Faggioli wrote: > > On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote: > > > Allow the schedulers to be independently enabled or disabled at > > > compile-time instead of just allowing the scheduler to be > > > selected on > > > the command line. > > > > > Reading this quickly, that "instead" gave me a bit of an hard time. > > I'm > > not a native English speaker, and I'm sure it's me that am wrong, > > but > > for some reason the sentence made me think that the patch would > > somehow > > disallow specifying a scheduler during boot, in Xen's command line. > > I think the "just" in the sentence is an attempt to disambiguate > (i.e., > instead of *only* allowing the scheduler to be chosen on the > command-line, *also* allow it to be chosen at compile-time). > Indeed (for what my opinion on English grammar is worth :-D). That "just" was what made me eventually get the meaning (together with looking at the code). And, in fact, I don't dispute correctness, it's just... > But I agree that the whole clause isn't really necessary and it would > be > clearer without it. > ... this :-D Regards, Dario
Andrew Cooper writes: > On 17/12/2015 20:59, Jonathan Creekmore wrote: >> Allow the schedulers to be independently enabled or disabled at >> compile-time instead of just allowing the scheduler to be selected on >> the command line. To match existing behavior, all four schedulers are >> compiled in by default, although the RTDS and ARINC653 are marked >> EXPERIMENTAL to match their not currently supported status. >> >> CC: George Dunlap <george.dunlap@eu.citrix.com> >> CC: Dario Faggioli <dario.faggioli@citrix.com> >> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> > > With the suggestions Dario made, and one minor nit below, > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > I plan on making those changes. >> diff --git a/xen/common/Makefile b/xen/common/Makefile >> index 8ab15ba..3a2026a 100644 >> --- a/xen/common/Makefile >> +++ b/xen/common/Makefile >> @@ -30,10 +30,10 @@ obj-y += rangeset.o >> obj-y += radix-tree.o >> obj-y += rbtree.o >> obj-y += rcupdate.o >> -obj-y += sched_credit.o >> -obj-y += sched_credit2.o >> -obj-y += sched_arinc653.o >> -obj-y += sched_rt.o >> +obj-$(CONFIG_SCHED_CREDIT) += sched_credit.o >> +obj-$(CONFIG_SCHED_CREDIT2) += sched_credit2.o >> +obj-$(CONFIG_SCHED_ARINC653) += sched_arinc653.o >> +obj-$(CONFIG_SCHED_RTDS) += sched_rt.o > > I know it was wrong before, but please reorder Easy enough to do. I will take care of that.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 7d0e9a9..378a063 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -44,4 +44,68 @@ config KEXEC If unsure, say Y. +# Enable schedulers +menu "Schedulers" +config SCHED_CREDIT + bool "Credit scheduler support" + default y + ---help--- + The traditional credit scheduler is a general purpose scheduler. + + If unsure, say Y. + +config SCHED_CREDIT2 + bool "Credit2 scheduler support" + default y + ---help--- + The credit2 scheduler is a general purpose scheduler that is + optimized for lower latency and higher VM density. + + If unsure, say Y. + +config SCHED_RTDS + bool "RTDS scheduler support (EXPERIMENTAL)" + default y + ---help--- + The RTDS scheduler is a soft and firm real-time scheduler for + multicore, targeted for embedded, automotive, graphics and gaming + in the cloud, and general low-latency workloads. + + If unsure, say N. + +config SCHED_ARINC653 + bool "ARINC653 scheduler support (EXPERIMENTAL)" + default y + ---help--- + The ARINC653 scheduler is a hard real-time scheduler for single + cores, targeted for avionics, drones, and medical devices. + + If unsure, say N. + +choice + prompt "Default Scheduler?" + default SCHED_CREDIT_DEFAULT if SCHED_CREDIT + default SCHED_CREDIT2_DEFAULT if SCHED_CREDIT2 + default SCHED_RTDS_DEFAULT if SCHED_RTDS + default SCHED_ARINC653_DEFAULT if SCHED_ARINC653 + + config SCHED_CREDIT_DEFAULT + bool "Credit Scheduler" if SCHED_CREDIT + config SCHED_CREDIT2_DEFAULT + bool "Credit2 Scheduler" if SCHED_CREDIT2 + config SCHED_RTDS_DEFAULT + bool "RT Scheduler" if SCHED_RTDS + config SCHED_ARINC653_DEFAULT + bool "ARINC653 Scheduler" if SCHED_ARINC653 +endchoice + +config SCHED_DEFAULT + string + default "credit" if SCHED_CREDIT_DEFAULT + default "credit2" if SCHED_CREDIT2_DEFAULT + default "rtds" if SCHED_RTDS_DEFAULT + default "arinc653" if SCHED_ARINC653_DEFAULT + +endmenu + endmenu diff --git a/xen/common/Makefile b/xen/common/Makefile index 8ab15ba..3a2026a 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -30,10 +30,10 @@ obj-y += rangeset.o obj-y += radix-tree.o obj-y += rbtree.o obj-y += rcupdate.o -obj-y += sched_credit.o -obj-y += sched_credit2.o -obj-y += sched_arinc653.o -obj-y += sched_rt.o +obj-$(CONFIG_SCHED_CREDIT) += sched_credit.o +obj-$(CONFIG_SCHED_CREDIT2) += sched_credit2.o +obj-$(CONFIG_SCHED_ARINC653) += sched_arinc653.o +obj-$(CONFIG_SCHED_RTDS) += sched_rt.o obj-y += schedule.o obj-y += shutdown.o obj-y += softirq.o diff --git a/xen/common/schedule.c b/xen/common/schedule.c index d121896..2f98a48 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -38,8 +38,8 @@ #include <public/sched.h> #include <xsm/xsm.h> -/* opt_sched: scheduler - default to credit */ -static char __initdata opt_sched[10] = "credit"; +/* opt_sched: scheduler - default to configured value */ +static char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT; string_param("sched", opt_sched); /* if sched_smt_power_savings is set, @@ -65,10 +65,18 @@ DEFINE_PER_CPU(struct schedule_data, schedule_data); DEFINE_PER_CPU(struct scheduler *, scheduler); static const struct scheduler *schedulers[] = { +#ifdef CONFIG_SCHED_CREDIT &sched_credit_def, +#endif +#ifdef CONFIG_SCHED_CREDIT2 &sched_credit2_def, +#endif +#ifdef CONFIG_SCHED_ARINC653 &sched_arinc653_def, +#endif +#ifdef CONFIG_SCHED_RTDS &sched_rtds_def, +#endif }; static struct scheduler __read_mostly ops;
Allow the schedulers to be independently enabled or disabled at compile-time instead of just allowing the scheduler to be selected on the command line. To match existing behavior, all four schedulers are compiled in by default, although the RTDS and ARINC653 are marked EXPERIMENTAL to match their not currently supported status. CC: George Dunlap <george.dunlap@eu.citrix.com> CC: Dario Faggioli <dario.faggioli@citrix.com> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> --- xen/common/Kconfig | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++ xen/common/Makefile | 8 +++---- xen/common/schedule.c | 12 ++++++++-- 3 files changed, 78 insertions(+), 6 deletions(-)