FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Crash Utility

 
 
LinkBack Thread Tools
 
Old 11-11-2010, 03:52 AM
Toshikazu Nakayama
 
Default To support module percpu symbol

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

Sounds fine.
pad1 is already used by patch_kernel_symbol().
I will rename pad2.

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

Because per_cpu_symbol_search() can search percpu symbol with
per_cpu__ prefix which is automatically renamed.

sprintf(old, "per_cpu__%s", symbol);
if ((sp = symbol_search(old)))
return sp;

This retry is valid for old kernel percpu symbol naming model.

DEFINE_PER_CPU(struct kernel_stat, kstat);

We try to enter "p per_cpu__kstat" in old kernel or "p kstat" in new kernel
and further, enter "struct kernel_stat <virtual address>".
Someone who does not know real DEFINE_PER_CPU(X) declared symbol name
force to try "sym X -> possible alternatives" or check kernel code, System.map.

I thought that per_cpu_symbol_search() aims to resolve UI gap
between tow kernel percpu model from above implementation
but present cmd_p() is not worked well because of symbol_search() failed.
("p kstat" works well in the old kernel with this patch)

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

Truth, I've also been touched first one when
I tried to make KVM command (based on linux-2.6.35 & CONFIG_KVM=m)
over private extension module.
So I actually "see" the name of KVM modules.
Old kernel dose not have such a module, so I made module for test.

Although extension module want to obtain KVM percpu symbol addresses
via crash symbol API, it can not answer.

My work is blocked by module's per-cpu (CONFIG_KVM=y is not prefer solution).
I send patch to crash utility with this background issue.

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

Oh, entire bug!

I'll fix remained problems and send next patch with updated portion only.
I would like to follow your advisement.

Thanks,
Toshi.

>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
 
Old 11-11-2010, 06:28 AM
Toshikazu Nakayama
 
Default To support module percpu symbol

Hi Dave,

I make additional patch which I learnt from you.

Add flags into struct syment and
get MODULE_SYMBOL bit with IS_MODULE_SYMBOL().
get_mod_percpu_sym_owner() can reject any syment that
doesn't have the MODULE_SYMBOL flag set now.

In case add_symbol_file_percpu() calls RESIZEBUF(),
it would be seen by gdb now.

Please apply attachment over my last patches.

Toshi.

>> 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
 
Old 11-11-2010, 01:30 PM
Dave Anderson
 
Default To support module percpu symbol

----- "Toshikazu Nakayama" <nakayama.ts@ncos.nec.co.jp> wrote:

> > 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?
>
> Sounds fine.
> pad1 is already used by patch_kernel_symbol().
> I will rename pad2.

Yes, but pad1 is only (temporarily) used by gdb_session_init() when a System.map
file is entered on the crash command line -- and long before module_init() is called.
And given that it's a boolean value in patch_kernel_symbol(), I'll change "pad1" to
be "flags", and have two flags defined for use there. Then "pad2" can be left alone
for possible future use.

> >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?
>
> Because per_cpu_symbol_search() can search percpu symbol with
> per_cpu__ prefix which is automatically renamed.
>
> sprintf(old, "per_cpu__%s", symbol);
> if ((sp = symbol_search(old)))
> return sp;
>
> This retry is valid for old kernel percpu symbol naming model.
>
> DEFINE_PER_CPU(struct kernel_stat, kstat);
>
> We try to enter "p per_cpu__kstat" in old kernel or "p kstat" in new kernel
> and further, enter "struct kernel_stat <virtual address>".
> Someone who does not know real DEFINE_PER_CPU(X) declared symbol name
> force to try "sym X -> possible alternatives" or check kernel code,
> System.map.
>
> I thought that per_cpu_symbol_search() aims to resolve UI gap
> between tow kernel percpu model from above implementation
> but present cmd_p() is not worked well because of symbol_search() failed.
> ("p kstat" works well in the old kernel with this patch)

That is true -- but my original intent was that when per_cpu_symbol_search()
gets passed a hard-coded symbol, then it would make the switch if necessary.
That way all of the currently-existing callers wouldn't have to call
symbol_search() twice, i.e. with and without the "per_cpu__" prefix.

However, cmd_p() is the only caller of the function that passes an unknown
string pointer as an argument, and I presumed that the user would only use
existing, *actual*, symbol names.

However, re-thinking the matter, I guess there should be no problem with
letting the user of the "p" command to "cheat" by leaving off, or adding,
the "per_cpu__" symbol prefix. As it is now, the command would just fail,
but with your patch, at least it would "try" to do the right thing.
So that change looks OK to me.

> > 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?
>
> Truth, I've also been touched first one when
> I tried to make KVM command (based on linux-2.6.35 & CONFIG_KVM=m)
> over private extension module.
> So I actually "see" the name of KVM modules.
> Old kernel dose not have such a module, so I made module for test.
>
> Although extension module want to obtain KVM percpu symbol addresses
> via crash symbol API, it can not answer.
>
> My work is blocked by module's per-cpu (CONFIG_KVM=y is not prefer solution).
> I send patch to crash utility with this background issue.

My mistake -- I can see them, although they show up out of order in the
"sym -[mM] listing. For example:

crash> sym -m ip_conntrack
ffffffff8041f540 (D) per_cpu__ip_conntrack_ecache
ffffffff8041f560 (D) per_cpu__ip_conntrack_stat
ffffffff88d9e000 MODULE START: ip_conntrack
ffffffff88d9e000 (t) kill_proto
ffffffff88d9e00f (t) ct_get_next
ffffffff88d9e052 (t) ct_seq_next
ffffffff88d9e057 (t) exp_seq_next
ffffffff88d9e06b (t) ct_cpu_seq_stop
...

I'll look into fixing that...

> >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:
>
> Oh, entire bug!
>
> I'll fix remained problems and send next patch with updated portion only.
> I would like to follow your advisement.

OK, I'll take a look.

Thanks,
Dave


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 11-12-2010, 08:03 PM
Dave Anderson
 
Default To support module percpu symbol

----- "Dave Anderson" <anderson@redhat.com> wrote:

> ----- "Toshikazu Nakayama" <nakayama.ts@ncos.nec.co.jp> wrote:
> >
> > I'll fix remained problems and send next patch with updated portion only.
> > I would like to follow your advisement.
>
> OK, I'll take a look.
>
> Thanks,
> Dave

Hello Toshi,

I made several changes to your last patch as we discussed before,
and successfully tested it on the x86, x86_64, ia64, s390 and s390x
architectures.

I've attached the patch as it is queued for the next release.

Because I was working against my own updated tree, the patch also
contains a couple other symbol-handling related changes that are
also queued for the next release.

Thanks for getting the ball rolling for this feature,
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 11-15-2010, 05:16 AM
Toshikazu Nakayama
 
Default To support module percpu symbol

Hello Dave,

>Hello Toshi,
>
>I made several changes to your last patch as we discussed before,
>and successfully tested it on the x86, x86_64, ia64, s390 and s390x
>architectures.
>
>I've attached the patch as it is queued for the next release.
>
>Because I was working against my own updated tree, the patch also
>contains a couple other symbol-handling related changes that are
>also queued for the next release.
>
>Thanks for getting the ball rolling for this feature,
> Dave

Because your patch could not apply to crash-5.0.9 with patch command,
I manually apply them and they can run well over my environs.
I'll use crash-5.0.9 with your patch
until crash-5.0.10 got available.

I saw that you've updated whole places where per-cpu or MODULE_SYMBOL
should be applicable in crash utility.

Thanks for your clarification,
Toshi.

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 11-15-2010, 12:54 PM
Dave Anderson
 
Default To support module percpu symbol

----- "Toshikazu Nakayama" <nakayama.ts@ncos.nec.co.jp> wrote:

> Hello Dave,
>
> >Hello Toshi,
> >
> >I made several changes to your last patch as we discussed before,
> >and successfully tested it on the x86, x86_64, ia64, s390 and s390x
> >architectures.
> >
> >I've attached the patch as it is queued for the next release.
> >
> >Because I was working against my own updated tree, the patch also
> >contains a couple other symbol-handling related changes that are
> >also queued for the next release.
> >
> >Thanks for getting the ball rolling for this feature,
> > Dave
>
> Because your patch could not apply to crash-5.0.9 with patch command,
> I manually apply them and they can run well over my environs.

Hmmm -- works for me:

# tar xzf crash-5.0.9.tar.gz
# cd crash-5.0.9
# patch -p1 < $HOME/symbols.patch
patching file help.c
patching file kernel.c
patching file alpha.c
patching file ppc.c
patching file s390.c
patching file s390x.c
patching file ppc64.c
patching file x86_64.c
patching file symbols.c
patching file lkcd_x86_trace.c
patching file unwind.c
patching file defs.h
#

Maybe you were trying to apply them on top of your last patch?
Anyway, as long as you're happy with the end result...

> I'll use crash-5.0.9 with your patch
> until crash-5.0.10 got available.
>
> I saw that you've updated whole places where per-cpu or MODULE_SYMBOL
> should be applicable in crash utility.
>

Thanks again Toshi,
Dave

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

Thread Tools




All times are GMT. The time now is 06:42 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org