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

[Satyam Sharma]

On 6/12/07, Jan Beulich <jbeulich@xxxxxxxxxx> 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 :-)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/