On 06/02/21 at 05:02am, HAGIO KAZUHITO(萩尾 一仁) wrote:

-----Original Message-----

quoted

On 06/02/21 at 01:11am, HAGIO KAZUHITO(萩尾 一仁) wrote:

quoted

-----Original Message-----

quoted

On Tue, 1 Jun 2021 10:40:09 +0200 David Hildenbrand [off-list ref] wrote:

quoted

quoted

Thanks, i explained the reason during my last reply.
Andrew has already picked this patch to -mm tree.

Just because it's in Andrews tree doesn't mean it will end up upstream. ;)

Anyhow, no really strong opinion, it's simply unnecessary code churn
that makes bisecting harder without real value IMHO.

I think it's a good change - mem_sections refers to multiple instances
of a mem_section.  Churn is a pain, but that's the price we pay for more
readable code.  And for having screwed it up originally ;)

From a makedumpfile/crash-utility viewpoint, I don't deny kernel improvement
and probably the change will not be hard for them to support, but I'd like
you to remember that the tool users will need to update them for the change.

As VIM user, I can understand Aisheng's feeling on the mem_section
variable which has the same symbol name as its type. Meanwhile it does
cause makedumpfile/crash having to be changed accordingly.

Maybe we can carry it when any essential change is needed in both kernel
and makedumpfile/crash around it.

Yes, that is a possible option.

quoted

quoted

The situation where we need to update the tools for new kernels is usual, but
there are not many cases that they cannot even start session, and this change

By the way, Kazu, about a case starting session, could you be more specific
or rephrase? I may not get it clearly. Thanks.

As for the current crash, the "mem_section" symbol is used to determine
which memory model is used.

        if (kernel_symbol_exists("mem_section"))
                vt->flags |= SPARSEMEM;
        else if (kernel_symbol_exists("mem_map")) {
                get_symbol_data("mem_map", sizeof(char *), &vt->mem_map);
                vt->flags |= FLATMEM;
        } else
                vt->flags |= DISCONTIGMEM;

So without updating, crash will assume that the memory model is DISCONTIGMEM,
fail during vm_init() and cannot start a session.  This is an imitation of
the situation though:

-       if (kernel_symbol_exists("mem_section"))
+       if (kernel_symbol_exists("mem_sectionX"))

# crash
...
crash: invalid structure member offset: pglist_data_node_mem_map
           FILE: memory.c  LINE: 16420  FUNCTION: dump_memory_nodes()

[/root/bin/crash] error trace: 465304 => 4ac2bf => 4aae19 => 57f4d7

  57f4d7: OFFSET_verify+164
  4aae19: dump_memory_nodes+5321
  4ac2bf: vm_init+4031
  465304: main_loop+392

#

Every time a kernel is released, there are some changes that crash can
start up with but cannot run a specific crash's command, but a change
that crash cannot start up like this case does not occur often.

Ah,I see. You mean this patch will cause startup failure of crash/makedumpfile
during application's earlier stage, and this is a severer situation than
others. Then we may need defer the patch acceptance to a future suitable
time. Thanks for explanation.

Also as for makedumpfile, the "SYMBOL(mem_section)" vmcore entry is used
to determine the memory model, so it will fail with the following error
without an update.

# ./makedumpfile --mem-usage /proc/kcore 
get_mem_map: Can't distinguish the memory type.

makedumpfile Failed.

Thanks,
Kazu