isolcpus option broken in 2.6.10-rc2-bk2

PATCH In review Dimitri Sivanich 3 participants 5 messages Dec 6, 2004 – Dec 8, 2004

Share & tools

Dimitri Sivanich

author

2004-12-06 18:53 UTC #0 lore raw

PATCH 1/1 isolcpus option broken in 2.6.10-rc2-bk2

The isolcpus option is broken in 2.6.10-rc2-bk2. The domains are no longer being properly initialized (which results in a panic at bootup).

The following patch fixes this.

Signed-off-by Dimitri Sivanich <[email protected]>

arch/ia64/kernel/domain.c +17 -0

Index: linux/arch/ia64/kernel/domain.c
===================================================================
@@ -220,6 +220,23 @@ void __devinit arch_init_sched_domains(v
 						&cpu_to_phys_group);
 	}
 
+	/* Initialize isolated CPU (physical) domains and groups */
+	for_each_cpu_mask(i, cpu_isolated_map) {
+		struct sched_domain *sd = NULL;
+		int group;
+
+		sd = &per_cpu(phys_domains, i);
+		group = cpu_to_phys_group(i);
+		*sd = SD_CPU_INIT;
+		cpu_set(i, sd->span);
+		sd->flags = 0;
+		sd->balance_interval = INT_MAX;
+		sd->groups = &sched_group_phys[group];
+		init_sched_build_groups(sched_group_phys, sd->span,
+						&cpu_to_phys_group);
+		sd->groups->cpu_power = SCHED_LOAD_SCALE;
+	}
+
 #ifdef CONFIG_NUMA
 	init_sched_build_groups(sched_group_allnodes, cpu_default_map,
 				&cpu_to_allnodes_group);

kernel/sched.c +17 -0

Index: linux/kernel/sched.c
===================================================================
@@ -4323,6 +4323,23 @@ static void __devinit arch_init_sched_do
 						&cpu_to_phys_group);
 	}
 
+        /* Initialize isolated CPU (physical) domains and groups */
+	for_each_cpu_mask(i, cpu_isolated_map) {
+		struct sched_domain *sd = NULL;
+		int group;
+
+		sd = &per_cpu(phys_domains, i);
+		group = cpu_to_phys_group(i);
+		*sd = SD_CPU_INIT;
+		cpu_set(i, sd->span);
+		sd->flags = 0;
+		sd->balance_interval = INT_MAX;
+		sd->groups = &sched_group_phys[group];
+		init_sched_build_groups(sched_group_phys, sd->span,
+						&cpu_to_phys_group);
+		sd->groups->cpu_power = SCHED_LOAD_SCALE;
+	}
+
 #ifdef CONFIG_NUMA
 	/* Set up node groups */
 	init_sched_build_groups(sched_group_nodes, cpu_default_map,

Ingo Molnar

Acked-by on v1 1/1

2004-12-06 21:18 UTC #1 lore raw

Quoted 6 lines · Dimitri Sivanich

The isolcpus option is broken in 2.6.10-rc2-bk2. The domains are no longer being properly initialized (which results in a panic at bootup).
The following patch fixes this.
Signed-off-by Dimitri Sivanich <[email protected]>

Acked-by Ingo Molnar <[email protected]>

only a minor nit:

arch/ia64/kernel/domain.c void __devinit arch_init_sched_domains(v · line 225

Quoted 3 lines

arch/ia64/kernel/domain.c +3 -0

+	/* Initialize isolated CPU (physical) domains and groups */
+	for_each_cpu_mask(i, cpu_isolated_map) {
+		struct sched_domain *sd = NULL;

there's no need to initialize 'sd' to NULL here.

arch/ia64/kernel/domain.c void __devinit arch_init_sched_domains(v

Quoted 7 lines

arch/ia64/kernel/domain.c +6 -0

+		int group;
+
+		sd = &per_cpu(phys_domains, i);
+		group = cpu_to_phys_group(i);
 
+	for_each_cpu_mask(i, cpu_isolated_map) {
+		struct sched_domain *sd = NULL;

ditto.

arch/ia64/kernel/domain.c void __devinit arch_init_sched_domains(v · line 230

Quoted 5 lines

arch/ia64/kernel/domain.c +5 -0

+		int group;
+
+		sd = &per_cpu(phys_domains, i);
+		group = cpu_to_phys_group(i);
+		*sd = SD_CPU_INIT;

Ingo

Nick Piggin

2004-12-08 08:57 UTC #2 lore raw

Quoted 37 lines · Ingo Molnar

Quoted 7 lines · Dimitri Sivanich
The isolcpus option is broken in 2.6.10-rc2-bk2. The domains are no longer being properly initialized (which results in a panic at bootup).
The following patch fixes this.
Signed-off-by Dimitri Sivanich <[email protected]>
Acked-by Ingo Molnar <[email protected]>
only a minor nit:
Review
Quoted 4 lines
(quoted diff) +3 -0
+	/* Initialize isolated CPU (physical) domains and groups */
+	for_each_cpu_mask(i, cpu_isolated_map) {
+		struct sched_domain *sd = NULL;
there's no need to initialize 'sd' to NULL here.
Review
Quoted 9 lines
(quoted diff) +6 -0
+		int group;
+
+		sd = &per_cpu(phys_domains, i);
+		group = cpu_to_phys_group(i);
 
 
+	for_each_cpu_mask(i, cpu_isolated_map) {
+		struct sched_domain *sd = NULL;
ditto.

Quoted 11 lines

Review

Quoted 6 lines

(quoted diff) +5 -0

+		int group;
+
+		sd = &per_cpu(phys_domains, i);
+		group = cpu_to_phys_group(i);
+		*sd = SD_CPU_INIT;

Ingo

Actually, this is wrong. And I was wrong to say that the fix was to initialize the dummy domain (because its ->flags are 0, there is no need for it to have anything else set up).

The isolated domains don't need to be set up because they correctly point to the dummy domain, which is already set up properly (zeroed). The oops is just a problem with sched_domain_debug.

Andrew and/or Linus, please make sure Dimitri's patch gets reverted and the attached one applied before 2.6.10.

Fix an oops in sched_domain_debug when using the isolcpus= option.

Also move a debug check for validating groups into the "for-each-group" loop, where it should be.

Signed-off-by Nick Piggin <[email protected]>

---

 linux-2.6-npiggin/kernel/sched.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/sched.c renamed from kernel/sched.c~sched-debug-oops +4 -2

diff -puN kernel/sched.c~sched-debug-oops kernel/sched.c
@@ -4446,6 +4446,7 @@ static void sched_domain_debug(void)
 				if (sd->parent)
 					printk(" ERROR !SD_LOAD_BALANCE domain has parent");
 				printk("\n");
+				break;
 			}
 
 			printk("span %s\n", str);

@@ -4454,8 +4455,6 @@ static void sched_domain_debug(void)
 				printk(KERN_DEBUG "ERROR domain->span does not contain CPU%d\n", i);
 			if (!cpu_isset(i, group->cpumask))
 				printk(KERN_DEBUG "ERROR domain->groups does not contain CPU%d\n", i);
-			if (!group->cpu_power)
-				printk(KERN_DEBUG "ERROR domain->cpu_power not set\n");
 
 			printk(KERN_DEBUG);
 			for (j = 0; j < level + 2; j++)

@@ -4466,6 +4465,9 @@ static void sched_domain_debug(void)
 					printk(" ERROR: NULL");
 					break;
 				}
+				
+				if (!group->cpu_power)
+					printk(KERN_DEBUG "ERROR group->cpu_power not set\n");
 
 				if (!cpus_weight(group->cpumask))
 					printk(" ERROR empty group:");

Ingo Molnar

Acked-by

2004-12-08 09:05 UTC #3 lore raw

Quoted 7 lines · Nick Piggin

Actually, this is wrong. And I was wrong to say that the fix was to initialize the dummy domain (because its ->flags are 0, there is no need for it to have anything else set up).
The isolated domains don't need to be set up because they correctly point to the dummy domain, which is already set up properly (zeroed). The oops is just a problem with sched_domain_debug.

(ah ... indeed, good catch. Dimitri's patch ended up working around the 'break' bug in sched_domain_debug() :-| At least this case shows that it was right to keep SCHED_DOMAIN_DEBUG defined.)

Quoted 2 lines

Andrew and/or Linus, please make sure Dimitri's patch gets reverted and the attached one applied before 2.6.10.

Acked-by Ingo Molnar <[email protected]>

Ingo

Nick Piggin

2004-12-07 22:59 UTC #4 lore raw

In reply to Dimitri Sivanich

Quoted 7 lines · Dimitri Sivanich

The isolcpus option is broken in 2.6.10-rc2-bk2. The domains are no longer being properly initialized (which results in a panic at bootup).
The following patch fixes this.
Signed-off-by Dimitri Sivanich <[email protected]>

Sorry for not replying earlier, I've been away for a few days.

I don't think this is quite needed, because isolated CPUs should have their domain set to sched_domain_dummy, I think?

The trick would be to just initialise sched_domain_dummy properly; it looks like that isn't being done for some reason.

Give me a couple of hours and I'll try testing something that should solve it.