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&gt; wrote: > > > On Tue, Aug 24, 2021 at 3:19 PM HAGIO KAZUHITO(萩尾　一仁) > > <k-hagio-ab(a)nec.com&gt; 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 -- Crash-utility mailing list Crash-utility(a)redhat.com https://listman.redhat.com/mailman/listinfo/crash-utility