speeding up cpu_up()

Thanks for testing, Aravind, Borislav,

I went with Ingo's suggestion to made this a quirk. However, I went with a white-list instead of a black-list, because fewer comparisons are needed for modern processors. Families are added infrequently, and finally, we get the option to stick something other than 0 in the table if it we need to.

Let me know if I got the AMD family #'s right.

thanks,
-Len Brown, Intel Open Source Technology Center

From: Len Brown <[email protected]>

Replace the hard-coded mdelay(10) in cpu_up() to a variable udelay.

Add a boot-time override, "cpu_init_udelay=N"

Default unchanged at 10ms on all systems.

---
 Documentation/kernel-parameters.txt |  6 ++++++
 arch/x86/kernel/smpboot.c           | 28 +++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

index bfcb1a6..0a16309 100644
@@ -737,6 +737,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	cpuidle.off=1	[CPU_IDLE]
 			disable the cpuidle sub-system
 
+	cpu_init_udelay=N
+			[X86] Delay for N microsec between assert and de-assert
+			of APIC INIT to start processors.  This delay occurs
+			on every CPU online, such as boot, and resume from suspend.
+			Default: 10000
+
 	cpcihp_generic=	[HW,PCI] Generic port I/O CompactPCI driver
 			Format:
 			<first_slot>,<last_slot>,<port>,<enum_bit>[,<debug>]

index febc6aa..76734f4 100644
@@ -517,6 +517,32 @@ void __inquire_remote_apic(int apicid)
 }
 
 /*
+ * The Multiprocessor Specification 1.4 (1997) example code suggests
+ * that there should be a 10ms delay between the BSP asserting INIT
+ * and de-asserting INIT, when starting a remote processor.
+ * But that slows boot and resume on modern processors, which include
+ * many cores and don't require that delay.
+ *
+ * cmdline "init_cpu_udelay=" is available to specify this delay.
+ */
+#define UDELAY_10MS_DEFAULT 10000
+
+static unsigned int init_udelay = UDELAY_10MS_DEFAULT;
+
+static int __init cpu_init_udelay(char *str)
+{
+	unsigned int new_udelay;
+
+	get_option(&str, &new_udelay);
+	pr_debug("cpu_init_udelay=%d, was %d", new_udelay, init_udelay);
+	init_udelay = new_udelay;
+
+	return 0;
+}
+
+early_param("cpu_init_udelay", cpu_init_udelay);
+
+/*
  * Poke the other CPU in the eye via NMI to wake it up. Remember that the normal
  * INIT, INIT, STARTUP sequence will reset the chip hard for us, and this
  * won't ... remember to clear down the APIC, etc later.

@@ -586,7 +612,7 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
 	pr_debug("Waiting for send to finish...\n");
 	send_status = safe_apic_wait_icr_idle();
 
-	mdelay(10);
+	udelay(init_udelay);
 
 	pr_debug("Deasserting INIT\n");
 

2.4.0.rc1

From: Len Brown <[email protected]>

Modern processor familes are on a white-list to remove the costly cpu_init_udelay 10000. Unknown processor families get the traditional 10ms delay in cpu_up().

This seemed more efficient than forcing modern processors to exhaustively search a black-list having all the old processor families that should have a 10ms delay. For not only are new processor familes infrequently added, the white list also allows a delay other than 0, if needed.

---
 arch/x86/kernel/smpboot.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

index 76734f4..34a08ff 100644
@@ -76,6 +76,7 @@
 #include <asm/i8259.h>
 #include <asm/realmode.h>
 #include <asm/misc.h>
+#include <asm/cpu_device_id.h>
 
 /* State of each CPU */
 DEFINE_PER_CPU(int, cpu_state) = { 0 };

@@ -521,14 +522,44 @@ void __inquire_remote_apic(int apicid)
  * that there should be a 10ms delay between the BSP asserting INIT
  * and de-asserting INIT, when starting a remote processor.
  * But that slows boot and resume on modern processors, which include
- * many cores and don't require that delay.
- *
+ * many cores and don't require that delay.  Here we default to the
+ * legacy delay, but quirk new processors to skip the delay.
  * cmdline "init_cpu_udelay=" is available to specify this delay.
  */
 #define UDELAY_10MS_DEFAULT 10000
 
 static unsigned int init_udelay = UDELAY_10MS_DEFAULT;
 
+static const struct x86_cpu_id init_udelay_ids[] = {
+	{ X86_VENDOR_INTEL, 0x6, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{ X86_VENDOR_AMD, 0x16, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{ X86_VENDOR_AMD, 0x15, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{ X86_VENDOR_AMD, 0x14, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{ X86_VENDOR_AMD, 0x12, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{ X86_VENDOR_AMD, 0x11, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{ X86_VENDOR_AMD, 0x10, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{ X86_VENDOR_AMD, 0xF, X86_MODEL_ANY, X86_FEATURE_ANY, 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, init_udelay_ids);
+
+static void __init smp_quirk_init_udelay(void)
+{
+	const struct x86_cpu_id *id;
+	unsigned int new_udelay;
+
+	id = x86_match_cpu(init_udelay_ids);
+	if (id == NULL)
+		return;	/* if no match, keep default */
+
+	if (init_udelay != UDELAY_10MS_DEFAULT)
+		return;	/* if cmdline changed from default, leave it alone */
+
+	new_udelay = (unsigned long) id->driver_data;
+	pr_debug("cpu_init_udelay quirk to %d, was %d", new_udelay, init_udelay);
+	init_udelay = new_udelay;
+}
+
 static int __init cpu_init_udelay(char *str)
 {
 	unsigned int new_udelay;

@@ -1196,6 +1227,8 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 		uv_system_init();
 
 	set_mtrr_aps_delayed_init();
+
+	smp_quirk_init_udelay();
 }
 
 void arch_enable_nonboot_cpus_begin(void)

2.4.0.rc1

I was about to say that...

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

...

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

I agree with you. Further, you made me think about it, and while it was helpful when I wrote the patch, I don't think any printk() is needed upstream.

see v3 just sent.

Len Brown, Intel Open Source Technology Center

Yeah, you're right.

If a user did override it with cpu_init_udelay, it'll be in /proc/cmdline. And dmesg, and so on...

Looks ok.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Ugh, so I see we have grown this gem some time ago:

#if defined(CONFIG_DYNAMIC_DEBUG)
/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */

 #define pr_debug(fmt, ...) \
         dynamic_pr_debug(fmt, ##__VA_ARGS__)

I didn't even realize it's there and it happend 6 years ago, in a very unintuitively titled commit:

346e15beb534 driver core: basic infrastructure for per-module dynamic debug messages

So in what way does that title tell us that all pr_debug() calls are redirected away if CONFIG_DYNAMIC_DEBUG is enabled (which distros do)?

So could we instead either add a dyndbg=all variant, or make 'debug' trigger all dynamic_pr_debug() messages?

Because this redirection breaks the whole pr_*() abstraction rather fundamentally, dyndebug stealing pr_debug() and hiding debug messages when the user specifically asked for them via 'debug' is pretty nasty IMHO ...

Thanks,

Ingo

(Resending my reply with more dyn-debug folks Cc:-ed)

Ugh, so I see we have grown this gem some time ago:

#if defined(CONFIG_DYNAMIC_DEBUG)
/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */

 #define pr_debug(fmt, ...) \
         dynamic_pr_debug(fmt, ##__VA_ARGS__)

I didn't even realize it's there and it happend 6 years ago, in a very unintuitively titled commit:

346e15beb534 driver core: basic infrastructure for per-module dynamic debug messages

So in what way does that title tell us that all pr_debug() calls are redirected away if CONFIG_DYNAMIC_DEBUG is enabled (which distros do)?

So could we instead either add a dyndbg=all variant, or make 'debug' trigger all dynamic_pr_debug() messages?

Because this redirection breaks the whole pr_*() abstraction rather fundamentally, dyndebug stealing pr_debug() and hiding debug messages when the user specifically asked for them via 'debug' is pretty nasty IMHO ...

Thanks,

Ingo

Yeah, this thing made pr_debug() completely unusable.

/me checks.

SUSE does, at least.

Not only that:

#elif defined(DEBUG)

#define pr_debug(fmt, ...) \
        printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)

now, AFAIU, this means if CONFIG_DYNAMIC_DEBUG=n, I still have to have DEBUG defined in order to see debug output.

Because debug= sets only console loglevel to CONSOLE_LOGLEVEL_DEBUG (debug_kernel() in main.c).

So lemme do some experiments:

$ make arch/x86/kernel/smpboot.i

I'm looking at the first function in there - smpboot_setup_warm_reset_vector() - which has some pr_debug statements:

---
static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) void smpboot_setup_warm_reset_vector(unsigned long start_eip) {
unsigned long flags;

do { do { ({ unsigned long __dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); flags = _raw_spin_lock_irqsave(spinlock_check(&rtc_lock)); } while (0); } while (0); rtc_cmos_write(0xa, 0xf);
spin_unlock_irqrestore(&rtc_lock, flags);
__native_flush_tlb();
no_printk("\001" "7" "smpboot" ": " "1.\n");
^^^^^^^^
---

Yeah, so that's not issuing anything.

Now, if I do

#define DEBUG

BUT! I have to remember to put it *before* all include statements in that file so that printk.h can see it defined, then I see the printk() call.

---
static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) void smpboot_setup_warm_reset_vector(unsigned long start_eip) {
unsigned long flags;

do { do { ({ unsigned long __dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); flags = _raw_spin_lock_irqsave(spinlock_check(&rtc_lock)); } while (0); } while (0); rtc_cmos_write(0xa, 0xf);
spin_unlock_irqrestore(&rtc_lock, flags);
__native_flush_tlb();
printk("\001" "7" "smpboot" ": " "1.\n");
^^^^^^
---

So great, I have a printk there but I have to rebuild the kernel. I can then just as well add my own debug statements, even use trace_printk for that matter...

So even a dyndbg=all won't work if you haven't defined CONFIG_DYNAMIC_DEBUG in your config.

So pr_debug() gets optimized away in the normal case but if we want to always build in important debug statements and turn them on without building the kernel, the easiest thing to do is to switch to good ole

printk(KERN_DEBUG

Hell, we can even do an x86-specific define: x86_debug() or so, which would also denote that this is a debug statement which should stay built-in and it gives important information. All the early SMP bootstrap code for example would use that.

And so on and so on...

Yeah, I think we should do our own which doesn't interfere with pr_*

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--