[Crash-utility] [PATCH 0/4] To support module percpu symbol

[Toshikazu Nakayama]

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
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-fixed-remained-problems.patch
Type: application/octet-stream
Size: 3817 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20101111/1a5a5fb4/attachment.obj>