Re: [Crash-utility] [PATCH v2] Improve the performance of symbol searching for kernel modules
-----Original Message-----
Hi,
On Tue, 24 Aug 2021 23:51:56 +0800
Tao Liu <ltao(a)redhat.com> wrote:
> On Tue, Aug 24, 2021 at 3:19 PM HAGIO KAZUHITO(萩尾 一仁)
> <k-hagio-ab(a)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(a)redhat.com
https://listman.redhat.com/mailman/listinfo/crash-utility