x86: allow number of additional hotplug CPUs to be set at compile time

From: Chuck Ebbert <[email protected]>

x86: allow number of additional hotplug CPUs to be set at compile time

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

And sometimes CPU hotplugging is enabled only for suspend/hibernate anyway, so the additional CPU IDs are not wanted. Allow the number to be set to zero at compile time.

Also, force the number of extra CPUs to zero if hotplugging is disabled which allows removing some conditional code.

Tested on uniprocessor x86_64 that ACPI claims has a disabled processor, with CPU hotplugging configured.

("After" has the number of additional CPUs set to 0)
Before: NR_CPUS: 512, nr_cpu_ids: 2, nr_node_ids 1
After: NR_CPUS: 512, nr_cpu_ids: 1, nr_node_ids 1

Signed-off-by Chuck Ebbert <[email protected]>

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

Quoted 5 lines · Chuck Ebbert

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

You can set this with additional_cpus=... at boot time. I don't think each runtime option needs a CONFIG option too.

Quoted 7 lines · On Thu, 02 Oct 2008 11:12:51 +0200 Andi Kleen <[email protected]> wrote:

Quoted 5 lines · Chuck Ebbert

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

Quoted 4 lines

You can set this with additional_cpus=... at boot time. I don't think each runtime option needs a CONFIG option too.

Well not all of them, but this one is a good candidate. Either that or we should just change the default to zero.

From: Chuck Ebbert <[email protected]>

x86: allow number of additional hotplug CPUs to be set at compile time, V2

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

And sometimes CPU hotplugging is enabled only for suspend/hibernate anyway, so the additional CPU IDs are not wanted. Allow the number to be set to zero at compile time.

Also, force the number of extra CPUs to zero if hotplugging is disabled which allows removing some conditional code.

Tested on uniprocessor x86_64 that ACPI claims has a disabled processor, with CPU hotplugging configured.

("After" has the number of additional CPUs set to 0)
Before: NR_CPUS: 512, nr_cpu_ids: 2, nr_node_ids 1
After: NR_CPUS: 512, nr_cpu_ids: 1, nr_node_ids 1

[Changed the name of the option and the prompt according to Ingo's
suggestion.]

arch/x86/kernel/smpboot.c +0 -1

@@ -1282,16 +1282,13 @@ __init void prefill_possible_map(void)
 	if (!num_processors)
 		num_processors = 1;
 
-#ifdef CONFIG_HOTPLUG_CPU

Quoted 7 lines · Ingo Molnar

Quoted 7 lines

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

Quoted 4 lines

Quoted 4 lines

And sometimes CPU hotplugging is enabled only for suspend/hibernate anyway, so the additional CPU IDs are not wanted. Allow the number to be set to zero at compile time.

Wouldn't this be better to have a runtime option?

Quoted 5 lines

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

And sometimes CPU hotplugging is enabled only for suspend/hibernate anyway, so the additional CPU IDs are not wanted. Allow the number to be set to zero at compile time.

Quoted 10 lines · Chuck Ebbert

Quoted 7 lines · On Thu, 02 Oct 2008 11:12:51 +0200 Andi Kleen <[email protected]> wrote:

Quoted 5 lines · Chuck Ebbert

The default number of additional CPU IDs for hotplugging is determined by asking ACPI or mptables how many "disabled" CPUs there are in the system, but many systems get this wrong so that e.g. a uniprocessor machine gets an extra CPU allocated and never switches to single CPU mode.

What do you mean with single cpu mode?

e.g. the lock prefix rewriting is only determined by online CPUs, not possible CPUs. And this only affects the possible ones.

The prefix rewriting doesn't happen unless I boot with additional_cpus=0, maxcpus=1, or with this patch applied and the config option set. I think the rules for when/if the rewriting happens changed a while ago to avoid multiple switches and now it's not happening at all on this machine by default.

Oh, and with NR_CPUS=512 I am seeing 1.6MB per-cpu data (I'll have to check that, but I remember being surprised at how big the number was.)

Quoted 5 lines

The prefix rewriting doesn't happen unless I boot with additional_cpus=0, maxcpus=1, or with this patch applied and the config option set. I think the rules for when/if the rewriting happens changed a while ago to avoid multiple switches and now it's not happening at all on this machine by default.

Well then something is broken, but the fix is not to lower num_possible_cpus(), but to fix the root cause.

Does the appended patch help?

---

Take disabled cpus into account in alternative.c

Otherwise an UP system with one hotplug CPU will not go into UP mode.

Signed-off-by Andi Kleen <[email protected]>

Index: linux-2.6.27-rc4-misc/arch/x86/kernel/alternative.c
===================================================================

arch/x86/kernel/alternative.c +1 -1

@@ -454,7 +454,7 @@ void __init alternative_instructions(voi
 					    _text, _etext);
 
 		/* Only switch to UP mode if we don't immediately boot others */
-		if (num_possible_cpus() == 1 || setup_max_cpus <= 1)
+		if (num_possible_cpus() - additional_cpus == 1 || setup_max_cpus <= 1)
 			alternatives_smp_switch(0);
 	}
 #endif

Index: linux-2.6.27-rc4-misc/arch/x86/kernel/smpboot.c
===================================================================

arch/x86/kernel/smpboot.c +1 -1

@@ -1276,7 +1276,7 @@ static void remove_siblinginfo(int cpu)
 	cpu_clear(cpu, cpu_sibling_setup_map);
 }
 
-static int additional_cpus __initdata = -1;
+int additional_cpus = -1;
 
 static __init int setup_additional_cpus(char *s)
 {

Index: linux-2.6.27-rc4-misc/include/asm-x86/smp.h
===================================================================

include/asm-x86/smp.h +2 -0

@@ -204,5 +204,7 @@ static inline int hard_smp_processor_id(
 extern void cpu_uninit(void);
 #endif
 
+extern int additional_cpus;
+
 #endif /* __ASSEMBLY__ */
 #endif

Signature · 1 line

[email protected]

Ping? Can you please test the patch? I think that's the correct fix.

I see Ingo unfortunately merged your initial broken hack, but it's wrong and when actually used on a distribution will break real CPU hotplug there. Please don't enable that CONFIG option in Fedora. Ideally drop the CONFIG patch completely because it cannot do much good.

It should be replaced with the appended patch, which should go into 2.6.27 after it is confirmed to fix the problem.

Quoted 49 lines

---

Take disabled cpus into account in alternative.c

Otherwise an UP system with one hotplug CPU will not go into UP mode.

Signed-off-by Andi Kleen <[email protected]>

Index: linux-2.6.27-rc4-misc/arch/x86/kernel/alternative.c
===================================================================

arch/x86/kernel/alternative.c +1 -1

@@ -454,7 +454,7 @@ void __init alternative_instructions(voi
 					    _text, _etext);
 
 		/* Only switch to UP mode if we don't immediately boot others */
-		if (num_possible_cpus() == 1 || setup_max_cpus <= 1)
+		if (num_possible_cpus() - additional_cpus == 1 || setup_max_cpus <= 1)
 			alternatives_smp_switch(0);
 	}
 #endif

Index: linux-2.6.27-rc4-misc/arch/x86/kernel/smpboot.c
===================================================================

arch/x86/kernel/smpboot.c +1 -1

@@ -1276,7 +1276,7 @@ static void remove_siblinginfo(int cpu)
 	cpu_clear(cpu, cpu_sibling_setup_map);
 }
 
-static int additional_cpus __initdata = -1;
+int additional_cpus = -1;
 
 static __init int setup_additional_cpus(char *s)
 {

Index: linux-2.6.27-rc4-misc/include/asm-x86/smp.h
===================================================================

include/asm-x86/smp.h +2 -0

@@ -204,5 +204,7 @@ static inline int hard_smp_processor_id(
 extern void cpu_uninit(void);
 #endif
 
+extern int additional_cpus;
+
 #endif /* __ASSEMBLY__ */
 #endif

Signature · 1 line

[email protected]

Yes, it works and I don't see how it could cause any problems.

Ingo, can we get this in 2.6.27? You can drop my original patch.

Tested-by Chuck Ebbert <[email protected]>

Quoted 5 lines · Chuck Ebbert

Yes, it works and I don't see how it could cause any problems.

Ingo, can we get this in 2.6.27? You can drop my original patch.

Tested-by Chuck Ebbert <[email protected]>

looks good, applied to tip/x86/core, thanks!

Quoted 6 lines · Ingo Molnar

Quoted 4 lines · Chuck Ebbert

Yes, it works and I don't see how it could cause any problems.

Ingo, can we get this in 2.6.27? You can drop my original patch.

Quoted 3 lines

Quoted 1 line

Tested-by Chuck Ebbert <[email protected]>

looks good, applied to tip/x86/core, thanks!

No, this patch is horrible.

The correct check is num_present_cpus(). There is no need to make the weird additional_cpus hackery globally available.

Btw, additional_cpus has interesting properties. Providing a negative number < -1 on the kernel command line - happened due to a typo - explodes in early boot, which is not really surprising, but should be sanity checked.

From a3f493ab543d300b3a01ad18bf12f73746ae5c9f Mon Sep 17 00:00:00 2001

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Ingo Molnar

Quoted 4 lines · Chuck Ebbert

Yes, it works and I don't see how it could cause any problems.

Ingo, can we get this in 2.6.27? You can drop my original patch.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Tested-by Chuck Ebbert <[email protected]>

looks good, applied to tip/x86/core, thanks!

No, this patch is horrible.

The correct check is num_present_cpus(). There is no need to make the weird additional_cpus hackery globally available.

ah, indeed!

applied to tip/x86/core and i've zapped Andi's patch.

Quoted 4 lines

Btw, additional_cpus has interesting properties. Providing a negative number < -1 on the kernel command line - happened due to a typo - explodes in early boot, which is not really surprising, but should be sanity checked.

indeed, and that mess was introduced, interestingly, by this commit, three years ago, by Andi:

| From 420f8f68c9c5148dddf946bebdbc7eacde2172cb Mon Sep 17 00:00:00 2001
| From: Andi Kleen <[email protected]>
| Date: Sat, 5 Nov 2005 17:25:54 +0100
| Subject: [PATCH] [PATCH] x86_64: New heuristics to find out hotpluggable CPUs.

so to clean up the mess i've removed the additional_cpus= boot parameter and the Kconfig entry as well - see the patch in x86/core below.

thanks Thomas for decoding this ...

and no way can any of this go into v2.6.27: this is fragile code with a lot of historic baggage and the original error is non-fatal to begin with. It can easily be backported to .27.1 if testing shows that it has no other adverse side-effects.

From 344707c1f43dd0d080828497aacb60c0cc0a8c13 Mon Sep 17 00:00:00 2001

Quoted 4 lines

and no way can any of this go into v2.6.27: this is fragile code with a lot of historic baggage and the original error is non-fatal to begin with. It can easily be backported to .27.1 if testing shows that it has no other adverse side-effects.

Please lets get rid of all this.

Thanks,

tglx
---------------->

Quoted 1 line

From 344707c1f43dd0d080828497aacb60c0cc0a8c13 Mon Sep 17 00:00:00 2001

From: Thomas Gleixner <[email protected]>
Date: Sun, 5 Oct 2008 17:27:56 +0200
Subject: [PATCH] x86, smpboot: remove additional_cpus

remove remainder of additional_cpus logic. We now just listen to the disabled_cpus value like we did for years. disabled_cpus is always >= 0 so no need for an extra check.

Signed-off-by Thomas Gleixner <[email protected]>

Quoted 6 lines

Btw, additional_cpus has interesting properties. Providing a negative number < -1 on the kernel command line - happened due to a typo - explodes in early boot, which is not really surprising, but should be sanity checked.

indeed, and that mess was introduced, interestingly, by this commit, three years ago, by Andi:

| From 420f8f68c9c5148dddf946bebdbc7eacde2172cb Mon Sep 17 00:00:00 2001
| From: Andi Kleen <[email protected]>
| Date: Sat, 5 Nov 2005 17:25:54 +0100
| Subject: [PATCH] [PATCH] x86_64: New heuristics to find out hotpluggable CPUs.

so to clean up the mess i've removed the additional_cpus= boot parameter and the Kconfig entry as well - see the patch in x86/core below.

and no way can any of this go into v2.6.27: this is fragile code with a lot of historic baggage

and the original error is non-fatal to begin
with.

Please lets get rid of all this.

Quoted 2 lines

Please lets get rid of all this.

The result is that there's no way to override a BIOS now that doesn't declare the number of hotplug CPUs with the Linux hotplug extension.

Completely trusting the BIOS seems like a bad idea.

So I think you still need the command line parameter back, otherwise there's no way to override the BIOS.

-Andi (who wished you guys would check with the original author before removing code you clearly don't understand)

Your proposed solution is:

exporting additional_cpus and change the check to:

if ((num_possible_cpus() - additional_cpus) == 1 ...

This is simply stupid. We have the information already in num_present_cpus() and that's the correct check in the alternatives code.

additional_cpus is useless ballast.

tglx - who refrains from adding a nasty insulting comment