[PATCH] kill useless SMT code in prom_hold_cpus

[PATCH] kill useless SMT code in prom_hold_cpus

[Nathan Lynch]

I think this code that counts SMT threads and compares against NR_CPUS
is an artifact of pre-powerpc-merge ppc64.  We care about starting
only primary threads in the OF client code.

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/kernel/prom_init.c |   39 +++------------------------------------
 1 files changed, 3 insertions(+), 36 deletions(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 1ea8c8d..b1dd86c 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -205,8 +205,6 @@ static int __initdata mem_reserve_cnt;
 static cell_t __initdata regbuf[1024];
 
 
-#define MAX_CPU_THREADS 2
-
 /*
  * Error results ... some OF calls will return "-1" on error, some
  * will return 0, some will return either. To simplify, here are
@@ -1332,10 +1330,6 @@ static void __init prom_hold_cpus(void)
 	unsigned int reg;
 	phandle node;
 	char type[64];
-	int cpuid = 0;
-	unsigned int interrupt_server[MAX_CPU_THREADS];
-	unsigned int cpu_threads, hw_cpu_num;
-	int propsize;
 	struct prom_t *_prom = &RELOC(prom);
 	unsigned long *spinloop
 		= (void *) LOW_ADDR(__secondary_hold_spinloop);
@@ -1379,7 +1373,6 @@ static void __init prom_hold_cpus(void)
 		reg = -1;
 		prom_getprop(node, "reg", &reg, sizeof(reg));
 
-		prom_debug("\ncpuid        = 0x%x\n", cpuid);
 		prom_debug("cpu hw idx   = 0x%x\n", reg);
 
 		/* Init the acknowledge var which will be reset by
@@ -1388,28 +1381,9 @@ static void __init prom_hold_cpus(void)
 		 */
 		*acknowledge = (unsigned long)-1;
 
-		propsize = prom_getprop(node, "ibm,ppc-interrupt-server#s",
-					&interrupt_server,
-					sizeof(interrupt_server));
-		if (propsize < 0) {
-			/* no property.  old hardware has no SMT */
-			cpu_threads = 1;
-			interrupt_server[0] = reg; /* fake it with phys id */
-		} else {
-			/* We have a threaded processor */
-			cpu_threads = propsize / sizeof(u32);
-			if (cpu_threads > MAX_CPU_THREADS) {
-				prom_printf("SMT: too many threads!\n"
-					    "SMT: found %x, max is %x\n",
-					    cpu_threads, MAX_CPU_THREADS);
-				cpu_threads = 1; /* ToDo: panic? */
-			}
-		}
-
-		hw_cpu_num = interrupt_server[0];
-		if (hw_cpu_num != _prom->cpu) {
+		if (reg != _prom->cpu) {
 			/* Primary Thread of non-boot cpu */
-			prom_printf("%x : starting cpu hw idx %x... ", cpuid, reg);
+			prom_printf("starting cpu hw idx %x... ", reg);
 			call_prom("start-cpu", 3, 0, node,
 				  secondary_hold, reg);
 
@@ -1424,17 +1398,10 @@ static void __init prom_hold_cpus(void)
 		}
 #ifdef CONFIG_SMP
 		else
-			prom_printf("%x : boot cpu     %x\n", cpuid, reg);
+			prom_printf("boot cpu hw idx %x\n", reg);
 #endif /* CONFIG_SMP */
-
-		/* Reserve cpu #s for secondary threads.   They start later. */
-		cpuid += cpu_threads;
 	}
 
-	if (cpuid > NR_CPUS)
-		prom_printf("WARNING: maximum CPUs (" __stringify(NR_CPUS)
-			    ") exceeded: ignoring extras\n");
-
 	prom_debug("prom_hold_cpus: end...\n");
 }
 
-- 
1.5.6.2

Re: [PATCH] kill useless SMT code in prom_hold_cpus

[Benjamin Herrenschmidt]

On Tue, 2008-07-08 at 17:36 -0500, Nathan Lynch wrote:
> I think this code that counts SMT threads and compares against NR_CPUS
> is an artifact of pre-powerpc-merge ppc64.  We care about starting
> only primary threads in the OF client code.
> 
> Signed-off-by: Nathan Lynch <ntl@pobox.com>

That looks good. I'm not merging it right now because I want to dbl
check that it's allright on all SMT machines. IE. We compare reg[0]
against _prom->cpu now instead of interrupt_server[0] and I thus
want to ensure it's the same everywhere.

Cheers,
Ben.

>  arch/powerpc/kernel/prom_init.c |   39 +++------------------------------------
>  1 files changed, 3 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 1ea8c8d..b1dd86c 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -205,8 +205,6 @@ static int __initdata mem_reserve_cnt;
>  static cell_t __initdata regbuf[1024];
>  
> 
> -#define MAX_CPU_THREADS 2
> -
>  /*
>   * Error results ... some OF calls will return "-1" on error, some
>   * will return 0, some will return either. To simplify, here are
> @@ -1332,10 +1330,6 @@ static void __init prom_hold_cpus(void)
>  	unsigned int reg;
>  	phandle node;
>  	char type[64];
> -	int cpuid = 0;
> -	unsigned int interrupt_server[MAX_CPU_THREADS];
> -	unsigned int cpu_threads, hw_cpu_num;
> -	int propsize;
>  	struct prom_t *_prom = &RELOC(prom);
>  	unsigned long *spinloop
>  		= (void *) LOW_ADDR(__secondary_hold_spinloop);
> @@ -1379,7 +1373,6 @@ static void __init prom_hold_cpus(void)
>  		reg = -1;
>  		prom_getprop(node, "reg", &reg, sizeof(reg));
>  
> -		prom_debug("\ncpuid        = 0x%x\n", cpuid);
>  		prom_debug("cpu hw idx   = 0x%x\n", reg);
>  
>  		/* Init the acknowledge var which will be reset by
> @@ -1388,28 +1381,9 @@ static void __init prom_hold_cpus(void)
>  		 */
>  		*acknowledge = (unsigned long)-1;
>  
> -		propsize = prom_getprop(node, "ibm,ppc-interrupt-server#s",
> -					&interrupt_server,
> -					sizeof(interrupt_server));
> -		if (propsize < 0) {
> -			/* no property.  old hardware has no SMT */
> -			cpu_threads = 1;
> -			interrupt_server[0] = reg; /* fake it with phys id */
> -		} else {
> -			/* We have a threaded processor */
> -			cpu_threads = propsize / sizeof(u32);
> -			if (cpu_threads > MAX_CPU_THREADS) {
> -				prom_printf("SMT: too many threads!\n"
> -					    "SMT: found %x, max is %x\n",
> -					    cpu_threads, MAX_CPU_THREADS);
> -				cpu_threads = 1; /* ToDo: panic? */
> -			}
> -		}
> -
> -		hw_cpu_num = interrupt_server[0];
> -		if (hw_cpu_num != _prom->cpu) {
> +		if (reg != _prom->cpu) {
>  			/* Primary Thread of non-boot cpu */
> -			prom_printf("%x : starting cpu hw idx %x... ", cpuid, reg);
> +			prom_printf("starting cpu hw idx %x... ", reg);
>  			call_prom("start-cpu", 3, 0, node,
>  				  secondary_hold, reg);
>  
> @@ -1424,17 +1398,10 @@ static void __init prom_hold_cpus(void)
>  		}
>  #ifdef CONFIG_SMP
>  		else
> -			prom_printf("%x : boot cpu     %x\n", cpuid, reg);
> +			prom_printf("boot cpu hw idx %x\n", reg);
>  #endif /* CONFIG_SMP */
> -
> -		/* Reserve cpu #s for secondary threads.   They start later. */
> -		cpuid += cpu_threads;
>  	}
>  
> -	if (cpuid > NR_CPUS)
> -		prom_printf("WARNING: maximum CPUs (" __stringify(NR_CPUS)
> -			    ") exceeded: ignoring extras\n");
> -
>  	prom_debug("prom_hold_cpus: end...\n");
>  }
>  

Re: [PATCH] kill useless SMT code in prom_hold_cpus

[Tony Breeds]

On Tue, Jul 08, 2008 at 05:36:31PM -0500, Nathan Lynch wrote:
> I think this code that counts SMT threads and compares against NR_CPUS
> is an artifact of pre-powerpc-merge ppc64.  We care about starting
> only primary threads in the OF client code.

<snip>

> -			prom_printf("%x : starting cpu hw idx %x... ", cpuid, reg);
> +			prom_printf("starting cpu hw idx %x... ", reg);

If we remove this, where else can we see the mapping of hardware IDs
to logical cpu IDs?  This is useful on POWER4 (at least where they can be
different).

<snip>
 
> -	if (cpuid > NR_CPUS)
> -		prom_printf("WARNING: maximum CPUs (" __stringify(NR_CPUS)
> -			    ") exceeded: ignoring extras\n");
> -

I think this printf() is valuable, if your boot a 128 thread machine on
a kernel with NR_CPUS=64, this is the only messaage you get to indicate
that you're wasting 64 threads, and how to resolve it.

Yours Tony

  linux.conf.au    http://www.marchsouth.org/
  Jan 19 - 24 2009 The Australian Linux Technical Conference!

Re: [PATCH] kill useless SMT code in prom_hold_cpus

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> On Tue, 2008-07-08 at 17:36 -0500, Nathan Lynch wrote:
> > I think this code that counts SMT threads and compares against NR_CPUS
> > is an artifact of pre-powerpc-merge ppc64.  We care about starting
> > only primary threads in the OF client code.
> > 
> > Signed-off-by: Nathan Lynch <ntl@pobox.com>
> 
> That looks good. I'm not merging it right now because I want to dbl
> check that it's allright on all SMT machines. IE. We compare reg[0]
> against _prom->cpu now instead of interrupt_server[0] and I thus
> want to ensure it's the same everywhere.

Thanks.  Looks like prom_find_boot_cpu is setting _prom->cpu to reg
(or 0), so I think it should be fine... a system where reg differed
from interrupt_server[0] would have been broken before this patch
anyway, I think?

Re: [PATCH] kill useless SMT code in prom_hold_cpus

[Benjamin Herrenschmidt]

On Mon, 2008-07-14 at 21:24 -0500, Nathan Lynch wrote:
> Benjamin Herrenschmidt wrote:
> > On Tue, 2008-07-08 at 17:36 -0500, Nathan Lynch wrote:
> > > I think this code that counts SMT threads and compares against NR_CPUS
> > > is an artifact of pre-powerpc-merge ppc64.  We care about starting
> > > only primary threads in the OF client code.
> > > 
> > > Signed-off-by: Nathan Lynch <ntl@pobox.com>
> > 
> > That looks good. I'm not merging it right now because I want to dbl
> > check that it's allright on all SMT machines. IE. We compare reg[0]
> > against _prom->cpu now instead of interrupt_server[0] and I thus
> > want to ensure it's the same everywhere.
> 
> Thanks.  Looks like prom_find_boot_cpu is setting _prom->cpu to reg
> (or 0), so I think it should be fine... a system where reg differed
> from interrupt_server[0] would have been broken before this patch
> anyway, I think?

I tend to agree, just didn't have time to look closely / test before
I get to prepare my batch of patches for Linus, I'll stick it in the
next one.

Cheers,
Ben.

Re: [PATCH] kill useless SMT code in prom_hold_cpus

[Nathan Lynch]

Tony Breeds wrote:
> On Tue, Jul 08, 2008 at 05:36:31PM -0500, Nathan Lynch wrote:
> 
> > -			prom_printf("%x : starting cpu hw idx %x... ", cpuid, reg);
> > +			prom_printf("starting cpu hw idx %x... ", reg);
> 
> If we remove this, where else can we see the mapping of hardware IDs
> to logical cpu IDs?  This is useful on POWER4 (at least where they can be
> different).

sysfs.  (e.g. /sys/devices/system/cpu/cpu0/physical_id)

> > -	if (cpuid > NR_CPUS)
> > -		prom_printf("WARNING: maximum CPUs (" __stringify(NR_CPUS)
> > -			    ") exceeded: ignoring extras\n");
> > -
> 
> I think this printf() is valuable, if your boot a 128 thread machine on
> a kernel with NR_CPUS=64, this is the only messaage you get to indicate
> that you're wasting 64 threads, and how to resolve it.

The proper place for such a message is in the kernel's smp bringup
code later on, and/or the code that initializes the various cpu maps.
The prom_init code should not really be concerned with the kernel's
NR_CPUS configuration or mapping of logical to physical ids.

Re: [PATCH] kill useless SMT code in prom_hold_cpus

[Tony Breeds]

On Mon, Jul 14, 2008 at 11:55:51PM -0500, Nathan Lynch wrote:
 
> sysfs.  (e.g. /sys/devices/system/cpu/cpu0/physical_id)

Hmm okay, sysfs is fine if you're gettign all the way up. I guess if we
have problems in that are we can readd the printf().
 
> The proper place for such a message is in the kernel's smp bringup
> code later on, and/or the code that initializes the various cpu maps.
> The prom_init code should not really be concerned with the kernel's
> NR_CPUS configuration or mapping of logical to physical ids.

Right, care to make this a series and add that ;P

Yours Tony

  linux.conf.au    http://www.marchsouth.org/
  Jan 19 - 24 2009 The Australian Linux Technical Conference!