On Thu, Jun 21, 2018 at 9:07 PM, Paul Burton <paul.burton@xxxxxxxx> wrote:
> Hi Daniel,
>
> Hmm... I only just noticed this because you CC'd an email address that
> is no longer functional. I presume you're not using .mailmap, which
> would have given you my current email address.
Hi Paul,
I do not remember exactly but I guess I used either get_maintainers
script or the email from your commit. I'm sorry for the inconvenience.
> On Fri, Mar 16, 2018 at 03:38:55PM +0100, Daniel Vacek wrote:
>> This reverts commit b92df1de5d289c0b5d653e72414bf0850b8511e0. The commit
>> is meant to be a boot init speed up skipping the loop in memmap_init_zone()
>> for invalid pfns. But given some specific memory mapping on x86_64 (or more
>> generally theoretically anywhere but on arm with CONFIG_HAVE_ARCH_PFN_VALID)
>
> My patch definitely wasn't ARM-specific & I have never tested it on ARM.
> It was motivated by a MIPS platform with an extremely sparse memory map.
> Could you explain why you think it depends on ARM or
> CONFIG_HAVE_ARCH_PFN_VALID?
Hopefully explained further below.
>> the implementation also skips valid pfns which is plain wrong and causes
>> 'kernel BUG at mm/page_alloc.c:1389!'
>
> Which VM_BUG_ON is that? I don't see one on line 1389 as of commit
> b92df1de5d28 ("mm: page_alloc: skip over regions of invalid pfns where
> possible") or any mainline final release since.
The report was from RHEL kernel actually. But it still applied to
upstream tree. It was this one
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/page_alloc.c?id=274a1ff0704bc8fef76dbe2d6fb197ddbc23f380#n1913
later changed with commit 3e04040df6d4 which I believe does not really
change or improve much anything, unfortunately...
>> crash> log | grep -e BUG -e RIP -e Call.Trace -e move_freepages_block -e rmqueue -e freelist -A1
>> kernel BUG at mm/page_alloc.c:1389!
>> invalid opcode: 0000 [#1] SMP
>> --
>> RIP: 0010:[<ffffffff8118833e>] [<ffffffff8118833e>] move_freepages+0x15e/0x160
>> RSP: 0018:ffff88054d727688 EFLAGS: 00010087
>> --
>> Call Trace:
>> [<ffffffff811883b3>] move_freepages_block+0x73/0x80
>> [<ffffffff81189e63>] __rmqueue+0x263/0x460
>> [<ffffffff8118c781>] get_page_from_freelist+0x7e1/0x9e0
>> [<ffffffff8118caf6>] __alloc_pages_nodemask+0x176/0x420
>> --
>> RIP [<ffffffff8118833e>] move_freepages+0x15e/0x160
>> RSP <ffff88054d727688>
>>
>> crash> page_init_bug -v | grep RAM
>> <struct resource 0xffff88067fffd2f8> 1000 - 9bfff System RAM (620.00 KiB)
>> <struct resource 0xffff88067fffd3a0> 100000 - 430bffff System RAM ( 1.05 GiB = 1071.75 MiB = 1097472.00 KiB)
>> <struct resource 0xffff88067fffd410> 4b0c8000 - 4bf9cfff System RAM ( 14.83 MiB = 15188.00 KiB)
>> <struct resource 0xffff88067fffd480> 4bfac000 - 646b1fff System RAM (391.02 MiB = 400408.00 KiB)
>> <struct resource 0xffff88067fffd560> 7b788000 - 7b7fffff System RAM (480.00 KiB)
>> <struct resource 0xffff88067fffd640> 100000000 - 67fffffff System RAM ( 22.00 GiB)
>>
>> crash> page_init_bug | head -6
>> <struct resource 0xffff88067fffd560> 7b788000 - 7b7fffff System RAM (480.00 KiB)
>> <struct page 0xffffea0001ede200> 1fffff00000000 0 <struct pglist_data 0xffff88047ffd9000> 1 <struct zone 0xffff88047ffd9800> DMA32 4096 1048575
>> <struct page 0xffffea0001ede200> 505736 505344 <struct page 0xffffea0001ed8000> 505855 <struct page 0xffffea0001edffc0>
>> <struct page 0xffffea0001ed8000> 0 0 <struct pglist_data 0xffff88047ffd9000> 0 <struct zone 0xffff88047ffd9000> DMA 1 4095
>> <struct page 0xffffea0001edffc0> 1fffff00000400 0 <struct pglist_data 0xffff88047ffd9000> 1 <struct zone 0xffff88047ffd9800> DMA32 4096 1048575
>> BUG, zones differ!
>>
>> crash> kmem -p 77fff000 78000000 7b5ff000 7b600000 7b787000 7b788000
>> PAGE PHYSICAL MAPPING INDEX CNT FLAGS
>> ffffea0001e00000 78000000 0 0 0 0
>> ffffea0001ed7fc0 7b5ff000 0 0 0 0
>> ffffea0001ed8000 7b600000 0 0 0 0 <<<<
>> ffffea0001ede1c0 7b787000 0 0 0 0
>> ffffea0001ede200 7b788000 0 0 1 1fffff00000000
>
> I'm not really sure what I'm looking at here. I presume you're saying
> that memmap_init_zone() didn't initialize the struct page for
> phys=0x7b788000?
Quite the opposite. It's the first one which gets correctly
initialized as it is the start of next usable range as returned by
memblock_next_valid_pfn(). Though early_pfn_valid() returns true for
all frames in this section starting with 0x78000 (at least on x86
where it is based on the memsection implementation) so the next valid
pfn should correctly be frame 0x78000 instead of 0x7b788. The crash
was caused by accessing page 0xffffea0001ed8000 (covering phys
0x7b600000) as move_freepages_block() aligns the start_pfn to
pageblock_nr_pages before calling move_freepages().
The arm implementation of early_pfn_valid() is actually based on
memblock and returns false for frames 0x78000 through 0x7b787 hence I
thought you based the memblock_next_valid_pfn() implementation on this
ARM semantics enabled by CONFIG_HAVE_ARCH_PFN_VALID instead of the
generic early_pfn_valid() version based on memory sections
implementation.
When I am thinking about it now, instead of reverting it could also
have been #ifdefed on CONFIG_HAVE_ARCH_PFN_VALID. That way ARM could
still use the advantage but not MIPS I believe.
> Could you describe the memblock region list, and what ranges
> memmap_init_zone() skipped over?
I guess that's already explained above. The memblock regions matched
the usable 'System RAM' ranges as dumped from iomem resources in my
commit message, IIRC. Let me dump the data if I can still find it.
crash> memblock.memory.cnt,memory.regions memblock
memory.cnt = 0x7,
memory.regions = 0xffffffff81af1140 <memblock_memory_init_regions>
crash> memblock_region.base,size,flags,nid
memblock_memory_init_regions 7 | sed 's/^ /\t/' | paste - - - - - |
column -ts' '
base = 0x1000 size = 0x9b000 flags = 0x0 nid = 0x0
base = 0x100000 size = 0x42fc0000 flags = 0x0 nid = 0x0
base = 0x4b0c8000 size = 0xed5000 flags = 0x0 nid = 0x0
base = 0x4bfac000 size = 0x18706000 flags = 0x0 nid = 0x0
base = 0x7b788000 size = 0x78000 flags = 0x0 nid = 0x0
base = 0x100000000 size = 0x380000000 flags = 0x0 nid = 0x0
base = 0x480000000 size = 0x200000000 flags = 0x0 nid = 0x1
Yeah, so it matches with the node break added.
> Thanks,
> Paul
Thank you for looking into it! If you have any further questions, just
drop me an email. And have a nice day.
--nX
