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

[Dave Anderson]

----- "Toshikazu Nakayama" <nakayama.ts at 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