sched_credit2.c: runqueue_per_core code

This patch do the following things:

(quoted diff) +0 -3

-Insertion of runqueue_per_core code
-Boot paarmeter creation to select runqueue
-Update of xen-command-line.markdown

Signed-off-by Uma Sharma <[email protected]>

---
 docs/misc/xen-command-line.markdown |  7 +++++++
 xen/common/sched_credit2.c          | 31 ++++++++++++++++++++++++-------
 2 files changed, 31 insertions(+), 7 deletions(-)

docs/misc/xen-command-line.markdown +0 -0

index 63871cb..12e5c01 100644

docs/misc/xen-command-line.markdown +1 -0

+
 ### dbgp
 > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
 

xen/common/sched_credit2.c +2 -2

index ad0a5d4..2067b3b 100644
@@ -85,8 +85,8 @@
  * to a small value, and a fixed credit is added to everyone.
  *
  * The plan is for all cores that share an L2 will share the same
- * runqueue.  At the moment, there is one global runqueue for all
- * cores.
+ * runqueue.  At the moment, the code allows the user to choose runqueue
+ * to be used. Default used core runqueue.
  */
 
 /*

xen/common/sched_credit2.c +3 -0

 int opt_migrate_resist=500;
 integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
+char __initdata opt_credit2_runqueue_string[10] = "core";
+string_param("credit2_runqueue", opt_credit2_runqueue_string);
+int opt_credit2_runqueue = CREDIT2_OPT_RUNQUEUE_CORE;
 
 /*
  * Useful macros

xen/common/sched_credit2.c +13 -2

     else
-        rqi = cpu_to_socket(cpu);
+        rqi = cpu ? cpu_to_core(cpu) : boot_cpu_to_core();
 
     if ( rqi < 0 )
     {

@@ -1988,7 +1994,7 @@ csched2_alloc_pdata(const struct scheduler *ops, int cpu)
 {
     /* Check to see if the cpu is online yet */
     /* Note: cpu 0 doesn't get a STARTING callback */
-    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )
+    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 || cpu_to_core(cpu) >= 0 )
         init_pcpu(ops, cpu);
     else
         printk("%s: cpu %d not online yet, deferring initializatgion\n",

@@ -2109,6 +2115,17 @@ csched2_init(struct scheduler *ops)
         opt_load_window_shift = LOADAVG_WINDOW_SHIFT_MIN;
     }
 
+    /* Defines the runqueue used. */
+    if ( !strcmp(opt_credit2_runqueue_string, "socket") )
+        opt_credit2_runqueue = CREDIT2_OPT_RUNQUEUE_SOCKET;
+    else if ( strcmp(opt_credit2_runqueue_string, "core") )
+        printk("WARNING, unrecognized credit2_runqueue option %s, using core\n",
+                opt_credit2_runqueue_string);
+
+    printk("Runqueue: Using %s\n",
+            opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_CORE ? "core" :
+            opt_credit2_runqueue_string);
+
     /* Basically no CPU information is available at this point; just
      * set up basic structures, and a callback when the CPU info is
      * available. */

xen/common/sched_credit2.c line 167

Quoted 2 lines

xen/common/sched_credit2.c +2 -0

+#define CREDIT2_OPT_RUNQUEUE_CORE 1
+#define CREDIT2_OPT_RUNQUEUE_SOCKET 2

You can drop _OPT out of the name. It serves only to make the constant longer.

Also, this should probably be an enum as the exact numbers themselves are not interesting.

Quoted 11 lines

int opt_migrate_resist=500; integer_param("sched_credit2_migrate_resist", opt_migrate_resist);

xen/common/sched_credit2.c +3 -0

+char __initdata opt_credit2_runqueue_string[10] = "core";
+string_param("credit2_runqueue", opt_credit2_runqueue_string);
+int opt_credit2_runqueue = CREDIT2_OPT_RUNQUEUE_CORE;
 
 /*
  * Useful macros

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1950

Quoted 7 lines

xen/common/sched_credit2.c +2 -2

@@ -1940,10 +1946,10 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 
     /* Figure out which runqueue to put it in */
     /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
-    if ( cpu == 0 )
-        rqi = 0;
+    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
+        rqi = cpu ? cpu_to_socket(cpu) : boot_cpu_to_socket();

This conditional is bogus. If cpu0 is offlined and re-onlined, it must use cpu_to_core()

This entire hunk should probably be

rqi = (opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET) ? cpu_to_socket(cpu) : cpu_to_core(cpu);

(with suitable alignment)

Quoted 1 line

Quoted 1 line · On 13.03.15 at 20:13, <[email protected]> wrote:

Quoted 17 lines · Andrew Cooper

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1950

Quoted 8 lines

xen/common/sched_credit2.c +2 -2

@@ -1940,10 +1946,10 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 
     /* Figure out which runqueue to put it in */
     /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
-    if ( cpu == 0 )
-        rqi = 0;
+    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
+        rqi = cpu ? cpu_to_socket(cpu) : boot_cpu_to_socket();

This conditional is bogus. If cpu0 is offlined and re-onlined, it must use cpu_to_core()

This entire hunk should probably be

rqi = (opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET) ? cpu_to_socket(cpu) : cpu_to_core(cpu);

Quoted 1 line

(with suitable alignment)

You're ignoring the fact that she's following suit from existing code; and that that code is there for a reason: When this is first called for cpu 0, cpu_to_socket() (and cpu_to_core()) return garbage since they haven't been initialized yet.

That is something that needs to be fixed, but it's not Uma's job to fix it.

Quoted 26 lines

Quoted 1 line

Quoted 1 line · On 13.03.15 at 20:13, <[email protected]> wrote:

Quoted 12 lines · Andrew Cooper

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1950

Quoted 8 lines

xen/common/sched_credit2.c +2 -2

@@ -1940,10 +1946,10 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 
     /* Figure out which runqueue to put it in */
     /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
-    if ( cpu == 0 )
-        rqi = 0;
+    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
+        rqi = cpu ? cpu_to_socket(cpu) : boot_cpu_to_socket();

This conditional is bogus. If cpu0 is offlined and re-onlined, it must use cpu_to_core()

Quoted 6 lines

This entire hunk should probably be

rqi = (opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET) ? cpu_to_socket(cpu) : cpu_to_core(cpu);

(with suitable alignment)

You're ignoring the fact that she's following suit from existing code; and that that code is there for a reason: When this is first called for cpu 0, cpu_to_socket() (and cpu_to_core()) return garbage since they haven't been initialized yet.

Quoted 1 line

That is something that needs to be fixed, but it's not Uma's job to fix it.

Them returning garbage isn't what needs fixing. Instead the code here should use a different condition to check whether this is the boot CPU (e.g. looking at system_state). And that can very well be done directly in this patch.

Quoted 1 line

Quoted 1 line · On 16.03.15 at 13:51, <[email protected]> wrote:

Quoted 3 lines · Jan Beulich

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.03.15 at 20:13, <[email protected]> wrote:

Quoted 1 line · Andrew Cooper

Review static void init_pcpu(const struct scheduler *ops,

Quoted 1 line

(quoted diff) +0 -0

@@ -1940,10 +1946,10 @@ static void init_pcpu(const struct scheduler *ops, 

int cpu)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review static void init_pcpu(const struct scheduler *ops, · line 1947

Quoted 3 lines

(quoted diff) +0 -0

     /* Figure out which runqueue to put it in */
     /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to 

runqueue 0. */

Quoted 20 lines

Quoted 20 lines

Quoted 8 lines

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1950

Quoted 4 lines

xen/common/sched_credit2.c +2 -2

-    if ( cpu == 0 )
-        rqi = 0;
+    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
+        rqi = cpu ? cpu_to_socket(cpu) : boot_cpu_to_socket();

This conditional is bogus. If cpu0 is offlined and re-onlined, it must use cpu_to_core()

Quoted 6 lines

This entire hunk should probably be

rqi = (opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET) ? cpu_to_socket(cpu) : cpu_to_core(cpu);

(with suitable alignment)

You're ignoring the fact that she's following suit from existing code; and that that code is there for a reason: When this is first called for cpu 0, cpu_to_socket() (and cpu_to_core()) return garbage since they haven't been initialized yet.

Quoted 6 lines

Quoted 1 line

That is something that needs to be fixed, but it's not Uma's job to fix it.

Them returning garbage isn't what needs fixing. Instead the code here should use a different condition to check whether this is the boot CPU (e.g. looking at system_state). And that can very well be done directly in this patch.

What do you suggest, then?

Quoted 8 lines

Quoted 1 line

Quoted 1 line · On 16.03.15 at 13:51, <[email protected]> wrote:

Quoted 4 lines · Jan Beulich

Them returning garbage isn't what needs fixing. Instead the code here should use a different condition to check whether this is the boot CPU (e.g. looking at system_state). And that can very well be done directly in this patch.

Quoted 1 line

What do you suggest, then?

My preferred solution would be, as said, to leverage system_state. Provided the state to look for is consistent between x86 and ARM.

Quoted 8 lines

Quoted 1 line

Quoted 1 line · On 16.03.15 at 13:51, <[email protected]> wrote:

Quoted 4 lines · Jan Beulich

Them returning garbage isn't what needs fixing. Instead the code here should use a different condition to check whether this is the boot CPU (e.g. looking at system_state). And that can very well be done directly in this patch.

Quoted 1 line

What do you suggest, then?

My preferred solution would be, as said, to leverage system_state. Provided the state to look for is consistent between x86 and ARM.

Quoted 1 line

Quoted 1 line · On 17.03.15 at 19:18, <[email protected]> wrote:

Quoted 5 lines · Jan Beulich

Quoted 2 lines

Quoted 1 line

Quoted 1 line · On 16.03.15 at 13:51, <[email protected]> wrote:

On 03/16/2015 12:48 PM, Jan Beulich wrote:

My preferred solution would be, as said, to leverage system_state. Provided the state to look for is consistent between x86 and ARM.

Would something like this make sense?

xen/common/sched_credit2.c +0 -1

@@ -1986,9 +1982,13 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 static void *
 csched2_alloc_pdata(const struct scheduler *ops, int cpu)
 {
-    /* Check to see if the cpu is online yet */

Quoted 7 lines

Quoted 1 line

Quoted 1 line · On 17.03.15 at 19:18, <[email protected]> wrote:

xen/common/sched_credit2.c +0 -0

@@ -1936,12 +1936,8 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
     }
 

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1939

Quoted 8 lines

xen/common/sched_credit2.c +1 -6

     /* Figure out which runqueue to put it in */
-    rqi = 0;
-
-    /* Figure out which runqueue to put it in */
-    /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
-    if ( cpu == 0 )
-        rqi = 0;
+    if ( system_state == SYS_STATE_boot )

... I'd suggest using <= SYS_STATE_boot or < SYS_STATE_smp_boot.

Quoted 10 lines

xen/common/sched_credit2.c +3 -3

@@ -1986,9 +1982,13 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 static void *
 csched2_alloc_pdata(const struct scheduler *ops, int cpu)
 {
-    /* Check to see if the cpu is online yet */
-    /* Note: cpu 0 doesn't get a STARTING callback */
-    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )
+    /*
+     * Actual initialization is deferred to when the pCPU will be
+     * online, via a STARTING callback. The only exception is

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1992

Quoted 5 lines

xen/common/sched_credit2.c +4 -0

+     * the boot cpu, which does not get such a notification, and
+     * hence needs to be taken care of here.
+     */
+    if ( system_state == SYS_STATE_boot )
         init_pcpu(ops, cpu);

Same here, plus the new condition at the first glance isn't matching the old one, but perhaps that's intentional.

Quoted 7 lines

Quoted 7 lines

xen/common/sched_credit2.c +0 -3

@@ -1986,9 +1982,13 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 static void *
 csched2_alloc_pdata(const struct scheduler *ops, int cpu)
 {
-    /* Check to see if the cpu is online yet */
-    /* Note: cpu 0 doesn't get a STARTING callback */
-    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )

Quoted 8 lines

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1992

Quoted 8 lines

xen/common/sched_credit2.c +7 -0

+    /*
+     * Actual initialization is deferred to when the pCPU will be
+     * online, via a STARTING callback. The only exception is
+     * the boot cpu, which does not get such a notification, and
+     * hence needs to be taken care of here.
+     */
+    if ( system_state == SYS_STATE_boot )
         init_pcpu(ops, cpu);

Quoted 4 lines

Same here, plus the new condition at the first glance isn't matching the old one, but perhaps that's intentional.

It is intentional, and that is why we're changing it! :-) Let me try to explain:

The idea, in both old and new code, is to call init_pcpu() either:
a) on the boot cpu, during boot
b) on non-boot cpu, *only* if they are online.

The issue is that, for assessing b), it checks whether cpu_to_socket(cpu) returns >=0 and, if it does, it assumes the cpu is online. That is not true, as cpu_data.phys_proc_id (which is what cpu_to_socket() go reading) is 0 even before any onlining and topolog identification has happened (it's a global).

Therefore, all the pcpus are initialized right away, and all are assigned to runqueue 0, as returned by cpu_to_socket() at this stage, which is clearly wrong.

In fact, this is the reason why, previous versions of this took the approach of initializing things such as cpu_to_socket() returned -1 before topology identification.

In the new version, as you suggested, I'm using system_state to figure out whether we are dealing with the boot cpu, and that's it. :-)

Hope this clarifies things... If yes, I'll make sure to put a similar explanation in the changelog, when sending the patch officially.

Quoted 1 line

Quoted 1 line · On 18.03.15 at 16:26, <[email protected]> wrote:

In both cases there's a slight risk in using system_state to determine whether to rely on cpu_data[] or not, because there's actually a window for each processor after system_state == SYS_STATE_smp_boot where cpu_data[] is *not* initialized, but it's not obvious from looking at the data itself. If the callback mechanisms ever change order with the cpu_data[] initializations in the future, we risk a situation where credit2 silently regresses to using a single massive runqueue.

Quoted 8 lines

Quoted 1 line

Quoted 1 line · On 18.03.15 at 16:26, <[email protected]> wrote:

In both cases there's a slight risk in using system_state to determine whether to rely on cpu_data[] or not, because there's actually a window for each processor after system_state == SYS_STATE_smp_boot where cpu_data[] is *not* initialized, but it's not obvious from looking at the data itself. If the callback mechanisms ever change order with the cpu_data[] initializations in the future, we risk a situation where credit2 silently regresses to using a single massive runqueue.

But such an ordering change is extremely unlikely, since the CPU_STARTING notification specifically tells you that the given CPU is now ready for normal use, which includes it having its topology related data set up.

Quoted 9 lines · Jan Beulich

Quoted 8 lines

Quoted 1 line

Quoted 1 line · On 18.03.15 at 16:26, <[email protected]> wrote:

In both cases there's a slight risk in using system_state to determine whether to rely on cpu_data[] or not, because there's actually a window for each processor after system_state == SYS_STATE_smp_boot where cpu_data[] is *not* initialized, but it's not obvious from looking at the data itself. If the callback mechanisms ever change order with the cpu_data[] initializations in the future, we risk a situation where credit2 silently regresses to using a single massive runqueue.

Quoted 4 lines

But such an ordering change is extremely unlikely, since the CPU_STARTING notification specifically tells you that the given CPU is now ready for normal use, which includes it having its topology related data set up.

Even if they look at init_pcpu() in credit2, they're unlikely to know that cpu_to_socket() isn't valid until after boot_secondary() has happened; and if they try it and boot it, everything will seem to work perfectly for credit2 -- except that there will be a single global runqueue.

If you, Dario and I don't happen to catch it -- or don't happen to remember the exact details of the constraints -- nobody may notice until a year later.

This is the point of having ASSERTs -- so that we don't have to worry about remembering and catching all these crazy constraints.

Quoted 9 lines · Dario Faggioli

Quoted 9 lines

Quoted 9 lines

xen/common/sched_credit2.c +2 -3

@@ -1986,9 +1982,13 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 static void *
 csched2_alloc_pdata(const struct scheduler *ops, int cpu)
 {
-    /* Check to see if the cpu is online yet */
-    /* Note: cpu 0 doesn't get a STARTING callback */
-    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )
+    /*
+     * Actual initialization is deferred to when the pCPU will be

Quoted 6 lines

Quoted 6 lines

xen/common/sched_credit2.c static void init_pcpu(const struct scheduler *ops, int cpu) · line 1992

Quoted 6 lines

xen/common/sched_credit2.c +5 -0

+     * online, via a STARTING callback. The only exception is
+     * the boot cpu, which does not get such a notification, and
+     * hence needs to be taken care of here.
+     */
+    if ( system_state == SYS_STATE_boot )
         init_pcpu(ops, cpu);

I think this will break adding a cpu to a cpupool after boot, won't it?

Don't we want effectively, "if ( is_boot_cpu() ||
is_cpu_topology_set_up() )"?

is_cpu_topology_set_up() could be "system_state >= SYS_STATE_smp_boot" I suppose?

Is "cpu == 0" the best test for "is_boot_cpu()", or is there another test?

So let's step back for a minute, and let me see if I can describe the situation.

Unlike the other schedulers, credit2 needs to know the topology of a cpus when setting it up, so that we can place it in the proper runqueue.

Setting up the per-cpu structures would normally happen in alloc_pdata.
This is called in three different ways:

* During boot, on the boot processer, alloc_pdata is called from scheduler_init().

* During boot, on the secondary processors, alloc_pdata is called from a CPU_UP_PREPARE callback, which happens just before the cpu is actually brought up.

* When switching a cpu to a new cpupool after boot, alloc_pdata is also called from cpu_schedule_switch().

The "normal" place the cpu topology information can be found is in global the cpu_data[] array, typically accessed by the cpu_to_socket() or cpu_to_core() macros.

This topology information is written to cpu_data[] by smp_store_cpu_info(). For the boot cpu, it happens in smp_prepare_cpus(); For secondary cpus, it's happens in start_secondary(), which is the code run on the cpu itself as it's being brought up.

Unfortunately, at the moment, both of these places are after the respective places where the alloc pdata is called for those cpus. Flattening the entire x86 setup at the moment you'd see:
alloc_pdata for boot cpu
smp_store_cpu_info for boot cpu
for each secondary cpu
alloc_pdata for secondary cpu
smp_store_cpu_info for secondary cpu

scheduler_init() is called before smp_prepare_cpus() in part because of a dependency chain elsewhere: we cannot set up the idle domain until scheduler_init() is called; and other places further on in the initialization but before setting up cpu topology information assume that the idle domain has been set up.

I haven't actually tracked down this dependency yet; nor have I explored the possibility of populating cpu_data[] for the boot cpu earier than it's done now.

I'm not sure exactly why we call alloc_pdata on CPU_UP_PREPARE rather than CPU_STARTING -- whether that's just what seemed most sensible when cpupools were created, or whether there's a dependency somewhere between those two points.

At the moment we deal with the fact that the topology information for a CPU is not available on CPU_UP_PREPARE by setting a callback that will call init_pcpu() on the CPU_STARTING action.

The boot cpu will not get such a callback; but information about the topology of the boot cpu *is* available in boot_cpu_data[].

We can use system_state to detect whether we've been called from scheduler_init (system_state < SYS_STATE_smp_boot), or whether we've been called after the whole thing has been done (system_state >= SYS_STATE_active).

But while system_state == SYS_STATE_smp_boot (which
is where both of the normal schedule callback and the csched2-specific callback happen at the moment), we can't determine whether a given cpu's cpu_data[] has been initialized yet.

So one thing we could do is this:

* In csched2_alloc_pdata(), call init_pcpu() if system_state <= SYS_STATE_smp_boot (to catch scheduler_init) or system_state >= SYS_STATE_active (to catch cpupool callbacks).

* Keep the credit2-specific callback on CPU_STARTING. In csched2_cpu_starting(), call init_pcpu(). (Not sure if this handles cpu offlining / onlining properly or not.)

* In init_pcpu(), if system_state < SYS_STATE_smp_boot, assume that we've been called from scheduler_init and use boot_cpu_data[]. Otherwise, assume that the calls from the CPU_UP_PREPARE callback have been filtered out, and that we're being called either from CPU_STARTING or from cpu_schedlue_switch(), and use cpu_data[].

Another thing we could explore is this:

* Change scheduler.c to call alloc_pdata on CPU_STARTING rather than CPU_UP_PREPARE, and get rid of the csched2-specific callback.

* In csched2_alloc_pdata, always call init_pcpu(). (Or perhaps just rename init_pcpu() to csched2_alloc_pdata().)

* In init_pcpu, if system_state < SYS_STATE_smp_boot, assume that we've been called from scheduler_init and use boot_cpu_data[]; otherwise, assume that we're being called either from CPU_STARTING, or from cpu_schedule_switch(), and use cpu_data[].

If this would work, I think it would be a lot better. It would remove the credit2-specific callback, and it would mean we wouldn't need an additional test to filter out

In both cases there's a slight risk in using system_state to determine whether to rely on cpu_data[] or not, because there's actually a window for each processor after system_state == SYS_STATE_smp_boot where cpu_data[] is *not* initialized, but it's not obvious from looking at the data itself. If the callback mechanisms ever change order with the cpu_data[] initializations in the future, we risk a situation where credit2 silently regresses to using a single massive runqueue.

So I think that at least for debug builds, we should make some way to determine whether a given cpu_data[] has been initialized, so that at least we an ASSERT() that it has.

Quoted 6 lines

scheduler_init() is called before smp_prepare_cpus() in part because of a dependency chain elsewhere: we cannot set up the idle domain until scheduler_init() is called; and other places further on in the initialization but before setting up cpu topology information assume that the idle domain has been set up.

However, as you say yourself below, there is boot_cpu_data, which stashes the info we need for the boot cpu. It gets filled after init_idle_domain()->scheduler_init() right now, but I don't see a reason why it can't be pulled up a bit (and from my tests, it seems to work).

Quoted 4 lines

If this would work, I think it would be a lot better. It would remove the credit2-specific callback, and it would mean we wouldn't need an additional test to filter out

I see. Ok, I guess I can give this a try. If it does not explode, because some weird dependency we can't see right now, we can either try harder to track it, or use the other solution outlined above as a fallback.

Quoted 4 lines · Dario Faggioli

Quoted 4 lines

If this would work, I think it would be a lot better. It would remove the credit2-specific callback, and it would mean we wouldn't need an additional test to filter out

Quoted 4 lines

I see. Ok, I guess I can give this a try. If it does not explode, because some weird dependency we can't see right now, we can either try harder to track it, or use the other solution outlined above as a fallback.

If you could look into this, that would be awesome. :-)

Quoted 1 line

Quoted 1 line · On 19.03.15 at 11:03, <[email protected]> wrote:

OTOH, CPU_STARTING callbacks run:
- on the cpu being brought up;
- with interrupt disabled (see how the call to local_irq_enable(), in start_secondary(), is *after* the invocation of notify_cpu_starting()).

Here we are. And the reason why things works ok in Credit2, is that csched2_alloc_pdata() doesn't really allocate anything! In fact, in general, handling alloc_pdata() during CPU_STARTING would mean that we can't allocate any memory which, given the name of the function, would look rather odd. :-)

Nevertheless I see the value of doing so, and hence I think what we could do would be to introduce a new hook in the scheduler interface, called .init_pdata or .init_pcpu, and, in sched_*.c, split the allocation and the initialization parts. The former will be handled during CPU_UP_PREPARE, when allocation is possible, the latter during CPU_STARTING, when we have more info available to perform actual initializations.

Quoted 13 lines

Quoted 1 line

Quoted 1 line · On 19.03.15 at 11:03, <[email protected]> wrote:

OTOH, CPU_STARTING callbacks run:
- on the cpu being brought up;
- with interrupt disabled (see how the call to local_irq_enable(), in start_secondary(), is *after* the invocation of notify_cpu_starting()).

Here we are. And the reason why things works ok in Credit2, is that csched2_alloc_pdata() doesn't really allocate anything! In fact, in general, handling alloc_pdata() during CPU_STARTING would mean that we can't allocate any memory which, given the name of the function, would look rather odd. :-)

Quoted 7 lines

Nevertheless I see the value of doing so, and hence I think what we could do would be to introduce a new hook in the scheduler interface, called .init_pdata or .init_pcpu, and, in sched_*.c, split the allocation and the initialization parts. The former will be handled during CPU_UP_PREPARE, when allocation is possible, the latter during CPU_STARTING, when we have more info available to perform actual initializations.

Another alternative would be a new CPU_ALIVE (name subject to change) notification after interrupts got enabled.

Quoted 13 lines

Quoted 1 line

Quoted 1 line · On 19.03.15 at 11:03, <[email protected]> wrote:

OTOH, CPU_STARTING callbacks run:
- on the cpu being brought up;
- with interrupt disabled (see how the call to local_irq_enable(), in start_secondary(), is *after* the invocation of notify_cpu_starting()).

Here we are. And the reason why things works ok in Credit2, is that csched2_alloc_pdata() doesn't really allocate anything! In fact, in general, handling alloc_pdata() during CPU_STARTING would mean that we can't allocate any memory which, given the name of the function, would look rather odd. :-)

Quoted 7 lines

Nevertheless I see the value of doing so, and hence I think what we could do would be to introduce a new hook in the scheduler interface, called .init_pdata or .init_pcpu, and, in sched_*.c, split the allocation and the initialization parts. The former will be handled during CPU_UP_PREPARE, when allocation is possible, the latter during CPU_STARTING, when we have more info available to perform actual initializations.

Another alternative would be a new CPU_ALIVE (name subject to change) notification after interrupts got enabled. That would (as a follow-up cleanup) also allow the MTRR and microcode setup on the CPU to no longer need explicit calls (which look reversed anyway - surely we should update microcode before fiddling with MTRRs).

Quoted 10 lines · Jan Beulich

Quoted 9 lines

Quoted 1 line

Quoted 1 line · On 19.03.15 at 11:03, <[email protected]> wrote:

Nevertheless I see the value of doing so, and hence I think what we could do would be to introduce a new hook in the scheduler interface, called .init_pdata or .init_pcpu, and, in sched_*.c, split the allocation and the initialization parts. The former will be handled during CPU_UP_PREPARE, when allocation is possible, the latter during CPU_STARTING, when we have more info available to perform actual initializations.

Quoted 6 lines

Another alternative would be a new CPU_ALIVE (name subject to change) notification after interrupts got enabled. That would (as a follow-up cleanup) also allow the MTRR and microcode setup on the CPU to no longer need explicit calls (which look reversed anyway - surely we should update microcode before fiddling with MTRRs).

local_irq_enable() happens after setting the cpu as online in cpu_online_map; not having the scheduler ready to actually schedule on it at that time seems like it's asking for trouble.

/me pokes around and thinks some more...

Quoted 18 lines · George Dunlap

Quoted 10 lines · Jan Beulich

Quoted 9 lines

Quoted 1 line

Quoted 1 line · On 19.03.15 at 11:03, <[email protected]> wrote:

Nevertheless I see the value of doing so, and hence I think what we could do would be to introduce a new hook in the scheduler interface, called .init_pdata or .init_pcpu, and, in sched_*.c, split the allocation and the initialization parts. The former will be handled during CPU_UP_PREPARE, when allocation is possible, the latter during CPU_STARTING, when we have more info available to perform actual initializations.

Quoted 6 lines

Another alternative would be a new CPU_ALIVE (name subject to change) notification after interrupts got enabled. That would (as a follow-up cleanup) also allow the MTRR and microcode setup on the CPU to no longer need explicit calls (which look reversed anyway - surely we should update microcode before fiddling with MTRRs).

Quoted 4 lines

local_irq_enable() happens after setting the cpu as online in cpu_online_map; not having the scheduler ready to actually schedule on it at that time seems like it's asking for trouble.

Right.

Quoted 2 lines

/me pokes around and thinks some more...

So, if I can ask, how about my idea of splitting alloc_ and init_ parts of pCPU initialization ? :-)

Quoted 2 lines · Dario Faggioli

So, if I can ask, how about my idea of splitting alloc_ and init_ parts of pCPU initialization ? :-)

Architecturally, from some points of view it makes sense

from
other points of view, it would be nicer not to multiply callbacks and make the interface more complicated.

But from a practical point of view, this path is already more work than I was expecting it to be, so I don't think we should spend *too* much time looking for alternatives. If that seems like the best option at the moment, then I'm fine with it.

xen/common/sched_credit2.c +0 -1

@@ -161,10 +161,16 @@
  */
 #define __CSFLAG_runq_migrate_request 3
 #define CSFLAG_runq_migrate_request (1<<__CSFLAG_runq_migrate_request)
-