Re: [PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

Re: [PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

[Chuck Ebbert]

In-Reply-To: <1155518783.5764.10.camel@localhost>

On Mon, 14 Aug 2006 10:26:23 +0900, Magnus Damm wrote:

> > > The different cpu_dev structures are all used from __cpuinit callers what
> > > I can tell. So mark them as __cpuinitdata instead of __initdata. I am a
> > > little bit unsure about arch/i386/common.c:default_cpu, especially when it
> > > comes to the purpose of this_cpu.
> > 
> > But none of these CPUs supports hotplug and only one (AMD) does SMP.
> > So this is just wasting space in the kernel at runtime.
> 
> How could this be wasting space? If you compile with CONFIG_HOTPLUG_CPU
> disabled then __cpuinitdata will become __initdata - ie the same as
> before. Not a single byte wasted what I can tell.

I was talking about wasted space with HOTPLUG_CPU enabled, of course.
Nobody is ever going to hotplug a VIA, Cyrix, Geode, etc. CPU, yet your
patch makes the kernel carry that code and data anyway.

Yes, the checking scripts will complain.  But we know it's OK.

-- 
Chuck

Re: [PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

[Arjan van de Ven]

On Tue, 2006-08-15 at 02:46 -0400, Chuck Ebbert wrote:
> In-Reply-To: <1155518783.5764.10.camel@localhost>
> 
> On Mon, 14 Aug 2006 10:26:23 +0900, Magnus Damm wrote:
> 
> > > > The different cpu_dev structures are all used from __cpuinit callers what
> > > > I can tell. So mark them as __cpuinitdata instead of __initdata. I am a
> > > > little bit unsure about arch/i386/common.c:default_cpu, especially when it
> > > > comes to the purpose of this_cpu.
> > > 
> > > But none of these CPUs supports hotplug and only one (AMD) does SMP.
> > > So this is just wasting space in the kernel at runtime.
> > 
> > How could this be wasting space? If you compile with CONFIG_HOTPLUG_CPU
> > disabled then __cpuinitdata will become __initdata - ie the same as
> > before. Not a single byte wasted what I can tell.
> 
> I was talking about wasted space with HOTPLUG_CPU enabled, of course.
> Nobody is ever going to hotplug a VIA, Cyrix, Geode, etc. CPU, yet your
> patch makes the kernel carry that code and data anyway.

remember that suspend uses software hot(un)plug as well...

Re: [PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

[Dave Jones]

On Tue, Aug 15, 2006 at 09:43:23AM +0200, Arjan van de Ven wrote:
 > On Tue, 2006-08-15 at 02:46 -0400, Chuck Ebbert wrote:
 > > In-Reply-To: <1155518783.5764.10.camel@localhost>
 > > 
 > > On Mon, 14 Aug 2006 10:26:23 +0900, Magnus Damm wrote:
 > > 
 > > > > > The different cpu_dev structures are all used from __cpuinit callers what
 > > > > > I can tell. So mark them as __cpuinitdata instead of __initdata. I am a
 > > > > > little bit unsure about arch/i386/common.c:default_cpu, especially when it
 > > > > > comes to the purpose of this_cpu.
 > > > > 
 > > > > But none of these CPUs supports hotplug and only one (AMD) does SMP.
 > > > > So this is just wasting space in the kernel at runtime.
 > > > 
 > > > How could this be wasting space? If you compile with CONFIG_HOTPLUG_CPU
 > > > disabled then __cpuinitdata will become __initdata - ie the same as
 > > > before. Not a single byte wasted what I can tell.
 > > 
 > > I was talking about wasted space with HOTPLUG_CPU enabled, of course.
 > > Nobody is ever going to hotplug a VIA, Cyrix, Geode, etc. CPU, yet your
 > > patch makes the kernel carry that code and data anyway.
 > 
 > remember that suspend uses software hot(un)plug as well...

Only for non-boot CPUs. The vendors above (with exception of VIA)
never made SMP systems.

		Dave
-- 
http://www.codemonkey.org.uk

Re: [PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

[Chuck Ebbert]

In-Reply-To: <20060810193740.9133413C0B@wotan.suse.de>

On Thu, 10 Aug 2006 21:37:40 +0200, Andi Kleen wrote:

> From: Magnus Damm <magnus@valinux.co.jp>
> 
> The different cpu_dev structures are all used from __cpuinit callers what
> I can tell. So mark them as __cpuinitdata instead of __initdata. I am a
> little bit unsure about arch/i386/common.c:default_cpu, especially when it
> comes to the purpose of this_cpu.

But none of these CPUs supports hotplug and only one (AMD) does SMP.
So this is just wasting space in the kernel at runtime.

If anything I would only do this for AMD.

Same for the other patch that does more of this kind of change.

(IIRC I tried to do this a while ago and was told not to.)

-- 
Chuck

Re: [PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

[Andi Kleen]

> But none of these CPUs supports hotplug and only one (AMD) does SMP.
> So this is just wasting space in the kernel at runtime.
> 
> If anything I would only do this for AMD.
> 
> Same for the other patch that does more of this kind of change.
> 
> (IIRC I tried to do this a while ago and was told not to.)

But wouldn't the reference check during build always warn if that 
wasn't fixed?

-Andi

Re: [PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

[Magnus Damm]

On Fri, 2006-08-11 at 11:24 -0400, Chuck Ebbert wrote:
> In-Reply-To: <20060810193740.9133413C0B@wotan.suse.de>
> 
> On Thu, 10 Aug 2006 21:37:40 +0200, Andi Kleen wrote:
> 
> > From: Magnus Damm <magnus@valinux.co.jp>
> > 
> > The different cpu_dev structures are all used from __cpuinit callers what
> > I can tell. So mark them as __cpuinitdata instead of __initdata. I am a
> > little bit unsure about arch/i386/common.c:default_cpu, especially when it
> > comes to the purpose of this_cpu.
> 
> But none of these CPUs supports hotplug and only one (AMD) does SMP.
> So this is just wasting space in the kernel at runtime.

How could this be wasting space? If you compile with CONFIG_HOTPLUG_CPU
disabled then __cpuinitdata will become __initdata - ie the same as
before. Not a single byte wasted what I can tell.

The first version of this patch simply added a missing __init to some
function, but I was then corrected by akpm that __cpuinit should be used
instead.

Thanks,

/ magnus

[PATCH for review] [140/145] i386: mark cpu_dev structures as __cpuinitdata

[Andi Kleen]

r

From: Magnus Damm <magnus@valinux.co.jp>

The different cpu_dev structures are all used from __cpuinit callers what
I can tell. So mark them as __cpuinitdata instead of __initdata. I am a
little bit unsure about arch/i386/common.c:default_cpu, especially when it
comes to the purpose of this_cpu.

Signed-off-by: Magnus Damm <magnus@valinux.co.jp>
Signed-off-by: Andi Kleen <ak@suse.de>

---

 arch/i386/kernel/cpu/amd.c       |    2 +-
 arch/i386/kernel/cpu/centaur.c   |    2 +-
 arch/i386/kernel/cpu/common.c    |    2 +-
 arch/i386/kernel/cpu/cyrix.c     |    4 ++--
 arch/i386/kernel/cpu/nexgen.c    |    2 +-
 arch/i386/kernel/cpu/rise.c      |    2 +-
 arch/i386/kernel/cpu/transmeta.c |    2 +-
 arch/i386/kernel/cpu/umc.c       |    2 +-
 8 files changed, 9 insertions(+), 9 deletions(-)

Index: linux/arch/i386/kernel/cpu/amd.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/amd.c
+++ linux/arch/i386/kernel/cpu/amd.c
@@ -259,7 +259,7 @@ static unsigned int amd_size_cache(struc
 	return size;
 }
 
-static struct cpu_dev amd_cpu_dev __initdata = {
+static struct cpu_dev amd_cpu_dev __cpuinitdata = {
 	.c_vendor	= "AMD",
 	.c_ident 	= { "AuthenticAMD" },
 	.c_models = {
Index: linux/arch/i386/kernel/cpu/centaur.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/centaur.c
+++ linux/arch/i386/kernel/cpu/centaur.c
@@ -457,7 +457,7 @@ static unsigned int centaur_size_cache(s
 	return size;
 }
 
-static struct cpu_dev centaur_cpu_dev __initdata = {
+static struct cpu_dev centaur_cpu_dev __cpuinitdata = {
 	.c_vendor	= "Centaur",
 	.c_ident	= { "CentaurHauls" },
 	.c_init		= init_centaur,
Index: linux/arch/i386/kernel/cpu/common.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/common.c
+++ linux/arch/i386/kernel/cpu/common.c
@@ -49,7 +49,7 @@ static void default_init(struct cpuinfo_
 	}
 }
 
-static struct cpu_dev default_cpu = {
+static struct cpu_dev __cpuinitdata default_cpu = {
 	.c_init	= default_init,
 	.c_vendor = "Unknown",
 };
Index: linux/arch/i386/kernel/cpu/cyrix.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/cyrix.c
+++ linux/arch/i386/kernel/cpu/cyrix.c
@@ -429,7 +429,7 @@ static void __init cyrix_identify(struct
 	}
 }
 
-static struct cpu_dev cyrix_cpu_dev __initdata = {
+static struct cpu_dev cyrix_cpu_dev __cpuinitdata = {
 	.c_vendor	= "Cyrix",
 	.c_ident 	= { "CyrixInstead" },
 	.c_init		= init_cyrix,
@@ -452,7 +452,7 @@ static int __init cyrix_exit_cpu(void)
 
 late_initcall(cyrix_exit_cpu);
 
-static struct cpu_dev nsc_cpu_dev __initdata = {
+static struct cpu_dev nsc_cpu_dev __cpuinitdata = {
 	.c_vendor	= "NSC",
 	.c_ident 	= { "Geode by NSC" },
 	.c_init		= init_nsc,
Index: linux/arch/i386/kernel/cpu/nexgen.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/nexgen.c
+++ linux/arch/i386/kernel/cpu/nexgen.c
@@ -40,7 +40,7 @@ static void __init nexgen_identify(struc
 	}
 }
 
-static struct cpu_dev nexgen_cpu_dev __initdata = {
+static struct cpu_dev nexgen_cpu_dev __cpuinitdata = {
 	.c_vendor	= "Nexgen",
 	.c_ident	= { "NexGenDriven" },
 	.c_models = {
Index: linux/arch/i386/kernel/cpu/rise.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/rise.c
+++ linux/arch/i386/kernel/cpu/rise.c
@@ -28,7 +28,7 @@ static void __init init_rise(struct cpui
 	set_bit(X86_FEATURE_CX8, c->x86_capability);
 }
 
-static struct cpu_dev rise_cpu_dev __initdata = {
+static struct cpu_dev rise_cpu_dev __cpuinitdata = {
 	.c_vendor	= "Rise",
 	.c_ident	= { "RiseRiseRise" },
 	.c_models = {
Index: linux/arch/i386/kernel/cpu/transmeta.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/transmeta.c
+++ linux/arch/i386/kernel/cpu/transmeta.c
@@ -97,7 +97,7 @@ static void __init transmeta_identify(st
 	}
 }
 
-static struct cpu_dev transmeta_cpu_dev __initdata = {
+static struct cpu_dev transmeta_cpu_dev __cpuinitdata = {
 	.c_vendor	= "Transmeta",
 	.c_ident	= { "GenuineTMx86", "TransmetaCPU" },
 	.c_init		= init_transmeta,
Index: linux/arch/i386/kernel/cpu/umc.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/umc.c
+++ linux/arch/i386/kernel/cpu/umc.c
@@ -10,7 +10,7 @@ static void __init init_umc(struct cpuin
 
 }
 
-static struct cpu_dev umc_cpu_dev __initdata = {
+static struct cpu_dev umc_cpu_dev __cpuinitdata = {
 	.c_vendor	= "UMC",
 	.c_ident 	= { "UMC UMC UMC" },
 	.c_models = {