[PATCH v4 0/5] Allow schedulers to be selectable through Kconfig

[PATCH v4 0/5] Allow schedulers to be selectable through Kconfig

[Jonathan Creekmore]

Add machinery to allow the schedulers to be individually selectable
through the Kconfig interface. The first patch in the series sets up
the CONFIG_EXPERT Kconfig variable that is only enabled by passing an
environment variable to the build. The second patch in the series sets up
the Kconfig options for the schedulers and places the appropriate CONFIG_
options around the scheduler list. The third, fourth, and fifth patches
rework the scheduler list from being manually curated into a model
where just compiling any schedulers into the hypervisor causes the
scheduler list to be built up.

---
Changed since v3:
 * Add defensive check for schedulers in the linker

Changed since v2:
 * Added a predecessor patch that introduces a environment
   variable for the build to enable expert configuration options
   (1/5)
 * Hide the scheduler menu behind the expert option (2/5)
 * Provide static inlines for credit functions that are assumed to be
   present if it is compiled out (2/5)
 * Provide an absolute default of the credit scheduler if the 
   scheduler menu is not visible (2/5)

Changed since v1:
 * Marked credit2 as EXPERIMENTAL
 * Removed confusing language from the commit message
 * Alphabetize the schedulers
 * rename the __start and __end symbols to better match
   the rest of the file
 * Simplify the calculation of the number of schedulers
 * Make the scheduler ops structures static to their files

Jonathan Creekmore (5):
  build: Env var to enable expert config options
  build: Hook the schedulers into Kconfig
  build: Alloc space for sched list in the link file
  sched: Register the schedulers into the list
  sched: Use the auto-generated list of schedulers

 xen/Kconfig                 |  4 +++
 xen/Makefile                |  1 +
 xen/arch/arm/xen.lds.S      |  5 ++++
 xen/arch/x86/xen.lds.S      |  5 ++++
 xen/common/Kconfig          | 67 +++++++++++++++++++++++++++++++++++++++++++++
 xen/common/Makefile         |  8 +++---
 xen/common/sched_arinc653.c |  4 ++-
 xen/common/sched_credit.c   |  4 ++-
 xen/common/sched_credit2.c  |  4 ++-
 xen/common/sched_rt.c       |  4 ++-
 xen/common/schedule.c       | 20 ++++++--------
 xen/include/xen/sched-if.h  |  7 ++---
 xen/include/xen/sched.h     |  5 ++++
 13 files changed, 114 insertions(+), 24 deletions(-)

-- 
2.6.4

[PATCH v4 1/5] build: Env var to enable expert config options

[Jonathan Creekmore]

Add an additional environment variable, defaulting to disabled,
that enables the CONFIG_EXPERT configuration option. The purpose
of the CONFIG_EXPERT configuration option is to make non-standard
Kconfig options visible during the configuration process. The
CONFIG_EXPERT option is not, itself, visible during the Kconfig
configuration process, so typical users will never see it nor
any of the non-standard configuration options.

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Keir Fraser <keir@xen.org>
CC: Tim Deegan <tim@xen.org>
Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/Kconfig  | 4 ++++
 xen/Makefile | 1 +
 2 files changed, 5 insertions(+)

diff --git a/xen/Kconfig b/xen/Kconfig
index ffe3f45..fa8b27c 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -22,3 +22,7 @@ config DEFCONFIG_LIST
 	string
 	option defconfig_list
 	default "$ARCH_DEFCONFIG"
+
+config EXPERT
+	string
+	option env="XEN_CONFIG_EXPERT"
diff --git a/xen/Makefile b/xen/Makefile
index 9023863..4950afb 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -11,6 +11,7 @@ export XEN_DOMAIN	?= $(shell ([ -x /bin/dnsdomainname ] && /bin/dnsdomainname) |
 export XEN_BUILD_DATE	?= $(shell LC_ALL=C date)
 export XEN_BUILD_TIME	?= $(shell LC_ALL=C date +%T)
 export XEN_BUILD_HOST	?= $(shell hostname)
+export XEN_CONFIG_EXPERT ?= n
 
 export BASEDIR := $(CURDIR)
 export XEN_ROOT := $(BASEDIR)/..
-- 
2.6.4

[PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jonathan Creekmore]

Allow the schedulers to be independently enabled or disabled at
compile-time. To match existing behavior, all four schedulers are
compiled in by default, although the Credit2, 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>
Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

---
Changed since v2:

  * Hid the scheduler menu behind the EXPERT option
  * Provide static inlines for credit functions that are assumed to be
    always available
  * Provide an absolute default of the credit scheduler if the
    scheduler menu is not visible

Changed since v1:

  * Marked credit2 as EXPERIMENTAL
  * Removed confusing language from the commit message
  * Alphabetize the schedulers
---
 xen/common/Kconfig      | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
 xen/common/Makefile     |  8 +++---
 xen/common/schedule.c   | 12 +++++++--
 xen/include/xen/sched.h |  5 ++++
 4 files changed, 86 insertions(+), 6 deletions(-)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 046e257..db7411b 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -51,4 +51,71 @@ config KEXEC
 
 	  If unsure, say Y.
 
+# Enable schedulers
+menu "Schedulers"
+	visible if EXPERT = "y"
+
+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 (EXPERIMENTAL)"
+	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
+	default "credit"
+
+endmenu
+
 endmenu
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 8ab15ba..29a5916 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_ARINC653) += sched_arinc653.o
+obj-$(CONFIG_SCHED_CREDIT) += sched_credit.o
+obj-$(CONFIG_SCHED_CREDIT2) += sched_credit2.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;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index fc61fc3..246338e 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -850,8 +850,13 @@ static inline bool_t is_vcpu_online(const struct vcpu *v)
     return !test_bit(_VPF_down, &v->pause_flags);
 }
 
+#ifdef CONFIG_SCHED_CREDIT
 void set_vcpu_migration_delay(unsigned int delay);
 unsigned int get_vcpu_migration_delay(void);
+#else
+static inline void set_vcpu_migration_delay(unsigned int delay) { }
+static inline unsigned int get_vcpu_migration_delay(void) { return 0; }
+#endif
 
 extern bool_t sched_smt_power_savings;
 
-- 
2.6.4

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Andrew Cooper]

On 08/01/16 21:22, Jonathan Creekmore wrote:
> +# Enable schedulers
> +menu "Schedulers"
> +	visible if EXPERT = "y"
> +
> +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 (EXPERIMENTAL)"
> +	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.

Sorry for not noticing this before.  The "If unsure, say $X" should
really match the default value.

On the other hand, given that we are hiding all these options behind
CONFIG_EXPERT, I am not sure that we need "If unsure" clauses.  Anyone
who isn't sure shouldn't have turned on CONFIG_EXPERT to start with.

~Andrew

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jonathan Creekmore]

Andrew Cooper writes:

> On 08/01/16 21:22, Jonathan Creekmore wrote:
>> +# Enable schedulers
>> +menu "Schedulers"
>> +	visible if EXPERT = "y"
>> +
>> +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 (EXPERIMENTAL)"
>> +	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.
>
> Sorry for not noticing this before.  The "If unsure, say $X" should
> really match the default value.
>
> On the other hand, given that we are hiding all these options behind
> CONFIG_EXPERT, I am not sure that we need "If unsure" clauses.  Anyone
> who isn't sure shouldn't have turned on CONFIG_EXPERT to start with.

I was trying to mimic language that the Linux kernel would use for
EXPERIMENTAL marked items. Given the documentation on the wiki, I think
marking those three schedulers EXPERIMENTAL is correct. Given that, I
still think that the language saying "If unsure, say N" is correct (the
thought being, the only people who should be messing with the
EXPERIMENTAL schedulers are people developing or specifically testing
them). The *only* reason I marked them default of Y is to keep backwards
compatibility with the current build.

However, if you would prefer me to remove the "If unsure" language
completely, I can do that. The text came in before the whole
CONFIG_EXPERT flag did.

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Andrew Cooper]

On 09/01/16 17:50, Jonathan Creekmore wrote:
> Andrew Cooper writes:
>
>> On 08/01/16 21:22, Jonathan Creekmore wrote:
>>> +# Enable schedulers
>>> +menu "Schedulers"
>>> +	visible if EXPERT = "y"
>>> +
>>> +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 (EXPERIMENTAL)"
>>> +	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.
>> Sorry for not noticing this before.  The "If unsure, say $X" should
>> really match the default value.
>>
>> On the other hand, given that we are hiding all these options behind
>> CONFIG_EXPERT, I am not sure that we need "If unsure" clauses.  Anyone
>> who isn't sure shouldn't have turned on CONFIG_EXPERT to start with.
> I was trying to mimic language that the Linux kernel would use for
> EXPERIMENTAL marked items. Given the documentation on the wiki, I think
> marking those three schedulers EXPERIMENTAL is correct.

I concur about their status.

>  Given that, I
> still think that the language saying "If unsure, say N" is correct (the
> thought being, the only people who should be messing with the
> EXPERIMENTAL schedulers are people developing or specifically testing
> them). The *only* reason I marked them default of Y is to keep backwards
> compatibility with the current build.

Also very important.

>
> However, if you would prefer me to remove the "If unsure" language
> completely, I can do that. The text came in before the whole
> CONFIG_EXPERT flag did.

I would suggest dropping it (although you probably want to wait for
opinions from others).  We have already diverged from Linux with regards
to the EXPERT flag; people who are unsure cannot accidentally get here.

~Andrew

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jonathan Creekmore]

Andrew Cooper writes:

> On 09/01/16 17:50, Jonathan Creekmore wrote:
>> Andrew Cooper writes:
>>
>>> On 08/01/16 21:22, Jonathan Creekmore wrote:
>>>> +# Enable schedulers
>>>> +menu "Schedulers"
>>>> +	visible if EXPERT = "y"
>>>> +
>>>> +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 (EXPERIMENTAL)"
>>>> +	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.
>>> Sorry for not noticing this before.  The "If unsure, say $X" should
>>> really match the default value.
>>>
>>> On the other hand, given that we are hiding all these options behind
>>> CONFIG_EXPERT, I am not sure that we need "If unsure" clauses.  Anyone
>>> who isn't sure shouldn't have turned on CONFIG_EXPERT to start with.
>> I was trying to mimic language that the Linux kernel would use for
>> EXPERIMENTAL marked items. Given the documentation on the wiki, I think
>> marking those three schedulers EXPERIMENTAL is correct.
>
> I concur about their status.
>
>>  Given that, I
>> still think that the language saying "If unsure, say N" is correct (the
>> thought being, the only people who should be messing with the
>> EXPERIMENTAL schedulers are people developing or specifically testing
>> them). The *only* reason I marked them default of Y is to keep backwards
>> compatibility with the current build.
>
> Also very important.
>
>>
>> However, if you would prefer me to remove the "If unsure" language
>> completely, I can do that. The text came in before the whole
>> CONFIG_EXPERT flag did.
>
> I would suggest dropping it (although you probably want to wait for
> opinions from others).  We have already diverged from Linux with regards
> to the EXPERT flag; people who are unsure cannot accidentally get here.

OK, if I haven't heard anything from any others by the time I send out
my v5 on Monday with the other items you caught, then I will go ahead
and remove the language.

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jan Beulich]

>>> On 09.01.16 at 19:08, <andrew.cooper3@citrix.com> wrote:
> On 09/01/16 17:50, Jonathan Creekmore wrote:
>> However, if you would prefer me to remove the "If unsure" language
>> completely, I can do that. The text came in before the whole
>> CONFIG_EXPERT flag did.
> 
> I would suggest dropping it (although you probably want to wait for
> opinions from others).  We have already diverged from Linux with regards
> to the EXPERT flag; people who are unsure cannot accidentally get here.

+1

Jan

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jan Beulich]

>>> On 08.01.16 at 22:22, <jonathan.creekmore@gmail.com> wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -51,4 +51,71 @@ config KEXEC
>  
>  	  If unsure, say Y.
>  
> +# Enable schedulers
> +menu "Schedulers"
> +	visible if EXPERT = "y"

Does "visible if EXPERT" not suffice here?

> +config SCHED_CREDIT
> +	bool "Credit scheduler support"
> +	default y

I continue to think that not making the primary scheduler configurable
would be the better solution to the problems resulting from possibly
all of them getting turned off.

> +config SCHED_CREDIT2
> +	bool "Credit2 scheduler support (EXPERIMENTAL)"
> +	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
> +	default "credit"

What use is this last line?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -850,8 +850,13 @@ static inline bool_t is_vcpu_online(const struct vcpu *v)
>      return !test_bit(_VPF_down, &v->pause_flags);
>  }
>  
> +#ifdef CONFIG_SCHED_CREDIT
>  void set_vcpu_migration_delay(unsigned int delay);
>  unsigned int get_vcpu_migration_delay(void);
> +#else
> +static inline void set_vcpu_migration_delay(unsigned int delay) { }
> +static inline unsigned int get_vcpu_migration_delay(void) { return 0; }
> +#endif

I don't think these are appropriate: The respective sysctl sub-ops
would probably better indicate failure to the caller.

Jan

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jonathan Creekmore]

Jan Beulich writes:

>>>> On 08.01.16 at 22:22, <jonathan.creekmore@gmail.com> wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -51,4 +51,71 @@ config KEXEC
>>
>>  	  If unsure, say Y.
>>
>> +# Enable schedulers
>> +menu "Schedulers"
>> +	visible if EXPERT = "y"
>
> Does "visible if EXPERT" not suffice here?

No, because EXPERT is a string and not a boolean. It has to be a string
since it is pulling in a string from the environment to set its value.

>
>> +config SCHED_CREDIT
>> +	bool "Credit scheduler support"
>> +	default y
>
> I continue to think that not making the primary scheduler configurable
> would be the better solution to the problems resulting from possibly
> all of them getting turned off.

Except that is completely contrary to my goal with this patchset (being
able to compile in just the scheduler that I want to use). Yes, at the
moment, credit is the only non-experimental scheduler and will likely be
the one we choose. However, in the future, when credit2 and possibly
others are non-experimental, we may choose one of the other schedulers
and do not want to carry along credit in our build just because it is
the primary scheduler.

>
>> +config SCHED_CREDIT2
>> +	bool "Credit2 scheduler support (EXPERIMENTAL)"
>> +	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
>> +	default "credit"
>
> What use is this last line?
>

When the scheduler menu is not visible, the choice selection does not
cause a scheduler to be chosen. The last line forces credit to be the
default if none of the _DEFAULT values are selected. It is purely an
artifact of introducing the visibility option on the menu. It could be
moved into the defaults section for the choice like this:

+	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
+       default SCHED_CREDIT_DEFAULT


>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -850,8 +850,13 @@ static inline bool_t is_vcpu_online(const struct vcpu *v)
>>      return !test_bit(_VPF_down, &v->pause_flags);
>>  }
>>
>> +#ifdef CONFIG_SCHED_CREDIT
>>  void set_vcpu_migration_delay(unsigned int delay);
>>  unsigned int get_vcpu_migration_delay(void);
>> +#else
>> +static inline void set_vcpu_migration_delay(unsigned int delay) { }
>> +static inline unsigned int get_vcpu_migration_delay(void) { return 0; }
>> +#endif
>
> I don't think these are appropriate: The respective sysctl sub-ops
> would probably better indicate failure to the caller.

I can make that change if you want me to. As it stands now, the existing
sysctl sub-ops are probably not doing the right thing since they are
setting and getting this migration delay in the credit scheduler
regardless of which scheduler is actually in use.

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jan Beulich]

>>> On 11.01.16 at 16:10, <jonathan.creekmore@gmail.com> wrote:
> Jan Beulich writes:
>>>>> On 08.01.16 at 22:22, <jonathan.creekmore@gmail.com> wrote:
>>> +config SCHED_CREDIT
>>> +	bool "Credit scheduler support"
>>> +	default y
>>
>> I continue to think that not making the primary scheduler configurable
>> would be the better solution to the problems resulting from possibly
>> all of them getting turned off.
> 
> Except that is completely contrary to my goal with this patchset (being
> able to compile in just the scheduler that I want to use). Yes, at the
> moment, credit is the only non-experimental scheduler and will likely be
> the one we choose. However, in the future, when credit2 and possibly
> others are non-experimental, we may choose one of the other schedulers
> and do not want to carry along credit in our build just because it is
> the primary scheduler.

I think we need to separate what we want as "upstream", and what
your internal intentions are. Any gap between that would need to be
taken care of by private patches you'd have to carry.

As "upstream", I think not allowing the default scheduler to be turned
off is quite reasonable an approach.

>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -850,8 +850,13 @@ static inline bool_t is_vcpu_online(const struct vcpu *v)
>>>      return !test_bit(_VPF_down, &v->pause_flags);
>>>  }
>>>
>>> +#ifdef CONFIG_SCHED_CREDIT
>>>  void set_vcpu_migration_delay(unsigned int delay);
>>>  unsigned int get_vcpu_migration_delay(void);
>>> +#else
>>> +static inline void set_vcpu_migration_delay(unsigned int delay) { }
>>> +static inline unsigned int get_vcpu_migration_delay(void) { return 0; }
>>> +#endif
>>
>> I don't think these are appropriate: The respective sysctl sub-ops
>> would probably better indicate failure to the caller.
> 
> I can make that change if you want me to. As it stands now, the existing
> sysctl sub-ops are probably not doing the right thing since they are
> setting and getting this migration delay in the credit scheduler
> regardless of which scheduler is actually in use.

Yes and no - it would still change / obtain the value for those
parts of the system (CPU pools) where that scheduler is being
used. That set may be empty, but the scheduler is still present,
and hence the operations succeeding is a valid thing as long as
that scheduler exists. This changes, though, if the scheduler
doesn't get built anymore. (And not how the problem would go
away altogether if credit wasn't configurable.)

Jan

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Doug Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 2727 bytes --]

On 1/11/16 9:43 AM, Jan Beulich wrote:
>>>> On 11.01.16 at 16:10, <jonathan.creekmore@gmail.com> wrote:
>> Jan Beulich writes:
>>>>>> On 08.01.16 at 22:22, <jonathan.creekmore@gmail.com> wrote:
>>>> +config SCHED_CREDIT
>>>> +	bool "Credit scheduler support"
>>>> +	default y
>>>
>>> I continue to think that not making the primary scheduler configurable
>>> would be the better solution to the problems resulting from possibly
>>> all of them getting turned off.
>>
>> Except that is completely contrary to my goal with this patchset (being
>> able to compile in just the scheduler that I want to use). Yes, at the
>> moment, credit is the only non-experimental scheduler and will likely be
>> the one we choose. However, in the future, when credit2 and possibly
>> others are non-experimental, we may choose one of the other schedulers
>> and do not want to carry along credit in our build just because it is
>> the primary scheduler.
> 
> I think we need to separate what we want as "upstream", and what
> your internal intentions are. Any gap between that would need to be
> taken care of by private patches you'd have to carry.
> 
> As "upstream", I think not allowing the default scheduler to be turned
> off is quite reasonable an approach.
> 

My immediate reaction to this request is "special casing is bad" and
that's what this is straight away. Special cases make the code worse and
weaker as a whole.

There are no internal intentions here. Our internal intentions are to
use the credit scheduler. The intentions are for configurability of all
the options and not just some of the options. The goal here is to be
more inclusive of various downstreams and the goal of Kconfig being a
means by which to support that? The idea here was to encourage more
upstream participation and not less. I can hardly see how
configurability for an extreme corner case that's very hidden away from
the average user would be harmful to upstream.

There have been a good deal number of different downstreams that have
been encouraging of changes like this but it seems like you are
fundamentally opposed. Which is plainly discouraging to people to
attempt to engage upstream.

At some point it becomes damaging to the Xen upstream where users are
unable to get what they need/want out of Xen upstream without having to
become their own downstreams and patching. Another example of this can
be seen with modern Lenovo laptops that do not properly follow the EFI
spec. I've seen numerous patches rejected with the comment of "tell
Lenovo to fix their hardware". The result is users of Lenovo hardware
walk away feeling that Xen is broken not their hardware.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jan Beulich]

>>> On 11.01.16 at 17:31, <cardoe@cardoe.com> wrote:
> There have been a good deal number of different downstreams that have
> been encouraging of changes like this but it seems like you are
> fundamentally opposed. Which is plainly discouraging to people to
> attempt to engage upstream.

I could see such a reaction as being valid if I objected to any of
the configurability changes, but I have a hard time following when
I just ask for the default scheduler to not become configurable.
Plus I'm not the only one involved in getting to a decision here, i.e.
I can easily be overruled by other maintainers.

> At some point it becomes damaging to the Xen upstream where users are
> unable to get what they need/want out of Xen upstream without having to
> become their own downstreams and patching. Another example of this can
> be seen with modern Lenovo laptops that do not properly follow the EFI
> spec. I've seen numerous patches rejected with the comment of "tell
> Lenovo to fix their hardware". The result is users of Lenovo hardware
> walk away feeling that Xen is broken not their hardware.

Well, if you followed the discussion closely you may have noticed
we already kind of agreed that using DMI based quirks would be
an acceptable mechanism to help the situation. As to my objection
to make Xen violate the EFI spec by default, I continue to think
this would be a bad idea, and I don't recall any substantial
comments from you proving this a bad position.

And yes, I'm having difficulty accepting massive amounts of
workarounds for various vendors' quirks (be it EFI related or
other), but commonly I accept such if they can be proven to not
negatively impact other systems / platforms. And yes, I sincerely
believe that hardware vendors should do a better job in
implementing their firmware, the more that they managed to
screw up many things in the PC BIOS days already, and one
would think (hope) they would learn from older mistakes. Since
they don't, getting people to put some pressure on them is,
I think, quite appropriate. Working out or accepting side effect
free workarounds in parallel is of course not excluded.

Jan

Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Doug Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 993 bytes --]

On 1/11/16 10:49 AM, Jan Beulich wrote:
>>>> On 11.01.16 at 17:31, <cardoe@cardoe.com> wrote:
>> There have been a good deal number of different downstreams that have
>> been encouraging of changes like this but it seems like you are
>> fundamentally opposed. Which is plainly discouraging to people to
>> attempt to engage upstream.
> 
> I could see such a reaction as being valid if I objected to any of
> the configurability changes, but I have a hard time following when
> I just ask for the default scheduler to not become configurable.
> Plus I'm not the only one involved in getting to a decision here, i.e.
> I can easily be overruled by other maintainers.
> 

I really hate to use Linux as an example here since its a different
project but I will anyway. Linux has a number of flags like this were
you can remove the default. The compression algorithm, the CPU schedule,
the I/O scheduler. So it doesn't seem that far fetched to allow that.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH v4 3/5] build: Alloc space for sched list in the link file

[Jonathan Creekmore]

Creates a section to contain scheduler entry pointers that are gathered
together into an array. This will allow, in a follow-on patch, scheduler
entries to be automatically gathered together into the array for
automatic parsing.

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

---
Changed since v3:
  * Add defensive check for schedulers in the linker

Changed since v1:
  * rename the __start and __end symbols to better match
    the rest of the file
---
 xen/arch/arm/xen.lds.S | 5 +++++
 xen/arch/x86/xen.lds.S | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 0488f37..2492def 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -57,6 +57,10 @@ SECTIONS
        . = ALIGN(PAGE_SIZE);
        *(.data.page_aligned)
        *(.data)
+       . = ALIGN(8);
+       __start_schedulers_array = .;
+       *(.data.schedulers)
+       __end_schedulers_array = .;
        *(.data.rel)
        *(.data.rel.*)
        CONSTRUCTORS
@@ -193,3 +197,4 @@ SECTIONS
  * code running on the boot time identity map cannot cross a section boundary.
  */
 ASSERT( _end_boot - start <= PAGE_SIZE, "Boot code is larger than 4K")
+ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index e18e08f..63f89af 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -80,6 +80,10 @@ SECTIONS
        __stop___pre_ex_table = .;
 
        *(.data.read_mostly)
+       . = ALIGN(8);
+       __start_schedulers_array = .;
+       *(.data.schedulers)
+       __end_schedulers_array = .;
        *(.data.rel.ro)
        *(.data.rel.ro.*)
   } :text
@@ -226,3 +230,4 @@ ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
 #endif
 
 ASSERT((cpu0_stack & (STACK_SIZE - 1)) == 0, "cpu0_stack misaligned")
+ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")
-- 
2.6.4

Re: [PATCH v4 3/5] build: Alloc space for sched list in the link file

[Andrew Cooper]

On 08/01/16 21:22, Jonathan Creekmore wrote:
> Creates a section to contain scheduler entry pointers that are gathered
> together into an array. This will allow, in a follow-on patch, scheduler
> entries to be automatically gathered together into the array for
> automatic parsing.
>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>
> ---
> Changed since v3:
>   * Add defensive check for schedulers in the linker
>
> Changed since v1:
>   * rename the __start and __end symbols to better match
>     the rest of the file
> ---
>  xen/arch/arm/xen.lds.S | 5 +++++
>  xen/arch/x86/xen.lds.S | 5 +++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 0488f37..2492def 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -57,6 +57,10 @@ SECTIONS
>         . = ALIGN(PAGE_SIZE);
>         *(.data.page_aligned)
>         *(.data)
> +       . = ALIGN(8);
> +       __start_schedulers_array = .;
> +       *(.data.schedulers)
> +       __end_schedulers_array = .;
>         *(.data.rel)
>         *(.data.rel.*)
>         CONSTRUCTORS
> @@ -193,3 +197,4 @@ SECTIONS
>   * code running on the boot time identity map cannot cross a section boundary.
>   */
>  ASSERT( _end_boot - start <= PAGE_SIZE, "Boot code is larger than 4K")
> +ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index e18e08f..63f89af 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -80,6 +80,10 @@ SECTIONS
>         __stop___pre_ex_table = .;
>  
>         *(.data.read_mostly)
> +       . = ALIGN(8);
> +       __start_schedulers_array = .;
> +       *(.data.schedulers)
> +       __end_schedulers_array = .;
>         *(.data.rel.ro)
>         *(.data.rel.ro.*)
>    } :text
> @@ -226,3 +230,4 @@ ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
>  #endif
>  
>  ASSERT((cpu0_stack & (STACK_SIZE - 1)) == 0, "cpu0_stack misaligned")
> +ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")

This patch won't build on its own, as the ASSERT() will fire. 
Therefore, it breaks bisectability.

In this patch (or the previous one), you need to move the schedulers
list into __section(".data.schedulers"), and undo the movement in patch
4 when populating .data.schedulers properly.

Alternatively, you could just merge patches 3 and 4.  I don't think that
would reduce the clarity of what you were doing.

~Andrew

Re: [PATCH v4 3/5] build: Alloc space for sched list in the link file

[Jonathan Creekmore]

Andrew Cooper writes:

> On 08/01/16 21:22, Jonathan Creekmore wrote:
>> Creates a section to contain scheduler entry pointers that are gathered
>> together into an array. This will allow, in a follow-on patch, scheduler
>> entries to be automatically gathered together into the array for
>> automatic parsing.
>>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
>> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>>
>> ---
>> Changed since v3:
>>   * Add defensive check for schedulers in the linker
>>
>> Changed since v1:
>>   * rename the __start and __end symbols to better match
>>     the rest of the file
>> ---
>>  xen/arch/arm/xen.lds.S | 5 +++++
>>  xen/arch/x86/xen.lds.S | 5 +++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index 0488f37..2492def 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -57,6 +57,10 @@ SECTIONS
>>         . = ALIGN(PAGE_SIZE);
>>         *(.data.page_aligned)
>>         *(.data)
>> +       . = ALIGN(8);
>> +       __start_schedulers_array = .;
>> +       *(.data.schedulers)
>> +       __end_schedulers_array = .;
>>         *(.data.rel)
>>         *(.data.rel.*)
>>         CONSTRUCTORS
>> @@ -193,3 +197,4 @@ SECTIONS
>>   * code running on the boot time identity map cannot cross a section boundary.
>>   */
>>  ASSERT( _end_boot - start <= PAGE_SIZE, "Boot code is larger than 4K")
>> +ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index e18e08f..63f89af 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -80,6 +80,10 @@ SECTIONS
>>         __stop___pre_ex_table = .;
>>
>>         *(.data.read_mostly)
>> +       . = ALIGN(8);
>> +       __start_schedulers_array = .;
>> +       *(.data.schedulers)
>> +       __end_schedulers_array = .;
>>         *(.data.rel.ro)
>>         *(.data.rel.ro.*)
>>    } :text
>> @@ -226,3 +230,4 @@ ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
>>  #endif
>>
>>  ASSERT((cpu0_stack & (STACK_SIZE - 1)) == 0, "cpu0_stack misaligned")
>> +ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")
>
> This patch won't build on its own, as the ASSERT() will fire.
> Therefore, it breaks bisectability.
>
> In this patch (or the previous one), you need to move the schedulers
> list into __section(".data.schedulers"), and undo the movement in patch
> 4 when populating .data.schedulers properly.
>
> Alternatively, you could just merge patches 3 and 4.  I don't think that
> would reduce the clarity of what you were doing.

Quite right. I think merging patches 3 and 4 is probably the most
straightforward way. That way, one patch introduces the new section and
actually populates that section.

[PATCH v4 4/5] sched: Register the schedulers into the list

[Jonathan Creekmore]

Adds a simple macro to place a pointer to a scheduler into an array
section at compile time. Also, goes ahead and generates the array
entries with each of the schedulers.

CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Josh Whitehead <josh.whitehead@dornerworks.com>
CC: Robert VanVossen <robert.vanvossen@dornerworks.com>
Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
Acked-by: Dario Faggioli <dario.faggioli@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/sched_arinc653.c | 2 ++
 xen/common/sched_credit.c   | 2 ++
 xen/common/sched_credit2.c  | 2 ++
 xen/common/sched_rt.c       | 2 ++
 xen/include/xen/sched-if.h  | 2 ++
 5 files changed, 10 insertions(+)

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index dbe02ed..3b59514 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -767,6 +767,8 @@ const struct scheduler sched_arinc653_def = {
     .tick_resume    = NULL,
 };
 
+REGISTER_SCHEDULER(sched_arinc653_def);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 0dce790..e586248 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -2027,3 +2027,5 @@ const struct scheduler sched_credit_def = {
     .tick_suspend   = csched_tick_suspend,
     .tick_resume    = csched_tick_resume,
 };
+
+REGISTER_SCHEDULER(sched_credit_def);
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 3c49ffa..38b02d0 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -2228,3 +2228,5 @@ const struct scheduler sched_credit2_def = {
     .alloc_domdata  = csched2_alloc_domdata,
     .free_domdata   = csched2_free_domdata,
 };
+
+REGISTER_SCHEDULER(sched_credit2_def);
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 3f1d047..7640cd0 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1199,3 +1199,5 @@ const struct scheduler sched_rtds_def = {
     .wake           = rt_vcpu_wake,
     .context_saved  = rt_context_saved,
 };
+
+REGISTER_SCHEDULER(sched_rtds_def);
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 493d43f..9c6e0f5 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -170,6 +170,8 @@ extern const struct scheduler sched_credit2_def;
 extern const struct scheduler sched_arinc653_def;
 extern const struct scheduler sched_rtds_def;
 
+#define REGISTER_SCHEDULER(x) static const struct scheduler *x##_entry \
+  __used_section(".data.schedulers") = &x;
 
 struct cpupool
 {
-- 
2.6.4

[PATCH v4 5/5] sched: Use the auto-generated list of schedulers

[Jonathan Creekmore]

Instead of having a manually-curated list of schedulers, use the array
that was auto-generated simply by compiling in the scheduler files as
the sole source of truth of the available schedulers.

CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
Acked-by: Dario Faggioli <dario.faggioli@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

---
Changed since v1:
  * Simplify the calculation of the number of schedulers
  * Make the scheduler ops structures static to their files
---
 xen/common/sched_arinc653.c |  2 +-
 xen/common/sched_credit.c   |  2 +-
 xen/common/sched_credit2.c  |  2 +-
 xen/common/sched_rt.c       |  2 +-
 xen/common/schedule.c       | 24 +++++++-----------------
 xen/include/xen/sched-if.h  |  5 -----
 6 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index 3b59514..0606988 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -724,7 +724,7 @@ a653sched_adjust_global(const struct scheduler *ops,
  * callback functions.
  * The symbol must be visible to the rest of Xen at link time.
  */
-const struct scheduler sched_arinc653_def = {
+static const struct scheduler sched_arinc653_def = {
     .name           = "ARINC 653 Scheduler",
     .opt_name       = "arinc653",
     .sched_id       = XEN_SCHEDULER_ARINC653,
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index e586248..028e41b 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1991,7 +1991,7 @@ static void csched_tick_resume(const struct scheduler *ops, unsigned int cpu)
 
 static struct csched_private _csched_priv;
 
-const struct scheduler sched_credit_def = {
+static const struct scheduler sched_credit_def = {
     .name           = "SMP Credit Scheduler",
     .opt_name       = "credit",
     .sched_id       = XEN_SCHEDULER_CREDIT,
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 38b02d0..78220a7 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -2194,7 +2194,7 @@ csched2_deinit(const struct scheduler *ops)
 
 static struct csched2_private _csched2_priv;
 
-const struct scheduler sched_credit2_def = {
+static const struct scheduler sched_credit2_def = {
     .name           = "SMP Credit Scheduler rev2",
     .opt_name       = "credit2",
     .sched_id       = XEN_SCHEDULER_CREDIT2,
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 7640cd0..2e5430f 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1170,7 +1170,7 @@ rt_dom_cntl(
 
 static struct rt_private _rt_priv;
 
-const struct scheduler sched_rtds_def = {
+static const struct scheduler sched_rtds_def = {
     .name           = "SMP RTDS Scheduler",
     .opt_name       = "rtds",
     .sched_id       = XEN_SCHEDULER_RTDS,
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 2f98a48..91e53c1 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -64,20 +64,10 @@ static void poll_timer_fn(void *data);
 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
-};
+extern const struct scheduler *__start_schedulers_array[], *__end_schedulers_array[];
+extern const size_t schedulers_array_size;
+#define NUM_SCHEDULERS (__end_schedulers_array - __start_schedulers_array)
+static const struct scheduler **schedulers = __start_schedulers_array;
 
 static struct scheduler __read_mostly ops;
 
@@ -1468,7 +1458,7 @@ void __init scheduler_init(void)
 
     open_softirq(SCHEDULE_SOFTIRQ, schedule);
 
-    for ( i = 0; i < ARRAY_SIZE(schedulers); i++ )
+    for ( i = 0; i < NUM_SCHEDULERS; i++)
     {
         if ( schedulers[i]->global_init && schedulers[i]->global_init() < 0 )
             schedulers[i] = NULL;
@@ -1479,7 +1469,7 @@ void __init scheduler_init(void)
     if ( !ops.name )
     {
         printk("Could not find scheduler: %s\n", opt_sched);
-        for ( i = 0; i < ARRAY_SIZE(schedulers); i++ )
+        for ( i = 0; i < NUM_SCHEDULERS; i++ )
             if ( schedulers[i] )
             {
                 ops = *schedulers[i];
@@ -1599,7 +1589,7 @@ struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr)
     int i;
     struct scheduler *sched;
 
-    for ( i = 0; i < ARRAY_SIZE(schedulers); i++ )
+    for ( i = 0; i < NUM_SCHEDULERS; i++ )
         if ( schedulers[i] && schedulers[i]->sched_id == sched_id )
             goto found;
     *perr = -ENOENT;
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 9c6e0f5..66dc9c8 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -165,11 +165,6 @@ struct scheduler {
     void         (*tick_resume)     (const struct scheduler *, unsigned int);
 };
 
-extern const struct scheduler sched_credit_def;
-extern const struct scheduler sched_credit2_def;
-extern const struct scheduler sched_arinc653_def;
-extern const struct scheduler sched_rtds_def;
-
 #define REGISTER_SCHEDULER(x) static const struct scheduler *x##_entry \
   __used_section(".data.schedulers") = &x;
 
-- 
2.6.4

Re: [PATCH v4 5/5] sched: Use the auto-generated list of schedulers

[Andrew Cooper]

On 08/01/16 21:22, Jonathan Creekmore wrote:
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 2f98a48..91e53c1 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -64,20 +64,10 @@ static void poll_timer_fn(void *data);
>  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
> -};
> +extern const struct scheduler *__start_schedulers_array[], *__end_schedulers_array[];
> +extern const size_t schedulers_array_size;

Is schedulers_array_size declared or used anywhere?  I can't see any use.

~Andrew

Re: [PATCH v4 5/5] sched: Use the auto-generated list of schedulers

[Jonathan Creekmore]

Andrew Cooper writes:

> On 08/01/16 21:22, Jonathan Creekmore wrote:
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index 2f98a48..91e53c1 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -64,20 +64,10 @@ static void poll_timer_fn(void *data);
>>  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
>> -};
>> +extern const struct scheduler *__start_schedulers_array[], *__end_schedulers_array[];
>> +extern const size_t schedulers_array_size;
>
> Is schedulers_array_size declared or used anywhere?  I can't see any use.

You are right. That is a holdover from a previous version. I will drop that.