Re: A crash on ARM64 in move_freepages_block due to uninitialized pages in reserved memory

[James Morse <james.morse@arm.com>]

Hi Michal,

(CC: +Russell, we're trying to work out if ARCH_HAS_HOLES_MEMORYMODEL is still
necessary)

On 03/09/18 20:33, Michal Hocko wrote:
> On Wed 29-08-18 18:37:55, James Morse wrote:
>> On 24/08/18 12:41, Michal Hocko wrote:
>>> On Thu 23-08-18 15:06:08, James Morse wrote:
>>> [...]
>>>> My best-guess is that pfn_valid_within() shouldn't be optimised out if
>>> ARCH_HAS_HOLES_MEMORYMODEL, even if HOLES_IN_ZONE isn't set.

>> After plenty of greping, git-archaeology and help from others, I think I've a
>> clearer picture of what these options do.
>>
>> Please correct me if I've explained something wrong here:
>>
>>> This is the first time I hear about CONFIG_ARCH_HAS_HOLES_MEMORYMODEL.
>>
>> The comment in include/linux/mmzone.h describes this as relevant when parts the
>> memmap have been free()d. This would happen on systems where memory is smaller
>> than a sparsemem-section, and the extra struct pages are expensive.
>> pfn_valid() on these systems returns true for the whole sparsemem-section, so an
>> extra memmap_valid_within() check is needed.
> 
> I have hard times to find an actual code that does this partial memmap
> initialization.

arch/arm64/mm/init.c:free_unused_memmap(), once it has walked all the memblocks
does this with the space after the last one:
|#ifdef CONFIG_SPARSEMEM
|	if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION))
|		free_memmap(prev_end, ALIGN(prev_end, PAGES_PER_SECTION));
|#endif

prev_end is the pfn of the end of the last memblock, rounded up to
MAX_ORDER_NR_PAGES. If this isn't aligned to a section boundary, whole pages of
memmap between prev_end and the section boundary are freed.

(The memblock walker does something similar for the gaps between memblocks)


>> This is independent of nomap, and isn't relevant on arm64 as our pfn_valid()
>> always tests the page in memblock due to nomap pages, which can occur anywhere.
>> (I will propose a patch removing ARCH_HAS_HOLES_MEMORYMODEL for arm64.)
> 
> It seems ARCH_HAS_HOLES_MEMORYMODEL is only defined for arm and arm64.
> Is it really needed for arm?

I don't know much about arch/arm, but from grepping around: arch/arm does the
same thing as above with its free_unused_memmap(), so this partial memmap
initialisation can happen.

For 32bit ARCH_HAS_HOLES_MEMORYMODEL is something different boards/platforms
opt-into. But to match the partial memmap-initialisation case above it should be
selected if SPARSEMEM. Doing this would make HAVE_ARCH_PFN_VALID always true,
meaning the checks ARCH_HAS_HOLES_MEMORYMODEL enables never need running because
pfn_valid() already does them, at which point it can be removed.

The way it is makes sense if each board/platform knows where/how-much memory it
will have and can size FORCE_MAX_ZONEORDER so it doesn't get holes. But doesn't
this stuff all come from DT nowadays?

I think arch/arm should select ARCH_HAS_HOLES_MEMORYMODEL if USE_OF, but I don't
think this extra configurability is useful. Selecting it unconditionally would
let us remove it.


Digging through the history I think the original commit:
eb33575cf67d ("[ARM] Double check memmap is actually valid with a memmap has
unexpected holes V2")
Was working around the pfn_valid() behaviour that was changed with:
7b7bf499f79d (" ARM: 6913/1: sparsemem: allow pfn_valid to be overridden when
using SPARSEMEM")

The two users that describe their memory layout just want HAVE_ARCH_PFN_VALID:
59f181aa9d633 ("ARM: brcmstb: Enable ARCH_HAS_HOLES_MEMORYMODEL")
e511333212de4 ("ARM: highbank: select ARCH_HAS_HOLES_MEMORYMODEL")


Thanks,

James