[PATCH][bugzilla #8679] therm_throt.c: Fix section mismatch

[PATCH][bugzilla #8679] therm_throt.c: Fix section mismatch

[Satyam Sharma]

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.

Signed-off-by: Satyam Sharma <satyam@infradead.org>

---

[The other section mismatch mentioned in bugzilla #8679 is already fixed.]

 arch/i386/kernel/cpu/mcheck/therm_throt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/i386/kernel/cpu/mcheck/therm_throt.c b/arch/i386/kernel/cpu/mcheck/therm_throt.c
index 1203dc5..494d320 100644
--- a/arch/i386/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/i386/kernel/cpu/mcheck/therm_throt.c
@@ -152,7 +152,7 @@ static __cpuinit int thermal_throttle_cpu_callback(struct notifier_block *nfb,
 	return NOTIFY_OK;
 }
 
-static struct notifier_block thermal_throttle_cpu_notifier =
+static struct notifier_block thermal_throttle_cpu_notifier __cpuinitdata =
 {
 	.notifier_call = thermal_throttle_cpu_callback,
 };

Re: [PATCH][bugzilla #8679] therm_throt.c: Fix section mismatch

[Andrew Morton]

On Tue, 21 Aug 2007 10:07:31 +0530 (IST) Satyam Sharma <satyam@infradead.org> 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.
> 
> Signed-off-by: Satyam Sharma <satyam@infradead.org>
> 
> ---
> 
> [The other section mismatch mentioned in bugzilla #8679 is already fixed.]
> 
>  arch/i386/kernel/cpu/mcheck/therm_throt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/i386/kernel/cpu/mcheck/therm_throt.c b/arch/i386/kernel/cpu/mcheck/therm_throt.c
> index 1203dc5..494d320 100644
> --- a/arch/i386/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/i386/kernel/cpu/mcheck/therm_throt.c
> @@ -152,7 +152,7 @@ static __cpuinit int thermal_throttle_cpu_callback(struct notifier_block *nfb,
>  	return NOTIFY_OK;
>  }
>  
> -static struct notifier_block thermal_throttle_cpu_notifier =
> +static struct notifier_block thermal_throttle_cpu_notifier __cpuinitdata =
>  {
>  	.notifier_call = thermal_throttle_cpu_callback,
>  };

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.

Note that because this works at compile time, it is also effective within
modules.  I'm not sure that we discard the unneeded sections from within
modules? (I forget).

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.

<looks at blk_cpu_notifier>

There are a few things to clear up here...

Re: [PATCH][bugzilla #8679] therm_throt.c: Fix section mismatch

[Andrew Morton]

On Mon, 20 Aug 2007 22:03:28 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

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

As long as the notifier block and the function are static.  I don't think
the toolchain is smart enough to remove them if they have global scope,
but I didn't check this..

Re: [PATCH][bugzilla #8679] therm_throt.c: Fix section mismatch

[Satyam Sharma]

On Mon, 20 Aug 2007, Andrew Morton wrote:

> On Tue, 21 Aug 2007 10:07:31 +0530 (IST) Satyam Sharma <satyam@infradead.org> 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.

Re: [PATCH][bugzilla #8679] therm_throt.c: Fix section mismatch

[Satyam Sharma]

On Thu, 23 Aug 2007, Satyam Sharma wrote:
> 
> 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.

Oh, wait. I just stumbled upon arch/i386/kernel/cpu/intel_cacheinfo.c
that references the callback function from generic CPU-initialization
startup code that must get executed even when HOTPLUG_CPU=n. The leave-it-
upto-toolchain method would see this reference, and preserve the function
even after initcalls stage for HOTPLUG_CPU=n. So the explicit __cpuinit
method scores here, admittedly only because of weird taste of said code.