On Mon, 20 Aug 2007, Andrew Morton wrote:
> On Tue, 21 Aug 2007 10:07:31 +0530 (IST) Satyam Sharma <[email protected]> wrote:
> >
> > WARNING: arch/i386/kernel/built-in.o(.data+0x2148): Section mismatch: reference
> > to .init.text: (between 'thermal_throttle_cpu_notifier' and 'mtrr_mutex')
> >
> > comes because struct notifier_block thermal_throttle_cpu_notifier in
> > arch/i386/kernel/cpu/mcheck/therm_throt.c goes in .data section but
> > the notifier callback function itself has been marked __cpuinit which
> > becomes __init == .init.text when HOTPLUG_CPU=n. The warning is bogus
> > because the callback will never be called out if HOTPLUG_CPU=n in the
> > first place (as one can see from kernel/cpu.c, the cpu_chain itself
> > is __cpuinitdata :-)
> >
> > So, let's mark thermal_throttle_cpu_notifier as __cpuinitdata to fix
> > the section mismatch warning.
> > [...]
>
> okay... However we're not being very consistent here.
>
> register_hotcpu_notifier() is cunning. If CONFIG_HOTPLUG_CPU=y, we need
> the notifier block and the function to which it points to be in .data and
> in .text. If CONFIG_HOTPLUG_CPU=n, we don't need them to be present at all.
>
> So what we can do is to just leave the notifier block in .data and the
> function in .text and then the compiler/linker will notice that nothing
> references them and they will be omitted at build time.
>
> So basically, the register_hotcpu_notifier() implementation (in league with
> the compiler) is taking the place of the manual notations.
Ok, I can see where you're getting at. When CONFIG_HOTPLUG_CPU=n,
the compiler will just optimize away the reference to (void)(nb);
and with it, the reference to the callback function too, entirely.
[ BTW I wonder if we should make the HOTPLUG_CPU=y implementation of
register_cpu_notifier as void-returning (notifier_chain_register()
never fails anyway) to preserve symmetry with the HOTPLUG_CPU=n stub.
Because of the do {} while, no callsite out there can ever check the
return from register_hotcpu_notifier() without breaking builds when
HOTPLUG_CPU=n. But that goes against the taste of register_foo()
functions in the kernel. So probably it is the HOTPLUG_CPU=n stub
that should be made "static inline int {return 0;}" instead? However,
on doing that, the compiler may fail to optimize away the callback
function, at least that's what the testcase I wrote to experiment
with all this proved:
http://www.cse.iitk.ac.in/users/ssatyam/xyz.c ]
Curiously, notifier_chain_unregister() _can_ return errors, but I bet
*none* of the unregister_foo_notifer()'s out there ever check its
return anyway. All the unregister_foo() and exit_foo() functions in the
kernel tend to be void-returning for good reason, so I wonder if we
should make notifier_chain_unregister() void-returning as well, and go
BUG() there (instead of returning -ENOENT which noone checks) when asked
to unregister a notifier block that didn't even exist. That does sound
BUG-worthy to me, and in all probability, we would've oopsed when trying
to call out the callback of said non-existent notifier_block already.
In fact, making notifier_chain_unregister() go BUG() on that error will
even catch certain bugs (such as somebody annotating the notifier_block
incorrectly with __init when he should not have) early, irrespective of
whether or not that notifier was actually ever even called out on! ]
> Note that because this works at compile time, it is also effective within
> modules.
I must say, this is an amazingly cunning idea. Only things I can think of
against this would be: the dropping of unneeded code is not guaranteed,
but depends on compiler. And we saw from the "static inline {return 0;}"
testcase that gcc can sometimes be not-so-smart.
> I'm not sure that we discard the unneeded sections from within
> modules? (I forget).
Didn't quite get this, but yes, __cpu{init,exit} can only become __init
or __exit at most, neither of which can be dropped from a module.
> So... what to do? I guess for consistency one could hunt down all the
> register_hotcpu_notifier() sites and remove all the __initfoo tags from all of
> them. But that makes register_hotcpu_notifier() inconsistent from
> everything else, so there's an argument that we should make all these
> things __cpuinit and __cpuinitdata for consistency rather than relying upon
> register_hotcpu_notifier()'s magical properties as a special case. Dunno.
I also like the above idea because it eliminates the need for authors
to have to mark their functions appropriately -- nobody gets that right
anyway, as we see from the zillions of section mismatch warnings every
-rc cycle. But the __cpuinit and __cpuinitdata consistency argument does
sound "safe". Sigh.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
