Re: [Crash-utility] [RFC PATCH 01/15] Add support for struct module_memory on Linux 6.4 and later

On Mon, May 22, 2023 at 2:56 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: >> Given that, I would suggest changing the above function name to the >> store_module_symbols_v6_4() with the kernel version suffix. That can >> differentiate them and avoid confusion. > > Yes, I had the same impression about this. > I would pefer store_module_symbols_6_4() for kernel version alignment. > > Also fine to me. Thanks. > >> For the above two definitions, I noticed that they should be in pairs, > and >> associated with another definition mod_mem_type. In addition, this looks >> like hard code. How about the following changes? >> >> #define NAME(s) #s >> #define MODULE_TAG(TYPE, SE) NAME(_MODULE_ ## TYPE ## _## SE ##_) >> >> struct mod_node { >> char *s; >> char *e; >> }; >> >> static const struct mod_node module_tags[] = { >> {MODULE_TAG(TEXT, START), MODULE_TAG(TEXT, END)}, >> {MODULE_TAG(DATA, START), MODULE_TAG(DATA, END)}, >> {MODULE_TAG(RODATA, START), MODULE_TAG(RODATA, END)}, >> {MODULE_TAG(RO_AFTER_INIT, START), MODULE_TAG(RO_AFTER_INIT, END)}, >> {MODULE_TAG(INIT_TEXT, START), MODULE_TAG(INIT_TEXT, END)}, >> {MODULE_TAG(INIT_DATA, START), MODULE_TAG(INIT_DATA, END)}, >> {MODULE_TAG(INIT_RODATA, START), MODULE_TAG(INIT_RODATA, END)}, >> >> {NULL, NULL} >> }; > > I don't like complicated code, but will think about something like this. > > It is only a macro definition, and eventually becomes a struct array.

> >>> + qsort(st->ext_module_symtable, mcnt, sizeof(struct syment), >>> + compare_syms); >>> + >>> + /* not sure whether this sort is meaningful anymore. */ >>> >> >> I tried to remove it and tested again, seems that the sort result is not >> expected. > > I thought when writing this, now the memory regions of a module are > cattered, what is the good point of sorting modules only by their text > start address? and it might be fine to preserve the order of the > "modules" list. > > Thank you for the explanation, Kazu. > However, I sorted them after all, with a header change in the patch 15/15. > > Ok, let's still keep it for now. >>> mod_symtable_hash_install_range(lm->symtable[i], lm->symend[i]); >>> + } >>> + } >>> + >>> + st->flags |= MODULE_SYMS; >>> + >>> + /* probably not needed anymore */ >>> >> >> Could you please explain the details why this is not needed anymore? > > Because it looks to be a very old facility. Some code comments show > "2.2.7" kernel version and I've not seen such symbols on recent kernels. > So I thought that it will not be needed for 6.4 and later at least. > > * Later versons of insmod store basic address information of each > * module in a format that looks like the following example of the > * nfsd module: > * > * d004d000 > __insmod_nfsd_O/lib/modules/2.2.17/fs/nfsd.o_M3A7EE300_V131601 > * d004d054 __insmod_nfsd_S.text_L30208 > > But do you know about them? > > I tried to find out some change logs, but it's not easy to track them for a very old kernel and crash-utility. So far I haven't seen the similar "__insmod_"-type symbols in the latest kernel version. >> >> >>> + if (symbol_query("__insmod_", NULL, NULL)) >>> + st->flags |= INSMOD_BUILTIN; >>> + > ... >>> + if (MODULE_MEMORY()) { >>> + if (type == MOD_TEXT) >>> + last = MOD_RO_AFTER_INIT; >>> + else >>> + last = MOD_INIT_RODATA; >>> >> >> The above if-else code block is to speed up the searching performance? Or >> are there overlapped address spaces in different module types? > > To realize these: > > >> +#define IN_MODULE(A,L) (_in_module(A, L, MOD_TEXT)) > >> +#define IN_MODULE_INIT(A,L) (_in_module(A, L, MOD_INIT_TEXT)) > > but finally, these are replaced in 5/15: > https://listman.redhat.com/archives/crash-utility/2023-May/010653.html > > OK, got it. Thanks. > >>> >> >> BTW: I noticed that they have the similar code between the _in_module() > and >> module_mem_end(), if we can improve the _in_module() and return the end >> address of a module memory region from _in_module(), instead of only >> returning TRUE or FALSE, maybe the module_mem_end() can be removed. > > um, these funcions are changed some later, so I said "fixes are piled > up". sorry about that. > > No worries, Kazu. After all this is a significant change, we can not expect it to be perfect overnight. BTW: maybe some changes(patches) can be merged as one patch, and eventually some changes will only be made one time. That will help speed up the review of patches.