Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Crash Utility (http://www.linux-archive.org/crash-utility/)
-   -   To support module percpu symbol (http://www.linux-archive.org/crash-utility/449358-support-module-percpu-symbol.html)

Toshikazu Nakayama 11-08-2010 03:48 AM

To support module percpu symbol
 
Hi,

The crash utility could not get module percpu symbols.
Since percpu symbols are not included in kallsyms nor module_core area,
they were not found out in module_init().

However, symbols are existing in module object file
.data..percpu (legacy kernel revision .data.percpu) section,
load_module_symbols() can find and resolve them.
(percpu symbols exist in mod_load_symtable, not in mod_ext_symtable)

'p' command is also updated to be made redundant legacy per_cpu__ prefix.

Note:
I have not tested these paches enough over any arch or kernel revisions,
also any distros.
Just have been tested x86 (64bit) with linux-2.6.10 and linux-2.6.35.


This is my first post to crash utility.
I don't understand well the manner of this mailing list.
(I'm sorry if there's anything wrong)

Thanks,
Toshi.
-----------------------------------------------------------------------------
CHANGES: module percpu symbols are resolved while module object loading.
cmd_p() can read percpu syombols without prefix in legacy environs.

SRPM: crash-5.0.9-0.src.rpm

TEST#1: Result on linux-2.6.10 x86(64) SMP(4 CPUs) * with test module

[before]

crash> sym -m test | grep abc
crash> p abc
p: gdb request failed: p abc

[after]

crash> sym -m test | grep abc
ffffffff80871928 (d) per_cpu__abc
crash> sym abc
symbol not found: abc
possible alternatives:
ffffffff80871928 (d) per_cpu__abc
crash> p abc
PER-CPU DATA TYPE:
int per_cpu__abc;
PER-CPU ADDRESSES:
[0]: 10008af0888
[1]: 10008b00888
[2]: 10008b10888
[3]: 10008b20888

TEST#2: Result on linux-2.6.35 x86(64) SMP (8 CPUs) * with KVM modules

crash> mod | grep kvm
ffffffffa0035c40 kvm_intel 39368 /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm-intel.ko
ffffffffa008dba0 kvm 175160 /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm.ko

[before]

crash> sym -m kvm_intel | grep vcpus_on_cpu
crash> p vcpus_on_cpu
p: gdb request failed: p vcpus_on_cpu

[after]

crash> sym -m kvm_intel | grep vcpus_on_cpu
12a40 (d) vcpus_on_cpu
crash> p vcpus_on_cpu
PER-CPU DATA TYPE:
struct list_head vcpus_on_cpu;
PER-CPU ADDRESSES:
[0]: ffff880001812a40
[1]: ffff880001852a40
[2]: ffff880001892a40
[3]: ffff8800018d2a40
[4]: ffff880001912a40
[5]: ffff880001952a40
[6]: ffff880001992a40
[7]: ffff8800019d2a40

Toshikazu Nakayama (4):
Add percpu member.
module's percpu basic procedure.
expand percpu area.
update cmd_p().

defs.h | 2 ++
kernel.c | 1 +
symbols.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 48 insertions(+), 5 deletions(-)

--
1.7.3.2.161.g3089c

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Dave Anderson 11-08-2010 04:14 PM

To support module percpu symbol
 
----- "Toshikazu Nakayama" <nakayama.ts@ncos.nec.co.jp> wrote:

> Hi,
>
> The crash utility could not get module percpu symbols.
> Since percpu symbols are not included in kallsyms nor module_core area,
> they were not found out in module_init().
>
> However, symbols are existing in module object file
> .data..percpu (legacy kernel revision .data.percpu) section,
> load_module_symbols() can find and resolve them.
> (percpu symbols exist in mod_load_symtable, not in mod_ext_symtable)
>
> 'p' command is also updated to be made redundant legacy per_cpu__
> prefix.
>
> Note:
> I have not tested these paches enough over any arch or kernel revisions,
> also any distros.
> Just have been tested x86 (64bit) with linux-2.6.10 and linux-2.6.35.
>
> This is my first post to crash utility.
> I don't understand well the manner of this mailing list.
> (I'm sorry if there's anything wrong)
>
> Thanks,
> Toshi.

You've come to the right place... ;-)

And your patch submission is fine, although I do prefer that you send the
the patches as email attachments instead of inline. (Although that's my
problem because of the Zimbra email client that I use)

I tested your patch with two kernel types, one of them the "legacy" version
with "pcpu_num_used" and "pcpu_size" symbols (RHEL5), and the second one with
current kernels with the "pcpu_reserved_chunk_limit" symbol (RHEL6 and 2.6.37-rc1).

Here's what I see, first without, and then with your patch applied.

(1) Testing with a 2.6.18-based RHEL5 kernel:

Without your patch, the stored __per_cpu_start and __per_cpu_end
values are as shown here in the symbol_table_data structure:

crash> help -s
...
__per_cpu_start: ffffffff80419000
__per_cpu_end: ffffffff8041f508
...

which reflect the beginning and end of the __per_cpu kernel
symbol range:

crash> sym -l
...
ffffffff80419000 (A) __per_cpu_start
ffffffff80419000 (D) per_cpu__init_tss
ffffffff8041b080 (D) per_cpu__orig_ist
ffffffff8041b100 (d) per_cpu__idle_state
...
ffffffff8041f480 (d) per_cpu__flow_flush_tasklets
ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
ffffffff8041f500 (d) per_cpu____icmp_socket
ffffffff8041f508 (A) __per_cpu_end

With your patch applied, the __per_cpu_end gets incremented based
upon the "pcpu_num_used" and "pcpu_size" contents:

crash> help -s
...
__per_cpu_start: ffffffff80419000
__per_cpu_end: ffffffff80427a10
...

But that doesn't seem right, because the resultant value
pushes it into the bss symbol range:

crash> sym ffffffff80427a10
ffffffff80427a10 (B) boot_exception_stacks+6672
crash>

crash> sym -l
...
ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
ffffffff8041f500 (d) per_cpu____icmp_socket
ffffffff8041f508 (A) __per_cpu_end <-- original value
ffffffff80420000 (d) .data_nosave
ffffffff80420000 (A) __nosave_begin
ffffffff80420000 (D) in_suspend
ffffffff80421000 (b) .bss
ffffffff80421000 (A) __bss_start
ffffffff80421000 (A) __nosave_end
ffffffff80421000 (B) empty_zero_page
ffffffff80422000 (B) boot_cpu_stack
ffffffff80426000 (B) boot_exception_stacks <-- bumped to ffffffff80427a10
ffffffff8042c000 (B) idt_table
ffffffff8042d000 (B) boot_delay
ffffffff8042d008 (B) printk_delay_msec
...

(2) Testing with a 2.6.32-based RHEL6 kernel:

Without your patch, the stored __per_cpu_start and __per_cpu_end
values are as shown here in the symbol_table_data structure:

crash> help -s
...
__per_cpu_start: 0
__per_cpu_end: 17358
...

which reflect the beginning and end of the kernel's __per_cpu
symbol range:

crash> sym -l
0 (D) __per_cpu_start
0 (V) per_cpu__irq_stack_union
4000 (V) per_cpu__gdt_page
5000 (V) per_cpu__exception_stacks
...
16940 (V) per_cpu__cpu_tlbstate
16980 (V) per_cpu__runqueues
17340 (V) per_cpu__sched_clock_data
17358 (D) __per_cpu_end
ffffffff81000000 (T) _text
...

With your patch applied, the st->__per_cpu_end value gets changed
like so:

crash> help -s
...
__per_cpu_start: 0
__per_cpu_end: ffffffff81bf36b8
...

But that doesn't seem right, where ffffffff81bf36b8 is the
"pcpu_reserved_chunk_limit" symbol value:

crash> sym ffffffff81bf36b8
ffffffff81bf36b8 (b) pcpu_reserved_chunk_limit
crash>

I'm guessing that you want it st->__per_cpu_end to be bumped from
0x17358 to 0x19358:

crash> px pcpu_reserved_chunk_limit
pcpu_reserved_chunk_limit = $4 = 0x19358
crash>

(3) Testing with a generic upstream 2.6.37-rc1 kernel, I see the same
behaviour as with the RHEL6 kernel.

In any case, there's no way I can change the st->__per_cpu_end values
as you propose -- even if it somehow works with your testing.

Comments?

Dave


> -----------------------------------------------------------------------------
> CHANGES: module percpu symbols are resolved while module object loading.
> cmd_p() can read percpu syombols without prefix in legacy environs.
>
> SRPM: crash-5.0.9-0.src.rpm
>
> TEST#1: Result on linux-2.6.10 x86(64) SMP(4 CPUs) * with test module
>
> [before]
>
> crash> sym -m test | grep abc
> crash> p abc
> p: gdb request failed: p abc
>
> [after]
>
> crash> sym -m test | grep abc
> ffffffff80871928 (d) per_cpu__abc
> crash> sym abc
> symbol not found: abc
> possible alternatives:
> ffffffff80871928 (d) per_cpu__abc
> crash> p abc
> PER-CPU DATA TYPE:
> int per_cpu__abc;
> PER-CPU ADDRESSES:
> [0]: 10008af0888
> [1]: 10008b00888
> [2]: 10008b10888
> [3]: 10008b20888
>
> TEST#2: Result on linux-2.6.35 x86(64) SMP (8 CPUs) * with KVM
> modules
>
> crash> mod | grep kvm
> ffffffffa0035c40 kvm_intel 39368
> /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm-intel.ko
> ffffffffa008dba0 kvm 175160
> /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm.ko
>
> [before]
>
> crash> sym -m kvm_intel | grep vcpus_on_cpu
> crash> p vcpus_on_cpu
> p: gdb request failed: p vcpus_on_cpu
>
> [after]
>
> crash> sym -m kvm_intel | grep vcpus_on_cpu
> 12a40 (d) vcpus_on_cpu
> crash> p vcpus_on_cpu
> PER-CPU DATA TYPE:
> struct list_head vcpus_on_cpu;
> PER-CPU ADDRESSES:
> [0]: ffff880001812a40
> [1]: ffff880001852a40
> [2]: ffff880001892a40
> [3]: ffff8800018d2a40
> [4]: ffff880001912a40
> [5]: ffff880001952a40
> [6]: ffff880001992a40
> [7]: ffff8800019d2a40
>
> Toshikazu Nakayama (4):
> Add percpu member.
> module's percpu basic procedure.
> expand percpu area.
> update cmd_p().
>
> defs.h | 2 ++
> kernel.c | 1 +
> symbols.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 48 insertions(+), 5 deletions(-)
>
> --
> 1.7.3.2.161.g3089c

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Toshikazu Nakayama 11-09-2010 06:51 AM

To support module percpu symbol
 
Hi Dave,

Thank you for welcome, review and testing.

>You've come to the right place... ;-)
>
>And your patch submission is fine, although I do prefer that you send the
>the patches as email attachments instead of inline. (Although that's my
>problem because of the Zimbra email client that I use)

Ok, I attach V2 patches. [PATCH 3/4] is updated.

>I tested your patch with two kernel types, one of them the "legacy" version
>with "pcpu_num_used" and "pcpu_size" symbols (RHEL5), and the second one with
>current kernels with the "pcpu_reserved_chunk_limit" symbol (RHEL6 and 2.6.37-rc1).
>
>Here's what I see, first without, and then with your patch applied.

>In any case, there's no way I can change the st->__per_cpu_end values
>as you propose -- even if it somehow works with your testing.
>
>Comments?

I did some wrong changes and would understand your mentions.

Updating __per_cpu_end itself makes original UI's corruption.
And for sure, I want __per_cpu_end to be bumped with your guess.

I fixed these bugs like attachments and tested again,
of course checked st->__per_cpu_end value advised from you.

One more update is to add is_per_cpu_range().
Because there are some conditions about percpu range in symbols.c,
I think it is better to use common function than inline.
The function returns 0 (not percpu), 1 (kernels) and 2 (modules)
although callers do not distinguish between 1 and 2 now.

Thanks,
Toshi

>(1) Testing with a 2.6.18-based RHEL5 kernel:
>
> Without your patch, the stored __per_cpu_start and __per_cpu_end
> values are as shown here in the symbol_table_data structure:
>
> crash> help -s
> ...
> __per_cpu_start: ffffffff80419000
> __per_cpu_end: ffffffff8041f508
> ...
>
> which reflect the beginning and end of the __per_cpu kernel
> symbol range:
>
> crash> sym -l
> ...
> ffffffff80419000 (A) __per_cpu_start
> ffffffff80419000 (D) per_cpu__init_tss
> ffffffff8041b080 (D) per_cpu__orig_ist
> ffffffff8041b100 (d) per_cpu__idle_state
> ...
> ffffffff8041f480 (d) per_cpu__flow_flush_tasklets
> ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
> ffffffff8041f500 (d) per_cpu____icmp_socket
> ffffffff8041f508 (A) __per_cpu_end
>
> With your patch applied, the __per_cpu_end gets incremented based
> upon the "pcpu_num_used" and "pcpu_size" contents:
>
> crash> help -s
> ...
> __per_cpu_start: ffffffff80419000
> __per_cpu_end: ffffffff80427a10
> ...
>
> But that doesn't seem right, because the resultant value
> pushes it into the bss symbol range:
>
> crash> sym ffffffff80427a10
> ffffffff80427a10 (B) boot_exception_stacks+6672
> crash>
>
> crash> sym -l
> ...
> ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
> ffffffff8041f500 (d) per_cpu____icmp_socket
> ffffffff8041f508 (A) __per_cpu_end <-- original value
> ffffffff80420000 (d) .data_nosave
> ffffffff80420000 (A) __nosave_begin
> ffffffff80420000 (D) in_suspend
> ffffffff80421000 (b) .bss
> ffffffff80421000 (A) __bss_start
> ffffffff80421000 (A) __nosave_end
> ffffffff80421000 (B) empty_zero_page
> ffffffff80422000 (B) boot_cpu_stack
> ffffffff80426000 (B) boot_exception_stacks <-- bumped to ffffffff80427a10
> ffffffff8042c000 (B) idt_table
> ffffffff8042d000 (B) boot_delay
> ffffffff8042d008 (B) printk_delay_msec
> ...
>
>(2) Testing with a 2.6.32-based RHEL6 kernel:
>
> Without your patch, the stored __per_cpu_start and __per_cpu_end
> values are as shown here in the symbol_table_data structure:
>
> crash> help -s
> ...
> __per_cpu_start: 0
> __per_cpu_end: 17358
> ...
>
> which reflect the beginning and end of the kernel's __per_cpu
> symbol range:
>
> crash> sym -l
> 0 (D) __per_cpu_start
> 0 (V) per_cpu__irq_stack_union
> 4000 (V) per_cpu__gdt_page
> 5000 (V) per_cpu__exception_stacks
> ...
> 16940 (V) per_cpu__cpu_tlbstate
> 16980 (V) per_cpu__runqueues
> 17340 (V) per_cpu__sched_clock_data
> 17358 (D) __per_cpu_end
> ffffffff81000000 (T) _text
> ...
>
> With your patch applied, the st->__per_cpu_end value gets changed
> like so:
>
> crash> help -s
> ...
> __per_cpu_start: 0
> __per_cpu_end: ffffffff81bf36b8
> ...
>
> But that doesn't seem right, where ffffffff81bf36b8 is the
> "pcpu_reserved_chunk_limit" symbol value:
>
> crash> sym ffffffff81bf36b8
> ffffffff81bf36b8 (b) pcpu_reserved_chunk_limit
> crash>
>
> I'm guessing that you want it st->__per_cpu_end to be bumped from
> 0x17358 to 0x19358:
>
> crash> px pcpu_reserved_chunk_limit
> pcpu_reserved_chunk_limit = $4 = 0x19358
> crash>
>
>(3) Testing with a generic upstream 2.6.37-rc1 kernel, I see the same
> behaviour as with the RHEL6 kernel.
>
>In any case, there's no way I can change the st->__per_cpu_end values
>as you propose -- even if it somehow works with your testing.
>
>Comments?
>
>Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Toshikazu Nakayama 11-09-2010 07:08 AM

To support module percpu symbol
 
I am aware my mistake over previous patch (when check Archive).

+ st->percpu_mod_used += block_size;

This += is bad, = is right.
Please switch patch.

Toshi.

>Hi Dave,
>
>Thank you for welcome, review and testing.
>
>>You've come to the right place... ;-)
>>
>>And your patch submission is fine, although I do prefer that you send the
>>the patches as email attachments instead of inline. (Although that's my
>>problem because of the Zimbra email client that I use)
>
>Ok, I attach V2 patches. [PATCH 3/4] is updated.
>
>>I tested your patch with two kernel types, one of them the "legacy" version
>>with "pcpu_num_used" and "pcpu_size" symbols (RHEL5), and the second one with
>>current kernels with the "pcpu_reserved_chunk_limit" symbol (RHEL6 and 2.6.37-rc1).
>>
>>Here's what I see, first without, and then with your patch applied.
>
>>In any case, there's no way I can change the st->__per_cpu_end values
>>as you propose -- even if it somehow works with your testing.
>>
>>Comments?
>
>I did some wrong changes and would understand your mentions.
>
>Updating __per_cpu_end itself makes original UI's corruption.
>And for sure, I want __per_cpu_end to be bumped with your guess.
>
>I fixed these bugs like attachments and tested again,
>of course checked st->__per_cpu_end value advised from you.
>
>One more update is to add is_per_cpu_range().
>Because there are some conditions about percpu range in symbols.c,
>I think it is better to use common function than inline.
>The function returns 0 (not percpu), 1 (kernels) and 2 (modules)
>although callers do not distinguish between 1 and 2 now.
>
>Thanks,
>Toshi
>
>>(1) Testing with a 2.6.18-based RHEL5 kernel:
>>
>> Without your patch, the stored __per_cpu_start and __per_cpu_end
>> values are as shown here in the symbol_table_data structure:
>>
>> crash> help -s
>> ...
>> __per_cpu_start: ffffffff80419000
>> __per_cpu_end: ffffffff8041f508
>> ...
>>
>> which reflect the beginning and end of the __per_cpu kernel
>> symbol range:
>>
>> crash> sym -l
>> ...
>> ffffffff80419000 (A) __per_cpu_start
>> ffffffff80419000 (D) per_cpu__init_tss
>> ffffffff8041b080 (D) per_cpu__orig_ist
>> ffffffff8041b100 (d) per_cpu__idle_state
>> ...
>> ffffffff8041f480 (d) per_cpu__flow_flush_tasklets
>> ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
>> ffffffff8041f500 (d) per_cpu____icmp_socket
>> ffffffff8041f508 (A) __per_cpu_end
>>
>> With your patch applied, the __per_cpu_end gets incremented based
>> upon the "pcpu_num_used" and "pcpu_size" contents:
>>
>> crash> help -s
>> ...
>> __per_cpu_start: ffffffff80419000
>> __per_cpu_end: ffffffff80427a10
>> ...
>>
>> But that doesn't seem right, because the resultant value
>> pushes it into the bss symbol range:
>>
>> crash> sym ffffffff80427a10
>> ffffffff80427a10 (B) boot_exception_stacks+6672
>> crash>
>>
>> crash> sym -l
>> ...
>> ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
>> ffffffff8041f500 (d) per_cpu____icmp_socket
>> ffffffff8041f508 (A) __per_cpu_end <-- original value
>> ffffffff80420000 (d) .data_nosave
>> ffffffff80420000 (A) __nosave_begin
>> ffffffff80420000 (D) in_suspend
>> ffffffff80421000 (b) .bss
>> ffffffff80421000 (A) __bss_start
>> ffffffff80421000 (A) __nosave_end
>> ffffffff80421000 (B) empty_zero_page
>> ffffffff80422000 (B) boot_cpu_stack
>> ffffffff80426000 (B) boot_exception_stacks <-- bumped to ffffffff80427a10
>> ffffffff8042c000 (B) idt_table
>> ffffffff8042d000 (B) boot_delay
>> ffffffff8042d008 (B) printk_delay_msec
>> ...
>>
>>(2) Testing with a 2.6.32-based RHEL6 kernel:
>>
>> Without your patch, the stored __per_cpu_start and __per_cpu_end
>> values are as shown here in the symbol_table_data structure:
>>
>> crash> help -s
>> ...
>> __per_cpu_start: 0
>> __per_cpu_end: 17358
>> ...
>>
>> which reflect the beginning and end of the kernel's __per_cpu
>> symbol range:
>>
>> crash> sym -l
>> 0 (D) __per_cpu_start
>> 0 (V) per_cpu__irq_stack_union
>> 4000 (V) per_cpu__gdt_page
>> 5000 (V) per_cpu__exception_stacks
>> ...
>> 16940 (V) per_cpu__cpu_tlbstate
>> 16980 (V) per_cpu__runqueues
>> 17340 (V) per_cpu__sched_clock_data
>> 17358 (D) __per_cpu_end
>> ffffffff81000000 (T) _text
>> ...
>>
>> With your patch applied, the st->__per_cpu_end value gets changed
>> like so:
>>
>> crash> help -s
>> ...
>> __per_cpu_start: 0
>> __per_cpu_end: ffffffff81bf36b8
>> ...
>>
>> But that doesn't seem right, where ffffffff81bf36b8 is the
>> "pcpu_reserved_chunk_limit" symbol value:
>>
>> crash> sym ffffffff81bf36b8
>> ffffffff81bf36b8 (b) pcpu_reserved_chunk_limit
>> crash>
>>
>> I'm guessing that you want it st->__per_cpu_end to be bumped from
>> 0x17358 to 0x19358:
>>
>> crash> px pcpu_reserved_chunk_limit
>> pcpu_reserved_chunk_limit = $4 = 0x19358
>> crash>
>>
>>(3) Testing with a generic upstream 2.6.37-rc1 kernel, I see the same
>> behaviour as with the RHEL6 kernel.
>>
>>In any case, there's no way I can change the st->__per_cpu_end values
>>as you propose -- even if it somehow works with your testing.
>>
>>Comments?
>>
>>Dave
>
>
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Dave Anderson 11-09-2010 07:29 PM

To support module percpu symbol
 
----- "Toshikazu Nakayama" <nakayama.ts@ncos.nec.co.jp> wrote:

> Hi Dave,
>
> I did some wrong changes and would understand your mentions.
>
> Updating __per_cpu_end itself makes original UI's corruption.
> And for sure, I want __per_cpu_end to be bumped with your guess.
>
> I fixed these bugs like attachments and tested again,
> of course checked st->__per_cpu_end value advised from you.
>
> One more update is to add is_per_cpu_range().
> Because there are some conditions about percpu range in symbols.c,
> I think it is better to use common function than inline.
> The function returns 0 (not percpu), 1 (kernels) and 2 (modules)
> although callers do not distinguish between 1 and 2 now.


Hello Toshi,

Unfortunately there are still problems with this patch design,
both with the "legacy" and "current" kernel types.

I modified your patch to display the new items added to the
structures in defs.h. For example, the new "st->percpu_mod_used"
value should be shown in dump_symbol_table(), along with the
effective (extended) end of percpu space that is used by your
is_per_cpu_range() function:

crash> help -s
...
__per_cpu_start: ffffffff80419000
__per_cpu_end: ffffffff8041f508
percpu_mod_used: 8508 (extended percpu end: ffffffff80427a10)
...

Anyway, as I mentioned in my review of your first patch, support
for the "legacy" module percpu symbols is not acceptable. In
the example above, note that the end of per-cpu space gets extended
from ffffffff8041f508 to ffffffff80427a10. But that value pushes
into legitimate symbol virtual address space above "__per_cpu_end":

crash> sym -l
...
ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
ffffffff8041f500 (d) per_cpu____icmp_socket
ffffffff8041f508 (A) __per_cpu_end <-- original value
ffffffff80420000 (d) .data_nosave
ffffffff80420000 (A) __nosave_begin
ffffffff80420000 (D) in_suspend
ffffffff80421000 (b) .bss
ffffffff80421000 (A) __bss_start
ffffffff80421000 (A) __nosave_end
ffffffff80421000 (B) empty_zero_page
ffffffff80422000 (B) boot_cpu_stack
ffffffff80426000 (B) boot_exception_stacks <-- bumped to ffffffff80427a10
ffffffff8042c000 (B) idt_table
ffffffff8042d000 (B) boot_delay
ffffffff8042d008 (B) printk_delay_msec
...

So, for example, take the "in_suspend" variable above, which is a global
integer variable at address ffffffff80420000:

crash> whatis in_suspend
int in_suspend;
crash>

Without your patch, it gets printed correctly like this:

crash> p in_suspend
in_suspend = $2 = 0
crash>

But with your patch, is_per_cpu_range() thinks that ffffffff80420000 is
a per-cpu symbol value because it is located within the extended range.
So the command incorrectly does this:

crash> p in_suspend
PER-CPU DATA TYPE:
int in_suspend;
PER-CPU ADDRESSES:
[0]: ffff81000157a000
[1]: ffff810001582580
[2]: ffff81000158ab00
[3]: ffff810001593080
[4]: ffff81000159b600
[5]: ffff8100015a3b80
[6]: ffff8100015ac100
[7]: ffff8100015b4680
crash>

I also have my doubts about the current kernels which have the single
"pcpu_reserved_chunk_limit" symbol, where its contents are simply added
to the base kernel's "__per_cpu_end" value. It seemingly would work OK
with x86_64, because the symbols are very low offset values, and cannot
be misconstrued for regular symbol address values:

crash> sym -l
0 (D) __per_cpu_start
0 (V) irq_stack_union
4000 (V) gdt_page
5000 (V) exception_stacks
b000 (V) tlb_vector_offset
...
15740 (V) call_single_queue
15780 (V) csd_data
157c0 (V) softnet_data
15900 (D) __per_cpu_end
ffffffff81000000 (T) _text
ffffffff81000000 (T) startup_64
ffffffff810000b7 (t) ident_complete
ffffffff81000100 (T) secondary_startup_64
...

But, taking a 32-bit x86 2.6.34-rc5 kernel as an example, the per-cpu symbols
are as shown below:

crash> sym -l
...
c0a90000 (D) __per_cpu_start <==
c0a90000 (V) gdt_page
c0a91000 (V) xen_vcpu
c0a91020 (V) xen_vcpu_info
c0a91060 (V) idt_desc
...
c0a995c0 (V) cfd_data
c0a99600 (V) call_single_queue
c0a99640 (V) csd_data
c0a99654 (D) __per_cpu_end <==
c0a9a000 (r) .smp_locks
c0a9a000 (D) __init_end
c0a9a000 (R) __smp_locks
c0aa1000 (b) .bss
c0aa1000 (B) __bss_start
c0aa1000 (R) __smp_locks_end
c0aa1000 (b) swapper_pg_pmd
c0aa2000 (b) swapper_pg_fixmap
c0aa3000 (B) empty_zero_page
c0aa4000 (b) level1_ident_pgt
...

So similar to the case of the "legacy" x86_64 example, if the
"pcpu_reserved_chunk_limit" value is simply added to the value
of the base kernel's "__per_cpu_end" value of c0a99654, it will
extend into (and overlap) legitimate base kernel virtual address
space.

And I haven't even bothered to look into how it would affect
the operation of all of the other architectures...

Perhaps the patch can be re-done so that the code can handle it on a
per-module basis without inadvertently affecting everything else?

In any case, there's no way I can accept the patch as it is
currently designed.

Thanks,
Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Toshikazu Nakayama 11-10-2010 08:39 AM

To support module percpu symbol
 
Hi Dave,

Thanks a lot, I caught several issues about current patches.

>> I did some wrong changes and would understand your mentions.
>>
>> Updating __per_cpu_end itself makes original UI's corruption.
>> And for sure, I want __per_cpu_end to be bumped with your guess.
>>
>> I fixed these bugs like attachments and tested again,
>> of course checked st->__per_cpu_end value advised from you.
>>
>> One more update is to add is_per_cpu_range().
>> Because there are some conditions about percpu range in symbols.c,
>> I think it is better to use common function than inline.
>> The function returns 0 (not percpu), 1 (kernels) and 2 (modules)
>> although callers do not distinguish between 1 and 2 now.
>
>
>Hello Toshi,
>
>Unfortunately there are still problems with this patch design,
>both with the "legacy" and "current" kernel types.

>So similar to the case of the "legacy" x86_64 example, if the
>"pcpu_reserved_chunk_limit" value is simply added to the value
>of the base kernel's "__per_cpu_end" value of c0a99654, it will
>extend into (and overlap) legitimate base kernel virtual address
>space.
>
>And I haven't even bothered to look into how it would affect
>the operation of all of the other architectures...
>
>Perhaps the patch can be re-done so that the code can handle it on a
>per-module basis without inadvertently affecting everything else?
>
>In any case, there's no way I can accept the patch as it is
>currently designed.

I handled module symbol in kernel scope (target is not System.map).

In kernel, percpu data can't access directly with mapped virtual address.
Related kernel macros are sometimes arch depends or looks compiler depend.
(I can not understand enough...)
Since percpu data area is entirely dynamic allocation,
additionally some arch locate part of them at VMALLOC area.
They are not straight-mapped so some arch are probably overlapped.

Anyway with previous approach,
there is no possible solution in my idea at least,
but I might get really right way from your proposal.

Retake attached patches with per module space.

I use module.percpu which guarantee straight-mapping in every module space.
Also its size get from bfd_section_size() for legacy percpu implementation
although latest kernel gives percpu_size.
- I am sorry but I don't have i386 target environs now, not tested about i386.
(I think that module.percpu can work well even if it is VMALLOC area.)

But there was still more problem at gdb interface.

When p &(module percpu) is not resolved by syment,
gdb_pass_through() require pre-registration about module sections.
The result gave wrong symbol value of module percpu.

I append percpu section in gdb request from add_symbol_file_kallsyms()
although percpu is not in kallsyms...
Because multiple GNU_ADD_SYMBOL_FILE requests for one module did not work well.
I am not very well around these implementations.
Can I resolve it with other better way?

Thanks,
Toshi.

>I modified your patch to display the new items added to the
>structures in defs.h. For example, the new "st->percpu_mod_used"
>value should be shown in dump_symbol_table(), along with the
>effective (extended) end of percpu space that is used by your
>is_per_cpu_range() function:
>
> crash> help -s
> ...
> __per_cpu_start: ffffffff80419000
> __per_cpu_end: ffffffff8041f508
> percpu_mod_used: 8508 (extended percpu end: ffffffff80427a10)
> ...
>
>Anyway, as I mentioned in my review of your first patch, support
>for the "legacy" module percpu symbols is not acceptable. In
>the example above, note that the end of per-cpu space gets extended
>from ffffffff8041f508 to ffffffff80427a10. But that value pushes
>into legitimate symbol virtual address space above "__per_cpu_end":
>
> crash> sym -l
> ...
> ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
> ffffffff8041f500 (d) per_cpu____icmp_socket
> ffffffff8041f508 (A) __per_cpu_end <-- original value
> ffffffff80420000 (d) .data_nosave
> ffffffff80420000 (A) __nosave_begin
> ffffffff80420000 (D) in_suspend
> ffffffff80421000 (b) .bss
> ffffffff80421000 (A) __bss_start
> ffffffff80421000 (A) __nosave_end
> ffffffff80421000 (B) empty_zero_page
> ffffffff80422000 (B) boot_cpu_stack
> ffffffff80426000 (B) boot_exception_stacks <-- bumped to ffffffff80427a10
> ffffffff8042c000 (B) idt_table
> ffffffff8042d000 (B) boot_delay
> ffffffff8042d008 (B) printk_delay_msec
> ...
>
>So, for example, take the "in_suspend" variable above, which is a global
>integer variable at address ffffffff80420000:
>
> crash> whatis in_suspend
> int in_suspend;
> crash>
>
>Without your patch, it gets printed correctly like this:
>
> crash> p in_suspend
> in_suspend = $2 = 0
> crash>
>
>But with your patch, is_per_cpu_range() thinks that ffffffff80420000 is
>a per-cpu symbol value because it is located within the extended range.
>So the command incorrectly does this:
>
> crash> p in_suspend
> PER-CPU DATA TYPE:
> int in_suspend;
> PER-CPU ADDRESSES:
> [0]: ffff81000157a000
> [1]: ffff810001582580
> [2]: ffff81000158ab00
> [3]: ffff810001593080
> [4]: ffff81000159b600
> [5]: ffff8100015a3b80
> [6]: ffff8100015ac100
> [7]: ffff8100015b4680
> crash>
>
>I also have my doubts about the current kernels which have the single
>"pcpu_reserved_chunk_limit" symbol, where its contents are simply added
>to the base kernel's "__per_cpu_end" value. It seemingly would work OK
>with x86_64, because the symbols are very low offset values, and cannot
>be misconstrued for regular symbol address values:
>
> crash> sym -l
> 0 (D) __per_cpu_start
> 0 (V) irq_stack_union
> 4000 (V) gdt_page
> 5000 (V) exception_stacks
> b000 (V) tlb_vector_offset
> ...
> 15740 (V) call_single_queue
> 15780 (V) csd_data
> 157c0 (V) softnet_data
> 15900 (D) __per_cpu_end
> ffffffff81000000 (T) _text
> ffffffff81000000 (T) startup_64
> ffffffff810000b7 (t) ident_complete
> ffffffff81000100 (T) secondary_startup_64
> ...
>
>But, taking a 32-bit x86 2.6.34-rc5 kernel as an example, the per-cpu symbols
>are as shown below:
>
> crash> sym -l
> ...
> c0a90000 (D) __per_cpu_start <==
> c0a90000 (V) gdt_page
> c0a91000 (V) xen_vcpu
> c0a91020 (V) xen_vcpu_info
> c0a91060 (V) idt_desc
> ...
> c0a995c0 (V) cfd_data
> c0a99600 (V) call_single_queue
> c0a99640 (V) csd_data
> c0a99654 (D) __per_cpu_end <==
> c0a9a000 (r) .smp_locks
> c0a9a000 (D) __init_end
> c0a9a000 (R) __smp_locks
> c0aa1000 (b) .bss
> c0aa1000 (B) __bss_start
> c0aa1000 (R) __smp_locks_end
> c0aa1000 (b) swapper_pg_pmd
> c0aa2000 (b) swapper_pg_fixmap
> c0aa3000 (B) empty_zero_page
> c0aa4000 (b) level1_ident_pgt
> ...
>
>So similar to the case of the "legacy" x86_64 example, if the
>"pcpu_reserved_chunk_limit" value is simply added to the value
>of the base kernel's "__per_cpu_end" value of c0a99654, it will
>extend into (and overlap) legitimate base kernel virtual address
>space.
>
>And I haven't even bothered to look into how it would affect
>the operation of all of the other architectures...
>
>Perhaps the patch can be re-done so that the code can handle it on a
>per-module basis without inadvertently affecting everything else?
>
>In any case, there's no way I can accept the patch as it is
>currently designed.
>
>Thanks,
> Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Dave Anderson 11-10-2010 08:00 PM

To support module percpu symbol
 
----- "Toshikazu Nakayama" <nakayama.ts@ncos.nec.co.jp> wrote:

> Hi Dave,
>
> ...
> > And I haven't even bothered to look into how it would affect
> > the operation of all of the other architectures...
> >
> > Perhaps the patch can be re-done so that the code can handle it on a
> > per-module basis without inadvertently affecting everything else?
> >
> > In any case, there's no way I can accept the patch as it is
> > currently designed.
>
> I handled module symbol in kernel scope (target is not System.map).
>
> In kernel, percpu data can't access directly with mapped virtual address.
> Related kernel macros are sometimes arch depends or looks compiler depend.
> (I can not understand enough...)
> Since percpu data area is entirely dynamic allocation,
> additionally some arch locate part of them at VMALLOC area.
> They are not straight-mapped so some arch are probably overlapped.

Right -- and because of that overlap, I still believe that it is possible
that cmd_p() may still (inadvertently) do the wrong thing -- in the same
way that the "p in_suspend" attempt failed using your second patch.

In other words, the code path taken for a symbol that is just beyond the
kernel's " __per_cpu_end" would be found OK by the symbol_search() call
in line 5669 -- but it would also be misconstrued as a module percpu
symbol by per_cpu_symbol_search() in line 5670:

5669 if ((sp = symbol_search(args[optind])) && !args[optind+1]) {
5670 if ((percpu_sp = per_cpu_symbol_search(args[optind])) &&
5671 display_per_cpu_info(percpu_sp))
5672 return;
5673 sprintf(buf2, "%s = ", args[optind]);
5674 leader = strlen(buf2);
5675 if (module_symbol(sp->value, NULL, NULL, NULL, *gdb_output_radix))
5676 do_load_module_filter = TRUE;
5677 } else if ((percpu_sp = per_cpu_symbol_search(args[optind])) &&
5678 display_per_cpu_info(percpu_sp))
5679 return;
5680 else if (st->flags & LOAD_MODULE_SYMS)
5681 do_load_module_filter = TRUE;

because your IN_MODULE_PCPU() macro would only check whether the symbol value was
located in a module percpu offset range.

To fix that, perhaps the syment structures for all module symbols could
have an indication that they are module-only symbols? There are a couple
of unused fields in the structure -- perhaps "pad1" could be changed to be
a "flags" field that could have a "MODULE_SYMBOL" bit set in it:

struct syment {
ulong value;
char *name;
struct syment *val_hash_next;
struct syment *name_hash_next;
char type;
unsigned char cnt;
unsigned char pad1;
unsigned char pad2;
};

If that were true, then get_mod_percpu_sym_owner() could reject
any syment that doesn't have the MODULE_SYMBOL flag set, and then
there should be no problem with "symbol overlap". What do you
think about doing something like that?

Also, I don't quite understand why it was necessary in your patch to
modify cmd_p() like this:

--- a/symbols.c
+++ b/symbols.c
@@ -5636,7 +5636,10 @@ cmd_p(void)
leader = strlen(buf2);
if (module_symbol(sp->value, NULL, NULL, NULL, *gdb_output_radix))
do_load_module_filter = TRUE;
- } else if (st->flags & LOAD_MODULE_SYMS)
+ } else if ((percpu_sp = per_cpu_symbol_search(args[optind])) &&
+ display_per_cpu_info(percpu_sp))
+ return;
+ else if (st->flags & LOAD_MODULE_SYMS)
do_load_module_filter = TRUE;

if (leader || do_load_module_filter)

If the symbol could not be found by symbol_search() in line 5669,
then how could it be found later by the second per_cpu_symbol_search()
added above?

And for for that matter, in the very few modules that have percpu sections
in my test machines/dumpfiles, I don't see any actual per-cpu symbols listed
by "sym -[mM]". How do you actually "see" the name of a module's percpu symbol?

> Anyway with previous approach,
> there is no possible solution in my idea at least,
> but I might get really right way from your proposal.
>
> Retake attached patches with per module space.
>
> I use module.percpu which guarantee straight-mapping in every module space.
> Also its size get from bfd_section_size() for legacy percpu implementation
> although latest kernel gives percpu_size.
> - I am sorry but I don't have i386 target environs now, not tested about i386.
> (I think that module.percpu can work well even if it is VMALLOC area.)
>
> But there was still more problem at gdb interface.
>
> When p &(module percpu) is not resolved by syment,
> gdb_pass_through() require pre-registration about module sections.
> The result gave wrong symbol value of module percpu.
>
> I append percpu section in gdb request from add_symbol_file_kallsyms()
> although percpu is not in kallsyms...
> Because multiple GNU_ADD_SYMBOL_FILE requests for one module did not
> work well.
> I am not very well around these implementations.
> Can I resolve it with other better way?

Not really. But I don't see a problem with the way that you did it -- which
seems to work just fine. The only thing that needs to be changed is where
add_symbol_file_percpu() calls RESIZEBUF() -- it needs to have "req" passed
as an argument instead of "req->buf", like this:

static long
add_symbol_file_percpu(struct load_module *lm, struct gnu_request *req, long buflen)
{
char pbuf[BUFSIZE];
int i;
char *secname;
long len;

len = strlen(req->buf);
for (i = 0; i < lm->mod_sections; i++) {
secname = lm->mod_section_data[i].name;
if ((lm->mod_section_data[i].flags & SEC_FOUND) &&
(STREQ(secname, ".data.percpu") ||
STREQ(secname, ".data..percpu"))) {
sprintf(pbuf, " -s %s 0x%lx", secname, lm->mod_percpu);
while ((len + strlen(pbuf)) >= buflen) {
RESIZEBUF(req->buf, buflen, buflen * 2);
buflen *= 2;
}
strcat(req->buf, pbuf);
len += strlen(pbuf);
}
}
return buflen;
}

The way you had it written, if RESIZEBUF() were to be called, it would never
be seen by gdb, because req->buf would still point to the old buffer.

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility


All times are GMT. The time now is 03:21 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.