Hi Tao, Found another issue. The first symbol of duplicated ones will not be shown by sym command after mod -s. # crash -s crash> sym cleanup_module | head -n 1 ffffffffc02527ff (t) cleanup_module [fuse] crash> mod -s fuse MODULE NAME BASE SIZE OBJECT FILE ffffffffc025eac0 fuse ffffffffc023f000 151552 /usr/lib/debug/lib/modules/4.18.0-305.el8.x86_64/kernel/fs/fuse/fuse.ko.debug crash> sym cleanup_module | grep fuse It looks like symbol_search() has to return the lowest address symbol and symbol_search_next() returns the next lowest symbol, so removing/installing module symbols from/to the hash table breaks this. The patch will need sorted installation or other solution.

Also, I've not looked at this enough, but kallsyms_module_symbol() changes spx->cnt from lm->mod_ext_symtable. Is there no interference with the patch?

Thanks, Kazu -----Original Message----- > 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; > Reviewed-by: Philipp Rudo <prudo(a)redhat.com&gt; > --- > defs.h | 1 + > kernel.c | 1 + > symbols.c | 176 ++++++++++++++++++++++++++++-------------------------- > 3 files changed, 92 insertions(+), 86 deletions(-) > > diff --git a/defs.h b/defs.h > index eb1c71b..c10ebff 100644 > --- a/defs.h > +++ b/defs.h > @@ -2751,6 +2751,7 @@ struct symbol_table_data { > double val_hash_searches; > double val_hash_iterations; > struct syment *symname_hash[SYMNAME_HASH]; > + struct syment *mod_symname_hash[SYMNAME_HASH]; > struct symbol_namespace kernel_namespace; > struct syment *ext_module_symtable; > struct syment *ext_module_symend; > diff --git a/kernel.c b/kernel.c > index b2c8a0c..307eeee 100644 > --- a/kernel.c > +++ b/kernel.c > @@ -4663,6 +4663,7 @@ reinit_modules(void) > kt->mods_installed = 0; > clear_text_value_cache(); > > + memset(st->mod_symname_hash, 0, sizeof(st->mod_symname_hash)); > module_init(); > } > > diff --git a/symbols.c b/symbols.c > index bf6d94d..7ab47f2 100644 > --- a/symbols.c > +++ b/symbols.c > @@ -64,8 +64,10 @@ static int namespace_ctl(int, struct symbol_namespace *, void *, void *); > static void symval_hash_init(void); > static struct syment *symval_hash_search(ulong); > static void symname_hash_init(void); > -static void symname_hash_install(struct syment *); > -static struct syment *symname_hash_search(char *); > +static void symname_hash_install(struct syment *[], struct syment *); > +static void symname_hash_remove(struct syment *[], struct syment *); > +static struct syment *symname_hash_search(struct syment *[], char *, > + int (*)(struct syment *, char *)); > static void gnu_qsort(bfd *, void *, long, unsigned int, asymbol *, asymbol *); > static int check_gnu_debuglink(bfd *); > static int separate_debug_file_exists(const char *, unsigned long, int *); > @@ -1119,7 +1121,7 @@ symname_hash_init(void) > struct syment *sp; > > for (sp = st->symtable; sp < st->symend; sp++) > - symname_hash_install(sp); > + symname_hash_install(st->symname_hash, sp); > > if ((sp = symbol_search("__per_cpu_start"))) > st->__per_cpu_start = sp->value; > @@ -1127,21 +1129,42 @@ symname_hash_init(void) > st->__per_cpu_end = sp->value; > } > > +static void > +mod_symtable_hash_install_range(struct syment *from, struct syment *to) > +{ > + struct syment *sp; > + > + for (sp = from; sp <= to; sp++) > + symname_hash_install(st->mod_symname_hash, sp); > +} > + > +static void > +mod_symtable_hash_remove_range(struct syment *from, struct syment *to) > +{ > + struct syment *sp; > + > + for (sp = from; sp <= to; sp++) > + symname_hash_remove(st->mod_symname_hash, sp); > +} > + > /* > * Install a single static kernel symbol into the symname_hash. > */ > static void > -symname_hash_install(struct syment *spn) > +symname_hash_install(struct syment *table[], struct syment *spn) > { > struct syment *sp; > int index; > > + if (!spn) > + return; > + > index = SYMNAME_HASH_INDEX(spn->name); > spn->cnt = 1; > > - if ((sp = st->symname_hash[index]) == NULL) > - st->symname_hash[index] = spn; > - else { > + if ((sp = table[index]) == NULL) { > + table[index] = spn; > + } else { > while (sp) { > if (STREQ(sp->name, spn->name)) { > sp->cnt++; > @@ -1157,17 +1180,47 @@ symname_hash_install(struct syment *spn) > } > } > > +static void > +symname_hash_remove(struct syment *table[], struct syment *spn) > +{ > + struct syment *sp; > + int index; > + > + if (!spn) > + return; > + > + index = SYMNAME_HASH_INDEX(spn->name); > + > + if (table[index] == spn) > + table[index] = spn->name_hash_next; > + > + for (sp = table[index]; sp; sp = sp->name_hash_next) { > + if (STREQ(sp->name, spn->name)) > + sp->cnt--; > + if (sp->name_hash_next == spn) > + sp->name_hash_next = spn->name_hash_next; > + } > +} > + > /* > * Static kernel symbol value search > */ > static struct syment * > -symname_hash_search(char *name) > +symname_hash_search(struct syment *table[], char *name, > + int (*skip_condition)(struct syment *, char *)) > { > struct syment *sp; > + int index; > > - sp = st->symname_hash[SYMNAME_HASH_INDEX(name)]; > + index = SYMNAME_HASH_INDEX(name); > + sp = table[index]; > > while (sp) { > + if (skip_condition && skip_condition(sp, name)) { > + sp = sp->name_hash_next; > + continue; > + } > + > if (STREQ(sp->name, name)) > return sp; > sp = sp->name_hash_next; > @@ -1595,6 +1648,7 @@ store_module_symbols_v1(ulong total, int mods_installed) > lm->mod_symend = sp; > } > } > + mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); > } > > st->flags |= MODULE_SYMS; > @@ -2075,6 +2129,8 @@ store_module_symbols_v2(ulong total, int mods_installed) > lm->mod_init_symend = sp; > } > } > + mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); > + mod_symtable_hash_install_range(lm->mod_init_symtable, lm->mod_init_symend); > } > > st->flags |= MODULE_SYMS; > @@ -4482,6 +4538,17 @@ symbol_query(char *s, char *print_pad, struct syment **spp) > return(cnt); > } > > +static int > +skip_symbols(struct syment *sp, char *s) > +{ > + int pseudos, skip = 0; > + > + pseudos = (strstr(s, "_MODULE_START_") || strstr(s, "_MODULE_END_") || > + strstr(s, "_MODULE_INIT_START_") || strstr(s, "_MODULE_INIT_END_")); > + if (!pseudos && MODULE_PSEUDO_SYMBOL(sp)) > + skip = 1; > + return skip; > +} > > /* > * Return the syment of a symbol. > @@ -4489,58 +4556,16 @@ symbol_query(char *s, char *print_pad, struct syment **spp) > struct syment * > symbol_search(char *s) > { > - int i; > - struct syment *sp_hashed, *sp, *sp_end; > - struct load_module *lm; > - int pseudos, search_init; > + struct syment *sp_hashed, *sp; > > - 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); > } > > /* > @@ -5432,33 +5457,11 @@ value_symbol(ulong value) > int > symbol_exists(char *symbol) > { > - int i; > - struct syment *sp, *sp_end; > - struct load_module *lm; > - > - 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 +5516,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 +5525,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 +12462,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 +12495,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 +12505,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 +12534,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 +12544,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; > -- > 2.29.2

Hi Tao, On Sat, 18 Sep 2021 15:59:32 +0800 Tao Liu <ltao(a)redhat.com&gt; wrote: > symname_hash_install won't check if spn has been installed before. If > it does, the second install will corrupt the hash table as well as > spn->cnt counting. This patch adds the check to avoid such risks. > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > --- > symbols.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/symbols.c b/symbols.c > index f7157b1..6d12c55 100644 > --- a/symbols.c > +++ b/symbols.c > @@ -1147,6 +1147,20 @@ mod_symtable_hash_remove_range(struct syment *from, struct syment *to) > symname_hash_remove(st->mod_symname_hash, sp); > } > > +static inline int > +syment_is_installed(struct syment *table[], struct syment *spn) > +{ > + struct syment *sp; > + int index; > + > + index = SYMNAME_HASH_INDEX(spn->name); > + for (sp = table[index]; sp; sp = sp->name_hash_next) { > + if (sp == spn) > + return TRUE; > + } > + return FALSE; > +} > + > /* > * Install a single static kernel symbol into the symname_hash. > */ > @@ -1156,7 +1170,7 @@ symname_hash_install(struct syment *table[], struct syment *spn) > struct syment *sp; > int index; > > - if (!spn) > + if (!spn || syment_is_installed(table, spn)) > return; > > index = SYMNAME_HASH_INDEX(spn->name); hmm... not sure if this is a little bit over the top. The idea I had was in your v3 simply replace assert(sp != spn); by if (sp == spn) { error(WARNING, "Symbol %s already installed in symname_hash\n", sp->name); continue; }

That's less code plus the warning makes it easier to detect that there is a problem (for me the case sp == spn is a sign for a bug in crash). What do you think?

From my point,

Thanks Philipp

Hi Tao, On Thu, 23 Sep 2021 21:46:14 +0800 Tao Liu <ltao(a)redhat.com&gt; wrote: > Hi Philipp, > > Thanks for reviewing the patch! > > On Thu, Sep 23, 2021 at 8:19 PM Philipp Rudo <prudo(a)redhat.com&gt; wrote: > > > > Hi Tao, > > > > On Sat, 18 Sep 2021 15:59:32 +0800 > > Tao Liu <ltao(a)redhat.com&gt; wrote: > > > > > symname_hash_install won't check if spn has been installed before. If > > > it does, the second install will corrupt the hash table as well as > > > spn->cnt counting. This patch adds the check to avoid such risks. > > > > > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > > > --- > > > symbols.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/symbols.c b/symbols.c > > > index f7157b1..6d12c55 100644 > > > --- a/symbols.c > > > +++ b/symbols.c > > > @@ -1147,6 +1147,20 @@ mod_symtable_hash_remove_range(struct syment *from, struct syment *to) > > > symname_hash_remove(st->mod_symname_hash, sp); > > > } > > > > > > +static inline int > > > +syment_is_installed(struct syment *table[], struct syment *spn) > > > +{ > > > + struct syment *sp; > > > + int index; > > > + > > > + index = SYMNAME_HASH_INDEX(spn->name); > > > + for (sp = table[index]; sp; sp = sp->name_hash_next) { > > > + if (sp == spn) > > > + return TRUE; > > > + } > > > + return FALSE; > > > +} > > > + > > > /* > > > * Install a single static kernel symbol into the symname_hash. > > > */ > > > @@ -1156,7 +1170,7 @@ symname_hash_install(struct syment *table[], struct syment *spn) > > > struct syment *sp; > > > int index; > > > > > > - if (!spn) > > > + if (!spn || syment_is_installed(table, spn)) > > > return; > > > > > > index = SYMNAME_HASH_INDEX(spn->name); > > > > hmm... not sure if this is a little bit over the top. The idea I had > > was in your v3 simply replace > > > > assert(sp != spn); > > > > by > > > > if (sp == spn) { > > error(WARNING, "Symbol %s already installed in symname_hash\n", > > sp->name); > > continue; > > } > > > > It may not be easy to replace with "check if sp == spn and continue". > For example, if we already have > 3 syments installed, which all have the same name, such as > (sp1{cnt == 3, name == "str"}, sp2{cnt == 3, name == "str"}, sp3{cnt > == 3, name == "str"}) > in the hashtable, and we will install sp3 again(sp3 == spn) into the hashtable. > > The cnt will get increased for sp1 and sp2 in the following code: > while (sp) { > if (sp == spn) { > error(....); > continue; > } > if (STREQ(sp->name, spn->name)) { > sp->cnt++; > spn->cnt++; > } > ..... > } > > However, when iteration reaches sp3, it will not increase cnt and not > install spn. Thus we will have > (sp1{cnt == 4}, sp2{cnt == 4}, sp3{cnt == 3}) in the hashtable, cnt > gets corrupted. In other words, > we need to revert all the previous cnt++ when we reach sp == spn. It > will require > more code to implement the 'revert' operation, and it is not as clear > as syment_is_installed > check. you are right. I totally missed that. In fact my suggestion is even worse. At the beginning the function sets spn->cnt = 1; So in the scenario you described above spn3->cnt == 1 should be true... Having that said, your patch looks good to me Reviewed-by: Philipp Rudo <prudo(a)redhat.com&gt; Thanks Philipp > > That's less code plus the warning makes it easier to detect that there > > is a problem (for me the case sp == spn is a sign for a bug in crash). > > What do you think? > > > > From my point, > 1) assert() check: Use least code, simple and clear, but too strict. > 2) syment_is_installed check: Use more code, clear, acceptable to me. > 3) introduce 'cnt++ revert' operation: I haven't think of a better way, from > the current inspection, it uses more code, and is not elegant. > What do you think? > > Thanks, > Tao Liu > > > Thanks > > Philipp > > >