Re: [PATCH] Check for online cpus before bringing them up

[Srivatsa Vaddagiri <vatsa@in.ibm.com>]

On Thu, Mar 16, 2006 at 05:08:14PM -0800, Andrew Morton wrote:
> Is x86 the only architecture which is exposed to this?

Currently only x86 implements smp_prepare_cpu(). On other arch, it is a
no-op. Hence yes, only x86 is exposed to this bug.

> >  
> >  	lock_cpu_hotplug();
> > +
> > +	if (cpu_online(cpu)) {
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> >  	apicid = x86_cpu_to_apicid[cpu];
> >  	if (apicid == BAD_APICID) {
> >  		ret = -ENODEV;
> 
> a) It's hard for the reader to understand what that test is doing there
> 
> b) People copy code from x86, so other architectures which are not
>    exposed to this problem will end up having a pointless test in there.

Well ..other arch-es need to have a similar check if they get around to
implement physical hot-add (even if they allow offlining of all CPUs). This is 
required since a user can (by mistake maybe) try to bring up an already online 
CPU by writing a '1' to it's sysfs 'online' file. 'store_online' 
(drivers/base/cpu.c) unconditionally calls 'smp_prepare_cpu' w/o checking for 
this error condition. The check added in the patch catches such error 
conditions as well.

> IOW: please comment your code.   I'll fix this one up.

Sorry about not commenting my code earlier! How does the patch below look?


Add check for online cpus.

Signed-off-by : Srivatsa Vaddagiri <vatsa@in.ibm.com>


 arch/i386/kernel/smpboot.c |   10 ++++++++++
 1 files changed, 10 insertions(+)

diff -puN arch/i386/kernel/smpboot.c~cpu_hp arch/i386/kernel/smpboot.c
--- linux-2.6.16-rc6/arch/i386/kernel/smpboot.c~cpu_hp	2006-03-17 14:27:15.000000000 +0530
+++ linux-2.6.16-rc6-root/arch/i386/kernel/smpboot.c	2006-03-17 14:38:50.000000000 +0530
@@ -1029,6 +1029,16 @@ int __devinit smp_prepare_cpu(int cpu)
 	int	apicid, ret;
 
 	lock_cpu_hotplug();
+
+	/* Check if CPU is already online. This can happen if user tries to 
+	 * bringup an already online CPU or a previous offline attempt
+	 * on this CPU has failed.
+	 */
+	if (cpu_online(cpu)) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	apicid = x86_cpu_to_apicid[cpu];
 	if (apicid == BAD_APICID) {
 		ret = -ENODEV;

_

-- 
Regards,
vatsa