Re: [Crash-utility] [RFC PATCH 03/15] Make "sym -m" option print symbols in address order

On Fri, May 26, 2023 at 9:56 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: > 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(a)nec.com&gt; >>> 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?