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

[PATCH 0/4] 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 Kconfig options for the schedulers and places the appropriate CONFIG_
options around the scheduler list. The second, third, and fourth 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.

Jonathan Creekmore (4):
  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/arch/arm/xen.lds.S      |  4 +++
 xen/arch/x86/xen.lds.S      |  4 +++
 xen/common/Kconfig          | 64 +++++++++++++++++++++++++++++++++++++++++++++
 xen/common/Makefile         |  8 +++---
 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       | 20 +++++++-------
 xen/include/xen/sched-if.h  |  7 ++---
 10 files changed, 95 insertions(+), 20 deletions(-)

-- 
2.6.4

[PATCH 1/4] build: Hook the schedulers into Kconfig

[Jonathan Creekmore]

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(-)

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;
-- 
2.6.4

[PATCH 2/4] 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>
---
 xen/arch/arm/xen.lds.S | 4 ++++
 xen/arch/x86/xen.lds.S | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 0488f37..39a4c86 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);
+       __schedulers_start = .;
+       *(.data.schedulers)
+       __schedulers_end = .;
        *(.data.rel)
        *(.data.rel.*)
        CONSTRUCTORS
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index e18e08f..70caf85 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -88,6 +88,10 @@ SECTIONS
        . = ALIGN(PAGE_SIZE);
        *(.data.page_aligned)
        *(.data)
+       . = ALIGN(8);
+       __schedulers_start = .;
+       *(.data.schedulers)
+       __schedulers_end = .;
        *(.data.rel)
        *(.data.rel.*)
        CONSTRUCTORS
-- 
2.6.4

[PATCH 3/4] 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>
---
 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 4/4] 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>
---
 xen/common/schedule.c      | 24 +++++++-----------------
 xen/include/xen/sched-if.h |  5 -----
 2 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 2f98a48..efbd67d 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 *__schedulers_start[], *__schedulers_end[];
+#define NUM_SCHEDULERS (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
+                        / sizeof(struct scheduler *))
+static const struct scheduler **schedulers = __schedulers_start;
 
 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 1/4] build: Hook the schedulers into Kconfig

[Dario Faggioli]

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

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
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 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 3/4] sched: Register the schedulers into the list

[Dario Faggioli]

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

On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
> 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>
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 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 4/4] sched: Use the auto-generated list of schedulers

[Dario Faggioli]

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

On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
> 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>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 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 1/4] build: Hook the schedulers into Kconfig

[Jonathan Creekmore]

> 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)
> 


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

Re: [PATCH 1/4] build: Hook the schedulers into Kconfig

[Andrew Cooper]

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

Re: [PATCH 2/4] build: Alloc space for sched list in the link file

[Andrew Cooper]

On 17/12/2015 20:59, 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>
> ---
>  xen/arch/arm/xen.lds.S | 4 ++++
>  xen/arch/x86/xen.lds.S | 4 ++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 0488f37..39a4c86 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);
> +       __schedulers_start = .;
> +       *(.data.schedulers)
> +       __schedulers_end = .;

These arrays are only ever used in __init scheduler_init().  They should
be in .init.data rather than .data, which allows their memory to be
reclaimed after boot.

With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 3/4] sched: Register the schedulers into the list

[Andrew Cooper]

On 17/12/2015 20:59, Jonathan Creekmore wrote:
> 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>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Andrew Cooper]

On 17/12/2015 20:59, Jonathan Creekmore wrote:
> 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>
> ---
>  xen/common/schedule.c      | 24 +++++++-----------------
>  xen/include/xen/sched-if.h |  5 -----
>  2 files changed, 7 insertions(+), 22 deletions(-)
>
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 2f98a48..efbd67d 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 *__schedulers_start[], *__schedulers_end[];
> +#define NUM_SCHEDULERS (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
> +                        / sizeof(struct scheduler *))
> +static const struct scheduler **schedulers = __schedulers_start;

You should be able to play some tricks with getting the linker to set a
size of a variable it creates, which would hopefully avoid some of this
complexity.

>  
>  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;
> -

With these changes, you can make the structures themselves static, which
would be a nice tidyup.

~Andrew

>  #define REGISTER_SCHEDULER(x) static const struct scheduler *x##_entry \
>    __used_section(".data.schedulers") = &x;
>  

Re: [PATCH 1/4] build: Hook the schedulers into Kconfig

[Dario Faggioli]

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

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
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 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 0/4] Allow schedulers to be selectable through Kconfig

[Jan Beulich]

>>> On 17.12.15 at 21:59, <jonathan.creekmore@gmail.com> wrote:
> Add machinery to allow the schedulers to be individually selectable
> through the Kconfig interface. The first patch in the series sets up
> the Kconfig options for the schedulers and places the appropriate CONFIG_
> options around the scheduler list. The second, third, and fourth 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.

This is very quickly moving us into a direction that previous
discussion indicated we don't want to move into right away:
A multitude of different configurations that people may be
using. Granted selecting schedulers is pretty self contained,
but if we allow this, where would we draw the line? At the
very least you should have Cc-ed people involved in that
original discussion and having indicated opposition to that
direction.

Jan

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Ian Campbell]

On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
> Add machinery to allow the schedulers to be individually selectable
> through the Kconfig interface.

So I don't want to pick on this series or schedulers specifically here, but
instead discuss the general premise of configurability of hypervisor
binaries, and this happens to be the first. I'm CCing Doug and "THE REST"
from MAINTAINERS

Currently (even with the current switch to Kconfig thus far) we have a
fairly small and manageable set of configurations which any given Xen
binary can be in and in terms of what users are actually running an even
smaller set I believe, most users fiddle with zero options and a small
number with one or two. I'd hazard a guess that the vast majority of Xen
binaries are using a single config today and that the second place is a
fairly distant second.

This means we have avoided the combinatorial explosion of configuration
options which Linux suffers from (which result in things like randconfig
build robots because no one can keep track of it all).

Just to be clear: I'm not at all opposed to more configurability for expert
users who have specific usecases, know what they are doing and are willing
to take responsibility for developments deviating form the norm.

However I am very wary of putting shiny looking nobs in front of the
average user, since they will find them and they will inevitably play with
them and we will end up in the situation where every bug report involves an
additional RTT while we ask for their .config (ok, in reality we'd often
ask at the same time we inevitably have to ask for logs and other key info,
so I guess I'm exaggerating, but still its a worry I think).

As well as support there is obviously a testing matrix impact.

How would people feel about a CONFIG_STANDARD_FEATURESET[0] with the
majority of tweakables depending on !STANDARD_FEATURESET? It would default
Y with a help text which dissuades normal users from touching it ("Say Y,
unless you are willing to pick up the pieces yourself. We do not routinely
test or validate configurations without this option set. We expect you to
offer to fix issues which you find. Beware of the leopard.").[1]

I might even go so far as to suggest that !STANDARD_FEATURESET
configurations would not be subject to security support (i.e. issues which
can _only_ arise with that option disabled are't covered).

The bar for adding a new option which does not depend on
!STANDARD_FEATURESET would be _high_.

Ian.

[0] I really mean CONFIG_STANDARD_CONFIG, but that sounds dumb. I don't
really intend for it to only control "features". CONFIG_STANDARD might be
an alternative.
[1] Hrm, CONFIG_LEOPARD?

        http://www.goodreads.com/quotes/40705-but-the-plans-were-on-display-on-display-i-eventually

    ;-)

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

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

[Jan Beulich]

>>> On 17.12.15 at 21:59, <jonathan.creekmore@gmail.com> wrote:
> +extern const struct scheduler *__schedulers_start[], *__schedulers_end[];
> +#define NUM_SCHEDULERS (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
> +                        / sizeof(struct scheduler *))
> +static const struct scheduler **schedulers = __schedulers_start;

I really wonder whether we should continue follow this route of
__start_ / __stop_ symbols, instead of leveraging gas+ld's
.startof. and .sizeof. operators.

Jan

Re: [PATCH 1/4] build: Hook the schedulers into Kconfig

[George Dunlap]

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
> 

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Jan Beulich]

>>> On 18.12.15 at 11:45, <ian.campbell@citrix.com> wrote:
> How would people feel about a CONFIG_STANDARD_FEATURESET[0] with the
> majority of tweakables depending on !STANDARD_FEATURESET? It would default
> Y with a help text which dissuades normal users from touching it ("Say Y,
> unless you are willing to pick up the pieces yourself. We do not routinely
> test or validate configurations without this option set. We expect you to
> offer to fix issues which you find. Beware of the leopard.").[1]

I like this; I'd suggest using Linux'es CONFIG_EXPERT though,
defaulting to off. One thing I'd like to be investigated then would be
the possibility of even hiding that option's prompt by default, so
that the average user won't say "I'm enough of an expert" just
because of seeing the option. It would then need manual switching
to Y in xen/.config.

> I might even go so far as to suggest that !STANDARD_FEATURESET
> configurations would not be subject to security support (i.e. issues which
> can _only_ arise with that option disabled are't covered).
> 
> The bar for adding a new option which does not depend on
> !STANDARD_FEATURESET would be _high_.

I second both of these.

Jan

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Juergen Gross]

On 18/12/15 11:45, Ian Campbell wrote:
> On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
>> Add machinery to allow the schedulers to be individually selectable
>> through the Kconfig interface.
> 
> So I don't want to pick on this series or schedulers specifically here, but
> instead discuss the general premise of configurability of hypervisor
> binaries, and this happens to be the first. I'm CCing Doug and "THE REST"
> from MAINTAINERS
> 
> Currently (even with the current switch to Kconfig thus far) we have a
> fairly small and manageable set of configurations which any given Xen
> binary can be in and in terms of what users are actually running an even
> smaller set I believe, most users fiddle with zero options and a small
> number with one or two. I'd hazard a guess that the vast majority of Xen
> binaries are using a single config today and that the second place is a
> fairly distant second.
> 
> This means we have avoided the combinatorial explosion of configuration
> options which Linux suffers from (which result in things like randconfig
> build robots because no one can keep track of it all).
> 
> Just to be clear: I'm not at all opposed to more configurability for expert
> users who have specific usecases, know what they are doing and are willing
> to take responsibility for developments deviating form the norm.
> 
> However I am very wary of putting shiny looking nobs in front of the
> average user, since they will find them and they will inevitably play with
> them and we will end up in the situation where every bug report involves an
> additional RTT while we ask for their .config (ok, in reality we'd often
> ask at the same time we inevitably have to ask for logs and other key info,
> so I guess I'm exaggerating, but still its a worry I think).
> 
> As well as support there is obviously a testing matrix impact.
> 
> How would people feel about a CONFIG_STANDARD_FEATURESET[0] with the
> majority of tweakables depending on !STANDARD_FEATURESET? It would default
> Y with a help text which dissuades normal users from touching it ("Say Y,
> unless you are willing to pick up the pieces yourself. We do not routinely
> test or validate configurations without this option set. We expect you to
> offer to fix issues which you find. Beware of the leopard.").[1]

What I'd really like to see in the config options are things like:

CONFIG_BIGMEM (instead of doing it via environment variable),
NR_CPUS, and possibly some other numerical bounds which won't select
a feature, but might be interesting to change for huge servers.


Juergen

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Ian Campbell]

On Fri, 2015-12-18 at 12:08 +0100, Juergen Gross wrote:
> On 18/12/15 11:45, Ian Campbell wrote:
> > On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
> > > Add machinery to allow the schedulers to be individually selectable
> > > through the Kconfig interface.
> > 
> > So I don't want to pick on this series or schedulers specifically here,
> > but
> > instead discuss the general premise of configurability of hypervisor
> > binaries, and this happens to be the first. I'm CCing Doug and "THE
> > REST"
> > from MAINTAINERS
> > 
> > Currently (even with the current switch to Kconfig thus far) we have a
> > fairly small and manageable set of configurations which any given Xen
> > binary can be in and in terms of what users are actually running an
> > even
> > smaller set I believe, most users fiddle with zero options and a small
> > number with one or two. I'd hazard a guess that the vast majority of
> > Xen
> > binaries are using a single config today and that the second place is a
> > fairly distant second.
> > 
> > This means we have avoided the combinatorial explosion of configuration
> > options which Linux suffers from (which result in things like
> > randconfig
> > build robots because no one can keep track of it all).
> > 
> > Just to be clear: I'm not at all opposed to more configurability for
> > expert
> > users who have specific usecases, know what they are doing and are
> > willing
> > to take responsibility for developments deviating form the norm.
> > 
> > However I am very wary of putting shiny looking nobs in front of the
> > average user, since they will find them and they will inevitably play
> > with
> > them and we will end up in the situation where every bug report
> > involves an
> > additional RTT while we ask for their .config (ok, in reality we'd
> > often
> > ask at the same time we inevitably have to ask for logs and other key
> > info,
> > so I guess I'm exaggerating, but still its a worry I think).
> > 
> > As well as support there is obviously a testing matrix impact.
> > 
> > How would people feel about a CONFIG_STANDARD_FEATURESET[0] with the
> > majority of tweakables depending on !STANDARD_FEATURESET? It would
> > default
> > Y with a help text which dissuades normal users from touching it ("Say
> > Y,
> > unless you are willing to pick up the pieces yourself. We do not
> > routinely
> > test or validate configurations without this option set. We expect you
> > to
> > offer to fix issues which you find. Beware of the leopard.").[1]
> 
> What I'd really like to see in the config options are things like:

Please can avoid getting derailed in to discussing the merits or otherwise
of particular potential configurables (and in particular whether they come
under standard/expert of not) for the time being.

(i.e. this is not, yet, a thread for everyone to list their own desires for
particular options)

Ian.

Re: [PATCH 1/4] build: Hook the schedulers into Kconfig

[Dario Faggioli]

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

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
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 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 0/4] Allow schedulers to be selectable through Kconfig

[Jan Beulich]

>>> On 18.12.15 at 12:08, <JGross@suse.com> wrote:
> What I'd really like to see in the config options are things like:
> 
> CONFIG_BIGMEM (instead of doing it via environment variable),
> NR_CPUS, and possibly some other numerical bounds which won't select
> a feature, but might be interesting to change for huge servers.

Yes, anything currently selectable via make option should be
converted (whether or not hidden behind the global gating option
is an open question); I have this on my todo list, but of course
others helping out would be appreciated.

Jan

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Juergen Gross]

On 18/12/15 12:30, Jan Beulich wrote:
>>>> On 18.12.15 at 12:08, <JGross@suse.com> wrote:
>> What I'd really like to see in the config options are things like:
>>
>> CONFIG_BIGMEM (instead of doing it via environment variable),
>> NR_CPUS, and possibly some other numerical bounds which won't select
>> a feature, but might be interesting to change for huge servers.
> 
> Yes, anything currently selectable via make option should be
> converted (whether or not hidden behind the global gating option
> is an open question); I have this on my todo list, but of course
> others helping out would be appreciated.

As I have some further tests on huge systems on my todo list I could
do the BIGMEM and NR_CPUS in January.


Juergen

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

[Jonathan Creekmore]

Jan Beulich writes:

>>>> On 17.12.15 at 21:59, <jonathan.creekmore@gmail.com> wrote:
>> +extern const struct scheduler *__schedulers_start[], *__schedulers_end[];
>> +#define NUM_SCHEDULERS (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
>> +                        / sizeof(struct scheduler *))
>> +static const struct scheduler **schedulers = __schedulers_start;
>
> I really wonder whether we should continue follow this route of
> __start_ / __stop_ symbols, instead of leveraging gas+ld's
> .startof. and .sizeof. operators.

So, I would love to explore using those operators if you can give me
some link to documentation for using them. I have yet to be able to find
a construct for LD and GCC that works correctly for generating
equivalent symbols.

Re: [PATCH 2/4] build: Alloc space for sched list in the link file

[Jonathan Creekmore]

> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 17/12/2015 20:59, 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>
>> ---
>> xen/arch/arm/xen.lds.S | 4 ++++
>> xen/arch/x86/xen.lds.S | 4 ++++
>> 2 files changed, 8 insertions(+)
>> 
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index 0488f37..39a4c86 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);
>> +       __schedulers_start = .;
>> +       *(.data.schedulers)
>> +       __schedulers_end = .;
> 
> These arrays are only ever used in __init scheduler_init().  They should
> be in .init.data rather than .data, which allows their memory to be
> reclaimed after boot.
> 
> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

So, they are used in scheduler_init() which is marked __init, but scheduler_alloc
also uses that array (and did before my patch) and it is not marked __init.

Re: [PATCH 1/4] build: Hook the schedulers into Kconfig

[Jonathan Creekmore]

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.

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

[Jan Beulich]

>>> On 18.12.15 at 17:00, <jonathan.creekmore@gmail.com> wrote:
> Jan Beulich writes:
>>>>> On 17.12.15 at 21:59, <jonathan.creekmore@gmail.com> wrote:
>>> +extern const struct scheduler *__schedulers_start[], *__schedulers_end[];
>>> +#define NUM_SCHEDULERS 
> (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
>>> +                        / sizeof(struct scheduler *))
>>> +static const struct scheduler **schedulers = __schedulers_start;
>>
>> I really wonder whether we should continue follow this route of
>> __start_ / __stop_ symbols, instead of leveraging gas+ld's
>> .startof. and .sizeof. operators.
> 
> So, I would love to explore using those operators if you can give me
> some link to documentation for using them. I have yet to be able to find
> a construct for LD and GCC that works correctly for generating
> equivalent symbols.

With binutils docs missing any notion of these, I can only refer
you to binutils sources, I'm afraid.

Jan

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

[Jonathan Creekmore]

Andrew Cooper writes:

> On 17/12/2015 20:59, Jonathan Creekmore wrote:
>> 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>
>> ---
>>  xen/common/schedule.c      | 24 +++++++-----------------
>>  xen/include/xen/sched-if.h |  5 -----
>>  2 files changed, 7 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index 2f98a48..efbd67d 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 *__schedulers_start[], *__schedulers_end[];
>> +#define NUM_SCHEDULERS (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
>> +                        / sizeof(struct scheduler *))
>> +static const struct scheduler **schedulers = __schedulers_start;
>
> You should be able to play some tricks with getting the linker to set a
> size of a variable it creates, which would hopefully avoid some of this
> complexity.

Yeah, that looks fairly straightforward. I can do that in a v2.

>>
>>  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;
>> -
>
> With these changes, you can make the structures themselves static, which
> would be a nice tidyup.
>

I like that as well and will handle that in the v2.

Re: [PATCH 2/4] build: Alloc space for sched list in the link file

[Andrew Cooper]

On 18/12/15 16:40, Jonathan Creekmore wrote:
>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> On 17/12/2015 20:59, 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>
>>> ---
>>> xen/arch/arm/xen.lds.S | 4 ++++
>>> xen/arch/x86/xen.lds.S | 4 ++++
>>> 2 files changed, 8 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>>> index 0488f37..39a4c86 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);
>>> +       __schedulers_start = .;
>>> +       *(.data.schedulers)
>>> +       __schedulers_end = .;
>> These arrays are only ever used in __init scheduler_init().  They should
>> be in .init.data rather than .data, which allows their memory to be
>> reclaimed after boot.
>>
>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> So, they are used in scheduler_init() which is marked __init, but scheduler_alloc
> also uses that array (and did before my patch) and it is not marked __init.
>

Ah yes - so they are.  Apologies for the noise.  This should be in .data
and my R-b stands.

~Andrew

Re: [PATCH 2/4] build: Alloc space for sched list in the link file

[Jan Beulich]

>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote:
> On 18/12/15 16:40, Jonathan Creekmore wrote:
>>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>
>>> On 17/12/2015 20:59, 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>
>>>> ---
>>>> xen/arch/arm/xen.lds.S | 4 ++++
>>>> xen/arch/x86/xen.lds.S | 4 ++++
>>>> 2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>>>> index 0488f37..39a4c86 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);
>>>> +       __schedulers_start = .;
>>>> +       *(.data.schedulers)
>>>> +       __schedulers_end = .;
>>> These arrays are only ever used in __init scheduler_init().  They should
>>> be in .init.data rather than .data, which allows their memory to be
>>> reclaimed after boot.
>>>
>>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> So, they are used in scheduler_init() which is marked __init, but 
> scheduler_alloc
>> also uses that array (and did before my patch) and it is not marked __init.
> 
> Ah yes - so they are.  Apologies for the noise.  This should be in .data
> and my R-b stands.

In .rodata perhaps?

Jan

Re: [PATCH 2/4] build: Alloc space for sched list in the link file

[Andrew Cooper]

On 18/12/15 17:07, Jan Beulich wrote:
>>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote:
>> On 18/12/15 16:40, Jonathan Creekmore wrote:
>>>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>
>>>> On 17/12/2015 20:59, 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>
>>>>> ---
>>>>> xen/arch/arm/xen.lds.S | 4 ++++
>>>>> xen/arch/x86/xen.lds.S | 4 ++++
>>>>> 2 files changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>>>>> index 0488f37..39a4c86 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);
>>>>> +       __schedulers_start = .;
>>>>> +       *(.data.schedulers)
>>>>> +       __schedulers_end = .;
>>>> These arrays are only ever used in __init scheduler_init().  They should
>>>> be in .init.data rather than .data, which allows their memory to be
>>>> reclaimed after boot.
>>>>
>>>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> So, they are used in scheduler_init() which is marked __init, but 
>> scheduler_alloc
>>> also uses that array (and did before my patch) and it is not marked __init.
>> Ah yes - so they are.  Apologies for the noise.  This should be in .data
>> and my R-b stands.
> In .rodata perhaps?

Ah yes - they don't need modifying at all.  They are just pointers to
each of the struct scheduler ops.  .rodata it is.

~Andrew

Re: [PATCH 2/4] build: Alloc space for sched list in the link file

[Jonathan Creekmore]

> On Dec 18, 2015, at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote:
>> On 18/12/15 16:40, Jonathan Creekmore wrote:
>>>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> 
>>>> On 17/12/2015 20:59, 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>
>>>>> ---
>>>>> xen/arch/arm/xen.lds.S | 4 ++++
>>>>> xen/arch/x86/xen.lds.S | 4 ++++
>>>>> 2 files changed, 8 insertions(+)
>>>>> 
>>>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>>>>> index 0488f37..39a4c86 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);
>>>>> +       __schedulers_start = .;
>>>>> +       *(.data.schedulers)
>>>>> +       __schedulers_end = .;
>>>> These arrays are only ever used in __init scheduler_init().  They should
>>>> be in .init.data rather than .data, which allows their memory to be
>>>> reclaimed after boot.
>>>> 
>>>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> So, they are used in scheduler_init() which is marked __init, but 
>> scheduler_alloc
>>> also uses that array (and did before my patch) and it is not marked __init.
>> 
>> Ah yes - so they are.  Apologies for the noise.  This should be in .data
>> and my R-b stands.
> 
> In .rodata perhaps?

I initially put it in .rodata, but the current algorithm for choosing the scheduler NULLs out
the pointers in the array if the global_init() function fails. To emulate that behavior, I had to 
move the section to .data instead of .rodata.

Re: [PATCH 2/4] build: Alloc space for sched list in the link file

[Andrew Cooper]

On 18/12/15 17:11, Jonathan Creekmore wrote:
>> On Dec 18, 2015, at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>
>>>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote:
>>> On 18/12/15 16:40, Jonathan Creekmore wrote:
>>>>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>>
>>>>> On 17/12/2015 20:59, 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>
>>>>>> ---
>>>>>> xen/arch/arm/xen.lds.S | 4 ++++
>>>>>> xen/arch/x86/xen.lds.S | 4 ++++
>>>>>> 2 files changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>>>>>> index 0488f37..39a4c86 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);
>>>>>> +       __schedulers_start = .;
>>>>>> +       *(.data.schedulers)
>>>>>> +       __schedulers_end = .;
>>>>> These arrays are only ever used in __init scheduler_init().  They should
>>>>> be in .init.data rather than .data, which allows their memory to be
>>>>> reclaimed after boot.
>>>>>
>>>>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> So, they are used in scheduler_init() which is marked __init, but 
>>> scheduler_alloc
>>>> also uses that array (and did before my patch) and it is not marked __init.
>>> Ah yes - so they are.  Apologies for the noise.  This should be in .data
>>> and my R-b stands.
>> In .rodata perhaps?
> I initially put it in .rodata, but the current algorithm for choosing the scheduler NULLs out
> the pointers in the array if the global_init() function fails. To emulate that behavior, I had to 
> move the section to .data instead of .rodata.

Hmm.  That is unfortunate.

Best to leave it in .data, and leave fixing the scheduler initialisation
to the other scheduler clean-up work; there is plenty of other cleanup
work to do, and conflating the two issues is best avoided.

~Andrew

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

[Jonathan Creekmore]

> On Dec 18, 2015, at 10:43 AM, Jan Beulich <jbeulich@suse.com> wrote:
> 
>>>> On 18.12.15 at 17:00, <jonathan.creekmore@gmail.com> wrote:
>> Jan Beulich writes:
>>>>>> On 17.12.15 at 21:59, <jonathan.creekmore@gmail.com> wrote:
>>>> +extern const struct scheduler *__schedulers_start[], *__schedulers_end[];
>>>> +#define NUM_SCHEDULERS 
>> (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
>>>> +                        / sizeof(struct scheduler *))
>>>> +static const struct scheduler **schedulers = __schedulers_start;
>>> 
>>> I really wonder whether we should continue follow this route of
>>> __start_ / __stop_ symbols, instead of leveraging gas+ld's
>>> .startof. and .sizeof. operators.
>> 
>> So, I would love to explore using those operators if you can give me
>> some link to documentation for using them. I have yet to be able to find
>> a construct for LD and GCC that works correctly for generating
>> equivalent symbols.
> 
> With binutils docs missing any notion of these, I can only refer
> you to binutils sources, I'm afraid.
> 

Well, I would prefer not to delve into undocumented behavior in what
should be a fairly straightforward patch set, so I plan on keeping the use
of the __start and __stop symbols.

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

[Andrew Cooper]

On 18/12/15 17:24, Jonathan Creekmore wrote:
>> On Dec 18, 2015, at 10:43 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>>>>> On 18.12.15 at 17:00, <jonathan.creekmore@gmail.com> wrote:
>>> Jan Beulich writes:
>>>>>>> On 17.12.15 at 21:59, <jonathan.creekmore@gmail.com> wrote:
>>>>> +extern const struct scheduler *__schedulers_start[], *__schedulers_end[];
>>>>> +#define NUM_SCHEDULERS 
>>> (((uintptr_t)__schedulers_end-(uintptr_t)__schedulers_start) \
>>>>> +                        / sizeof(struct scheduler *))
>>>>> +static const struct scheduler **schedulers = __schedulers_start;
>>>> I really wonder whether we should continue follow this route of
>>>> __start_ / __stop_ symbols, instead of leveraging gas+ld's
>>>> .startof. and .sizeof. operators.
>>> So, I would love to explore using those operators if you can give me
>>> some link to documentation for using them. I have yet to be able to find
>>> a construct for LD and GCC that works correctly for generating
>>> equivalent symbols.
>> With binutils docs missing any notion of these, I can only refer
>> you to binutils sources, I'm afraid.
>>
> Well, I would prefer not to delve into undocumented behavior in what
> should be a fairly straightforward patch set, so I plan on keeping the use
> of the __start and __stop symbols.

One hint to pick up from the Linux side of things is that you can do:

extern const struct scheduler *__schedulers_start[], *__schedulers_end[];
#define NUM_SCHEDULERS (__schedulers_end - __schedulers_start)

and rely on the semantics of pointer arithmetic.

~Andrew

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Doug Goldstein]

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

On 12/18/15 4:45 AM, Ian Campbell wrote:
> On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
>> Add machinery to allow the schedulers to be individually selectable
>> through the Kconfig interface.
> 
> So I don't want to pick on this series or schedulers specifically here, but
> instead discuss the general premise of configurability of hypervisor
> binaries, and this happens to be the first. I'm CCing Doug and "THE REST"
> from MAINTAINERS
> 
> Currently (even with the current switch to Kconfig thus far) we have a
> fairly small and manageable set of configurations which any given Xen
> binary can be in and in terms of what users are actually running an even
> smaller set I believe, most users fiddle with zero options and a small
> number with one or two. I'd hazard a guess that the vast majority of Xen
> binaries are using a single config today and that the second place is a
> fairly distant second.
> 
> This means we have avoided the combinatorial explosion of configuration
> options which Linux suffers from (which result in things like randconfig
> build robots because no one can keep track of it all).
> 
> Just to be clear: I'm not at all opposed to more configurability for expert
> users who have specific usecases, know what they are doing and are willing
> to take responsibility for developments deviating form the norm.
> 
> However I am very wary of putting shiny looking nobs in front of the
> average user, since they will find them and they will inevitably play with
> them and we will end up in the situation where every bug report involves an
> additional RTT while we ask for their .config (ok, in reality we'd often
> ask at the same time we inevitably have to ask for logs and other key info,
> so I guess I'm exaggerating, but still its a worry I think).
> 
> As well as support there is obviously a testing matrix impact.
> 
> How would people feel about a CONFIG_STANDARD_FEATURESET[0] with the
> majority of tweakables depending on !STANDARD_FEATURESET? It would default
> Y with a help text which dissuades normal users from touching it ("Say Y,
> unless you are willing to pick up the pieces yourself. We do not routinely
> test or validate configurations without this option set. We expect you to
> offer to fix issues which you find. Beware of the leopard.").[1]
> 
> I might even go so far as to suggest that !STANDARD_FEATURESET
> configurations would not be subject to security support (i.e. issues which
> can _only_ arise with that option disabled are't covered).
> 
> The bar for adding a new option which does not depend on
> !STANDARD_FEATURESET would be _high_.
> 
> Ian.
> 
> [0] I really mean CONFIG_STANDARD_CONFIG, but that sounds dumb. I don't
> really intend for it to only control "features". CONFIG_STANDARD might be
> an alternative.
> [1] Hrm, CONFIG_LEOPARD?
> 
>         http://www.goodreads.com/quotes/40705-but-the-plans-were-on-display-on-display-i-eventually
> 
>     ;-)
> 

So I know this is going to be a thread hijack but I think its important
for me to explain where Jonathan Creekmore and I are coming from with
our recently submitted patches.

We recently took over a project based on an old version of Xen that has
a lot of customization. Part of our goal was to understand those
modifications and re-implement them in a fashion that would be
acceptable to upstream. So for example in the code base we worked with
all other schedulers were entirely ripped out of the code base. Jonathan
has reworked this to make it possible to disable each scheduler
independently. Additionally Jonathan improved the way that extra
schedulers can be added in the future.

Effectively we want to engage the Xen community as much as possible and
come up with solutions that are workable for the community as well as
our needs. Now here's why I believe in this approach. I've looked at
some of the other XenProject derived projects (e.g. XenServer, Qubes,
OpenXT) and Linux distros shipping Xen (Ubuntu, Debian, Gentoo, Yocto,
etc) and I saw that they all contain a number of patches against
XenProject. To me this weakens the overall project by everyone
maintaining their own fork. Everyone's goal should be to work on
upstreaming changes in a fashion that is acceptable to the community in
an agnostic fashion. Recently I took Joanna Rutkowska to task for not
contributing work upstream and maintaining downstream changes and
expecting Xen to confirm to her project's needs [1]. But on the flip
side of the coin Ian Campbell has been helping Debian to upstream
changes they need [2].

In the end I really see the primary people that build Xen on their own
as project maintainers (XenServer, Qubes, OpenXT) or distro maintainers
(Ubuntu, Debian, Gentoo, Yocto) or "expert" users. Most people will use
Xen as it comes packaged for them already and the Kconfig changes give
the project/distro/experts the flexibility they need to build up Xen
without maintaining downstream patches. So these won't really be shiny
knobs for users to twiddle.

What provides the defaults that users should use are the defconfig
files. The reason this doesn't work out so well for Linux is that there
are really not defaults that work for everyone. I believe for a long
time the x86_64 defconfig was a setup for Linus' development laptop and
most of the other arches defconfigs were setup for their respective
maintainers as well. But in the case of Xen the defconfig will be the
defaults and we don't even require the user to run `make *config` so it
really is more of an "expert" option already vs Linux where you must
first run `make *config` before building your kernel.

[1]
http://lists.xenproject.org/archives/html/xen-devel/2015-11/msg00813.html
[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=805508

-- 
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 0/4] Allow schedulers to be selectable through Kconfig

[Andrew Cooper]

On 18/12/15 17:56, Doug Goldstein wrote:
> On 12/18/15 4:45 AM, Ian Campbell wrote:
>> On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
>>> Add machinery to allow the schedulers to be individually selectable
>>> through the Kconfig interface.
>> So I don't want to pick on this series or schedulers specifically here, but
>> instead discuss the general premise of configurability of hypervisor
>> binaries, and this happens to be the first. I'm CCing Doug and "THE REST"
>> from MAINTAINERS
>>
>> Currently (even with the current switch to Kconfig thus far) we have a
>> fairly small and manageable set of configurations which any given Xen
>> binary can be in and in terms of what users are actually running an even
>> smaller set I believe, most users fiddle with zero options and a small
>> number with one or two. I'd hazard a guess that the vast majority of Xen
>> binaries are using a single config today and that the second place is a
>> fairly distant second.
>>
>> This means we have avoided the combinatorial explosion of configuration
>> options which Linux suffers from (which result in things like randconfig
>> build robots because no one can keep track of it all).
>>
>> Just to be clear: I'm not at all opposed to more configurability for expert
>> users who have specific usecases, know what they are doing and are willing
>> to take responsibility for developments deviating form the norm.
>>
>> However I am very wary of putting shiny looking nobs in front of the
>> average user, since they will find them and they will inevitably play with
>> them and we will end up in the situation where every bug report involves an
>> additional RTT while we ask for their .config (ok, in reality we'd often
>> ask at the same time we inevitably have to ask for logs and other key info,
>> so I guess I'm exaggerating, but still its a worry I think).
>>
>> As well as support there is obviously a testing matrix impact.
>>
>> How would people feel about a CONFIG_STANDARD_FEATURESET[0] with the
>> majority of tweakables depending on !STANDARD_FEATURESET? It would default
>> Y with a help text which dissuades normal users from touching it ("Say Y,
>> unless you are willing to pick up the pieces yourself. We do not routinely
>> test or validate configurations without this option set. We expect you to
>> offer to fix issues which you find. Beware of the leopard.").[1]
>>
>> I might even go so far as to suggest that !STANDARD_FEATURESET
>> configurations would not be subject to security support (i.e. issues which
>> can _only_ arise with that option disabled are't covered).
>>
>> The bar for adding a new option which does not depend on
>> !STANDARD_FEATURESET would be _high_.
>>
>> Ian.
>>
>> [0] I really mean CONFIG_STANDARD_CONFIG, but that sounds dumb. I don't
>> really intend for it to only control "features". CONFIG_STANDARD might be
>> an alternative.
>> [1] Hrm, CONFIG_LEOPARD?
>>
>>         http://www.goodreads.com/quotes/40705-but-the-plans-were-on-display-on-display-i-eventually
>>
>>     ;-)
>>
> So I know this is going to be a thread hijack but I think its important
> for me to explain where Jonathan Creekmore and I are coming from with
> our recently submitted patches.
>
> We recently took over a project based on an old version of Xen that has
> a lot of customization. Part of our goal was to understand those
> modifications and re-implement them in a fashion that would be
> acceptable to upstream. So for example in the code base we worked with
> all other schedulers were entirely ripped out of the code base. Jonathan
> has reworked this to make it possible to disable each scheduler
> independently. Additionally Jonathan improved the way that extra
> schedulers can be added in the future.
>
> Effectively we want to engage the Xen community as much as possible and
> come up with solutions that are workable for the community as well as
> our needs. Now here's why I believe in this approach. I've looked at
> some of the other XenProject derived projects (e.g. XenServer, Qubes,
> OpenXT) and Linux distros shipping Xen (Ubuntu, Debian, Gentoo, Yocto,
> etc) and I saw that they all contain a number of patches against
> XenProject. To me this weakens the overall project by everyone
> maintaining their own fork. Everyone's goal should be to work on
> upstreaming changes in a fashion that is acceptable to the community in
> an agnostic fashion. Recently I took Joanna Rutkowska to task for not
> contributing work upstream and maintaining downstream changes and
> expecting Xen to confirm to her project's needs [1]. But on the flip
> side of the coin Ian Campbell has been helping Debian to upstream
> changes they need [2].
>
> In the end I really see the primary people that build Xen on their own
> as project maintainers (XenServer, Qubes, OpenXT) or distro maintainers
> (Ubuntu, Debian, Gentoo, Yocto) or "expert" users. Most people will use
> Xen as it comes packaged for them already and the Kconfig changes give
> the project/distro/experts the flexibility they need to build up Xen
> without maintaining downstream patches. So these won't really be shiny
> knobs for users to twiddle.
>
> What provides the defaults that users should use are the defconfig
> files. The reason this doesn't work out so well for Linux is that there
> are really not defaults that work for everyone. I believe for a long
> time the x86_64 defconfig was a setup for Linus' development laptop and
> most of the other arches defconfigs were setup for their respective
> maintainers as well. But in the case of Xen the defconfig will be the
> defaults and we don't even require the user to run `make *config` so it
> really is more of an "expert" option already vs Linux where you must
> first run `make *config` before building your kernel.
>
> [1]
> http://lists.xenproject.org/archives/html/xen-devel/2015-11/msg00813.html
> [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=805508

The distro packages and maintainers are likely to be the vast majority
of users of the config system, and they by definition know what they are
doing.

Next we get onto the few people casually using Xen.  As the defaults run
automatically, they need not even know that kconfig exists under the
hood, unless they go looking for it.

Anyone who runs `make menuconfig` thinks they know what they are doing,
and will ignore a wall of text telling them that things may be
dangerous.  They implicitly acknowledge responsibility for shooting
themselves in the foot.  At the end of the day, technical measures do
not help in defending against stupidity.

The fact that the defconfig gets applied automatically mitigates against
people playing with options simply because the need to to get something
out in the end.  This is a strict improvement over the current state of
play for Linux.

There will be options which should live behind a CONFIG_EXPERT (or
equivalent) (e.g. lock profiling?) but these should be in the minority. 
Another advantage we have is that, given Xen's size, it is never going
to have the quantity of options currently available for Linux.

As long as the defconfig files are kept up to date as new features are
added, everything will be fine.

~Andrew

P.S. One very good side effect of the Linux options with randconfig is
that it helps force clean APIs on the code.  The code in Xen has grown
organically without these stricter boundaries, and I will be looking to
clean bits of this up going forwards.

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Ian Campbell]

On Fri, 2015-12-18 at 11:56 -0600, Doug Goldstein wrote:
> In the end I really see the primary people that build Xen on their own
> as project maintainers (XenServer, Qubes, OpenXT) or distro maintainers
> (Ubuntu, Debian, Gentoo, Yocto) or "expert" users. Most people will use
> Xen as it comes packaged for them already and the Kconfig changes give
> the project/distro/experts the flexibility they need to build up Xen
> without maintaining downstream patches. So these won't really be shiny
> knobs for users to twiddle.

In this context I would consider the distro maintainers (at least for
generic, non-Xen specialised distros, e.g. XenServer and QUBES don't fit
here) to be users.

I don't think we want such distros to all be shipping subtly varying
versions of Xen any more than I want individual end users to be doing so.
Users of community (rather than Enterprise/Commercial) distros use many of
our upstream resources, including reporting bugs on xen-users and -devel.

But of course you (quite naturally, I think) expect that they will want to
play with these things, and I think that they will be no less prone to
fiddling with the shiny nobs than a individual user would be.

I don't want to distro users coming to use for support because they tried
to follow http://wiki.xenproject.org/wiki/Credit2_Scheduler_Development#Usi
ng_Credit2 and it didn't work, only to find that their distro has decided
to turn off credit2 in the packages, for whatever random reason the package
maintainer had.

I don't see this as contrary to your stated goals (e.g. ripping out all the
other schedulers), but I consider you to be within the expert camp for
wanting to do so (and having the chops to handle whatever pieces you find
yourselves with). I have no objections at all to allowing experts such as
yourselves to configure things and I applaud you for doing this in an
upstream way (it is the right thing to do).

My concern is that while you rightly consider yourselves expert enough and
are building something for a specific (and AIUI targeted) use case many
normal users tend to think that if they are expert enough to find and flip
the switch then they are expert enough to deal with the consequences, when
they are not and/or they do not have the specific use case which the switch
was added to support i.e. they want common or garden Xen and we want that
to mean the same for everyone.

It's those people (including general purpose distro maintainers) who I
think need to be strongly discouraged from messing with these options
because there will be a strong gravity towards them doing so.

> What provides the defaults that users should use are the defconfig
> files. The reason this doesn't work out so well for Linux is that there
> are really not defaults that work for everyone.

... and sadly by switching Xen to Kconfig this expectation _is_ going to
flow over to us, once someone sees "make menuconfig" mentioned somewhere
they are going to get their Linux head on and start messing with it.

>  But on the flip
> side of the coin Ian Campbell has been helping Debian to upstream
> changes they need [2].

BTW, I'm also a Debian Developer (although not the Xen package maintainer),
so I am doing this with as much downstream as upstream hat on.

Ian.

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

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Jonathan Creekmore]

Ian Campbell writes:

> I don't see this as contrary to your stated goals (e.g. ripping out all the
> other schedulers), but I consider you to be within the expert camp for
> wanting to do so (and having the chops to handle whatever pieces you find
> yourselves with). I have no objections at all to allowing experts such as
> yourselves to configure things and I applaud you for doing this in an
> upstream way (it is the right thing to do).
>
> My concern is that while you rightly consider yourselves expert enough and
> are building something for a specific (and AIUI targeted) use case many
> normal users tend to think that if they are expert enough to find and flip
> the switch then they are expert enough to deal with the consequences, when
> they are not and/or they do not have the specific use case which the switch
> was added to support i.e. they want common or garden Xen and we want that
> to mean the same for everyone.
>
> It's those people (including general purpose distro maintainers) who I
> think need to be strongly discouraged from messing with these options
> because there will be a strong gravity towards them doing so.

So, if I add a patch in a v3 of this series that introduces a
CONFIG_EXPERT option and hides all of the scheduler options behind that,
would that be acceptible? That is a proposal that was mentioned on this
thread before. Is there any specific language that you would like to
have around the CONFIG_EXPERT option?

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Jan Beulich]

>>> On 07.01.16 at 15:01, <jonathan.creekmore@gmail.com> wrote:

> Ian Campbell writes:
> 
>> I don't see this as contrary to your stated goals (e.g. ripping out all the
>> other schedulers), but I consider you to be within the expert camp for
>> wanting to do so (and having the chops to handle whatever pieces you find
>> yourselves with). I have no objections at all to allowing experts such as
>> yourselves to configure things and I applaud you for doing this in an
>> upstream way (it is the right thing to do).
>>
>> My concern is that while you rightly consider yourselves expert enough and
>> are building something for a specific (and AIUI targeted) use case many
>> normal users tend to think that if they are expert enough to find and flip
>> the switch then they are expert enough to deal with the consequences, when
>> they are not and/or they do not have the specific use case which the switch
>> was added to support i.e. they want common or garden Xen and we want that
>> to mean the same for everyone.
>>
>> It's those people (including general purpose distro maintainers) who I
>> think need to be strongly discouraged from messing with these options
>> because there will be a strong gravity towards them doing so.
> 
> So, if I add a patch in a v3 of this series that introduces a
> CONFIG_EXPERT option and hides all of the scheduler options behind that,
> would that be acceptible? That is a proposal that was mentioned on this
> thread before.

With me asking for that option to not have a visible prompt by default,
but nevertheless being settable. I do realize that this may not be
possible with the current kconfig tool, but that's imo the only way to
keep people from playing with expert options just because they see
there's a prompt. No textual warning will help this, I'm afraid.

Jan

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Ian Campbell]

On Thu, 2016-01-07 at 07:37 -0700, Jan Beulich wrote:
> > > > On 07.01.16 at 15:01, <jonathan.creekmore@gmail.com> wrote:
> 
> > Ian Campbell writes:
> > 
> > > I don't see this as contrary to your stated goals (e.g. ripping out all the
> > > other schedulers), but I consider you to be within the expert camp for
> > > wanting to do so (and having the chops to handle whatever pieces you find
> > > yourselves with). I have no objections at all to allowing experts such as
> > > yourselves to configure things and I applaud you for doing this in an
> > > upstream way (it is the right thing to do).
> > > 
> > > My concern is that while you rightly consider yourselves expert enough and
> > > are building something for a specific (and AIUI targeted) use case many
> > > normal users tend to think that if they are expert enough to find and flip
> > > the switch then they are expert enough to deal with the consequences, when
> > > they are not and/or they do not have the specific use case which the switch
> > > was added to support i.e. they want common or garden Xen and we want that
> > > to mean the same for everyone.
> > > 
> > > It's those people (including general purpose distro maintainers) who I
> > > think need to be strongly discouraged from messing with these options
> > > because there will be a strong gravity towards them doing so.
> > 
> > So, if I add a patch in a v3 of this series that introduces a
> > CONFIG_EXPERT option and hides all of the scheduler options behind that,
> > would that be acceptible? That is a proposal that was mentioned on this
> > thread before.

Thinking about it I think I'd avoid the specific name CONFIG_EXPERT due to
the expectations which Linux's use of the name has set.

If we invert the sense then we could call it e.g. CONFIG_STANDARD_PLATFORM
and default it to y, I expect it will be easier to discourage people from
turning such an option off than to discourage them from turning something
like CONFIG_EXPERT on.

> With me asking for that option to not have a visible prompt by default,
> but nevertheless being settable. I do realize that this may not be
> possible with the current kconfig tool, but that's imo the only way to
> keep people from playing with expert options just because they see
> there's a prompt. No textual warning will help this, I'm afraid.

While I have reasonably strong opinions about this issue, I do not think
they warrant forking Kconfig over.

With a suitably strong wording IMHO we have covered ourselves sufficiently.

Ian.

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Doug Goldstein]

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

On 1/7/16 9:00 AM, Ian Campbell wrote:
> On Thu, 2016-01-07 at 07:37 -0700, Jan Beulich wrote:
>>>>> On 07.01.16 at 15:01, <jonathan.creekmore@gmail.com> wrote:
>>
>>> Ian Campbell writes:
>>>
>>>> I don't see this as contrary to your stated goals (e.g. ripping out all the
>>>> other schedulers), but I consider you to be within the expert camp for
>>>> wanting to do so (and having the chops to handle whatever pieces you find
>>>> yourselves with). I have no objections at all to allowing experts such as
>>>> yourselves to configure things and I applaud you for doing this in an
>>>> upstream way (it is the right thing to do).
>>>>
>>>> My concern is that while you rightly consider yourselves expert enough and
>>>> are building something for a specific (and AIUI targeted) use case many
>>>> normal users tend to think that if they are expert enough to find and flip
>>>> the switch then they are expert enough to deal with the consequences, when
>>>> they are not and/or they do not have the specific use case which the switch
>>>> was added to support i.e. they want common or garden Xen and we want that
>>>> to mean the same for everyone.
>>>>
>>>> It's those people (including general purpose distro maintainers) who I
>>>> think need to be strongly discouraged from messing with these options
>>>> because there will be a strong gravity towards them doing so.
>>>
>>> So, if I add a patch in a v3 of this series that introduces a
>>> CONFIG_EXPERT option and hides all of the scheduler options behind that,
>>> would that be acceptible? That is a proposal that was mentioned on this
>>> thread before.
> 
> Thinking about it I think I'd avoid the specific name CONFIG_EXPERT due to
> the expectations which Linux's use of the name has set.



> 
> If we invert the sense then we could call it e.g. CONFIG_STANDARD_PLATFORM
> and default it to y, I expect it will be easier to discourage people from
> turning such an option off than to discourage them from turning something
> like CONFIG_EXPERT on.

So I actually like this approach. Maybe tweak the name slightly to
CONFIG_SUPPORTED_XEN? It can force settings a certain way and can also
make menus invisible. But I'm hoping we can get Jan's buy in here before
we post any patchset.

> 
>> With me asking for that option to not have a visible prompt by default,
>> but nevertheless being settable. I do realize that this may not be
>> possible with the current kconfig tool, but that's imo the only way to
>> keep people from playing with expert options just because they see
>> there's a prompt. No textual warning will help this, I'm afraid.
> 
> While I have reasonably strong opinions about this issue, I do not think
> they warrant forking Kconfig over.
> 
> With a suitably strong wording IMHO we have covered ourselves sufficiently.
> 
> Ian.
> 

So Jonathan and I have been messing with the hidden option behind a
non-menu option (e.g. environment variable). The only way we can get it
to work is to require the environment variable to be passed at
configuration time and at build time and I doubt you'd want the steps to
be "CONFIG_EXPERT=n make menuconfig && CONFIG_EXPERT=n make" to build
Xen. We'll play with this some more if that's desired but given Ian's
response I don't know if it is.

-- 
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 0/4] Allow schedulers to be selectable through Kconfig

[Ian Campbell]

On Thu, 2016-01-07 at 15:00 +0000, Ian Campbell wrote:
> On Thu, 2016-01-07 at 07:37 -0700, Jan Beulich wrote:
> > > > > On 07.01.16 at 15:01, <jonathan.creekmore@gmail.com> wrote:
> > 
> > > Ian Campbell writes:
> > > 
> > > > I don't see this as contrary to your stated goals (e.g. ripping out
> > > > all the
> > > > other schedulers), but I consider you to be within the expert camp
> > > > for
> > > > wanting to do so (and having the chops to handle whatever pieces
> > > > you find
> > > > yourselves with). I have no objections at all to allowing experts
> > > > such as
> > > > yourselves to configure things and I applaud you for doing this in
> > > > an
> > > > upstream way (it is the right thing to do).
> > > > 
> > > > My concern is that while you rightly consider yourselves expert
> > > > enough and
> > > > are building something for a specific (and AIUI targeted) use case
> > > > many
> > > > normal users tend to think that if they are expert enough to find
> > > > and flip
> > > > the switch then they are expert enough to deal with the
> > > > consequences, when
> > > > they are not and/or they do not have the specific use case which
> > > > the switch
> > > > was added to support i.e. they want common or garden Xen and we
> > > > want that
> > > > to mean the same for everyone.
> > > > 
> > > > It's those people (including general purpose distro maintainers)
> > > > who I
> > > > think need to be strongly discouraged from messing with these
> > > > options
> > > > because there will be a strong gravity towards them doing so.
> > > 
> > > So, if I add a patch in a v3 of this series that introduces a
> > > CONFIG_EXPERT option and hides all of the scheduler options behind that,
> > > would that be acceptible? That is a proposal that was mentioned on this
> > > thread before.
> 
> Thinking about it I think I'd avoid the specific name CONFIG_EXPERT due to
> the expectations which Linux's use of the name has set.
> 
> If we invert the sense then we could call it e.g. CONFIG_STANDARD_PLATFORM
> and default it to y, I expect it will be easier to discourage people from
> turning such an option off than to discourage them from turning something
> like CONFIG_EXPERT on.

Some proposed text for discussion:

config STANDARD_PLATFORM
	bool "Standard/Supported Platform"
	default y
	help
            This option enables everything which is part of the standard and
            supported Xen platform. You should say 'Y' to this question.

            Turning off this option will expose additional options which will
            cause the resulting Xen binary to deviate from the supported
            configuration. Resulting configurations are not support by the Xen
            Project. Specifically configurations             which disable this option:            :

 *                 are not tested by the Xen Project; If you disable this option you should
                   perform your own QA.
 *                 are not Supported by the Xen Project; Guidance will be given but
                   ultimately you are responsible for fixing the issues you discover.
 *                 do not receive security support; Issues which are seen only with the
                   option disabled are not treated as security bugs and are not subject to
                   the security process                 http://www.xenproject.org/security-policy.html                ).

            Again, you should say 'Y' to this question.

Too much? In particular is the second half of each bullet over egging the
pudding somewhat?

The state of this option should be logged during boot. Somewhere in this
lot I suppose:

(XEN) Xen version 4.7-unstable (osstest@bad) (gcc (Debian 4.9.2-10) 4.9.2) debug=y Mon Nov 30 10:33:03 UTC 2015
(XEN) Latest ChangeSet: Thu Nov 26 16:01:27 2015 +0100 git:b1d398b

Either "Platform: Standard" or "Platform: Custom" perhaps? The first line
is too long already so standard=y|n doesn't really work.

NB I don't think the logs should make a big song and dance about
STANDARD_PLATFORM=n, since I appreciate that people may want to ship such
things

Ian.

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

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Ian Campbell]

On Thu, 2016-01-07 at 09:18 -0600, Doug Goldstein wrote:
> > > With me asking for that option to not have a visible prompt by
> > > default,
> > > but nevertheless being settable. I do realize that this may not be
> > > possible with the current kconfig tool, but that's imo the only way
> > > to
> > > keep people from playing with expert options just because they see
> > > there's a prompt. No textual warning will help this, I'm afraid.
> > 
> > While I have reasonably strong opinions about this issue, I do not
> > think
> > they warrant forking Kconfig over.
> > 
> > With a suitably strong wording IMHO we have covered ourselves
> > sufficiently.
> > 
> > Ian.
> > 
> 
> So Jonathan and I have been messing with the hidden option behind a
> non-menu option (e.g. environment variable). The only way we can get it
> to work is to require the environment variable to be passed at
> configuration time and at build time and I doubt you'd want the steps to
> be "CONFIG_EXPERT=n make menuconfig && CONFIG_EXPERT=n make" to build
> Xen. We'll play with this some more if that's desired but given Ian's
> response I don't know if it is.

You mean it has to be given as either =n or =y i.e. it has no default? I
don't think we can live with that indeed.

Ian.

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Jonathan Creekmore]

Doug Goldstein writes:

> On 1/7/16 9:00 AM, Ian Campbell wrote:
>> On Thu, 2016-01-07 at 07:37 -0700, Jan Beulich wrote:
>>>>>> On 07.01.16 at 15:01, <jonathan.creekmore@gmail.com> wrote:
>>>
>>>> Ian Campbell writes:
>>>>
>>>>> I don't see this as contrary to your stated goals (e.g. ripping out all the
>>>>> other schedulers), but I consider you to be within the expert camp for
>>>>> wanting to do so (and having the chops to handle whatever pieces you find
>>>>> yourselves with). I have no objections at all to allowing experts such as
>>>>> yourselves to configure things and I applaud you for doing this in an
>>>>> upstream way (it is the right thing to do).
>>>>>
>>>>> My concern is that while you rightly consider yourselves expert enough and
>>>>> are building something for a specific (and AIUI targeted) use case many
>>>>> normal users tend to think that if they are expert enough to find and flip
>>>>> the switch then they are expert enough to deal with the consequences, when
>>>>> they are not and/or they do not have the specific use case which the switch
>>>>> was added to support i.e. they want common or garden Xen and we want that
>>>>> to mean the same for everyone.
>>>>>
>>>>> It's those people (including general purpose distro maintainers) who I
>>>>> think need to be strongly discouraged from messing with these options
>>>>> because there will be a strong gravity towards them doing so.
>>>>
>>>> So, if I add a patch in a v3 of this series that introduces a
>>>> CONFIG_EXPERT option and hides all of the scheduler options behind that,
>>>> would that be acceptible? That is a proposal that was mentioned on this
>>>> thread before.
>>
>> Thinking about it I think I'd avoid the specific name CONFIG_EXPERT due to
>> the expectations which Linux's use of the name has set.
>
>
>
>>
>> If we invert the sense then we could call it e.g. CONFIG_STANDARD_PLATFORM
>> and default it to y, I expect it will be easier to discourage people from
>> turning such an option off than to discourage them from turning something
>> like CONFIG_EXPERT on.
>
> So I actually like this approach. Maybe tweak the name slightly to
> CONFIG_SUPPORTED_XEN? It can force settings a certain way and can also
> make menus invisible. But I'm hoping we can get Jan's buy in here before
> we post any patchset.
>
>>
>>> With me asking for that option to not have a visible prompt by default,
>>> but nevertheless being settable. I do realize that this may not be
>>> possible with the current kconfig tool, but that's imo the only way to
>>> keep people from playing with expert options just because they see
>>> there's a prompt. No textual warning will help this, I'm afraid.
>>
>> While I have reasonably strong opinions about this issue, I do not think
>> they warrant forking Kconfig over.
>>
>> With a suitably strong wording IMHO we have covered ourselves sufficiently.
>>
>> Ian.
>>
>
> So Jonathan and I have been messing with the hidden option behind a
> non-menu option (e.g. environment variable). The only way we can get it
> to work is to require the environment variable to be passed at
> configuration time and at build time and I doubt you'd want the steps to
> be "CONFIG_EXPERT=n make menuconfig && CONFIG_EXPERT=n make" to build
> Xen. We'll play with this some more if that's desired but given Ian's
> response I don't know if it is.

So, I have been playing with the environment variable option more and,
by introducing a XEN_CONFIG_EXPERT ?= n variable, normal users will not
see any of the "expert" options (they will be invisible in the
Kconfig). If you enable the environment variable, then the "expert"
options are visible for selection.

Would that be acceptible?

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Jan Beulich]

>>> On 07.01.16 at 16:18, <cardoe@cardoe.com> wrote:
> On 1/7/16 9:00 AM, Ian Campbell wrote:
>> If we invert the sense then we could call it e.g. CONFIG_STANDARD_PLATFORM
>> and default it to y, I expect it will be easier to discourage people from
>> turning such an option off than to discourage them from turning something
>> like CONFIG_EXPERT on.
> 
> So I actually like this approach. Maybe tweak the name slightly to
> CONFIG_SUPPORTED_XEN? It can force settings a certain way and can also
> make menus invisible. But I'm hoping we can get Jan's buy in here before
> we post any patchset.

With Ian's proposed wording, and with it being either
STANDARD_PLATFORM as Ian suggests or just SUPPORTED, I'd
be fine I think.

> So Jonathan and I have been messing with the hidden option behind a
> non-menu option (e.g. environment variable). The only way we can get it
> to work is to require the environment variable to be passed at
> configuration time and at build time and I doubt you'd want the steps to
> be "CONFIG_EXPERT=n make menuconfig && CONFIG_EXPERT=n make" to build
> Xen. We'll play with this some more if that's desired but given Ian's
> response I don't know if it is.

If these overrides were only required for people wanting the non-
default setting, I think it wouldn't be too bad. In fact one more
thing to keep them from fiddling with the option.

Jan

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Jan Beulich]

>>> On 07.01.16 at 16:30, <ian.campbell@citrix.com> wrote:
> Some proposed text for discussion:
> 
> config STANDARD_PLATFORM
> 	bool "Standard/Supported Platform"
> 	default y
> 	help
>             This option enables everything which is part of the standard and
>             supported Xen platform. You should say 'Y' to this question.
> 
>             Turning off this option will expose additional options which will
>             cause the resulting Xen binary to deviate from the supported
>             configuration. Resulting configurations are not support by the Xen
>             Project. Specifically configurations             which disable this option:            :
> 
>  *                 are not tested by the Xen Project; If you disable this option you should
>                    perform your own QA.
>  *                 are not Supported by the Xen Project; Guidance will be given but
>                    ultimately you are responsible for fixing the issues you discover.
>  *                 do not receive security support; Issues which are seen only with the
>                    option disabled are not treated as security bugs and are not subject to
>                    the security process                 http://www.xenproject.org/security-policy.html                ).
> 
>             Again, you should say 'Y' to this question.
> 
> Too much? In particular is the second half of each bullet over egging the
> pudding somewhat?

I think these second halves are quite helpful.

> The state of this option should be logged during boot. Somewhere in this
> lot I suppose:
> 
> (XEN) Xen version 4.7-unstable (osstest@bad) (gcc (Debian 4.9.2-10) 4.9.2) 
> debug=y Mon Nov 30 10:33:03 UTC 2015
> (XEN) Latest ChangeSet: Thu Nov 26 16:01:27 2015 +0100 git:b1d398b
> 
> Either "Platform: Standard" or "Platform: Custom" perhaps? The first line
> is too long already so standard=y|n doesn't really work.

Perhaps there could be a 3rd line here saying: "Supported: yes
(xenproject.org)" or "Supported: no", allowing distros to further
customize this to direct their users to where support for the given
configuration can be got?

Jan

Re: [PATCH 0/4] Allow schedulers to be selectable through Kconfig

[Jan Beulich]

>>> On 07.01.16 at 16:43, <jonathan.creekmore@gmail.com> wrote:
> So, I have been playing with the environment variable option more and,
> by introducing a XEN_CONFIG_EXPERT ?= n variable, normal users will not
> see any of the "expert" options (they will be invisible in the
> Kconfig). If you enable the environment variable, then the "expert"
> options are visible for selection.
> 
> Would that be acceptible?

Sounds as reasonable to me as the other approach.

Jan