Hi Philipp,
On Fri, Sep 17, 2021 at 5:00 PM Philipp Rudo <prudo@xxxxxxxxxx> wrote:
>
> Hi Tao,
>
> On Tue, 14 Sep 2021 17:01:51 +0800
> Tao Liu <ltao@xxxxxxxxxx> wrote:
>
> > Currently the sequence for symbol_search to search 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)
> >
> > stage 3 takes too much time when comparing to stage 1. This patch introduces
> > a symname hash table for kernel modules, to improve the performance of symbol
> > searching.
> >
> > Functions symbol_search and symbol_exists are fundamental and widely used by
> > other crash functions, thus the benefit of performance improvement can
> > get accumulated. For example, "ps -m" and "irq" commands, which call
> > the functions many times, will become faster with the patch.
> >
> > Signed-off-by: Tao Liu <ltao@xxxxxxxxxx>
> > ---
> > defs.h | 1 +
> > kernel.c | 1 +
> > symbols.c | 167 +++++++++++++++++++++++++++++-------------------------
> > 3 files changed, 91 insertions(+), 78 deletions(-)
>
> [...]
>
> > diff --git a/symbols.c b/symbols.c
> > index bf6d94d..44415da 100644
> > --- a/symbols.c
> > +++ b/symbols.c
>
> [...]
>
> > @@ -4494,53 +4561,14 @@ symbol_search(char *s)
> > struct load_module *lm;
> > int pseudos, search_init;
>
> with big parts of the function removed some variables are no longer
> used. Please remove their declaration.
>
Sure, thanks for pointing it out.
> > - sp_hashed = symname_hash_search(s);
> > + sp_hashed = symname_hash_search(st->symname_hash, s, NULL);
> >
> > for (sp = sp_hashed ? sp_hashed : st->symtable; sp < st->symend; sp++) {
> > if (STREQ(s, sp->name))
> > return(sp);
> > }
> >
> > - pseudos = (strstr(s, "_MODULE_START_") || strstr(s, "_MODULE_END_"));
> > - search_init = FALSE;
> > -
> > - for (i = 0; i < st->mods_installed; i++) {
> > - lm = &st->load_modules[i];
> > - if (lm->mod_flags & MOD_INIT)
> > - search_init = TRUE;
> > - sp = lm->mod_symtable;
> > - sp_end = lm->mod_symend;
> > -
> > - for ( ; sp <= sp_end; sp++) {
> > - if (!pseudos && MODULE_PSEUDO_SYMBOL(sp))
> > - continue;
> > - if (STREQ(s, sp->name))
> > - return(sp);
> > - }
> > - }
> > -
> > - if (!search_init)
> > - return((struct syment *)NULL);
> > -
> > - pseudos = (strstr(s, "_MODULE_INIT_START_") || strstr(s, "_MODULE_INIT_END_"));
> > -
> > - for (i = 0; i < st->mods_installed; i++) {
> > - lm = &st->load_modules[i];
> > - if (!lm->mod_init_symtable)
> > - continue;
> > - sp = lm->mod_init_symtable;
> > - sp_end = lm->mod_init_symend;
> > -
> > - for ( ; sp < sp_end; sp++) {
> > - if (!pseudos && MODULE_PSEUDO_SYMBOL(sp))
> > - continue;
> > -
> > - if (STREQ(s, sp->name))
> > - return(sp);
> > - }
> > - }
> > -
> > - return((struct syment *)NULL);
> > + return symname_hash_search(st->mod_symname_hash, s, skip_symbols);
> > }
> >
> > /*
> > @@ -5436,29 +5464,11 @@ symbol_exists(char *symbol)
> > struct syment *sp, *sp_end;
> > struct load_module *lm;
>
> same like above.
>
> Thanks
> Philipp
>
Thanks,
Tao Liu
>
> > - if ((sp = symname_hash_search(symbol)))
> > + if (symname_hash_search(st->symname_hash, symbol, NULL))
> > return TRUE;
> >
> > - for (i = 0; i < st->mods_installed; i++) {
> > - lm = &st->load_modules[i];
> > - sp = lm->mod_symtable;
> > - sp_end = lm->mod_symend;
> > -
> > - for ( ; sp < sp_end; sp++) {
> > - if (STREQ(symbol, sp->name))
> > - return(TRUE);
> > - }
> > -
> > - if (lm->mod_init_symtable) {
> > - sp = lm->mod_init_symtable;
> > - sp_end = lm->mod_init_symend;
> > -
> > - for ( ; sp < sp_end; sp++) {
> > - if (STREQ(symbol, sp->name))
> > - return(TRUE);
> > - }
> > - }
> > - }
> > + if (symname_hash_search(st->mod_symname_hash, symbol, NULL))
> > + return TRUE;
> >
> > return(FALSE);
> > }
> > @@ -5513,12 +5523,7 @@ per_cpu_symbol_search(char *symbol)
> > int
> > kernel_symbol_exists(char *symbol)
> > {
> > - struct syment *sp;
> > -
> > - if ((sp = symname_hash_search(symbol)))
> > - return TRUE;
> > - else
> > - return FALSE;
> > + return !!(symname_hash_search(st->symname_hash, symbol, NULL));
> > }
> >
> > /*
> > @@ -5527,7 +5532,7 @@ kernel_symbol_exists(char *symbol)
> > struct syment *
> > kernel_symbol_search(char *symbol)
> > {
> > - return symname_hash_search(symbol);
> > + return symname_hash_search(st->symname_hash, symbol, NULL);
> > }
> >
> > /*
> > @@ -12464,8 +12469,10 @@ store_load_module_symbols(bfd *bfd, int dynamic, void *minisyms,
> > error(INFO, "%s: last symbol: %s is not _MODULE_END_%s?\n",
> > lm->mod_name, lm->mod_load_symend->name, lm->mod_name);
> >
> > + mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend);
> > lm->mod_symtable = lm->mod_load_symtable;
> > lm->mod_symend = lm->mod_load_symend;
> > + mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
> >
> > lm->mod_flags &= ~MOD_EXT_SYMS;
> > lm->mod_flags |= MOD_LOAD_SYMS;
> > @@ -12495,6 +12502,7 @@ delete_load_module(ulong base_addr)
> > req->name = lm->mod_namelist;
> > gdb_interface(req);
> > }
> > + mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend);
> > if (lm->mod_load_symtable) {
> > free(lm->mod_load_symtable);
> > namespace_ctl(NAMESPACE_FREE,
> > @@ -12504,6 +12512,7 @@ delete_load_module(ulong base_addr)
> > unlink_module(lm);
> > lm->mod_symtable = lm->mod_ext_symtable;
> > lm->mod_symend = lm->mod_ext_symend;
> > + mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
> > lm->mod_flags &= ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH);
> > lm->mod_flags |= MOD_EXT_SYMS;
> > lm->mod_load_symtable = NULL;
> > @@ -12532,6 +12541,7 @@ delete_load_module(ulong base_addr)
> > req->name = lm->mod_namelist;
> > gdb_interface(req);
> > }
> > + mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend);
> > if (lm->mod_load_symtable) {
> > free(lm->mod_load_symtable);
> > namespace_ctl(NAMESPACE_FREE,
> > @@ -12541,6 +12551,7 @@ delete_load_module(ulong base_addr)
> > unlink_module(lm);
> > lm->mod_symtable = lm->mod_ext_symtable;
> > lm->mod_symend = lm->mod_ext_symend;
> > + mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
> > lm->mod_flags &= ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH);
> > lm->mod_flags |= MOD_EXT_SYMS;
> > lm->mod_load_symtable = NULL;
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
