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
>
Attachment:
0005-fixed-remained-problems.patch
Description: Binary data
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility
