[PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Rakib Mullick]

  Impact: Fix section mismatch

The function bts_trace_init() references a variable
bts_hotcpu_notifier which is marked
as __cpuinitdata. Thus causes section mismatch. This patch fixes it.

  LD      kernel/trace/built-in.o
WARNING: kernel/trace/built-in.o(.text+0xc90c): Section mismatch in
reference from the function bts_trace_init() to the variable
.cpuinit.data:bts_hotcpu_notifier
The function bts_trace_init() references
the variable __cpuinitdata bts_hotcpu_notifier.
This is often because bts_trace_init lacks a __cpuinitdata
annotation or the annotation of bts_hotcpu_notifier is wrong.

WARNING: kernel/trace/built-in.o(.text+0xc92a): Section mismatch in
reference from the function bts_trace_reset() to the variable
.cpuinit.data:bts_hotcpu_notifier
The function bts_trace_reset() references
the variable __cpuinitdata bts_hotcpu_notifier.
This is often because bts_trace_reset lacks a __cpuinitdata
annotation or the annotation of bts_hotcpu_notifier is wrong.

Thanks.

---
Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>

--- linus/kernel/trace/trace_hw_branches.c	2009-02-13 11:23:55.000000000 +0600
+++ rakib/kernel/trace/trace_hw_branches.c	2009-02-13 22:12:30.000000000 +0600
@@ -127,7 +127,7 @@ static struct notifier_block bts_hotcpu_
 	.notifier_call = bts_hotcpu_handler
 };

-static int bts_trace_init(struct trace_array *tr)
+static int __cpuinit bts_trace_init(struct trace_array *tr)
 {
 	hw_branch_trace = tr;

@@ -137,7 +137,7 @@ static int bts_trace_init(struct trace_a
 	return 0;
 }

-static void bts_trace_reset(struct trace_array *tr)
+static void __cpuinit bts_trace_reset(struct trace_array *tr)
 {
 	bts_trace_stop(tr);
 	unregister_hotcpu_notifier(&bts_hotcpu_notifier);

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Ingo Molnar]

* Rakib Mullick <rakib.mullick@gmail.com> wrote:

>   Impact: Fix section mismatch
> 
> The function bts_trace_init() references a variable
> bts_hotcpu_notifier which is marked
> as __cpuinitdata. Thus causes section mismatch. This patch fixes it.
> 
>   LD      kernel/trace/built-in.o
> WARNING: kernel/trace/built-in.o(.text+0xc90c): Section mismatch in
> reference from the function bts_trace_init() to the variable
> .cpuinit.data:bts_hotcpu_notifier
> The function bts_trace_init() references
> the variable __cpuinitdata bts_hotcpu_notifier.
> This is often because bts_trace_init lacks a __cpuinitdata
> annotation or the annotation of bts_hotcpu_notifier is wrong.
> 
> WARNING: kernel/trace/built-in.o(.text+0xc92a): Section mismatch in
> reference from the function bts_trace_reset() to the variable
> .cpuinit.data:bts_hotcpu_notifier
> The function bts_trace_reset() references
> the variable __cpuinitdata bts_hotcpu_notifier.
> This is often because bts_trace_reset lacks a __cpuinitdata
> annotation or the annotation of bts_hotcpu_notifier is wrong.
> 
> Thanks.

Applied to tip:tracing/hw-branch-tracing, thanks Rakib!

	Ingo

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Frederic Weisbecker]

On Sat, Feb 14, 2009 at 09:36:00AM +0600, Rakib Mullick wrote:
>   Impact: Fix section mismatch
> 
> The function bts_trace_init() references a variable
> bts_hotcpu_notifier which is marked
> as __cpuinitdata. Thus causes section mismatch. This patch fixes it.
> 
>   LD      kernel/trace/built-in.o
> WARNING: kernel/trace/built-in.o(.text+0xc90c): Section mismatch in
> reference from the function bts_trace_init() to the variable
> .cpuinit.data:bts_hotcpu_notifier
> The function bts_trace_init() references
> the variable __cpuinitdata bts_hotcpu_notifier.
> This is often because bts_trace_init lacks a __cpuinitdata
> annotation or the annotation of bts_hotcpu_notifier is wrong.
> 
> WARNING: kernel/trace/built-in.o(.text+0xc92a): Section mismatch in
> reference from the function bts_trace_reset() to the variable
> .cpuinit.data:bts_hotcpu_notifier
> The function bts_trace_reset() references
> the variable __cpuinitdata bts_hotcpu_notifier.
> This is often because bts_trace_reset lacks a __cpuinitdata
> annotation or the annotation of bts_hotcpu_notifier is wrong.
> 
> Thanks.
> 
> ---
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> 
> --- linus/kernel/trace/trace_hw_branches.c	2009-02-13 11:23:55.000000000 +0600
> +++ rakib/kernel/trace/trace_hw_branches.c	2009-02-13 22:12:30.000000000 +0600
> @@ -127,7 +127,7 @@ static struct notifier_block bts_hotcpu_
>  	.notifier_call = bts_hotcpu_handler
>  };
> 
> -static int bts_trace_init(struct trace_array *tr)
> +static int __cpuinit bts_trace_init(struct trace_array *tr)
>  {
>  	hw_branch_trace = tr;
> 
> @@ -137,7 +137,7 @@ static int bts_trace_init(struct trace_a
>  	return 0;
>  }
> 
> -static void bts_trace_reset(struct trace_array *tr)
> +static void __cpuinit bts_trace_reset(struct trace_array *tr)
>  {
>  	bts_trace_stop(tr);
>  	unregister_hotcpu_notifier(&bts_hotcpu_notifier);


Hi,

When I saw this patch, I searched the real purpose of __cpuinit and its
real impact.
But I didn't find any comments about it inside the kernel.

But today, by looking at the discussion around latest git pull for x86
to mainline, I discover that __cpuinit becomes __init on UP.

So, unless I missed something, this patch seems to me very dangerous.
The init and reset callbacks of a tracer can be called at any time, not only
on initcalls time (__init functions are freed from memory after the middle stage
of the boot).
With this patch, on UP we will dereference freed memory while activating this tracer.

The old code was fine because register_hotplug_cpu does nothing on UP.
Unfortunately the warning still existed though this was a kind of false positive.
This is a section mismatch, but harmless.

So instead I would suggest to:

- call register_hotcpu_notifier(&bts_hotcpu_notifier) from init_bts_trace() which
  is called only one time on boot.

- never unregister this notifier

- inside bts_hotcpu_handler(), only call bts_trace_{start,stop}_cpu() on the given
  cpu if trace_hw_branches_enabled == 1
  Ok, now the handler will be called on each cpu hotplug event but this is fine since
  this is a rare path.

Hm?


> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Rakib Mullick]

On 2/21/09, Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Hi,
>
>  When I saw this patch, I searched the real purpose of __cpuinit and its
>  real impact.
>  But I didn't find any comments about it inside the kernel.
>
>  But today, by looking at the discussion around latest git pull for x86
>  to mainline, I discover that __cpuinit becomes __init on UP.
>
>  So, unless I missed something, this patch seems to me very dangerous.
>  The init and reset callbacks of a tracer can be called at any time, not only
>  on initcalls time (__init functions are freed from memory after the middle stage
>  of the boot).
>  With this patch, on UP we will dereference freed memory while activating this tracer.
If the init and reset callbacks of a tracer can be called regardless
of cpu hotpluging then it is. If the tracer's init or reset doesn't
rely on cpuhotplug then it shouldn't use it.
There's a another way to fix the warning is by remove __cpuinitdata
from bts_hotcpu_notifier.

Thanks,

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[KOSAKI Motohiro]

> When I saw this patch, I searched the real purpose of __cpuinit and its
> real impact.
> But I didn't find any comments about it inside the kernel.

AFAIK, __cpuinit mean

if UP,                     __cpuinit == __init
if SMP=Y && HOTPLUG_CPU=y, __cpuinit == "" (do nothing)
if SMP=Y && HOTPLUG_CPU=n, __cpuinit == __init

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Frederic Weisbecker]

On Sun, Feb 22, 2009 at 07:33:08PM +0600, Rakib Mullick wrote:
> On 2/21/09, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Hi,
> >
> >  When I saw this patch, I searched the real purpose of __cpuinit and its
> >  real impact.
> >  But I didn't find any comments about it inside the kernel.
> >
> >  But today, by looking at the discussion around latest git pull for x86
> >  to mainline, I discover that __cpuinit becomes __init on UP.
> >
> >  So, unless I missed something, this patch seems to me very dangerous.
> >  The init and reset callbacks of a tracer can be called at any time, not only
> >  on initcalls time (__init functions are freed from memory after the middle stage
> >  of the boot).
> >  With this patch, on UP we will dereference freed memory while activating this tracer.
> If the init and reset callbacks of a tracer can be called regardless
> of cpu hotpluging then it is. If the tracer's init or reset doesn't
> rely on cpuhotplug then it shouldn't use it.
> There's a another way to fix the warning is by remove __cpuinitdata
> from bts_hotcpu_notifier.


Yes, they can be called on UP, on SMP with or without cpu hotplug,
and everytime (boot, runtime).

init() is called when you switch to a tracer:

echo tracer_name > /debug/tracing/current_tracer

and reset() is called when you switch to another one.

But removing __cpuinitdata will mean a kind of waste of memory (though it's only
a little struct).

 
> Thanks,

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Frederic Weisbecker]

On Mon, Feb 23, 2009 at 01:15:08AM +0900, KOSAKI Motohiro wrote:
> > When I saw this patch, I searched the real purpose of __cpuinit and its
> > real impact.
> > But I didn't find any comments about it inside the kernel.
> 
> AFAIK, __cpuinit mean
> 
> if UP,                     __cpuinit == __init
> if SMP=Y && HOTPLUG_CPU=y, __cpuinit == "" (do nothing)
> if SMP=Y && HOTPLUG_CPU=n, __cpuinit == __init
> 
> 
>

Ok, thanks for the information. 

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Sam Ravnborg]

On Mon, Feb 23, 2009 at 01:15:08AM +0900, KOSAKI Motohiro wrote:
> > When I saw this patch, I searched the real purpose of __cpuinit and its
> > real impact.
> > But I didn't find any comments about it inside the kernel.
> 
> AFAIK, __cpuinit mean
> 
> if UP,                     __cpuinit == __init
> if SMP=Y && HOTPLUG_CPU=y, __cpuinit == "" (do nothing)
> if SMP=Y && HOTPLUG_CPU=n, __cpuinit == __init

A function annotated __cpuinit always end up in the same section these
days. And that section are then:

if HOTPLUG_CPU=y, __cpuinit => kept
if HOTPLUG_CPU=n, __cpuinit => discarded

SMP has no influence.

	Sam

Re: [PATCH -mm] tracing: Fix section mismatch in trace_hw_branches.c

[Markus Metzger]

On Sun, Feb 22, 2009 at 5:22 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Sun, Feb 22, 2009 at 07:33:08PM +0600, Rakib Mullick wrote:
>> On 2/21/09, Frederic Weisbecker <fweisbec@gmail.com> wrote:

>> >  So, unless I missed something, this patch seems to me very dangerous.
>> >  The init and reset callbacks of a tracer can be called at any time, not only
>> >  on initcalls time (__init functions are freed from memory after the middle stage
>> >  of the boot).
>> >  With this patch, on UP we will dereference freed memory while activating this tracer.
>> If the init and reset callbacks of a tracer can be called regardless
>> of cpu hotpluging then it is. If the tracer's init or reset doesn't
>> rely on cpuhotplug then it shouldn't use it.
>> There's a another way to fix the warning is by remove __cpuinitdata
>> from bts_hotcpu_notifier.
[...]
> But removing __cpuinitdata will mean a kind of waste of memory (though it's only
> a little struct).

Plus the hotplug handler function. It's still not much.

I'll send out a patch following Frederic's suggestion to register the
hotplug notifier once in init.

thanks and regards,
markus.