[PATCH] i386 boottime for_each_cpu broken

[PATCH] i386 boottime for_each_cpu broken

[Zwane Mwaikambo]

for_each_cpu walks through all processors in cpu_possible_map, which is 
defined as cpu_callout_map on i386 and isn't initialised until all 
processors have been booted. This breaks things which do for_each_cpu 
iterations early during boot. So, define cpu_possible_map as a bitmap with 
NR_CPUS bits populated. This was triggered by a patch i'm working on which 
does alloc_percpu before bringing up secondary processors.

 arch/i386/kernel/smpboot.c           |    1 +
 arch/i386/mach-voyager/voyager_smp.c |    1 +
 include/asm-i386/smp.h               |    2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

Signed-off-by: Zwane Mwaikambo <zwane@arm.linux.org.uk>

Index: linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 smpboot.c
--- linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c	7 Aug 2005 21:38:03 -0000	1.1.1.1
+++ linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c	11 Aug 2005 04:26:06 -0000
@@ -87,6 +87,7 @@ EXPORT_SYMBOL(cpu_online_map);
 
 cpumask_t cpu_callin_map;
 cpumask_t cpu_callout_map;
+cpumask_t cpu_possible_map = CPU_MASK_ALL;
 EXPORT_SYMBOL(cpu_callout_map);
 static cpumask_t smp_commenced_mask;
 
Index: linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 voyager_smp.c
--- linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c	7 Aug 2005 21:38:04 -0000	1.1.1.1
+++ linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c	11 Aug 2005 04:26:29 -0000
@@ -241,6 +241,7 @@ static cpumask_t smp_commenced_mask = CP
 /* This is for the new dynamic CPU boot code */
 cpumask_t cpu_callin_map = CPU_MASK_NONE;
 cpumask_t cpu_callout_map = CPU_MASK_NONE;
+cpumask_t cpu_possible_map = CPU_MASK_ALL;
 EXPORT_SYMBOL(cpu_callout_map);
 
 /* The per processor IRQ masks (these are usually kept in sync) */
Index: linux-2.6.13-rc5-mm1/include/asm-i386/smp.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.13-rc5-mm1/include/asm-i386/smp.h,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 smp.h
--- linux-2.6.13-rc5-mm1/include/asm-i386/smp.h	7 Aug 2005 21:38:37 -0000	1.1.1.1
+++ linux-2.6.13-rc5-mm1/include/asm-i386/smp.h	11 Aug 2005 04:25:26 -0000
@@ -59,7 +59,7 @@ extern void cpu_uninit(void);
 
 extern cpumask_t cpu_callout_map;
 extern cpumask_t cpu_callin_map;
-#define cpu_possible_map cpu_callout_map
+extern cpumask_t cpu_possible_map;
 
 /* We don't mark CPUs online until __cpu_up(), so we need another measure */
 static inline int num_booting_cpus(void)

Re: [PATCH] i386 boottime for_each_cpu broken

[Bharata B Rao]

On Thu, Aug 11, 2005 at 04:54:44AM +0000, Zwane Mwaikambo wrote:
> for_each_cpu walks through all processors in cpu_possible_map, which is 
> defined as cpu_callout_map on i386 and isn't initialised until all 
> processors have been booted. This breaks things which do for_each_cpu 
> iterations early during boot. So, define cpu_possible_map as a bitmap with 
> NR_CPUS bits populated. This was triggered by a patch i'm working on which 
> does alloc_percpu before bringing up secondary processors.
> 

Zwane,

I don't know the context of your work here, but a couple of 
observations.

Since you populate cpu_possible_map with NR_CPUS, alloc_percpu()
would end up allocating for all NR_CPUS.  Wouldn't you have achieved
the same thing by compile time allocation ? Wouldn't this change
lead to NR_CPUS allocations from alloc_percpu() for all users ?

Now since you have separated cpu_possible_map from cpu_callout_map,
do we need to reflect cpu_possible_map with the value from
cpu_callout_map after the cpu_callout_map is initialized fully from
smp_prepare_cpus().

BTW, I am working on Kiran's dynamic percpu allocator patch and making
it cpu hotplug aware. With that, alloc_percpu would initially allocate
only for the possible cpus and would allocate for other cpus as and when
they come up.

Regards,
Bharata.

Re: [PATCH] i386 boottime for_each_cpu broken

[Zwane Mwaikambo]

Hello Bharata,

On Thu, 11 Aug 2005, Bharata B Rao wrote:

> I don't know the context of your work here, but a couple of 
> observations.
> 
> Since you populate cpu_possible_map with NR_CPUS, alloc_percpu()
> would end up allocating for all NR_CPUS.  Wouldn't you have achieved
> the same thing by compile time allocation ? Wouldn't this change
> lead to NR_CPUS allocations from alloc_percpu() for all users ?

The patch has been modified (courtesy Andi Kleen) to do something a bit 
smarter and only allocate for detected cpus in MADT or MP table. But my 
prime concern was that for_each_cpu didn't work at all during boot.

> Now since you have separated cpu_possible_map from cpu_callout_map,
> do we need to reflect cpu_possible_map with the value from
> cpu_callout_map after the cpu_callout_map is initialized fully from
> smp_prepare_cpus().

This should be taken care of using above.

> BTW, I am working on Kiran's dynamic percpu allocator patch and making
> it cpu hotplug aware. With that, alloc_percpu would initially allocate
> only for the possible cpus and would allocate for other cpus as and when
> they come up.

Great, that takes care of my concerns regarding processors which aren't 
enumerated during smp boot.

Thanks for the feedback,
	Zwane

Re: [PATCH] i386 boottime for_each_cpu broken

[Andi Kleen]

On Wed, Aug 10, 2005 at 10:59:28PM -0600, Zwane Mwaikambo wrote:
> for_each_cpu walks through all processors in cpu_possible_map, which is 
> defined as cpu_callout_map on i386 and isn't initialised until all 
> processors have been booted. This breaks things which do for_each_cpu 
> iterations early during boot. So, define cpu_possible_map as a bitmap with 
> NR_CPUS bits populated. This was triggered by a patch i'm working on which 
> does alloc_percpu before bringing up secondary processors.

Better is to initialize it in mpparse.c. That is what x86-64 is doing now.

-Andi

Re: [PATCH] i386 boottime for_each_cpu broken

[Zwane Mwaikambo]

On Thu, 11 Aug 2005, Andi Kleen wrote:

> On Wed, Aug 10, 2005 at 10:59:28PM -0600, Zwane Mwaikambo wrote:
> > for_each_cpu walks through all processors in cpu_possible_map, which is 
> > defined as cpu_callout_map on i386 and isn't initialised until all 
> > processors have been booted. This breaks things which do for_each_cpu 
> > iterations early during boot. So, define cpu_possible_map as a bitmap with 
> > NR_CPUS bits populated. This was triggered by a patch i'm working on which 
> > does alloc_percpu before bringing up secondary processors.
> 
> Better is to initialize it in mpparse.c. That is what x86-64 is doing now.

Good idea, here is an updated version, i left Voyager alone as i have no 
way of testing it.

 arch/i386/kernel/mpparse.c           |    8 +++++++-
 arch/i386/kernel/smpboot.c           |    1 +
 arch/i386/mach-voyager/voyager_smp.c |    1 +
 include/asm-i386/smp.h               |    2 +-
 4 files changed, 10 insertions(+), 2 deletions(-)

Signed-off-by: Zwane Mwaikambo <zwane@arm.linux.org.uk>

Index: linux-2.6.13-rc5-mm1/arch/i386/kernel/mpparse.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.13-rc5-mm1/arch/i386/kernel/mpparse.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 mpparse.c
--- linux-2.6.13-rc5-mm1/arch/i386/kernel/mpparse.c	7 Aug 2005 21:38:03 -0000	1.1.1.1
+++ linux-2.6.13-rc5-mm1/arch/i386/kernel/mpparse.c	11 Aug 2005 17:37:23 -0000
@@ -122,7 +122,7 @@ static int MP_valid_apicid(int apicid, i
 
 static void __init MP_processor_info (struct mpc_config_processor *m)
 {
- 	int ver, apicid;
+ 	int ver, apicid, cpu, found_bsp = 0;
 	physid_mask_t tmp;
  	
 	if (!(m->mpc_cpuflag & CPU_ENABLED))
@@ -181,6 +181,7 @@ static void __init MP_processor_info (st
 	if (m->mpc_cpuflag & CPU_BOOTPROCESSOR) {
 		Dprintk("    Bootup CPU\n");
 		boot_cpu_physical_apicid = m->mpc_apicid;
+		found_bsp = 1;
 	}
 
 	if (num_processors >= NR_CPUS) {
@@ -204,6 +205,11 @@ static void __init MP_processor_info (st
 		return;
 	}
 
+	if (found_bsp)
+		cpu = 0;
+	else
+		cpu = num_processors - 1;
+	cpu_set(cpu, cpu_possible_map);
 	tmp = apicid_to_cpu_present(apicid);
 	physids_or(phys_cpu_present_map, phys_cpu_present_map, tmp);
 	
Index: linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 smpboot.c
--- linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c	7 Aug 2005 21:38:03 -0000	1.1.1.1
+++ linux-2.6.13-rc5-mm1/arch/i386/kernel/smpboot.c	11 Aug 2005 17:07:44 -0000
@@ -87,6 +87,7 @@ EXPORT_SYMBOL(cpu_online_map);
 
 cpumask_t cpu_callin_map;
 cpumask_t cpu_callout_map;
+cpumask_t cpu_possible_map;
 EXPORT_SYMBOL(cpu_callout_map);
 static cpumask_t smp_commenced_mask;
 
Index: linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 voyager_smp.c
--- linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c	7 Aug 2005 21:38:04 -0000	1.1.1.1
+++ linux-2.6.13-rc5-mm1/arch/i386/mach-voyager/voyager_smp.c	11 Aug 2005 17:40:30 -0000
@@ -241,6 +241,7 @@ static cpumask_t smp_commenced_mask = CP
 /* This is for the new dynamic CPU boot code */
 cpumask_t cpu_callin_map = CPU_MASK_NONE;
 cpumask_t cpu_callout_map = CPU_MASK_NONE;
+cpumask_t cpu_possible_map = CPU_MASK_ALL;
 EXPORT_SYMBOL(cpu_callout_map);
 
 /* The per processor IRQ masks (these are usually kept in sync) */
Index: linux-2.6.13-rc5-mm1/include/asm-i386/smp.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.13-rc5-mm1/include/asm-i386/smp.h,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 smp.h
--- linux-2.6.13-rc5-mm1/include/asm-i386/smp.h	7 Aug 2005 21:38:37 -0000	1.1.1.1
+++ linux-2.6.13-rc5-mm1/include/asm-i386/smp.h	11 Aug 2005 04:25:26 -0000
@@ -59,7 +59,7 @@ extern void cpu_uninit(void);
 
 extern cpumask_t cpu_callout_map;
 extern cpumask_t cpu_callin_map;
-#define cpu_possible_map cpu_callout_map
+extern cpumask_t cpu_possible_map;
 
 /* We don't mark CPUs online until __cpu_up(), so we need another measure */
 static inline int num_booting_cpus(void)

Re: [PATCH] i386 boottime for_each_cpu broken

[Andrew Morton]

Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
>
> tree 090c471fdb44d8fe88c52e95be0e8e43e31fcd5a
> parent d7271b14b2e9e5905aba0fbf5c4dc4f8980c0cb2
> author Zwane Mwaikambo <zwane@arm.linux.org.uk> Sun, 04 Sep 2005 05:56:51 -0700
> committer Linus Torvalds <torvalds@evo.osdl.org> Mon, 05 Sep 2005 14:06:13 -0700
> 
> [PATCH] i386 boottime for_each_cpu broken
> 
> for_each_cpu walks through all processors in cpu_possible_map, which is
> defined as cpu_callout_map on i386 and isn't initialised until all
> processors have been booted. This breaks things which do for_each_cpu
> iterations early during boot. So, define cpu_possible_map as a bitmap with
> NR_CPUS bits populated. This was triggered by a patch i'm working on which
> does alloc_percpu before bringing up secondary processors.
> 
> From: Alexander Nyberg <alexn@telia.com>
> 
> i386-boottime-for_each_cpu-broken.patch
> i386-boottime-for_each_cpu-broken-fix.patch
> 
> The SMP version of __alloc_percpu checks the cpu_possible_map before
> allocating memory for a certain cpu.  With the above patches the BSP cpuid
> is never set in cpu_possible_map which breaks CONFIG_SMP on uniprocessor
> machines (as soon as someone tries to dereference something allocated via
> __alloc_percpu, which in fact is never allocated since the cpu is not set
> in cpu_possible_map).
> 

Kills my old 4-way Xeon.  cpu_possible_map has a value of 0x7 and
alloc_percpu() does bad things for the fourth CPU.


> diff --git a/arch/i386/kernel/mpparse.c b/arch/i386/kernel/mpparse.c
> --- a/arch/i386/kernel/mpparse.c
> +++ b/arch/i386/kernel/mpparse.c
> @@ -122,7 +122,7 @@ static int MP_valid_apicid(int apicid, i
>  
>  static void __init MP_processor_info (struct mpc_config_processor *m)
>  {
> - 	int ver, apicid;
> + 	int ver, apicid, cpu, found_bsp = 0;
>  	physid_mask_t tmp;
>   	
>  	if (!(m->mpc_cpuflag & CPU_ENABLED))
> @@ -181,6 +181,7 @@ static void __init MP_processor_info (st
>  	if (m->mpc_cpuflag & CPU_BOOTPROCESSOR) {
>  		Dprintk("    Bootup CPU\n");
>  		boot_cpu_physical_apicid = m->mpc_apicid;
> +		found_bsp = 1;
>  	}
>  
>  	if (num_processors >= NR_CPUS) {
> @@ -204,6 +205,11 @@ static void __init MP_processor_info (st
>  		return;
>  	}
>  
> +	if (found_bsp)
> +		cpu = 0;
> +	else
> +		cpu = num_processors - 1;
> +	cpu_set(cpu, cpu_possible_map);

Looky here:

akpm: found_bsp=0 cpu=0 tmp=0x0001 num_processors=1
akpm: found_bsp=0 cpu=1 tmp=0x0002 num_processors=2
akpm: found_bsp=0 cpu=2 tmp=0x0004 num_processors=3
akpm: found_bsp=1 cpu=0 tmp=0x0008 num_processors=4

On this machine, the BSP is the last one to pass through
MP_processor_info(), so the rude-looking assumption above screws things up.

I don't know what that found_bsp code is trying to do.  It wasn't
changelogged and it wasn't commented and removing it makes by box boot again.

What did I break?


diff -puN arch/i386/kernel/mpparse.c~a arch/i386/kernel/mpparse.c
--- devel/arch/i386/kernel/mpparse.c~a	2005-09-08 23:56:25.000000000 -0700
+++ devel-akpm/arch/i386/kernel/mpparse.c	2005-09-09 00:23:55.000000000 -0700
@@ -122,8 +122,8 @@ static int MP_valid_apicid(int apicid, i
 
 static void __init MP_processor_info (struct mpc_config_processor *m)
 {
- 	int ver, apicid, cpu, found_bsp = 0;
-	physid_mask_t tmp;
+ 	int ver, apicid;
+	physid_mask_t phys_cpu;
  	
 	if (!(m->mpc_cpuflag & CPU_ENABLED))
 		return;
@@ -181,7 +181,6 @@ static void __init MP_processor_info (st
 	if (m->mpc_cpuflag & CPU_BOOTPROCESSOR) {
 		Dprintk("    Bootup CPU\n");
 		boot_cpu_physical_apicid = m->mpc_apicid;
-		found_bsp = 1;
 	}
 
 	if (num_processors >= NR_CPUS) {
@@ -195,24 +194,19 @@ static void __init MP_processor_info (st
 			" Processor ignored.\n", maxcpus); 
 		return;
 	}
-	num_processors++;
 	ver = m->mpc_apicver;
 
 	if (!MP_valid_apicid(apicid, ver)) {
 		printk(KERN_WARNING "Processor #%d INVALID. (Max ID: %d).\n",
 			m->mpc_apicid, MAX_APICS);
-		--num_processors;
 		return;
 	}
 
-	if (found_bsp)
-		cpu = 0;
-	else
-		cpu = num_processors - 1;
-	cpu_set(cpu, cpu_possible_map);
-	tmp = apicid_to_cpu_present(apicid);
-	physids_or(phys_cpu_present_map, phys_cpu_present_map, tmp);
-	
+	cpu_set(num_processors, cpu_possible_map);
+	num_processors++;
+	phys_cpu = apicid_to_cpu_present(apicid);
+	physids_or(phys_cpu_present_map, phys_cpu_present_map, phys_cpu);
+
 	/*
 	 * Validate version
 	 */
_

Re: [PATCH] i386 boottime for_each_cpu broken

[Zwane Mwaikambo]

On Fri, 9 Sep 2005, Andrew Morton wrote:

> >  static void __init MP_processor_info (struct mpc_config_processor *m)
> >  {
> > - 	int ver, apicid;
> > + 	int ver, apicid, cpu, found_bsp = 0;
> >  	physid_mask_t tmp;
> >   	
> >  	if (!(m->mpc_cpuflag & CPU_ENABLED))
> > @@ -181,6 +181,7 @@ static void __init MP_processor_info (st
> >  	if (m->mpc_cpuflag & CPU_BOOTPROCESSOR) {
> >  		Dprintk("    Bootup CPU\n");
> >  		boot_cpu_physical_apicid = m->mpc_apicid;
> > +		found_bsp = 1;
> >  	}
> >  
> >  	if (num_processors >= NR_CPUS) {
> > @@ -204,6 +205,11 @@ static void __init MP_processor_info (st
> >  		return;
> >  	}
> >  
> > +	if (found_bsp)
> > +		cpu = 0;
> > +	else
> > +		cpu = num_processors - 1;
> > +	cpu_set(cpu, cpu_possible_map);
> 
> Looky here:
> 
> akpm: found_bsp=0 cpu=0 tmp=0x0001 num_processors=1
> akpm: found_bsp=0 cpu=1 tmp=0x0002 num_processors=2
> akpm: found_bsp=0 cpu=2 tmp=0x0004 num_processors=3
> akpm: found_bsp=1 cpu=0 tmp=0x0008 num_processors=4
> 
> On this machine, the BSP is the last one to pass through
> MP_processor_info(), so the rude-looking assumption above screws things up.

Yes that's a terrible assumption, 

> I don't know what that found_bsp code is trying to do.  It wasn't
> changelogged and it wasn't commented and removing it makes by box boot again.
> 
> What did I break?

Nothing gov =)

> -	if (found_bsp)
> -		cpu = 0;
> -	else
> -		cpu = num_processors - 1;
> -	cpu_set(cpu, cpu_possible_map);
> -	tmp = apicid_to_cpu_present(apicid);
> -	physids_or(phys_cpu_present_map, phys_cpu_present_map, tmp);
> -	
> +	cpu_set(num_processors, cpu_possible_map);
> +	num_processors++;
> +	phys_cpu = apicid_to_cpu_present(apicid);
> +	physids_or(phys_cpu_present_map, phys_cpu_present_map, phys_cpu);
> +

That looks fine to me, my BSP assumption was bad bad bad!

Thanks,
	Zwane