On 2023/05/25 11:44, HAGIO KAZUHITO(萩尾 一仁) wrote:
> On 2023/05/24 16:10, lijiang wrote:
>> On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@nec.com>
>> wrote:
>>
>>> To "sym -m" print the symbols of a module in address order.
>>> (but "sym -l" and "sym -M" still print modules in text address order.)
>>>
>>>
>> The current text address order is better to me, basically it can keep the
>> same order with the definition mod_mem_type, which looks more natural. For
>> example:
>> crash> sym -m kvm |grep MODULE
>> ffffffffc136e000 MODULE TEXT START: kvm
>> ffffffffc13e0000 MODULE TEXT END: kvm
>> ffffffffc1ceb000 MODULE DATA START: kvm
>> ffffffffc1d6b000 MODULE DATA END: kvm
>> ffffffffc291a000 MODULE RODATA START: kvm
>> ffffffffc296c000 MODULE RODATA END: kvm
>> ffffffffc11cf000 MODULE RO_AFTER_INIT START: kvm
>> ffffffffc11d1000 MODULE RO_AFTER_INIT END: kvm
sorry, apparently I misunderstood your comments.
Does this mean that sorting memory types in a module by address is
unnecessary?
Yes. In fact, the above output is sorted naturally by the enum mod_mem_type order.
At least for me, the *text address order* is more friendly and readable to me.
>>
>> And the internal output in each module memory type is sorted by address as
>> below:
Yes, I see this.
>> crash> sym -m kvm
>> ffffffffc136e000 MODULE TEXT START: kvm
>> ffffffffc136e000 (T) __pfx___traceiter_kvm_userspace_exit
>> ffffffffc136e010 (T) __traceiter_kvm_userspace_exit
>> ffffffffc136e050 (T) __pfx___traceiter_kvm_vcpu_wakeup
>> ffffffffc136e060 (T) __traceiter_kvm_vcpu_wakeup
>> ffffffffc136e0b0 (T) __pfx___traceiter_kvm_set_irq
>> ffffffffc136e0c0 (T) __traceiter_kvm_set_irq
>> ffffffffc136e110 (T) __pfx___traceiter_kvm_ioapic_set_irq
>> ffffffffc136e120 (T) __traceiter_kvm_ioapic_set_irq
>> ffffffffc136e170 (T) __pfx___traceiter_kvm_ioapic_delayed_eoi_inj
>> ...
>> ffffffffc13df650 (t) kvm_x86_exit
>> ffffffffc13df650 (T) cleanup_module
>> ffffffffc13e0000 MODULE TEXT END: kvm
>>
>> In addition, the sym -m option will be consistent with the styles of sym
>> -l/-M options,
What does this mean?
Because you mentioned that(in patch log):
"(but "sym -l" and "sym -M" still print modules in text address order.)"
Without this change, the "sym -m" has also the same output style as the "sym -l/-M" options.
This patch's "sym -m" is already consistent with the "sym -l|-M" options:
crash-6.4> sym -m cdrom | grep MODULE
ffffffffc084d000 MODULE RO_AFTER_INIT START: cdrom
ffffffffc084e000 MODULE RO_AFTER_INIT END: cdrom
ffffffffc08da000 MODULE TEXT START: cdrom
ffffffffc08e1000 MODULE TEXT END: cdrom
ffffffffc0aa3000 MODULE RODATA START: cdrom
ffffffffc0aa8000 MODULE RODATA END: cdrom
ffffffffc0b04000 MODULE DATA START: cdrom
ffffffffc0b0b000 MODULE DATA END: cdrom
crash-6.4> sym -l | grep MODULE
...
ffffffffc084d000 MODULE RO_AFTER_INIT START: cdrom
ffffffffc084e000 MODULE RO_AFTER_INIT END: cdrom
ffffffffc08da000 MODULE TEXT START: cdrom
ffffffffc08e1000 MODULE TEXT END: cdrom
ffffffffc0aa3000 MODULE RODATA START: cdrom
ffffffffc0aa8000 MODULE RODATA END: cdrom
ffffffffc0b04000 MODULE DATA START: cdrom
ffffffffc0b0b000 MODULE DATA END: cdrom
...
> and really simplify the code.
Does this mean that we can remove lm->address_order and related code?
Yes, that is my original thoughts, but you mentioned the following patches will use it to speed up searching addresses. I'm not sure how much it would benefit as I do not have the test(performance) data.
I have another issue: the variable address_order is initialized once in the store_module_symbols_v3()(called by the module_init()), how does it sort the modules loaded by "mod -s" command in address order? The relevant data can be updated to the "address_order", or not needed?
btw, this facility is helpful to speed up searching addresses in some
codes, e.g. patch 05/15. I think this is a good preprocessing.
I don't have any performance data about it, although it might speed up searching addresses from the code perspective.
If the performance can significantly benefit from these changes, it's worth doing that. But we can still keep the text address order, no need to make the following changes, seems they have no conflicts?
@@ -1317,7 +1317,8 @@ module_symbol_dump(char *module)
if (received_SIGINT() || output_closed())
return;
- for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) {
+ for (m = 0; m < lm->nr_mems; m++) {
+ j = lm->address_order[m];
Thanks.
Lianbo