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

[Toshikazu Nakayama]

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