[PATCH 0/2 v2] speeding up cpu_up()

[PATCH 0/2 v2] speeding up cpu_up()

[Len Brown]

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

[PATCH 1/2] x86: replace cpu_up hard-coded mdelay with variable udelay

[Len Brown]

From: Len Brown <len.brown@intel.com>

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.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 Documentation/kernel-parameters.txt |  6 ++++++
 arch/x86/kernel/smpboot.c           | 28 +++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index bfcb1a6..0a16309 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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>]
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index febc6aa..76734f4 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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

[PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Len Brown]

From: Len Brown <len.brown@intel.com>

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.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/smpboot.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 76734f4..34a08ff 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> 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.

>  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 },
> +	{}
> +};

So since especially AMD likes to iterate the family upwards, why not 
make this a simple open ended check:

	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
	    boot_cpu_data.x86 >= 6 ||
	    boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
	    boot_cpu_data.x86 >= 15) {

		... 0 delay ...
	}

... which is much smaller and more future proof?

Thanks,

	Ingo

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Borislav Petkov]

On Fri, May 08, 2015 at 09:51:11AM +0200, Ingo Molnar wrote:
> > +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 },
> > +	{}
> > +};
> 
> So since especially AMD likes to iterate the family upwards, why not 
> make this a simple open ended check:
> 
> 	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> 	    boot_cpu_data.x86 >= 6 ||
> 	    boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> 	    boot_cpu_data.x86 >= 15) {
> 
> 		... 0 delay ...
> 	}
> 
> ... which is much smaller and more future proof?

I was about to say that...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Borislav Petkov]

On Fri, May 08, 2015 at 02:37:55AM -0400, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> 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.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  arch/x86/kernel/smpboot.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)

...

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

Can we make this printk(KERN_DEBUG please?

I'd like to be able to slap "debug" on the command line and not
recompile the kernel. And no, dyndbg="file smpboot.c +p" or whatever the
syntax is, simply doesn't scale if I want to see all debug messages from
early boot.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Len Brown]

On Fri, May 8, 2015 at 4:32 AM, Borislav Petkov <bp@alien8.de> wrote:

>> +     pr_debug("cpu_init_udelay quirk to %d, was %d", new_udelay, init_udelay);
>
> Can we make this printk(KERN_DEBUG please?
>
> I'd like to be able to slap "debug" on the command line and not
> recompile the kernel. And no, dyndbg="file smpboot.c +p" or whatever the
> syntax is, simply doesn't scale if I want to see all debug messages from
> early boot.

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

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Borislav Petkov]

On Fri, May 08, 2015 at 02:15:42PM -0400, Len Brown wrote:
> 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.

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

> see v3 just sent.

Looks ok.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> On Fri, May 8, 2015 at 4:32 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> >> +     pr_debug("cpu_init_udelay quirk to %d, was %d", new_udelay, init_udelay);
> >
> > Can we make this printk(KERN_DEBUG please?
> >
> > I'd like to be able to slap "debug" on the command line and not 
> > recompile the kernel. And no, dyndbg="file smpboot.c +p" or 
> > whatever the syntax is, simply doesn't scale if I want to see all 
> > debug messages from early boot.

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

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Ingo Molnar]

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

* Len Brown <lenb@kernel.org> wrote:

> On Fri, May 8, 2015 at 4:32 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> >> +     pr_debug("cpu_init_udelay quirk to %d, was %d", new_udelay, init_udelay);
> >
> > Can we make this printk(KERN_DEBUG please?
> >
> > I'd like to be able to slap "debug" on the command line and not 
> > recompile the kernel. And no, dyndbg="file smpboot.c +p" or 
> > whatever the syntax is, simply doesn't scale if I want to see all 
> > debug messages from early boot.

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

Re: [PATCH 2/2] x86: speed cpu_up by quirking cpu_init_udelay

[Borislav Petkov]

On Sat, May 09, 2015 at 09:22:38AM +0200, Ingo Molnar wrote:
> 
> * Len Brown <lenb@kernel.org> wrote:
> 
> > On Fri, May 8, 2015 at 4:32 AM, Borislav Petkov <bp@alien8.de> wrote:
> > 
> > >> +     pr_debug("cpu_init_udelay quirk to %d, was %d", new_udelay, init_udelay);
> > >
> > > Can we make this printk(KERN_DEBUG please?
> > >
> > > I'd like to be able to slap "debug" on the command line and not 
> > > recompile the kernel. And no, dyndbg="file smpboot.c +p" or 
> > > whatever the syntax is, simply doesn't scale if I want to see all 
> > > debug messages from early boot.
> 
> 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

Yeah, this thing made pr_debug() completely unusable.

> (which distros do)?

/me checks.

SUSE does, at least.

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

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

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

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