Re: [patch 2/5] try2: x86_64: CPU hotplug support.

[Andrew Morton <akpm@osdl.org>]

Ashok Raj <ashok.raj@intel.com> wrote:
>
> Experimental CPU hotplug patch for x86_64

What does "experimental" mean?

>  static int __cpuinit do_boot_cpu(int cpu, int apicid)
>  {
> -	struct task_struct *idle;
>  	unsigned long boot_error;
>  	int timeout;
>  	unsigned long start_rip;
> -	/*
> -	 * We can't use kernel_thread since we must avoid to
> -	 * reschedule the child.
> -	 */
> -	idle = fork_idle(cpu);
> -	if (IS_ERR(idle)) {
> +	struct create_idle c_idle = {
> +		.cpu = cpu,
> +		.done = COMPLETION_INITIALIZER(c_idle.done),
> +	};
> +	DECLARE_WORK(work, do_fork_idle, &c_idle);
> +
> +	c_idle.idle = get_idle_for_cpu(cpu);
> +
> +	if (c_idle.idle) {
> +		c_idle.idle->thread.rsp = (unsigned long) (((struct pt_regs *)
> +			(THREAD_SIZE + (unsigned long) c_idle.idle->thread_info)) - 1);
> +		init_idle(c_idle.idle, cpu);
> +		goto do_rest;
> +	}
> +
> +	if (!keventd_up() || current_is_keventd())
> +		work.func(work.data);
> +	else {
> +		schedule_work(&work);
> +		wait_for_completion(&c_idle.done);
> +	}

This shouldn't be diddling with workqueue internals.  Why is this code
here?  If the workqueue API is inadequate then we should prefer to extend
it rather than working around any shortcoming.

> +		Dprintk ("do_boot_cpu %d Already started\n", cpu);

Please try to adopt a consistent coding style.

Using printk("%s", __FUNCTION__); is preferred, as it will still work if
someone later refactors this code into a new function.  (It can increase
code size.  Or decrease it if the string gets shared.  But that's moot if
the code is inside a normally-disabled macro like Dprintk.  Whatever that
is.)

> +static void
> +remove_siblinginfo(int cpu)

Unneeded newline here.

> +/* 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;
> +
> +	while (1)
> +		safe_halt();
> +}

The memory barrier needs a comment, please.  It is otherwise not possible
to determine why it is there.

>  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();
>  
> +	if (!cpu_online(cpu))
> +		return;
> +

Why would an offlined CPU receive an NMI?  (A comment would be handy)