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

[Tao Liu]

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. :-)

> >
> > I measured each stage's time by adding the following code to symbol_search:
> >
> >         struct load_module *lm;
> >         int pseudos, search_init;
> > +       double T1, T2, T3;
> > +       clock_t start, end;
> >
> > +       start = clock();
> >         sp_hashed = symname_hash_search(s);
> > +       end = clock();
> > +       T1 = (double)(end - start) * 1000.0 / CLOCKS_PER_SEC;
> >
> > +       start = clock();
> >          for (sp = sp_hashed ? sp_hashed : st->symtable; sp < st->symend; sp++) {
> >                  if (STREQ(s, sp->name))
> >                          return(sp);
> >          }
> > +        end = clock();
> > +        T2 = (double)(end - start) * 1000.0 / CLOCKS_PER_SEC;
> >
> >         pseudos = (strstr(s, "_MODULE_START_") || strstr(s, "_MODULE_END_"));
> >         search_init = FALSE;
> >
> > +       start = clock();
> >          for (i = 0; i < st->mods_installed; i++) {
> >                  lm = &st->load_modules[i];
> >                 if (lm->mod_flags & MOD_INIT)
> >          ....
> >                                 return(sp);
> >                  }
> >          }
> > +        end = clock();
> > +        T3 = (double)(end - start) * 1000.0 / CLOCKS_PER_SEC;
> > +       printf("%lf %lf %lf\n", T1, T2, T3);
> >
> >         if (!search_init)
> >                 return((struct syment *)NULL);
> >
> > crash> sym blah
> > T1:0.008000 T2:0.562000 T3:2.435000
> > symbol not found: blah
> > possible alternatives:
> >   (none found)
> >
> > With the v2 patch applied and the time measurement code added:
> >
> > crash> sym blah
> > T1:0.003000 T2:0.545000 T3:0.017000
> > symbol not found: blah
> > possible alternatives:
> >   (none found)
> >
> > We can see T3 performs better.
>
> Thanks for the info, that's good.
>
> One more, do you have any slow command that will be especially improved
> by the patch?  I'm ok with the sym command taking 3ms :-)
> I'd like to know why the patch should be applied or how much the worst
> command etc. will be improved, to see the importance of it.
>

No, I don't have a specific slow command. Because the modified functions
symbol_search and symbol_exists are fundamental and widely used by
other crash functions, thus the benefit of performance improvement can
get accumulated. In addition, the time consumption for T3 shortened greatly,
from 2.4 to 0.017, maybe this can be one of the reasons.

Thanks,
Tao Liu

> Thanks,
> Kazu
>
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility