Hi Mikulas,

On 30/08/18 16:58, Mikulas Patocka wrote:

On Wed, 29 Aug 2018, James Morse wrote:

quoted

On 24/08/18 12:41, Michal Hocko wrote:

quoted

On Thu 23-08-18 15:06:08, James Morse wrote:
[...]

quoted

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.

quoted

Does something like this solve the problem?:
============================%<============================

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 32699b2dc52a..5e27095a15f4 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h

@@ -1295,7 +1295,7 @@ void memory_present(int nid, unsigned long start, unsigned

long end);
  * pfn_valid_within() should be used in this case; we optimise this away
  * when we have no holes within a MAX_ORDER_NR_PAGES block.
  */
-#ifdef CONFIG_HOLES_IN_ZONE
+#if defined(CONFIG_HOLES_IN_ZONE) || defined(CONFIG_ARCH_HAS_HOLES_MEMORYMODEL)
 #define pfn_valid_within(pfn) pfn_valid(pfn)
 #else
 #define pfn_valid_within(pfn) (1)
============================%<============================

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:

quoted

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.

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.)


HOLES_IN_ZONE is similar, if some memory is smaller than MAX_ORDER_NR_PAGES,
possibly due to nomap holes.

6d526ee26ccd only enabled it for NUMA systems on arm64, because the NUMA code
was first to fall foul of this, but there is nothing NUMA specific about nomap
holes within a MAX_ORDER_NR_PAGES region.

I'm convinced arm64 should always enable HOLES_IN_ZONE because nomap pages can
occur anywhere. I'll post a fix.

But x86 had the same bug -
https://bugzilla.redhat.com/show_bug.cgi?id=1598462

(Context: e181ae0c5db "mm: zero unavailable pages before memmap init")

Its the same symptom, but not quite the same bug.

And x86 fixed it without enabling HOLES_IN_ZONE. On x86, the BIOS can also 
reserve any memory range - so you can have arbitrary holes there that are 
not predictable when the kernel is compiled.

x86's pfn_valid() says the struct-page is accessible, the problem was it wasn't
initialized correctly.

On arm64 pfn_valid() says these struct-pages are not accessible. The problem was
the pfn_valid_within()->pfn_valid() calls being removed, causing the
uninitialized struct-page to be accessed.

Currently HOLES_IN_ZONE is selected only for ia64, mips/octeon - so does 
it mean that all the other architectures don't have holes in the memory 
map?

I think there is just more than way of handling these, depending on whether
holes have struct-pages and what pfn_valid() reports for them.

What should be architecture-independent way how to handle the holes?

We already diverge with e820/memblock. I'm not sure what the x86 holes
correspond to, but on arm64 these are holes in the linear-map because the
corresponding memory needs mapping with particular attributes, and we can't
mix-and-match.


Thanks,

James