2006-08-10 19:39:38

by Andi Kleen

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

r

From: Magnus Damm <[email protected]>

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 <[email protected]>
Signed-off-by: Andi Kleen <[email protected]>

---

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 = {


2006-08-11 15:29:04

by Chuck Ebbert

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

In-Reply-To: <[email protected]>

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

> From: Magnus Damm <[email protected]>
>
> 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

2006-08-11 16:10:27

by Andi Kleen

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


> 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

2006-08-14 01:25:15

by Magnus Damm

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

On Fri, 2006-08-11 at 11:24 -0400, Chuck Ebbert wrote:
> In-Reply-To: <[email protected]>
>
> On Thu, 10 Aug 2006 21:37:40 +0200, Andi Kleen wrote:
>
> > From: Magnus Damm <[email protected]>
> >
> > 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

2006-08-15 06:51:47

by Chuck Ebbert

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

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

2006-08-15 07:44:09

by Arjan van de Ven

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

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...


2006-08-15 17:00:18

by Dave Jones

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

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