[PATCH] x86: fix improper .init-type section references

[PATCH] x86: fix improper .init-type section references

[Jan Beulich]

.. which modpost is warning about.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

 arch/i386/kernel/cpu/intel_cacheinfo.c    |    6 +++---
 arch/i386/kernel/cpu/mcheck/therm_throt.c |    2 +-
 arch/i386/kernel/cpu/mtrr/generic.c       |    2 +-
 arch/i386/kernel/cpu/mtrr/main.c          |    2 +-
 arch/i386/kernel/smpboot.c                |    2 +-
 arch/x86_64/kernel/mce.c                  |    6 +++---
 arch/x86_64/mm/init.c                     |    2 +-
 7 files changed, 11 insertions(+), 11 deletions(-)

--- linux-2.6.22-rc4/arch/i386/kernel/cpu/intel_cacheinfo.c	2007-06-11 18:09:52.000000000 +0200
+++ 2.6.22-rc4-x86-init-attribs/arch/i386/kernel/cpu/intel_cacheinfo.c	2007-06-11 09:10:41.000000000 +0200
@@ -240,7 +240,7 @@ static int __cpuinit cpuid4_cache_lookup
 }
 
 /* will only be called once; __init is safe here */
-static int __init find_num_cache_leaves(void)
+static int noinline __init_refok find_num_cache_leaves(void)
 {
 	unsigned int		eax, ebx, ecx, edx;
 	union _cpuid4_leaf_eax	cache_eax;
@@ -448,7 +448,7 @@ static void __cpuinit cache_shared_cpu_m
 		}
 	}
 }
-static void __cpuinit cache_remove_shared_cpu_map(unsigned int cpu, int index)
+static void __cpuexit cache_remove_shared_cpu_map(unsigned int cpu, int index)
 {
 	struct _cpuid4_info	*this_leaf, *sibling_leaf;
 	int sibling;
@@ -461,7 +461,7 @@ static void __cpuinit cache_remove_share
 }
 #else
 static void __init cache_shared_cpu_map_setup(unsigned int cpu, int index) {}
-static void __init cache_remove_shared_cpu_map(unsigned int cpu, int index) {}
+static void __exit cache_remove_shared_cpu_map(unsigned int cpu, int index) {}
 #endif
 
 static void free_cache_attributes(unsigned int cpu)
--- linux-2.6.22-rc4/arch/i386/kernel/cpu/mcheck/therm_throt.c	2007-06-11 18:09:52.000000000 +0200
+++ 2.6.22-rc4-x86-init-attribs/arch/i386/kernel/cpu/mcheck/therm_throt.c	2007-06-11 09:10:41.000000000 +0200
@@ -150,7 +150,7 @@ static __cpuinit int thermal_throttle_cp
 	return NOTIFY_OK;
 }
 
-static struct notifier_block thermal_throttle_cpu_notifier =
+static __cpuinitdata struct notifier_block thermal_throttle_cpu_notifier =
 {
 	.notifier_call = thermal_throttle_cpu_callback,
 };
--- linux-2.6.22-rc4/arch/i386/kernel/cpu/mtrr/generic.c	2007-06-11 18:09:52.000000000 +0200
+++ 2.6.22-rc4-x86-init-attribs/arch/i386/kernel/cpu/mtrr/generic.c	2007-06-11 09:10:41.000000000 +0200
@@ -78,7 +78,7 @@ static void __cpuinit print_fixed(unsign
 }
 
 /*  Grab all of the MTRR state for this CPU into *state  */
-void get_mtrr_state(void)
+void __init get_mtrr_state(void)
 {
 	unsigned int i;
 	struct mtrr_var_range *vrs;
--- linux-2.6.22-rc4/arch/i386/kernel/cpu/mtrr/main.c	2007-06-11 18:09:52.000000000 +0200
+++ 2.6.22-rc4-x86-init-attribs/arch/i386/kernel/cpu/mtrr/main.c	2007-06-11 09:10:41.000000000 +0200
@@ -639,7 +639,7 @@ static struct sysdev_driver mtrr_sysdev_
  * initialized (i.e. before smp_init()).
  * 
  */
-void mtrr_bp_init(void)
+void __init mtrr_bp_init(void)
 {
 	init_ifs();
 
--- linux-2.6.22-rc4/arch/i386/kernel/smpboot.c	2007-06-11 18:09:52.000000000 +0200
+++ 2.6.22-rc4-x86-init-attribs/arch/i386/kernel/smpboot.c	2007-06-11 09:10:41.000000000 +0200
@@ -118,7 +118,7 @@ DEFINE_PER_CPU(int, cpu_state) = { 0 };
  * has made sure it's suitably aligned.
  */
 
-static unsigned long __devinit setup_trampoline(void)
+static unsigned long __cpuinit setup_trampoline(void)
 {
 	memcpy(trampoline_base, trampoline_data, trampoline_end - trampoline_data);
 	return virt_to_phys(trampoline_base);
--- linux-2.6.22-rc4/arch/x86_64/kernel/mce.c	2007-06-11 18:10:04.000000000 +0200
+++ 2.6.22-rc4-x86-init-attribs/arch/x86_64/kernel/mce.c	2007-06-11 09:10:41.000000000 +0200
@@ -701,7 +701,7 @@ static __cpuinit int mce_create_device(u
 	return err;
 }
 
-static void mce_remove_device(unsigned int cpu)
+static __cpuexit void mce_remove_device(unsigned int cpu)
 {
 	int i;
 
@@ -713,7 +713,7 @@ static void mce_remove_device(unsigned i
 }
 
 /* Get notified when a cpu comes on/off. Be hotplug friendly. */
-static int
+static __cpuinit int
 mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
 	unsigned int cpu = (unsigned long)hcpu;
@@ -731,7 +731,7 @@ mce_cpu_callback(struct notifier_block *
 	return NOTIFY_OK;
 }
 
-static struct notifier_block mce_cpu_notifier = {
+static __cpuinitdata struct notifier_block mce_cpu_notifier = {
 	.notifier_call = mce_cpu_callback,
 };
 
--- linux-2.6.22-rc4/arch/x86_64/mm/init.c	2007-06-11 18:10:04.000000000 +0200
+++ 2.6.22-rc4-x86-init-attribs/arch/x86_64/mm/init.c	2007-06-11 10:28:44.000000000 +0200
@@ -762,7 +762,7 @@ int in_gate_area_no_task(unsigned long a
 	return (addr >= VSYSCALL_START) && (addr < VSYSCALL_END);
 }
 
-void *alloc_bootmem_high_node(pg_data_t *pgdat, unsigned long size)
+void *__init alloc_bootmem_high_node(pg_data_t *pgdat, unsigned long size)
 {
 	return __alloc_bootmem_core(pgdat->bdata, size,
 			SMP_CACHE_BYTES, (4UL*1024*1024*1024), 0);

Re: [PATCH] x86: fix improper .init-type section references

[Satyam Sharma]

On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> .. which modpost is warning about.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
>  arch/i386/kernel/cpu/intel_cacheinfo.c    |    6 +++---
>  arch/i386/kernel/cpu/mcheck/therm_throt.c |    2 +-
>  arch/i386/kernel/cpu/mtrr/generic.c       |    2 +-
>  arch/i386/kernel/cpu/mtrr/main.c          |    2 +-
>  arch/i386/kernel/smpboot.c                |    2 +-
>  arch/x86_64/kernel/mce.c                  |    6 +++---
>  arch/x86_64/mm/init.c                     |    2 +-
>  7 files changed, 11 insertions(+), 11 deletions(-)

> +++ 2.6.22-rc4-x86-init-attribs/arch/i386/kernel/cpu/intel_cacheinfo.c
>
>  /* will only be called once; __init is safe here */
> -static int __init find_num_cache_leaves(void)
> +static int noinline __init_refok find_num_cache_leaves(void)

Same __init_refok semantics confusion here ...

This time the actual issue is a __cpuinit -> __init transition in the call
chain, so the proper fix would be to mark find_num_cache_leaves() as
__cpuinit also. Also, the comment above it must be kept consistent
with the function's annotation.

> @@ -448,7 +448,7 @@ static void __cpuinit cache_shared_cpu_m
> -static void __cpuinit cache_remove_shared_cpu_map(unsigned int cpu, int index)
> +static void __cpuexit cache_remove_shared_cpu_map(unsigned int cpu, int index)

You might've done this because the caller of
cache_remove_shared_cpu_map() is cache_remove_dev()
which is __cpuexit. However, cache_remove_dev() itself is called
from cacheinfo_cpu_callback() which is ... __cpuinit, because it's
caller cache_sysfs_init() is *definitely* __cpuinit. What a mess ...

device_initcall(cache_sysfs_init):

__cpuinit cache_sysfs_init
  -> __cpuinit cacheinfo_cpu_callback  <======== PROBLEM
    -> __cpuexit cache_remove_dev  <======== PROBLEM
      -> __cpuinit cache_remove_shared_cpu_map

Where cacheinfo_cpu_callback() itself is the callback for the
cacheinfo_cpu_notifier notifier, which is itself marked __cpuinitdata.

I suspect the __cpuexit-marking of cache_remove_dev() here is totally
bogus. When !CONFIG_HOTPLUG_CPU, __cpuexit == __exit, and
__exit simply means that the function can be discarded and ignored
from being linked in for non-modular (built-in) build of that particular
module. I'm not sure how the code in question here can ever be built
as a module, so cache_remove_dev shouldn't be a __cpuexit in the
first place. But not marking it anything would take us back to the
modpost warnings. So ... something needs to be done about the
logic in this whole place ...

I did some more code inspection and found:

The cacheinfo_cpu_notifier itself is called out from _cpu_down() which
is ... yep, *not* __cpuinit (and obviously *cannot* be so). _cpu_down()
is called from cpu_down() which is (thankfully!) protected inside
#ifdef CONFIG_HOTPLUG_CPU, so we've /just about/ escaped trouble
so far, but another of it's callers is disable_nonboot_cpus() which does
*not* depend on HOTPLUG_CPU, but is #ifdef'ed inside SUSPEND_SMP
(gargh!) instead, and ... is _not_ __cpuinit either (obviously, again).

I got tired at that point, and stopped following the code around :-)

[ Crazy suggestion: why not just simply do away with this whole
init-section-freeing-after-initcalls / exit-section-not-linked-if-built-in
business itself?!?! I mean, it's 2007, memory is cheap, and we
only save a few 100KB at most but get so many kernel developer
man-hours totally wasted on all of this in return, which could
instead be better utilized, definitely. Anybody listening? ]

> @@ -461,7 +461,7 @@ static void __cpuinit cache_remove_share
>  }
>  #else
>  static void __init cache_shared_cpu_map_setup(unsigned int cpu, int index) {}
> -static void __init cache_remove_shared_cpu_map(unsigned int cpu, int index) {}
> +static void __exit cache_remove_shared_cpu_map(unsigned int cpu, int index) {}

So this shouldn't be __exit either. BTW you could continue to use
__cpu{init, exit} variants for consistency's sake here too. Note that
!SMP => !HOTPLUG_CPU => __cpu{init, exit} become just the same
as the {__init, __exit} automatically.

> --- linux-2.6.22-rc4/arch/i386/kernel/cpu/mcheck/therm_throt.c
> +++ 2.6.22-rc4-x86-init-attribs/arch/i386/kernel/cpu/mcheck/therm_throt.c
> @@ -150,7 +150,7 @@ static __cpuinit int thermal_throttle_cp
>         return NOTIFY_OK;
>  }
>
> -static struct notifier_block thermal_throttle_cpu_notifier =
> +static __cpuinitdata struct notifier_block thermal_throttle_cpu_notifier =
>  {
>         .notifier_call = thermal_throttle_cpu_callback,
>  };

If this is made __cpuinitdata, then thermal_throttle_init_device()
that references it must also be made __cpuinit (it is presently
simply __init). These patches are on 22-rc4, right?

Note that this is another cpu_chain notifier callback, and called
out from non-__{cpu}init functions (as discussed above) so could
be problematic.


> -void get_mtrr_state(void)
> +void __init get_mtrr_state(void)

> -void mtrr_bp_init(void)
> +void __init mtrr_bp_init(void)

These two changes seem to be OK.
Should've been a separate patch, IMHO.

> --- linux-2.6.22-rc4/arch/x86_64/kernel/mce.c
>
> -static void mce_remove_device(unsigned int cpu)
> +static __cpuexit void mce_remove_device(unsigned int cpu)

Again, __cpuexit might not be what we want here ...

>  /* Get notified when a cpu comes on/off. Be hotplug friendly. */
> -static int
> +static __cpuinit int
>  mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)

> -static struct notifier_block mce_cpu_notifier = {
> +static __cpuinitdata struct notifier_block mce_cpu_notifier = {
>         .notifier_call = mce_cpu_callback,
>  };

It's the same (problematic) notifier / remove_dev idiom all over again ...

> --- linux-2.6.22-rc4/arch/x86_64/mm/init.c
>
> -void *alloc_bootmem_high_node(pg_data_t *pgdat, unsigned long size)
> +void *__init alloc_bootmem_high_node(pg_data_t *pgdat, unsigned long size)

This one's correct again.

Satyam

Re: [PATCH] x86: fix improper .init-type section references

[Satyam Sharma]

Hi Jan,

On 6/12/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> > [...]
> > @@ -448,7 +448,7 @@ static void __cpuinit cache_shared_cpu_m
> > -static void __cpuinit cache_remove_shared_cpu_map(unsigned int cpu, int index)
> > +static void __cpuexit cache_remove_shared_cpu_map(unsigned int cpu, int index)
>
> You might've done this because the caller of
> cache_remove_shared_cpu_map() is cache_remove_dev()
> which is __cpuexit. However, cache_remove_dev() itself is called
> from cacheinfo_cpu_callback() which is ... __cpuinit, because it's
> caller cache_sysfs_init() is *definitely* __cpuinit. What a mess ...
>
> device_initcall(cache_sysfs_init):
>
> __cpuinit cache_sysfs_init
>   -> __cpuinit cacheinfo_cpu_callback  <======== PROBLEM
>     -> __cpuexit cache_remove_dev  <======== PROBLEM
>       -> __cpuinit cache_remove_shared_cpu_map
>
> Where cacheinfo_cpu_callback() itself is the callback for the
> cacheinfo_cpu_notifier notifier, which is itself marked __cpuinitdata.
>
> I suspect the __cpuexit-marking of cache_remove_dev() here is totally
> bogus. When !CONFIG_HOTPLUG_CPU, __cpuexit == __exit, and
> __exit simply means that the function can be discarded and ignored
> from being linked in for non-modular (built-in) build of that particular
> module. I'm not sure how the code in question here can ever be built
> as a module, so cache_remove_dev shouldn't be a __cpuexit in the
> first place. But not marking it anything would take us back to the
> modpost warnings. So ... something needs to be done about the
> logic in this whole place ...
>
> I did some more code inspection and found:
>
> The cacheinfo_cpu_notifier itself is called out from _cpu_down() which
> is ... yep, *not* __cpuinit (and obviously *cannot* be so). _cpu_down()
> is called from cpu_down() which is (thankfully!) protected inside
> #ifdef CONFIG_HOTPLUG_CPU, so we've /just about/ escaped trouble
> so far, but another of it's callers is disable_nonboot_cpus() which does
> *not* depend on HOTPLUG_CPU, but is #ifdef'ed inside SUSPEND_SMP
> (gargh!) instead, and ... is _not_ __cpuinit either (obviously, again).

Wait, SUSPEND_SMP again depends on HOTPLUG_CPU, so there
are no issues here in marking cache_remove_dev() __cpuexit
(which effectively simply becomes #ifdef HOTPLUG_CPU) as all
callsites that call out the notifier with CPU_DEAD/FROZEN are also
going to be disabled. [ Ok, looks like using __cpuexit as this kind of
pseudo-#ifdef HOTPLUG_CPU is a standard practice? ]

But modpost will still complain (bogus warning) about calling __exit
from __init (when HOTPLUG_CPU=n) for cacheinfo_cpu_callback()
calling cache_remove_dev(), no?

So I'd guess all the various notifier callback functions in your patch
need to be whitelisted by modpost, as those are __init functions
who can legally reference __exit (when HOTPLUG_CPU=n).
__init_refok can't be used here, so some special whitelisting might
be required, I presume.

Satyam

Re: [PATCH] x86: fix improper .init-type section references

[Jan Beulich]

>> The cacheinfo_cpu_notifier itself is called out from _cpu_down() which
>> is ... yep, *not* __cpuinit (and obviously *cannot* be so). _cpu_down()
>> is called from cpu_down() which is (thankfully!) protected inside
>> #ifdef CONFIG_HOTPLUG_CPU, so we've /just about/ escaped trouble
>> so far, but another of it's callers is disable_nonboot_cpus() which does
>> *not* depend on HOTPLUG_CPU, but is #ifdef'ed inside SUSPEND_SMP
>> (gargh!) instead, and ... is _not_ __cpuinit either (obviously, again).
>
>Wait, SUSPEND_SMP again depends on HOTPLUG_CPU, so there
>are no issues here in marking cache_remove_dev() __cpuexit
>(which effectively simply becomes #ifdef HOTPLUG_CPU) as all
>callsites that call out the notifier with CPU_DEAD/FROZEN are also
>going to be disabled. [ Ok, looks like using __cpuexit as this kind of
>pseudo-#ifdef HOTPLUG_CPU is a standard practice? ]
>
>But modpost will still complain (bogus warning) about calling __exit
>from __init (when HOTPLUG_CPU=n) for cacheinfo_cpu_callback()
>calling cache_remove_dev(), no?

No, it doesn't, at least not for me. And from a purely theoretical
perspective I don't think such references should be considered bad -
.exit.* should be discarded together with .init.* if unloading is
impossible (built-in or configured off), not before module/kernel
initialization. This is specifically because init code may want to utilize
exit code in case of initialization failure.

Jan

Re: [PATCH] x86: fix improper .init-type section references

[Satyam Sharma]

On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> >> The cacheinfo_cpu_notifier itself is called out from _cpu_down() which
> >> is ... yep, *not* __cpuinit (and obviously *cannot* be so). _cpu_down()
> >> is called from cpu_down() which is (thankfully!) protected inside
> >> #ifdef CONFIG_HOTPLUG_CPU, so we've /just about/ escaped trouble
> >> so far, but another of it's callers is disable_nonboot_cpus() which does
> >> *not* depend on HOTPLUG_CPU, but is #ifdef'ed inside SUSPEND_SMP
> >> (gargh!) instead, and ... is _not_ __cpuinit either (obviously, again).
> >
> >Wait, SUSPEND_SMP again depends on HOTPLUG_CPU, so there
> >are no issues here in marking cache_remove_dev() __cpuexit
> >(which effectively simply becomes #ifdef HOTPLUG_CPU) as all
> >callsites that call out the notifier with CPU_DEAD/FROZEN are also
> >going to be disabled. [ Ok, looks like using __cpuexit as this kind of
> >pseudo-#ifdef HOTPLUG_CPU is a standard practice? ]
> >
> >But modpost will still complain (bogus warning) about calling __exit
> >from __init (when HOTPLUG_CPU=n) for cacheinfo_cpu_callback()
> >calling cache_remove_dev(), no?
>
> No, it doesn't, at least not for me.

That's weird. Some bug in modpost, I'd say.

(Googling around for: section mismatch cacheinfo_cpu_callback)

Yup, it seems a new version of modpost does catch it:
http://readlist.com/lists/vger.kernel.org/linux-kernel/69/349920.html

(Sam's patch there isn't quite right, though, it'll unnecessarily
link in cache_remove_dev() even when HOTPLUG_CPU=n.)

> And from a purely theoretical
> perspective I don't think such references should be considered bad -
> .exit.* should be discarded together with .init.* if unloading is
> impossible (built-in or configured off), not before module/kernel
> initialization.

Hmm, but that's not how things are, presently. __exit marked
functions are simply not linked into the kernel (when that module
is being built-in) at all -- this "discard" happens at _build time_
(to save on kernel image size).

> This is specifically because init code may want to utilize
> exit code in case of initialization failure.

You're right, such code (presently) has to remove the __exit
from the cleanup functions, if it wants the __init functions to
use them too.

Coming back to the code at hand, so we have two ways to fix this
(also applicable to the other such examples, as per your patch):

1. Mark cache_remove_dev() and cache_remove_shared_cpu_map()
as __cpuexit, and then whitelist (the __cpuinit marked)
cacheinfo_cpu_callback() so that modpost doesn't barf.

2. Place cache_remove_dev() and cache_remove_shared_cpu_map()
definitions inside #ifdef CONFIG_HOTPLUG_CPU and provide dummy
{} definitions (not marked either __init or __exit) when
HOTPLUG_CPU=n.

BTW at arch/i386/kernel/cpu/intel_cacheinfo.c:463:

static void __init cache_shared_cpu_map_setup(unsigned int cpu, int index) {}
static void __init cache_remove_shared_cpu_map(unsigned int cpu, int index) {}

is so crazy! What are we trying to "save" by marking these dummy "{}"
functions as __init??? These should in fact be inline, IMO.

There's the "third" option too, of course. Just say bye to the
entire .init.text/.exit.text-discarding concept. We lose a few (ok,
perhaps even several) 100 KB of memory, but save on developer
maintenance effort. But I'm not too sure if this suggestion would
be popular, though :-)

Re: [PATCH] x86: fix improper .init-type section references

[Jan Beulich]

>> And from a purely theoretical
>> perspective I don't think such references should be considered bad -
>> .exit.* should be discarded together with .init.* if unloading is
>> impossible (built-in or configured off), not before module/kernel
>> initialization.
>
>Hmm, but that's not how things are, presently. __exit marked
>functions are simply not linked into the kernel (when that module
>is being built-in) at all -- this "discard" happens at _build time_
>(to save on kernel image size).

Not really, at least not for i386 and x86-64 - see their vmlinux.lds.S files.

Jan

Re: [PATCH] x86: fix improper .init-type section references

[Satyam Sharma]

On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> >> And from a purely theoretical
> >> perspective I don't think such references should be considered bad -
> >> .exit.* should be discarded together with .init.* if unloading is
> >> impossible (built-in or configured off), not before module/kernel
> >> initialization.
> >
> >Hmm, but that's not how things are, presently. __exit marked
> >functions are simply not linked into the kernel (when that module
> >is being built-in) at all -- this "discard" happens at _build time_
> >(to save on kernel image size).
>
> Not really, at least not for i386 and x86-64 - see their vmlinux.lds.S files.

For those archs, yes, you're right that modpost should be
special-casing (based on arch) before complaining for
.init -> .exit references.

Re: [PATCH] x86: fix improper .init-type section references

[Sam Ravnborg]

On Tue, Jun 12, 2007 at 07:39:30PM +0530, Satyam Sharma wrote:
> On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> >>> And from a purely theoretical
> >>> perspective I don't think such references should be considered bad -
> >>> .exit.* should be discarded together with .init.* if unloading is
> >>> impossible (built-in or configured off), not before module/kernel
> >>> initialization.
> >>
> >>Hmm, but that's not how things are, presently. __exit marked
> >>functions are simply not linked into the kernel (when that module
> >>is being built-in) at all -- this "discard" happens at _build time_
> >>(to save on kernel image size).
> >
> >Not really, at least not for i386 and x86-64 - see their vmlinux.lds.S 
> >files.
> 
> For those archs, yes, you're right that modpost should be
> special-casing (based on arch) before complaining for
> .init -> .exit references.
No.
References from __init to __exit is wrong independent on architecture.
powerpc discards them at buildtime, i386 at runtime.
So for the latter we will have an oops where we for the first have a build
time failure.
It is better to let modpost warn always - independent on architecture.

	Sam

Re: [PATCH] x86: fix improper .init-type section references

[Satyam Sharma]

Hi Sam,

On 6/12/07, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Tue, Jun 12, 2007 at 07:39:30PM +0530, Satyam Sharma wrote:
> > On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> > >>> And from a purely theoretical
> > >>> perspective I don't think such references should be considered bad -
> > >>> .exit.* should be discarded together with .init.* if unloading is
> > >>> impossible (built-in or configured off), not before module/kernel
> > >>> initialization.
> > >>
> > >>Hmm, but that's not how things are, presently. __exit marked
> > >>functions are simply not linked into the kernel (when that module
> > >>is being built-in) at all -- this "discard" happens at _build time_
> > >>(to save on kernel image size).
> > >
> > >Not really, at least not for i386 and x86-64 - see their vmlinux.lds.S
> > >files.
> >
> > For those archs, yes, you're right that modpost should be
> > special-casing (based on arch) before complaining for
> > .init -> .exit references.
> No.
> References from __init to __exit is wrong independent on architecture.
[...]
> So for the latter we will have an oops where we for the first have a build
> time failure.

Is that (oops possibility on i386) really true? (.exit.{text, data}) is also
between __init_begin and __init_end and hence will be discarded
_along_ with (not before) .init.{text, data} during run-time I think,
which means .init -> .exit reference modpost warnings (on said
archs) are truly bogus ...

> powerpc discards them at buildtime, i386 at runtime.

Yup, we were only discussing possibility that modpost not complain
about .init -> .exit references that will never go oops (because the arch
guarantees that).

Satyam

Re: [PATCH] x86: fix improper .init-type section references

[Sam Ravnborg]

> 
> Yup, we were only discussing possibility that modpost not complain
> about .init -> .exit references that will never go oops (because the arch
> guarantees that).

And there are no good reasosns why the rules should be different for i386
and powerpc.
This type of special casing is always bad.
Think about it a little.
Someone writes a generic driver and test it on i386 - OK.
But for powerpc it result in a build failure. It would be so much better
to warn about this situation early.

We had this exact issue in loop.c recently - so it is not a made up example.
[I recall it was ia64 that had a build failure but the example still holds ture]

	Sam

Re: [PATCH] x86: fix improper .init-type section references

[Jan Beulich]

>>> Sam Ravnborg <sam@ravnborg.org> 13.06.07 06:35 >>>
>> 
>> Yup, we were only discussing possibility that modpost not complain
>> about .init -> .exit references that will never go oops (because the arch
>> guarantees that).
>
>And there are no good reasosns why the rules should be different for i386
>and powerpc.
>This type of special casing is always bad.
>Think about it a little.
>Someone writes a generic driver and test it on i386 - OK.
>But for powerpc it result in a build failure. It would be so much better
>to warn about this situation early.

And I didn't mean to special case it - I meant to suggest changing the semantics
generally, which is why I gave the example of calling cleanup code (__exit)
from error paths in startup code (__init).

Jan

Re: [PATCH] x86: fix improper .init-type section references

[Satyam Sharma]

On 6/13/07, Jan Beulich <jbeulich@novell.com> wrote:
> >>> Sam Ravnborg <sam@ravnborg.org> 13.06.07 06:35 >>>
> >>
> >> Yup, we were only discussing possibility that modpost not complain
> >> about .init -> .exit references that will never go oops (because the arch
> >> guarantees that).
> >
> >And there are no good reasosns why the rules should be different for i386
> >and powerpc.
> >This type of special casing is always bad.
> >Think about it a little.
> >Someone writes a generic driver and test it on i386 - OK.
> >But for powerpc it result in a build failure. It would be so much better
> >to warn about this situation early.

Ok, that makes sense.

> And I didn't mean to special case it - I meant to suggest changing the semantics
> generally, which is why I gave the example of calling cleanup code (__exit)
> from error paths in startup code (__init).

i.e. don't discard anything at build time, and link the .exit.{text,
data} sections
into the kernel image for _all_ archs? (otherwise how do we avoid
special-casing/
checking for the arch in modpost and warn/not-warn about invalid/valid cases)

But then why not simply lose the __exit (and .exit.*) altogether? Because
__exit becomes redundant in the suggested changed semantics -- just mark
all the cleanup code as __init too (when it's built-in, the only
callsite for the
cleanup code would be from the startup code in .init.*, and when modular,
__init and __exit lose all relevance anyway).

Satyam

Re: [PATCH] x86: fix improper .init-type section references

[Jan Beulich]

>But then why not simply lose the __exit (and .exit.*) altogether? Because
>__exit becomes redundant in the suggested changed semantics -- just mark
>all the cleanup code as __init too (when it's built-in, the only
>callsite for the
>cleanup code would be from the startup code in .init.*, and when modular,
>__init and __exit lose all relevance anyway).

Because of the non-builtin case (__init still has significance in the modular case,
it's only __exit that doesn't). For the builtin case, __init could certainly be
identical to __exit.

Jan

Re: [PATCH] x86: fix improper .init-type section references

[Sam Ravnborg]

On Wed, Jun 13, 2007 at 01:09:07PM +0530, Satyam Sharma wrote:
> 
> i.e. don't discard anything at build time, and link the .exit.{text,
> data} sections
> into the kernel image for _all_ archs? (otherwise how do we avoid
> special-casing/
> checking for the arch in modpost and warn/not-warn about invalid/valid 
> cases)
> 
> But then why not simply lose the __exit (and .exit.*) altogether? Because
> __exit becomes redundant in the suggested changed semantics -- just mark
> all the cleanup code as __init too (when it's built-in, the only
> callsite for the
> cleanup code would be from the startup code in .init.*, and when modular,
> __init and __exit lose all relevance anyway).

The reason we do it today is to save memory in a memory constrained environment.
So with such a suggestion you should back it up with numbers.

How much RAM is wasted by keeping the __init and _exit sections
in memory for a normal embedded build?

You could try a random defconfig for arm or mips since they are
embedded in general. Using defconfig for i386 could tell us
a little but not that important.

And when evaluating the numbers think of maybe 8 MB RAM in total, no swap
storage, and sloow filesystem for permanent storage.

	Sam

Re: [PATCH] x86: fix improper .init-type section references

[Sam Ravnborg]

On Tue, Jun 12, 2007 at 02:43:55PM +0200, Jan Beulich wrote:
> >> The cacheinfo_cpu_notifier itself is called out from _cpu_down() which
> >> is ... yep, *not* __cpuinit (and obviously *cannot* be so). _cpu_down()
> >> is called from cpu_down() which is (thankfully!) protected inside
> >> #ifdef CONFIG_HOTPLUG_CPU, so we've /just about/ escaped trouble
> >> so far, but another of it's callers is disable_nonboot_cpus() which does
> >> *not* depend on HOTPLUG_CPU, but is #ifdef'ed inside SUSPEND_SMP
> >> (gargh!) instead, and ... is _not_ __cpuinit either (obviously, again).
> >
> >Wait, SUSPEND_SMP again depends on HOTPLUG_CPU, so there
> >are no issues here in marking cache_remove_dev() __cpuexit
> >(which effectively simply becomes #ifdef HOTPLUG_CPU) as all
> >callsites that call out the notifier with CPU_DEAD/FROZEN are also
> >going to be disabled. [ Ok, looks like using __cpuexit as this kind of
> >pseudo-#ifdef HOTPLUG_CPU is a standard practice? ]
> >
> >But modpost will still complain (bogus warning) about calling __exit
> >from __init (when HOTPLUG_CPU=n) for cacheinfo_cpu_callback()
> >calling cache_remove_dev(), no?
> 
> No, it doesn't, at least not for me. And from a purely theoretical
> perspective I don't think such references should be considered bad -
> .exit.* should be discarded together with .init.* if unloading is
> impossible (built-in or configured off), not before module/kernel
> initialization. This is specifically because init code may want to utilize
> exit code in case of initialization failure.

For the builtin case some arch's drop exit code during link time
so using function marked __exit from __init will cause a
linker bug. For i386 __exit is discarded at runtime - but dunno when in the
init sequence.

	Sam