arm: mm: Update mem bank ordering comment to reflect code

From: Michael Bohan <[email protected]>

The memory banks are now sorted in bootmem_init() -- thus this code no longer needs fixing.

---
 arch/arm/mm/init.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

index 7829cb5..ab4890f 100644
@@ -531,9 +531,8 @@ static void __init free_unused_memmap_node(int node, struct meminfo *mi)
 	unsigned int i;
 
 	/*
-	 * [FIXME] This relies on each bank being in address order.  This
-	 * may not be the case, especially if the user has provided the
-	 * information on the command line.
+	 * This relies on each bank being in address order. The banks
+	 * are sorted previously in bootmem_init().
 	 */
 	for_each_nodebank(i, mi, node) {
 		struct membank *bank = &mi->bank[i];

1.6.2.3

From: Michael Bohan <[email protected]>

This option is for FLATMEM configurations with multiple memory banks that have end addresses not aligned to MAX_ORDER_NR_PAGES.

To save memory, ARM frees memmap regions for pfns associated with gaps in the address map. When the VM code migrates pages, it will assume that the memmap entries are valid.

This option adds extra checking with pfn_valid() to ensure the memmap entries are valid. It may adversely impact performance.

---
 arch/arm/Kconfig |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

index c5408bf..8dadce9 100644
@@ -1233,6 +1233,20 @@ config OABI_COMPAT
 config ARCH_HAS_HOLES_MEMORYMODEL
 	bool
 
+config HOLES_IN_ZONE
+	bool "Memory map has holes ending within MAX_ORDER_NR_PAGES"
+	default n
+	depends on FLATMEM
+	help
+	  Say 'y' here if you have multiple memory banks where the bank
+	  end addresses coresponding to the holes are not MAX_ORDER_NR_PAGES
+	  aligned. Failure to enable this in those circumstances will result
+	  in the VM code assuming ownership of pages with no memmap memory
+	  allocated for them. This option could impact performance as it
+	  performs extra checks in the VM code. As alternative, consider
+	  aligning end addresses to MAX_ORDER_NR_PAGES or changing MAX_ORDER
+	  to compensate for your alignment requirements.
+
 # Discontigmem is deprecated
 config ARCH_DISCONTIGMEM_ENABLE
 	bool

1.6.2.3

From: Michael Bohan <[email protected]>

In FLATMEM configurations, the VM code assumes that memory bank end addresses in a hole are MAX_ORDER_NR_PAGES aligned if CONFIG_HOLES_IN_ZONE is not enabled. The crash scenarios are hard to debug, so let's not proceed if we aren't configured right.

---
 arch/arm/mm/init.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

index ab4890f..3fee7d4 100644
@@ -254,6 +254,24 @@ static void __init bootmem_init_node(int node, struct meminfo *mi,
 	for_each_nodebank(i, mi, node) {
 		struct membank *bank = &mi->bank[i];
 
+#if defined(CONFIG_FLATMEM) && !defined(CONFIG_HOLES_IN_ZONE)
+		/*
+		 * The VM code assumes that hole end addresses are aligned if
+		 * CONFIG_HOLES_IN_ZONE is not enabled. This results in
+		 * panics since we free unused memmap entries on ARM.
+		 * This check shouldn't be necessary for the last bank's end
+		 * address, since the VM code accounts for the total zone size.
+		 */
+		if ((i < (mi->nr_banks - 1)) &&
+		    (bank_pfn_end(bank) & (MAX_ORDER_NR_PAGES - 1))) {
+			pr_err("Memory bank[%d] not aligned to 0x%x bytes.\n"
+			       "\tMake bank end address align with MAX_ORDER\n"
+			       "\tor enable option CONFIG_HOLES_IN_ZONE.\n",
+			       i, __pfn_to_phys(MAX_ORDER_NR_PAGES));
+			BUG();
+		}
+#endif
+
 		if (!bank->highmem)
 			map_memory_bank(bank);
 	}

1.6.2.3

It will impact performance although not necessarily in a way that's easy to detect because the biggest impact is on page-allocator-intensive-loads.

Have you managed to trigger this problem? I am assuming you are not referring to page migration as such but more likely when pages shuffle between lists in move_freepages(). To trigger a problem there, I think you would have to have a memory layout that had an max-order-unaligned-hole in the middle of a zone. Is that happening?

If so, another possibility you may want to consider (if you haven't already) is to ensure in free_memmap() that if holes are being punched in the middle of a zone that they are only max-order aligned. This would avoid corruption problems without taking a performance hit.

Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Yes, that is exactly what's happening. I communed with you some time ago regarding this problem. I see crashes in move_freepages_block() without this patch.

I did consider this. The only problem here is that for each unaligned memory hole, we waste some more amount of memory (8, 16, or 24KB) assuming 1 MB aligned end addresses and MAX_ORDER == 11. That doesn't sound that bad, but there's always the chance that somebody has a lot of
holes in their memory map. I suppose it's not truly 'wasted' in the
sense that it's necessary for correctness; and I agree that it may be better than taking a run-time performance hit.

As you suggested, it's not easy to quantify what sort of performance hit we're taking. Without solid data, it might be safer to go your suggested route of freeing only legitimate memmap entries. I will test on my end that this does indeed keep the allocator happy (eg. no crashes) and does not cause any bad observable side effects.

I guess the only questions for linux-arm-kernel is whether they would be okay with freeing a little less memmap in the case of unaligned bank end addresses, and whether they think this warrants an info message to the user during bootup that they should consider aligning them to save a little memory.

Thanks,
Michael