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

[HAGIO KAZUHITO(萩尾 一仁)]

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

Anyway, seems that it's another issue..

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