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

[permalink] [raw]

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

On 6/12/07, Jan Beulich <[email protected]> wrote:
> .. which modpost is warning about.
>
> Signed-off-by: Jan Beulich <[email protected]>
>
> 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

2007-06-12 12:09:19

[permalink] [raw]

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

Hi Jan,

On 6/12/07, Satyam Sharma <[email protected]> wrote:
> On 6/12/07, Jan Beulich <[email protected]> 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

2007-06-12 12:43:17

[permalink] [raw]

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

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

2007-06-12 13:32:26

[permalink] [raw]

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

On 6/12/07, Jan Beulich <[email protected]> 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 :-)

2007-06-12 13:48:53

[permalink] [raw]

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

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

2007-06-12 14:09:44

[permalink] [raw]

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

On 6/12/07, Jan Beulich <[email protected]> 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.

2007-06-12 17:55:18

[permalink] [raw]

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

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

2007-06-12 17:59:12

[permalink] [raw]

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

On Tue, Jun 12, 2007 at 07:39:30PM +0530, Satyam Sharma wrote:
> On 6/12/07, Jan Beulich <[email protected]> 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

2007-06-13 03:18:41

[permalink] [raw]

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

Hi Sam,

On 6/12/07, Sam Ravnborg <[email protected]> wrote:
> On Tue, Jun 12, 2007 at 07:39:30PM +0530, Satyam Sharma wrote:
> > On 6/12/07, Jan Beulich <[email protected]> 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

2007-06-13 04:34:32

[permalink] [raw]

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

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

2007-06-13 06:59:18

[permalink] [raw]

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

>>> Sam Ravnborg <[email protected]> 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

2007-06-13 07:39:18

[permalink] [raw]

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

On 6/13/07, Jan Beulich <[email protected]> wrote:
> >>> Sam Ravnborg <[email protected]> 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

2007-06-13 07:49:17

[permalink] [raw]

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

>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

2007-06-13 08:10:00