CPU Hotplug support for X86_64
Subsystems (2)
cover CPU Hotplug support for X86_64
[Oops: This is correct version: i send from the wrong directory earlier] [Sorry about that spam... now i thin i know how to use quilt :-)]
This implements cpu hotplug support for X86_64.
Modified after initial feedback from Andi.
[PS: hopefully subject is now fixed with different subject lines]
Seems to hold up for make -j, with online/offline activity.
The series of patches are split as follows:
1. initcall cleanup
- Left __cpuinit cases as before per Andi
- Added a few new ones, and removed couple that could stay __init even with cpu hotplug.
2. Core logical online/offline of cpus
- start with maxcpus=1, and then echo 1 to /sys/devices/system/online
- Can also bringup all cpus and then bring up/down all but cpu0.
- Also tested with numa=fake=2
3. Cleanup sibling map for cpu hotplug support.
4. Dont use IPI broadcast in smp_call_function() when using CPU hotplug. Hopefully this is some reasonable middle ground starting point.
- Dont let a new cpu respond to IPI's.
- Automaticaly selected if CPU hotplus is choosen.
- Can also be turned on cmdline via safe_ipi=1
TBD:
1. Track down CONFIG_SCHED_SMT Oops with both cpu up/down.
2. Test on real NUMA hw.
Cheers,
Ashok Raj
--
PATCH 1/4 CPU Hotplug support for X86_64
Subject: [patch 1/4] x86_64: Change init sections for CPU hotplug support
This patch adds __cpuinit and __cpuinitdata sections that need to exist past boot to support cpu hotplug.
Caveat: This is done *only* for EM64T CPU Hotplug support, on request from Andi Kleen. Much of the generic hotplug code in kernel, and none of the other archs that support CPU hotplug today, i386, ia64, ppc64, s390 and parisc dont do this, and only mark them as __devinit, and __devinitdata.
If someone is motivated to change generic code, we need to make sure all existing hotplug code does not break.
-----------------------------------------
arch/x86_64/kernel/apic.c | 8 ++++---- arch/x86_64/kernel/i387.c | 2 +- arch/x86_64/kernel/nmi.c | 4 ++-- arch/x86_64/kernel/process.c | 2 +- arch/x86_64/kernel/setup.c | 18 +++++++++--------- arch/x86_64/kernel/setup64.c | 6 +++--- arch/x86_64/kernel/smpboot.c | 15 +++++---------- arch/x86_64/mm/numa.c | 2 +- include/linux/init.h | 13 +++++++++++++ mm/page_alloc.c | 2 +- 10 files changed, 40 insertions(+), 32 deletions(-)
include/linux/init.h +13 -0
@@ -222,6 +222,19 @@ void __init parse_early_param(void);#define __devinitdata#define __devexit#define __devexitdata + +#ifdef CONFIG_HOTPLUG_CPU +#define __cpuinit +#define __cpuinitdata +#define __cpuexit +#define __cpuexitdata +#else +#define __cpuinit __init +#define __cpuinitdata __initdata +#define __cpuexit __exit +#define __cpuexitdata __exitdata +#endif +#else #define __devinit __init#define __devinitdata __initdata
arch/x86_64/kernel/smpboot.c +5 -10
@@ -58,11 +58,6 @@#include <asm/proto.h> #include <asm/nmi.h> -/* Change for real CPU hotplug. Note other files need to be fixed -first too. */ -#define __cpuinit __init -#define __cpuinitdata __initdata -/* Number of siblings per CPU package */ int smp_num_siblings =1 ;/* Package ID of each logical CPU */
@@ -822,7 +817,7 @@ static __cpuinit void smp_cleanup_boot(v* * RED-PEN audit/test this more. I bet there is more state messed up here. */ -static __cpuinit void disable_smp (void ) +static __init void disable_smp (void ) { cpu_present_map =cpumask_of_cpu (0 ); cpu_possible_map =cpumask_of_cpu (0 );
@@ -837,7 +832,7 @@ static __cpuinit void disable_smp(void)/* * Handle user cpus=... parameter. */ -static __cpuinit void enforce_max_cpus (unsigned max_cpus) +static __init void enforce_max_cpus (unsigned max_cpus) {int i, k; k =0 ;
@@ -854,7 +849,7 @@ static __cpuinit void enforce_max_cpus(u/* * Various sanity checks. */ -static int __cpuinit smp_sanity_check (unsigned max_cpus) +static int __init smp_sanity_check (unsigned max_cpus) {if (!physid_isset (hard_smp_processor_id (), phys_cpu_present_map)) { printk("weird, boot CPU (#%d) not listed by the BIOS.\n",
@@ -912,7 +907,7 @@ static int __cpuinit smp_sanity_check(un* Prepare for SMP bootup. The MP table or ACPI has been read * earlier. Just do some sanity checking here and enable APIC mode. */ -void __cpuinit smp_prepare_cpus (unsigned int max_cpus) +void __init smp_prepare_cpus (unsigned int max_cpus) {int i;
@@ -1018,7 +1013,7 @@ int __cpuinit __cpu_up(unsigned int cpu)/* * Finish the SMP boot. */ -void __cpuinit smp_cpus_done (unsigned int max_cpus) +void __init smp_cpus_done (unsigned int max_cpus) {zap_low_mappings ();smp_cleanup_boot ();
arch/x86_64/kernel/process.c +1 -1
@@ -204,7 +204,7 @@ static void mwait_idle(void) } } -void __init select_idle_routine (const struct cpuinfo_x86 *c) +void __cpuinit select_idle_routine (const struct cpuinfo_x86 *c) {static int printed;if (cpu_has (c,X86_FEATURE_MWAIT )) {
arch/x86_64/kernel/setup.c +9 -9
@@ -696,7 +696,7 @@ void __init setup_arch(char **cmdline_p)#endif } -static int __init get_model_name (struct cpuinfo_x86 *c) +static int __cpuinit get_model_name (struct cpuinfo_x86 *c) {unsigned int *v;
@@ -712,7 +712,7 @@ static int __init get_model_name(struct } -static void __init display_cacheinfo (struct cpuinfo_x86 *c) +static void __cpuinit display_cacheinfo (struct cpuinfo_x86 *c) {unsigned int n, dummy, eax, ebx, ecx, edx;
@@ -823,7 +823,7 @@ static int __init init_amd(struct cpuinfreturn r; } -static void __init detect_ht (struct cpuinfo_x86 *c) +static void __cpuinit detect_ht (struct cpuinfo_x86 *c) {#ifdef CONFIG_SMP u32 eax, ebx, ecx, edx;
@@ -884,7 +884,7 @@ static void __init detect_ht(struct cpui/* * find out the number of processor cores on the die */ -static int __init intel_num_cpu_cores (struct cpuinfo_x86 *c) +static int __cpuinit intel_num_cpu_cores (struct cpuinfo_x86 *c) {unsigned int eax;
@@ -902,7 +902,7 @@ static int __init intel_num_cpu_cores(streturn 1 ; } -static void __init init_intel (struct cpuinfo_x86 *c) +static void __cpuinit init_intel (struct cpuinfo_x86 *c) {/* Cache sizes */ unsigned n;
@@ -922,7 +922,7 @@ static void __init init_intel(struct cpu c->x86_num_cores =intel_num_cpu_cores (c); } -void __init get_cpu_vendor (struct cpuinfo_x86 *c) +void __cpuinit get_cpu_vendor (struct cpuinfo_x86 *c) {char *v = c->x86_vendor_id;
@@ -943,7 +943,7 @@ struct cpu_model_info {/* Do some early cpuid on the boot CPU to get some parameter that are needed before check_bugs. Everything advanced is in identify_cpu below. */ -void __init early_identify_cpu (struct cpuinfo_x86 *c) +void __cpuinit early_identify_cpu (struct cpuinfo_x86 *c) {u32 tfms;
@@ -998,7 +998,7 @@ void __init early_identify_cpu(struct cp/* * This does the hard work of actually picking apart the CPU stuff... */ -void __init identify_cpu (struct cpuinfo_x86 *c) +void __cpuinit identify_cpu (struct cpuinfo_x86 *c) {int i;u32 xlvl;
@@ -1075,7 +1075,7 @@ void __init identify_cpu(struct cpuinfo_ } -void __init print_cpu_info (struct cpuinfo_x86 *c) +void __cpuinit print_cpu_info (struct cpuinfo_x86 *c) {if (c->x86_model_id[0 ])printk ("%s" , c->x86_model_id);
arch/x86_64/kernel/apic.c +4 -4
@@ -321,7 +321,7 @@ void __init init_bsp_APIC(void)apic_write_around (APIC_LVT1 , value); } -void __init setup_local_APIC (void ) +void __cpuinit setup_local_APIC (void ) {unsigned int value, ver, maxlvt;
@@ -570,7 +570,7 @@ static struct sys_device device_lapic = .cls = &lapic_sysclass, }; -static void __init apic_pm_activate (void ) +static void __cpuinit apic_pm_activate (void ) { apic_pm_state.active =1 ; }
@@ -810,14 +810,14 @@ void __init setup_boot_APIC_clock (void)local_irq_enable (); } -void __init setup_secondary_APIC_clock (void ) +void __cpuinit setup_secondary_APIC_clock (void ) {local_irq_disable ();/* FIXME: Do we need this? --RR */ setup_APIC_timer (calibration_result);local_irq_enable (); } -void __init disable_APIC_timer (void ) +void __cpuinit disable_APIC_timer (void ) {if (using_apic_timer) {unsigned long v;
arch/x86_64/kernel/setup64.c +3 -3
@@ -29,7 +29,7 @@char x86_boot_params[BOOT_PARAM_SIZE ]__initdata = {0 ,}; -cpumask_t cpu_initialized__initdata =CPU_MASK_NONE ; +cpumask_t cpu_initialized__cpuinitdata =CPU_MASK_NONE ; struct x8664_pda cpu_pda[NR_CPUS] __cacheline_aligned;
@@ -171,7 +171,7 @@ void syscall_init(void)wrmsrl (MSR_SYSCALL_MASK ,EF_TF |EF_DF |EF_IE |0x3000 ); } -void __init check_efer (void ) +void __cpuinit check_efer (void ) {unsigned long efer;
@@ -188,7 +188,7 @@ void __init check_efer(void)* 'CPU state barrier', nothing should get across. * A lot of state is already set up in PDA init. */ -void __init cpu_init (void ) +void __cpuinit cpu_init (void ) {#ifdef CONFIG_SMP int cpu =stack_smp_processor_id ();
arch/x86_64/kernel/i387.c +1 -1
@@ -42,7 +42,7 @@ void mxcsr_feature_mask_init(void)* Called at bootup to set up the initial FPU state that is later cloned * into all processes. */ -void __init fpu_init (void ) +void __cpuinit fpu_init (void ) {unsigned long oldcr0 =read_cr0 ();extern void __bad_fxsave_alignment (void );
mm/page_alloc.c +1 -1
@@ -72,7 +72,7 @@ EXPORT_SYMBOL(zone_table);#ifdef CONFIG_NUMA static struct per_cpu_pageset - pageset_table[MAX_NR_ZONES *MAX_NUMNODES *NR_CPUS ]__initdata ; + pageset_table[MAX_NR_ZONES *MAX_NUMNODES *NR_CPUS ]__cpuinitdata ;#endif static char *zone_names[MAX_NR_ZONES ] = {"DMA" ,"Normal" ,"HighMem" };
arch/x86_64/kernel/nmi.c +2 -2
@@ -98,7 +98,7 @@ static unsigned int nmi_p4_cccr_val; (P4_CCCR_OVF_PMI0 |P4_CCCR_THRESHOLD (15 )|P4_CCCR_COMPLEMENT | \ P4_CCCR_COMPARE|P4_CCCR_REQUIRED|P4_CCCR_ESCR_SELECT(4)|P4_CCCR_ENABLE) -static __init inline int nmi_known_cpu (void ) +static __cpuinit inline int nmi_known_cpu (void ) {switch (boot_cpu_data.x86_vendor) {case X86_VENDOR_AMD :
@@ -110,7 +110,7 @@ static __init inline int nmi_known_cpu(v }/* Run after command line and cpu_init init, but before all other checks */ -void __init nmi_watchdog_default (void ) +void __cpuinit nmi_watchdog_default (void ) {if (nmi_watchdog !=NMI_DEFAULT )return ;
arch/x86_64/mm/numa.c +1 -1
@@ -243,7 +243,7 @@ void __init numa_initmem_init(unsigned lsetup_node_bootmem (0 , start_pfn <<PAGE_SHIFT , end_pfn <<PAGE_SHIFT ); } -__init void numa_add_cpu (int cpu) +__cpuinit void numa_add_cpu (int cpu) {/* BP is initialized elsewhere */ if (cpu)
--
arch/x86_64/kernel/smpboot.c static __cpuinit void smp_cleanup_boot(v · line 820
Quoted 4 lines · Ashok Raj
* RED-PEN audit/test this more. I bet there is more state messed up here. */
arch/x86_64/kernel/smpboot.c +1
-1-static __cpuinit void disable_smp (void ) +static __init void disable_smp (void )
Why all these cpuinit->init changes? I think they should stay __cpuinit
The other way round looks ok.
-Andi
Quoted 9 lines · Andi Kleen
arch/x86_64/kernel/smpboot.c static __cpuinit void smp_cleanup_boot(v · line 820
Quoted 4 lines · Ashok Raj
* RED-PEN audit/test this more. I bet there is more state messed up here. */
arch/x86_64/kernel/smpboot.c +1
-1-static __cpuinit void disable_smp (void ) +static __init void disable_smp (void )Why all these
cpuinit->initchanges? I think they should stay__cpuinitThe other way round looks ok.
disable_smp() is called only in smp_prepare_cpus() which is not required for hotplug. Its currently only required only for startup, and not later.
I changed the ones from __cpuinit to __init, just in functions marked with paranoia... i think it can stay cpuinit, unless there is another reason i didnt catch.
Quoted 2 lines
-Andi
Signature · 3 lines
Cheers, Ashok Raj - Open Source Technology Center
Quoted 9 lines · Ashok Raj
Quoted 8 lines · Andi Kleen
arch/x86_64/kernel/smpboot.c static __cpuinit void smp_cleanup_boot(v · line 820
Quoted 4 lines · Ashok Raj
* RED-PEN audit/test this more. I bet there is more state messed up here. */
arch/x86_64/kernel/smpboot.c +1
-1-static __cpuinit void disable_smp (void ) +static __init void disable_smp (void )Why all these
cpuinit->initchanges? I think they should stay__cpuinit
Quoted 8 lines
Quoted 1 line
The other way round looks ok.
disable_smp()is called only insmp_prepare_cpus()which is not required for hotplug. Its currently only required only for startup, and not later.I changed the ones from
__cpuinitto__init, just in functions marked with paranoia... i think it can stay cpuinit, unless there is another reason i didnt catch.
Ok. Makes sense.
But how about all the other functions that you changed too? Can you double check them. SOme looked suspicious.
-Andi
PATCH 2/4 CPU Hotplug support for X86_64
In reply to Ashok RajSubject: [patch 2/4] x86_64: CPU hotplug support.
From: Ashok Raj <[email protected]>
Experimental CPU hotplug patch for x86_64
-----------------------------------------
This supports logical CPU online and offline.
- Test with maxcpus=1, and then kick other cpu's off to test if init code is all cleaned up.
- idle threads are forked on demand from keventd threads for clean startup
TBD:
1. Not tested on a real NUMA machine (tested with numa=fake=2)
2. CONFIG_SCHED_SMT seems to have trouble handling cpu add/removal
3. Handle ACPI pieces for physical hotplug support.
-----------
arch/i386/mach-default/topology.c | 7 - arch/x86_64/Kconfig | 9 + arch/x86_64/kernel/irq.c | 30 +++++ arch/x86_64/kernel/process.c | 33 +++++ arch/x86_64/kernel/smp.c | 12 ++ arch/x86_64/kernel/smpboot.c | 213 ++++++++++++++++++++++++++++++++++---- arch/x86_64/kernel/traps.c | 9 + include/asm-x86_64/irq.h | 5 include/asm-x86_64/smp.h | 4 9 files changed, 297 insertions(+), 25 deletions(-)
arch/x86_64/Kconfig +9 -0
@@ -294,6 +294,15 @@ config NR_CPUS This is purely to save memory - each supported CPU requires memory in the static kernel configuration. +config HOTPLUG_CPU + bool "Support for hot-pluggable CPUs (EXPERIMENTAL)" + depends on SMP && HOTPLUG && EXPERIMENTAL + help + Say Y here to experiment with turning CPUs off and on. CPUs + can be controlled through /sys/devices/system/cpu/cpu#. + Say N if you want to disable CPU hotplug. + + config HPET_TIMER bool default y
arch/x86_64/kernel/smpboot.c +192 -21
332 lines not on this page · raw message
arch/x86_64/kernel/irq.c +30 -0
@@ -14,6 +14,7 @@#include <linux/interrupt.h> #include <linux/seq_file.h> #include <linux/module.h> +#include <linux/delay.h> #include <asm/uaccess.h> #include <asm/io_apic.h>
@@ -106,3 +107,32 @@ asmlinkage unsigned int do_IRQ(struct ptreturn 1 ; } + +#ifdef CONFIG_HOTPLUG_CPU +void fixup_irqs (cpumask_t map) +{ +unsigned int irq; +static int warned; + +for (irq =0 ; irq <NR_IRQS ; irq++) { +cpumask_t mask; +if (irq ==2 ) +continue ; + +cpus_and (mask, irq_affinity[irq], map); +if (any_online_cpu (mask) ==NR_CPUS ) { +printk ("Breaking affinity for irq %i\n" , irq); + mask = map; + } +if (irq_desc[irq].handler->set_affinity) + irq_desc[irq].handler->set_affinity (irq, mask); +else if (irq_desc[irq].action && !(warned++)) +printk ("Cannot set affinity for irq %i\n" , irq); + } + +/* That doesn't seem sufficient. Give it 1ms. */ +local_irq_enable (); +mdelay (1 ); +local_irq_disable (); +} +#endif
include/asm-x86_64/irq.h +5 -0
@@ -52,4 +52,9 @@ struct irqaction;struct pt_regs ;int handle_IRQ_event (unsigned int ,struct pt_regs *,struct irqaction *); +#ifdef CONFIG_HOTPLUG_CPU +#include <linux/cpumask.h> +extern void fixup_irqs (cpumask_t map); +#endif +#endif /* _ASM_IRQ_H */
arch/x86_64/kernel/process.c +32 -1
@@ -8,7 +8,8 @@* * X86-64 port * Andi Kleen. -* +* +* CPU hotplug support - [email protected] * $Id: process.c,v 1.38 2002/01/15 10:08:03 ak Exp $ */
@@ -18,6 +19,7 @@#include <stdarg.h> +#include <linux/cpu.h> #include <linux/errno.h> #include <linux/sched.h> #include <linux/kernel.h>
@@ -154,6 +156,33 @@ void cpu_idle_wait(void) }EXPORT_SYMBOL_GPL (cpu_idle_wait); +#ifdef CONFIG_HOTPLUG_CPU +DECLARE_PER_CPU (int, cpu_state); + +#include <asm/nmi.h> +/* We don't actually take CPU down, just spin without interrupts. */ +static inline void play_dead (void ) +{ +idle_task_exit (); +mb (); +/* Ack it */ +__get_cpu_var (cpu_state) =CPU_DEAD ; + +local_irq_disable (); +while (1 ) +safe_halt (); +#if 0 +while (1 ) + __asm__ __volatile__("hlt" :::"memory" ); +#endif +} +#else +static inline void play_dead (void ) +{ +BUG (); +} +#endif /* CONFIG_HOTPLUG_CPU */ +/* * The idle thread. There's no useful work to be * done, so just try to conserve power and have a
@@ -174,6 +203,8 @@ void cpu_idle (void) idle = pm_idle;if (!idle) idle = default_idle; +if (cpu_is_offline (smp_processor_id ())) +play_dead ();idle (); }
include/asm-x86_64/smp.h +4 -0
@@ -44,6 +44,8 @@ extern void smp_alloc_memory(void);extern volatile unsigned long smp_invalidate_needed;extern int pic_mode;extern int smp_num_siblings; +extern void lock_ipi_calllock (void ); +extern void unlock_ipi_calllock (void );extern void smp_flush_tlb (void );extern void smp_message_irq (int cpl,void *dev_id,struct pt_regs *regs);extern void smp_send_reschedule (int cpu);
@@ -77,6 +79,8 @@ extern __inline int hard_smp_processor_i }extern int safe_smp_processor_id (void ); +extern int __cpu_disable (void ); +extern void __cpu_die (unsigned int cpu);#endif /* !ASSEMBLY */
arch/x86_64/kernel/traps.c +8 -1
@@ -587,11 +587,18 @@ static void unknown_nmi_error(unsigned c asmlinkage void default_do_nmi(struct pt_regs *regs) {unsigned char reason =0 ; +int cpu; + + cpu =smp_processor_id ();/* Only the BSP gets external NMIs from the system. */ -if (!smp_processor_id ()) +if (!cpu) reason =get_nmi_reason (); +#ifdef CONFIG_HOTPLUG_CPU +if (!cpu_online (cpu)) +return ; +#endif if (!(reason &0xc0 )) {if (notify_die (DIE_NMI_IPI ,"nmi_ipi" , regs, reason,0 ,SIGINT ) ==NOTIFY_STOP )
arch/x86_64/kernel/smp.c +12 -0
@@ -295,6 +295,18 @@ struct call_data_struct {static struct call_data_struct * call_data; +void +lock_ipi_calllock (void ) +{ +spin_lock_irq (&call_lock); +} + +void +unlock_ipi_calllock (void ) +{ +spin_unlock_irq (&call_lock); +} +/* * this function sends a 'generic call function' IPI to all other CPUs * in the system.
arch/i386/mach-default/topology.c +5 -2
@@ -88,8 +88,11 @@ static int __init topology_init(void) {int i; -for (i =0 ; i <NR_CPUS ; i++) -if (cpu_possible (i))arch_register_cpu (i); +for (i =0 ; i <NR_CPUS ; i++) { +if (cpu_possible (i)) { +arch_register_cpu (i); + } + }return 0 ; }
--
Quoted 8 lines · Ashok Raj
/*
arch/x86_64/kernel/smpboot.c +3
-0@@ -97,6 +97,26 @@ cpumask_t cpu_core_map[NR_CPUS] __cachelextern unsigned char trampoline_data[];extern unsigned char trampoline_end[]; +/* State of each CPU */ +DEFINE_PER_CPU(int, cpu_state) = { 0 }; +
arch/x86_64/kernel/smpboot.c cpumask_t cpu_core_map[NR_CPUS] __cachel · line 112
Quoted 10 lines
arch/x86_64/kernel/smpboot.c +10
-0+#ifdef CONFIG_HOTPLUG_CPU +/* +* Store all idle threads, this can be reused instead of creating +* a new thread. Also avoids complicated thread destroy functionality +* for idle threads. +*/ +struct task_struct *idle_thread_array[NR_CPUS ]; + +#define get_idle_for_cpu (x) (idle_thread_array[(x)]) +#define set_idle_for_cpu (x,p) (idle_thread_array[(x)] = (p))
Why is this only enabled for HOTPLUG_CPU? It looks like it could be used for the !HOTPLUG case too. That would be preferable to have less ifdefs.
arch/x86_64/kernel/smpboot.c static void __cpuinit smp_store_cpu_info · line 207
Quoted 7 lines
arch/x86_64/kernel/smpboot.c +3
-3-static __cpuinitdata DEFINE_SPINLOCK (tsc_sync_lock); -static volatile __cpuinitdata unsigned long go[SLAVE +1 ]; -static int notscsync__cpuinitdata ; +static __devinitdata DEFINE_SPINLOCK (tsc_sync_lock); +static volatile __devinitdata unsigned long go[SLAVE +1 ]; +static int notscsync__devinitdata ;
Should be __cpuinitdata
arch/x86_64/kernel/smpboot.c static int notscsync __cpuinitdata; · line 215
Quoted 9 lines
arch/x86_64/kernel/smpboot.c +1
-1#undef DEBUG_TSC_SYNC@@ -192,7 +212,7 @@ static int notscsync __cpuinitdata;#define NUM_ITERS 5/* likewise */ /* Callback on boot CPU */ -static __cpuinit void sync_master (void *arg) +static __devinit void sync_master (void *arg)
Didnt we agree to not do these changes?
Lots more cases in this file. The patch would be a lot smaller without them.
arch/x86_64/kernel/smpboot.c void __cpuinit smp_callin(void) · line 434
Quoted 6 lines
arch/x86_64/kernel/smpboot.c +2
-0@@ -410,6 +430,8 @@ void __cpuinit smp_callin(void)* Allow the master to continue. */ cpu_set (cpuid, cpu_callin_map); +mb (); +local_flush_tlb ();
Why is this needed?
arch/x86_64/kernel/smpboot.c void __init smp_prepare_cpus(unsigned in · line 994
Quoted 8 lines
arch/x86_64/kernel/smpboot.c +6
-0+#ifndef CONFIG_HOTPLUG_CPU cpu_set (i, cpu_possible_map); +#endif } +#ifdef CONFIG_HOTPLUG_CPU +printk ("Setting possible cpus %d\n" , i); +cpu_set (i, cpu_possible_map); +#endif
Why these two ifdefs? If possible remove them.
arch/x86_64/kernel/smpboot.c int __cpuinit __cpu_up(unsigned int cpu) · line 1085
Quoted 8 lines
arch/x86_64/kernel/smpboot.c +3
-1@@ -1007,7 +1080,10 @@ int __cpuinit __cpu_up(unsigned int cpu)while (!cpu_isset (cpu, cpu_online_map))cpu_relax (); -return 0 ; + err =0 ; +ret: +flush_tlb_all ();
Why this flush again?
How do you prevent the BP from being offlined? Currently some stuff (NMIs, timer) rely on it being present :/ Longer term they need to be fixed of course, but short term I would refuse to offline it. Needs an audit probably.
arch/x86_64/kernel/smpboot.c void __init smp_cpus_done(unsigned int m · line 1154
Quoted 7 lines
arch/x86_64/kernel/smpboot.c +7
-0+return -EBUSY ; + +disable_APIC_timer (); + +/* Allow any queued timer interrupts to get serviced */ +local_irq_enable (); +mdelay (1 );
This wont work with variable timer tick. Need some other way to kick the timer. It looks unreliable anyways.
arch/x86_64/kernel/smpboot.c void __init smp_cpus_done(unsigned int m · line 1164
Quoted 10 lines
arch/x86_64/kernel/smpboot.c +10
-0+ +/* +* Need this per zwane, but this uses IPI, so cannot be used +* in the machine down state. Need to find something else +* +* flush_tlb_all(); +*/ +local_flush_tlb (); +local_irq_disable (); +remove_siblinginfo (cpu);
No idea what all these TLB flushes are good for. Can you describe them?
arch/x86_64/kernel/traps.c static void unknown_nmi_error(unsigned c · line 595
Quoted 3 lines
arch/x86_64/kernel/traps.c +1
-1/* Only the BSP gets external NMIs from the system. */ -if (!smp_processor_id ()) +if (!cpu)
e.g. here you have the first BP dependency... Ideally it would shift if it is ever offlined.
arch/x86_64/kernel/traps.c static void unknown_nmi_error(unsigned c · line 601
Quoted 6 lines
arch/x86_64/kernel/traps.c +4
-0reason =get_nmi_reason (); +#ifdef CONFIG_HOTPLUG_CPU +if (!cpu_online (cpu)) +return ; +#endif
Please remove the ifdef.
-Andi
PATCH 3/4 CPU Hotplug support for X86_64
In reply to Ashok RajSubject: [patch 3/4] x86_64: CPU hotplug sibling map cleanup
From: Ashok Raj <[email protected]>
This patch is a minor cleanup to the cpu sibling/core map. It is required that this setup happens on a per-cpu bringup time.
------------------------------
smpboot.c | 89 ++++++++++++++++++++++++++++---------------------------------- 1 files changed, 41 insertions(+), 48 deletions(-)
arch/x86_64/kernel/smpboot.c +41 -48
@@ -62,9 +62,12 @@/* Number of siblings per CPU package */ int smp_num_siblings =1 ;/* Package ID of each logical CPU */ -u8 phys_proc_id[NR_CPUS ] = { [0 ...NR_CPUS -1 ] =BAD_APICID }; -u8 cpu_core_id[NR_CPUS ] = { [0 ...NR_CPUS -1 ] =BAD_APICID }; +u8 phys_proc_id[NR_CPUS] __cacheline_aligned = + { [0 ...NR_CPUS -1 ] =BAD_APICID };EXPORT_SYMBOL (phys_proc_id); + +u8 cpu_core_id[NR_CPUS] __cacheline_aligned = + { [0 ...NR_CPUS -1 ] =BAD_APICID };EXPORT_SYMBOL (cpu_core_id);/* Bitmask of currently online CPUs */
@@ -434,6 +437,34 @@ void __cpuinit smp_callin(void)local_flush_tlb (); } +static inline void +set_cpu_sibling_map (int cpu) +{ +int i; + +if (smp_num_siblings >1 ) { +for_each_online_cpu (i) { +if (cpu_core_id[cpu] == cpu_core_id[i]) { +cpu_set (i, cpu_sibling_map[cpu]); +cpu_set (cpu, cpu_sibling_map[i]); + } + } + }else { +cpu_set (cpu, cpu_sibling_map[cpu]); + } + +if (current_cpu_data.x86_num_cores >1 ) { +for_each_online_cpu (i) { +if (phys_proc_id[cpu] == phys_proc_id[i]) { +cpu_set (i, cpu_core_map[cpu]); +cpu_set (cpu, cpu_core_map[i]); + } + } + }else { + cpu_core_map[cpu] = cpu_sibling_map[cpu]; + } +} +/* * Setup code on secondary processor (after comming out of the trampoline) */
@@ -463,6 +494,12 @@ void __cpuinit start_secondary(void)enable_APIC_timer (); +/* +* The sibling maps must be set before turing the online map on for +* this cpu +*/ +set_cpu_sibling_map (smp_processor_id ()); +/* * Allow the master to continue. */
@@ -798,51 +835,6 @@ cycles_t cacheflush_time;unsigned long cache_decay_ticks;/* -* Construct cpu_sibling_map[], so that we can tell the sibling CPU -* on SMT systems efficiently. -*/ -static __cpuinit void detect_siblings (void ) -{ -int cpu; - -for (cpu =0 ; cpu <NR_CPUS ; cpu++) { -cpus_clear (cpu_sibling_map[cpu]); -cpus_clear (cpu_core_map[cpu]); - } - -for_each_online_cpu (cpu) { -struct cpuinfo_x86 *c = cpu_data + cpu; -int siblings =0 ; -int i; -if (smp_num_siblings >1 ) { -for_each_online_cpu (i) { -if (cpu_core_id[cpu] == cpu_core_id[i]) { - siblings++; -cpu_set (i, cpu_sibling_map[cpu]); - } - } - }else { - siblings++; -cpu_set (cpu, cpu_sibling_map[cpu]); - } - -if (siblings != smp_num_siblings) { -printk (KERN_WARNING -"WARNING: %d siblings found for CPU%d, should be %d\n" , - siblings, cpu, smp_num_siblings); - smp_num_siblings = siblings; - } -if (c->x86_num_cores >1 ) { -for_each_online_cpu (i) { -if (phys_proc_id[cpu] == phys_proc_id[i]) -cpu_set (i, cpu_core_map[cpu]); - } - }else - cpu_core_map[cpu] = cpu_sibling_map[cpu]; - } -} - -/* * Cleanup possible dangling ends... */ static __cpuinit void smp_cleanup_boot (void )
@@ -1036,6 +1028,8 @@ void __init smp_prepare_boot_cpu(void)int me =smp_processor_id ();cpu_set (me, cpu_online_map);cpu_set (me, cpu_callout_map); +cpu_set (0 , cpu_sibling_map[0 ]); +cpu_set (0 , cpu_core_map[0 ]); }/*
@@ -1100,7 +1094,6 @@ void __init smp_cpus_done(unsigned int msetup_ioapic_dest ();#endif -detect_siblings ();time_init_gtod ();check_nmi_watchdog ();
--
arch/x86_64/kernel/smpboot.c line 66
Quoted 11 lines · Ashok Raj
===================================================================
arch/x86_64/kernel/smpboot.c +2
-2@@ -62,9 +62,12 @@/* Number of siblings per CPU package */ int smp_num_siblings =1 ;/* Package ID of each logical CPU */ -u8 phys_proc_id[NR_CPUS ] = { [0 ...NR_CPUS -1 ] =BAD_APICID }; -u8 cpu_core_id[NR_CPUS ] = { [0 ...NR_CPUS -1 ] =BAD_APICID }; +u8 phys_proc_id[NR_CPUS] __cacheline_aligned = + { [0 ...NR_CPUS -1 ] =BAD_APICID };
Why this change?
arch/x86_64/kernel/smpboot.c line 70
Quoted 4 lines
arch/x86_64/kernel/smpboot.c +3
-0EXPORT_SYMBOL (phys_proc_id); + +u8 cpu_core_id[NR_CPUS] __cacheline_aligned = + { [0 ...NR_CPUS -1 ] =BAD_APICID };
And that one?
It does not seem related to CPUhotplug. May as a separate patch, but in this case the "per cpu readonly" section C.Lameter recently added should be used for these which are near always read-only (I hope the section made it into -mm* now)
-Andi
PATCH 4/4 CPU Hotplug support for X86_64
In reply to Ashok RajSubject: [patch 4/4] x86_64: Replace IPI Broadcast when using CPU hotplug.
From: Ashok Raj <[email protected]>
This patch tries to eliminate a race condition with IPI broadcast and CPU hotplug. Since when we broadcast we dont have control on what cpus see a spurious intr, or possiblity of receiving an intr when the cpu is just in process of comming up, we choose to send targetted IPI only to online cpus.
This same workaround could be done in other places too, like genapic locations, but this is a simpler change done here.
for the time being: we do the send_IPI_mask() version if CPU hotplug is selected by default.
Use both CONFIG option if CPU_HOTPLUG is selected, and a cmdline option to turn it on during startup if necessary.
1. Bringup cpu in complete cli state, and also hold call_lock when we update online_map. (Not sure if the IPI would be pended and delivered later causing trouble)
2. SledgeHammer approach to use stop_machine() to achive automicity. *Very Undesirable*
3. May be setup intr_gate a little later just before setting online_map. [still ugly bandaid, but may work] online_map should still be updated under call_lock for safety.
--------------------------------------
Kconfig | 4 ++++ kernel/smp.c | 24 +++++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-)
arch/x86_64/kernel/smp.c +23 -1
@@ -43,6 +43,13 @@ static cpumask_t flush_cpumask;static struct mm_struct * flush_mm;static unsigned long flush_va;static DEFINE_SPINLOCK (tlbstate_lock); + +#ifdef CONFIG_SAFE_IPI_CALLFUNC +static int safe_ipi_call =1 ; +#else +static int safe_ipi_call =0 ; +#endif +#define FLUSH_ALL -1ULL/*
@@ -307,6 +314,15 @@ unlock_ipi_calllock(void)spin_unlock_irq (&call_lock); } +static int __init safe_ipi_setup (char *s) +{ + safe_ipi_call =1 ; +printk ("Turning off broadcast IPI in smp_call_function\n" ); +return 0 ; +} + +__setup ("safe_ipi" , safe_ipi_setup); +/* * this function sends a 'generic call function' IPI to all other CPUs * in the system.
@@ -330,7 +346,13 @@ static void __smp_call_function (void (* call_data = &data;wmb ();/* Send a message to all other CPUs and wait for them to respond */ -send_IPI_allbutself (CALL_FUNCTION_VECTOR ); + +if (safe_ipi_call) { +cpumask_t cpu_mask = cpu_online_map; +cpu_clear (smp_processor_id (), cpu_mask); +send_IPI_mask (cpu_mask,CALL_FUNCTION_VECTOR ); + }else +send_IPI_allbutself (CALL_FUNCTION_VECTOR );/* Wait for response */ while (atomic_read (&data.started) != cpus)
arch/x86_64/Kconfig +4 -0
@@ -302,6 +302,10 @@ config HOTPLUG_CPU can be controlled through /sys/devices/system/cpu/cpu#. Say N if you want to disable CPU hotplug. +config SAFE_IPI_CALLFUNC + bool + default y + depends on HOTPLUG_CPU config HPET_TIMER bool
--
Quoted 5 lines · Ashok Raj
This patch tries to eliminate a race condition with IPI broadcast and CPU hotplug. Since when we broadcast we dont have control on what cpus see a spurious intr, or possiblity of receiving an intr when the cpu is just in process of comming up, we choose to send targetted IPI only to online cpus.
I thought you had fixed that already in the other patch with the locking of call_lock. Or did I miss something?
I dont like this patch.
-Andi
Quoted 5 lines · Ashok Raj
TBD:
1. Track down
CONFIG_SCHED_SMTOops with both cpu up/down.
2. Test on real NUMA hw.
With below patch, cpu hotplug works with SCHED_SMT enabled in my test. set_cpu_sibling_map is invoked before cpu is set to online.
Thanks,
Shaohua
arch/x86_64/kernel/smpboot.c renamed from arch/x86_64/kernel/smpboot.c.orig +2 -2
@@ -443,7 +443,7 @@ set_cpu_sibling_map(int cpu)int i;if (smp_num_siblings >1 ) { -for_each_online_cpu (i) { +for (i =0 ; i <NR_CPUS ; i++) {if (cpu_core_id[cpu] == cpu_core_id[i]) {cpu_set (i, cpu_sibling_map[cpu]);cpu_set (cpu, cpu_sibling_map[i]);
@@ -454,7 +454,7 @@ set_cpu_sibling_map(int cpu) }if (current_cpu_data.x86_num_cores >1 ) { -for_each_online_cpu (i) { +for (i =0 ; i <NR_CPUS ; i++) {if (phys_proc_id[cpu] == phys_proc_id[i]) {cpu_set (i, cpu_core_map[cpu]);cpu_set (cpu, cpu_core_map[i]);
Quoted 8 lines · Shaohua Li
Quoted 5 lines · Ashok Raj
TBD:
1. Track down
CONFIG_SCHED_SMTOops with both cpu up/down.
2. Test on real NUMA hw.
Quoted 5 lines
With below patch, cpu hotplug works with
SCHED_SMTenabled in my test.set_cpu_sibling_mapis invoked before cpu is set to online.Thanks,
Shaohua
I'm not sure, but you probably want "for_each_cpu(i)" instead of "for (i = 0; i < NR_CPUS; i++)" below.
It should be rare that you really want to explicitly itterate over [0..NR_CPUS] cpus. for_each_cpu() will itterate over all "possible" cpus, which may well be [0..NR_CPUS], but it at least is cleaner and more readable. If your arch actually sets up cpu_possible_map correctly, then it will likely be quite a bit faster and more efficient to use for_each_cpu().
Again, I haven't looked at the surrounding code, but it's very likely that for_each_cpu() will do what you want.
-Matt
Quoted 20 lines
arch/x86_64/kernel/smpboot.c renamed from arch/x86_64/kernel/smpboot.c.orig +2
-2@@ -443,7 +443,7 @@ set_cpu_sibling_map(int cpu)int i;if (smp_num_siblings >1 ) { -for_each_online_cpu (i) { +for (i =0 ; i <NR_CPUS ; i++) {if (cpu_core_id[cpu] == cpu_core_id[i]) {cpu_set (i, cpu_sibling_map[cpu]);cpu_set (cpu, cpu_sibling_map[i]);@@ -454,7 +454,7 @@ set_cpu_sibling_map(int cpu) }if (current_cpu_data.x86_num_cores >1 ) { -for_each_online_cpu (i) { +for (i =0 ; i <NR_CPUS ; i++) {if (phys_proc_id[cpu] == phys_proc_id[i]) {cpu_set (i, cpu_core_map[cpu]);cpu_set (cpu, cpu_core_map[i]);
Quoted 12 lines · Matthew Dobson
Quoted 7 lines · Shaohua Li
On Tue, 2005-05-24 at 01:11 -0700, Ashok Raj wrote:
With below patch, cpu hotplug works with
SCHED_SMTenabled in my test.set_cpu_sibling_mapis invoked before cpu is set to online.Thanks,
ShaohuaI'm not sure, but you probably want "
for_each_cpu(i)" instead of "for (i = 0; i <NR_CPUS; i++)" below.
I have a new set of patches just getting ready with final comments from Andi This version already uses for_each_cpu varient.
Cheers,
ashok