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