On 2023/05/23 23:40, lijiang wrote:
> 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.
Tried to simplify them, please see the 2/15 thread.
>
>
>>
>>>> + 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.
Yes, this series was just a draft, will squash some next time.
(but there might also be a split e.g. a small bug fix in 9/15 [1])
[1] https://listman.redhat.com/archives/crash-utility/2023-May/010656.html
Thanks,
Kazu
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
