Re: [patch 2/4] CPU Hotplug support for X86_64

Andi Kleen
2005-05-24T12:26:56Z

On Tue, May 24, 2005 at 01:11:15AM -0700, Ashok Raj wrote:
> /*
> @@ -97,6 +97,26 @@ cpumask_t cpu_core_map[NR_CPUS] __cachel
> extern unsigned char trampoline_data[];
> extern unsigned char trampoline_end[];
>
> +/* State of each CPU */
> +DEFINE_PER_CPU(int, cpu_state) = { 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.

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

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

> @@ -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?

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

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

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

> +
> + /*
> + * 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?

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

> reason = get_nmi_reason();
>
> +#ifdef CONFIG_HOTPLUG_CPU
> + if (!cpu_online(cpu))
> + return;
> +#endif

Please remove the ifdef.

-Andi