On Mon, May 22, 2023 at 2:56 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio...@nec.com>
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.
Thanks
Lianbo
--
Crash-utility mailing list
Crash-utility@redhat.com
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
