2006-01-25 20:03:37

by Ashok Raj

[permalink] [raw]
Subject: wrongly marked __init/__initdata for CPU hotplug

Hi Andrew

attached patch is 2 more cases i found via running the reference_init.pl
script. These were easy to spot just knowing the file names. There is
one another about init/main.c that i cant exactly zero in. (partly
because i dont know how to interpret the data thats spewed out of the tool).

If there is a small example on how to co-related the data and find the culprit
would be real handy.

Maybe Keith could help parse/give an example for me.

PS: There is another tool i noticed from Randy Dunlap that claims is fixed
for 2.6, and it give me couple more pci functions.

I still get the following:

[araj@araj-sfield linux-2.6.16-rc1-mm2]$ perl scripts/reference_init.pl
Finding objects, 1137 objects, ignoring 0 module(s)
Finding conglomerates, ignoring 139 conglomerate(s)
Scanning objects
Error: ./drivers/char/hw_random.o .data refers to 0000000000000028 R_X86_64_64 .init.text
Error: ./drivers/char/hw_random.o .data refers to 0000000000000050 R_X86_64_64 .init.text+0x00000000000000a0
Error: ./drivers/char/hw_random.o .data refers to 0000000000000078 R_X86_64_64 .init.text+0x000000000000017a
Error: ./init/main.o .text refers to 00000000000001dc R_X86_64_PC32 .init.data+0x000000000000015b
Done

--
Cheers,
Ashok Raj
- Open Source Technology Center


data/functions wrongly marked as __init with cpu hotplug.

Signed-off-by: Ashok Raj <[email protected]>
-----------------------------------------------
arch/x86_64/kernel/mce.c | 2 +-
drivers/acpi/processor_idle.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6.16-rc1-mm2/arch/x86_64/kernel/mce.c
===================================================================
--- linux-2.6.16-rc1-mm2.orig/arch/x86_64/kernel/mce.c
+++ linux-2.6.16-rc1-mm2/arch/x86_64/kernel/mce.c
@@ -380,7 +380,7 @@ static void __cpuinit mce_cpu_features(s
*/
void __cpuinit mcheck_init(struct cpuinfo_x86 *c)
{
- static cpumask_t mce_cpus __initdata = CPU_MASK_NONE;
+ static cpumask_t mce_cpus = CPU_MASK_NONE;

mce_cpu_quirks(c);

Index: linux-2.6.16-rc1-mm2/drivers/acpi/processor_idle.c
===================================================================
--- linux-2.6.16-rc1-mm2.orig/drivers/acpi/processor_idle.c
+++ linux-2.6.16-rc1-mm2/drivers/acpi/processor_idle.c
@@ -94,7 +94,7 @@ static int set_max_cstate(struct dmi_sys
return 0;
}

-static struct dmi_system_id __initdata processor_power_dmi_table[] = {
+static struct dmi_system_id __cpuinitdata processor_power_dmi_table[] = {
{ set_max_cstate, "IBM ThinkPad R40e", {
DMI_MATCH(DMI_BIOS_VENDOR,"IBM"),
DMI_MATCH(DMI_BIOS_VERSION,"1SET60WW")}, (void *)1},


2006-01-25 20:13:39

by Andrew Morton

[permalink] [raw]
Subject: Re: wrongly marked __init/__initdata for CPU hotplug

Ashok Raj <[email protected]> wrote:
>
> void __cpuinit mcheck_init(struct cpuinfo_x86 *c)
> {
> - static cpumask_t mce_cpus __initdata = CPU_MASK_NONE;
> + static cpumask_t mce_cpus = CPU_MASK_NONE;

Should that be __cpuinitdata?

2006-01-25 21:48:44

by Ashok Raj

[permalink] [raw]
Subject: Re: wrongly marked __init/__initdata for CPU hotplug

On Wed, Jan 25, 2006 at 12:13:17PM -0800, Andrew Morton wrote:
> Ashok Raj <[email protected]> wrote:
> >
> > void __cpuinit mcheck_init(struct cpuinfo_x86 *c)
> > {
> > - static cpumask_t mce_cpus __initdata = CPU_MASK_NONE;
> > + static cpumask_t mce_cpus = CPU_MASK_NONE;
>
> Should that be __cpuinitdata?

Yes.. i sent an updated one to Andrew, but missed replying to group.

--
Cheers,
Ashok Raj
- Open Source Technology Center

2006-01-26 02:35:43

by Keith Owens

[permalink] [raw]
Subject: Re: wrongly marked __init/__initdata for CPU hotplug

Ashok Raj (on Wed, 25 Jan 2006 12:02:53 -0800) wrote:
>There is
>one another about init/main.c that i cant exactly zero in. (partly
>because i dont know how to interpret the data thats spewed out of the tool).
>
>If there is a small example on how to co-related the data and find the culprit
>would be real handy.
>
>Maybe Keith could help parse/give an example for me.

# reference_init.pl
Error: init/main.o .text refers to 00000000000001dc R_X86_64_PC32 .init.data+0x000000000000015b

Grep for the combination of the offset (1dc) and the target section
type (.init.data), also list the function names.

# objdump -Sr init/main.o | egrep '1dc: .*init.data|<.*>:'
0000000000000000 <rest_init>:
000000000000002a <run_init_process>:
0000000000000044 <init>:
1dc: R_X86_64_PC32 .init.data+0x15b
0000000000000000 <nosmp>:
0000000000000010 <maxcpus>:
000000000000002b <debug_kernel>:
000000000000003f <quiet_kernel>:
0000000000000053 <loglevel>:
000000000000006f <unknown_bootoption>:
0000000000000278 <init_setup>:
000000000000029f <rdinit_setup>:
00000000000002c6 <do_early_param>:
000000000000031d <parse_early_param>:
0000000000000369 <start_kernel>:
000000000000054f <initcall_debug_setup>:

So the reference is coming from the init function.

Next dump the symbols of the .init.data section.

# objdump -d -j .init.data init/main.o | fgrep '>:'

0000000000000000 <__setup_str_initcall_debug_setup>:
000000000000000f <__setup_str_rdinit_setup>:
0000000000000017 <__setup_str_init_setup>:
000000000000001d <__setup_str_loglevel>:
0000000000000027 <__setup_str_quiet_kernel>:
000000000000002d <__setup_str_debug_kernel>:
0000000000000033 <__setup_str_maxcpus>:
000000000000003c <__setup_str_nosmp>:
0000000000000060 <tmp_cmdline.23394>:
0000000000000160 <done.23393>:
0000000000000164 <initcall_debug>:

That would normally list the name of the offending variable, but
sometimes (like now), it does not. There is no symbol listed for
.init.data+15b. Very strange.

Try disassembling the code around the offending reference.

# objdump -Sr init/main.o | grep -B10 -A10 '1dc: .*init.data'
1b3: R_X86_64_PC32 sysctl_init+0xfffffffffffffffc
1b7: 65 48 8b 04 25 10 00 mov %gs:0x10,%rax
1be: 00 00
1c0: 8b a8 44 e0 ff ff mov 0xffffffffffffe044(%rax),%ebp
1c6: 48 c7 c3 00 00 00 00 mov $0x0,%rbx
1c9: R_X86_64_32S __initcall_start
1cd: 48 81 fb 00 00 00 00 cmp $0x0,%rbx
1d0: R_X86_64_32S __initcall_end
1d4: 0f 83 96 00 00 00 jae 270 <init+0x22c>
1da: 83 3d 00 00 00 00 00 cmpl $0x0,0(%rip) # 1e1 <init+0x19d>
1dc: R_X86_64_PC32 .init.data+0x15b <===
1e1: 74 2e je 211 <init+0x1cd>
1e3: 48 8b 33 mov (%rbx),%rsi
1e6: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
1e9: R_X86_64_32S .rodata.str1.1+0x17b
1ed: 31 c0 xor %eax,%eax
1ef: e8 00 00 00 00 callq 1f4 <init+0x1b0>
1f0: R_X86_64_PC32 printk+0xfffffffffffffffc
1f4: 48 8b 33 mov (%rbx),%rsi
1f7: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
1fa: R_X86_64_32S .rodata.str1.1+0x194

So the reference is after the use of __initcall_start/__initcall_end
and before the printk(). Get the cpp generated code.

# make init/main.i

View init/main.i, find __initcall_start/__initcall_end. The only
references are in do_initcalls() which is defined as .init.text. But
the objdump that listed function names showed the reference was from
init(), not from do_initcalls().

This is nasty. init() calls do_basic_setup() which calls
do_initcalls(). init is normal text. do_basic_setup and do_initcalls
are .init.text. gcc has inlined do_basic_setup and do_initcalls into
init, even though they have different section attributes. Naughty gcc.

This was using GCC: (GNU) 4.0.2 20050901 (prerelease) (SUSE Linux).
Log a gcc bug. Not a good omen for the idea of letting gcc decide when
to inline!

Looking at the C code for do_initcalls(), the reference is obviously to
initcall_debug. I am puzzled about why the objdump lists
.init.data+0x15b when initcall_debug is really at .init.data+0x164.

BTW, does anybody know why init() is not defined as __init?

2006-01-26 03:36:05

by Ashok Raj

[permalink] [raw]
Subject: Re: wrongly marked __init/__initdata for CPU hotplug

On Thu, Jan 26, 2006 at 01:35:17PM +1100, Keith Owens wrote:
> Ashok Raj (on Wed, 25 Jan 2006 12:02:53 -0800) wrote:
> >There is
> >one another about init/main.c that i cant exactly zero in. (partly
> >because i dont know how to interpret the data thats spewed out of the tool).
> >
> >If there is a small example on how to co-related the data and find the culprit
> >would be real handy.
> >
> >Maybe Keith could help parse/give an example for me.
>
> # reference_init.pl
> Error: init/main.o .text refers to 00000000000001dc R_X86_64_PC32 .init.data+0x000000000000015b
>

Awesome annotation... thanks very much keith...

2006-01-26 04:26:12

by Andi Kleen

[permalink] [raw]
Subject: Re: wrongly marked __init/__initdata for CPU hotplug

On Wed, Jan 25, 2006 at 12:02:53PM -0800, Ashok Raj wrote:
> Hi Andrew
>
> attached patch is 2 more cases i found via running the reference_init.pl
> script. These were easy to spot just knowing the file names. There is
> one another about init/main.c that i cant exactly zero in. (partly
> because i dont know how to interpret the data thats spewed out of the tool).

I think that's the reference to __initcall_start / end
That one is unavoidable, but not a bug.

-Andi

2006-01-26 04:28:50

by Andi Kleen

[permalink] [raw]
Subject: Re: wrongly marked __init/__initdata for CPU hotplug

> This is nasty. init() calls do_basic_setup() which calls
> do_initcalls(). init is normal text. do_basic_setup and do_initcalls
> are .init.text. gcc has inlined do_basic_setup and do_initcalls into
> init, even though they have different section attributes. Naughty gcc.
>
> This was using GCC: (GNU) 4.0.2 20050901 (prerelease) (SUSE Linux).
> Log a gcc bug. Not a good omen for the idea of letting gcc decide when
> to inline!

Someone should file a bug in the gcc bugzilla then I guess.
Can you do that or should I?

>
> Looking at the C code for do_initcalls(), the reference is obviously to
> initcall_debug. I am puzzled about why the objdump lists
> .init.data+0x15b when initcall_debug is really at .init.data+0x164.

Ah thanks - ok i mislooked then. Anyways, it's not a bug.

> BTW, does anybody know why init() is not defined as __init?

Because it would crash then after returning from free_initmem.

-Andi

2006-01-27 01:36:39

by Randy Dunlap

[permalink] [raw]
Subject: Re: wrongly marked __init/__initdata for CPU hotplug

On 26 Jan 2006 05:26:03 +0100 Andi Kleen wrote:

> On Wed, Jan 25, 2006 at 12:02:53PM -0800, Ashok Raj wrote:
> > Hi Andrew
> >
> > attached patch is 2 more cases i found via running the reference_init.pl
> > script. These were easy to spot just knowing the file names. There is
> > one another about init/main.c that i cant exactly zero in. (partly
> > because i dont know how to interpret the data thats spewed out of the tool).
>
> I think that's the reference to __initcall_start / end
> That one is unavoidable, but not a bug.

Yes, that's the conclusion that I came to also.

---
~Randy