Re: [Crash-utility] [PATCH v3 1/4] Improve the performance of symbol searching for kernel modules

Hi Tao, On Tue, 14 Sep 2021 17:01:51 +0800 Tao Liu <ltao(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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.

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

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