[Crash-utility] [PATCH v2] Improve the performance of symbol searching for kernel modules

[Philipp Rudo]

Hi Kazu,

On Thu, 9 Sep 2021 05:05:22 +0000
HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab at nec.com> wrote:

> -----Original Message-----
> > Hi,
> > 
> > On Tue, 24 Aug 2021 23:51:56 +0800
> > Tao Liu <ltao at redhat.com> wrote:
> >   
> > > On Tue, Aug 24, 2021 at 3:19 PM HAGIO KAZUHITO(萩尾　一仁)
> > > <k-hagio-ab at nec.com> wrote:  
> > > >
> > > > -----Original Message-----  
> > > > > On Mon, Aug 23, 2021 at 08:24:28AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:  
> > > > > > Hi Tao Liu,
> > > > > >
> > > > > > -----Original Message-----  
> > > > > > > Currently the sequence for crash searching a symbol is: 1) kernel symname
> > > > > > > hash table, 2) iterate all kernel symbols, 3) iterate all kernel modules
> > > > > > > and their symbols. In the worst case, if a non-exist symbol been searched,
> > > > > > > all 3 stages will be went through. The time consuming status for each stage
> > > > > > > is like:
> > > > > > >
> > > > > > >     stage 1         stage 2         stage 3
> > > > > > >     0.007000(ms)    0.593000(ms)    2.421000(ms)  
> > > > > >
> > > > > > Just to clarify, which parts of code are meant by the stage 1 and 2?
> > > > > >
> > > > > > Do you have a result with the patch?
> > > > > >  
> > > > > > >
> > > > > > > stage 3 takes too much time when comparing to stage 1. So let's introduce a
> > > > > > > symname hash table for kernel modules to improve the performance of symbol
> > > > > > > searching.  
> > > > > >
> > > > > > I see that 3) is relatively time-consuming, but I have not had a delay
> > > > > > due to the symbol search.  Which command did you observe a delay?  
> > > > >
> > > > > Hello Kazu,
> > > > >
> > > > > The target function is symbols.c:symbol_search. The code involved in the 3 stages are:
> > > > >
> > > > > 1) sp_hashed = symname_hash_search(s);
> > > > > 2) for (sp = sp_hashed ? sp_hashed : st->symtable; sp < st->symend; sp++) {
> > > > >                  if (STREQ(s, sp->name))
> > > > >                          return(sp);
> > > > > 3) for (i = 0; i < st->mods_installed; i++) {
> > > > >                  lm = &st->load_modules[i];
> > > > >                  ....
> > > > >    }  
> > > >
> > > > OK, I see.
> > > >
> > > > (BTW, at a glance I'm not sure why the 2nd stage is needed...
> > > > sp_hashed was already checked with STREQ in symname_hash_search if found,
> > > > otherwise st->synmane_hash should include all kernel symbols..)
> > > >  
> > > Hello Kazu,
> > >
> > > Yes, from my view I also think the 2nd stage is not necessary, I have
> > > thought about removing
> > > it as well, but I just don't want to go too far at one step. :-)  
> > 
> > in my opinion this function has a way bigger problem. It always returns
> > the first symbol with a matching name which might not be the one you
> > want. Some of the users (e.g. cmd_sym or cmd_dis (in
> > resolve_text_symbol)) have extra code to handle symbol name collisions
> > but most users simply rely on the symbol name being unique (e.g.
> > cmd_rd).  
> 
> hmm, maybe there has been no need/demand to do it for the other users, or
> hard to implement depending on a command.  For example, p command passes
> the whole command to gdb directory, how can we make gdb process each the
> symbol name not being unique differently?

I believe it's a hard to implement in combination with no reported
issue and to be honest I'm not sure if there is a proper way.

BTW, my main concern aren't the commands entered by the user. I only
used them as they are a fast and simple way to reproduce the problem. My
main concern are locations where crash needs specific symbols to
properly interpret the dump data. If there is name collision for those
symbols crash most likely will interpret garbage. This also includes
indirect uses of symbol_search in e.g. per_cpu_symbol_search or
{try_}get_symbol_data.

> Anyway, seems that it's another issue..

Definitely a separate issue, but one we should be aware of...

Thanks
Philipp

> Thanks,
> Kazu
> 
> > 
> > For example on my F34 this leads to
> >   
> > crash> sym cleanup_module  
> > ffffffffc00a9f62 (T) cleanup_module [zram]
> > ffffffffc00d6f7f (T) cleanup_module [ip_tables]
> > ffffffffc00fe675 (T) cleanup_module [failover]
> > [...]  
> > crash> dis cleanup_module  
> > dis: cleanup_module: duplicate text symbols found:
> > ffffffffc00a9f62 (T) cleanup_module [zram]
> > ffffffffc00d6f7f (T) cleanup_module [ip_tables]
> > ffffffffc00fe675 (T) cleanup_module [failover]
> > [...]  
> > crash> rd cleanup_module  
> > ffffffffc00a9f62:  000000ffffff50e9                    .P......  
> > crash>  
> > 
> > Thanks
> > Philipp
> > 
> > --
> > Crash-utility mailing list
> > Crash-utility at redhat.com
> > https://listman.redhat.com/mailman/listinfo/crash-utility  
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility