[PATCH v3 0/2] mm: fix initialization of struct page for holes in memory layout

[PATCH v3 0/2] mm: fix initialization of struct page for holes in memory layout

[Mike Rapoport]

From: Mike Rapoport <rppt@linux.ibm.com>

Hi,

Commit 73a6e474cb37 ("mm: memmap_init: iterate over
memblock regions rather that check each PFN") exposed several issues with
the memory map initialization and these patches fix those issues.

Initially there were crashes during compaction that Qian Cai reported back
in April [1]. It seemed back then that the problem was fixed, but a few
weeks ago Andrea Arcangeli hit the same bug [2] and there was an additional
discussion at [3].

v3: 
* use architectural zone constraints to set zone links for struct pages
  corresponding to the holes
* drop implicit update of memblock.memory
* add a patch that sets pfn 0 to E820_TYPE_RAM on x86

v2: https://lore.kernel.org/lkml/20201209214304.6812-1-rppt@kernel.org/):
* added patch that adds all regions in memblock.reserved that do not
overlap with memblock.memory to memblock.memory in the beginning of
free_area_init()

[1] https://lore.kernel.org/lkml/8C537EB7-85EE-4DCF-943E-3CC0ED0DF56D@lca.pw
[2] https://lore.kernel.org/lkml/20201121194506.13464-1-aarcange@redhat.com
[3] https://lore.kernel.org/mm-commits/20201206005401.qKuAVgOXr%akpm@linux-foundation.org

Mike Rapoport (2):
  x86/setup: don't remove E820_TYPE_RAM for pfn 0
  mm: fix initialization of struct page for holes in memory layout

 arch/x86/kernel/setup.c | 20 +++++-----
 mm/page_alloc.c         | 84 ++++++++++++++++++++++++-----------------
 2 files changed, 59 insertions(+), 45 deletions(-)

-- 
2.28.0

[PATCH v3 1/2] x86/setup: don't remove E820_TYPE_RAM for pfn 0

[Mike Rapoport]

From: Mike Rapoport <rppt@linux.ibm.com>

The first 4Kb of memory is a BIOS owned area and to avoid its allocation
for the kernel it was not listed in e820 tables as memory. As the result,
pfn 0 was never recognised by the generic memory management and it is not a
part of neither node 0 nor ZONE_DMA.

If set_pfnblock_flags_mask() would be ever called for the pageblock
corresponding to the first 2Mbytes of memory, having pfn 0 outside of
ZONE_DMA would trigger

	VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);

Along with reserving the first 4Kb in e820 tables, several first pages are
reserved with memblock in several places during setup_arch(). These
reservations are enough to ensure the kernel does not touch the BIOS area
and it is not necessary to remove E820_TYPE_RAM for pfn 0.

Remove the update of e820 table that changes the type of pfn 0 and move the
comment describing why it was done to trim_low_memory_range() that reserves
the beginning of the memory.

Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
---
 arch/x86/kernel/setup.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 740f3bdb3f61..3412c4595efd 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -660,17 +660,6 @@ static void __init trim_platform_memory_ranges(void)
 
 static void __init trim_bios_range(void)
 {
-	/*
-	 * A special case is the first 4Kb of memory;
-	 * This is a BIOS owned area, not kernel ram, but generally
-	 * not listed as such in the E820 table.
-	 *
-	 * This typically reserves additional memory (64KiB by default)
-	 * since some BIOSes are known to corrupt low memory.  See the
-	 * Kconfig help text for X86_RESERVE_LOW.
-	 */
-	e820__range_update(0, PAGE_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-
 	/*
 	 * special case: Some BIOSes report the PC BIOS
 	 * area (640Kb -> 1Mb) as RAM even though it is not.
@@ -728,6 +717,15 @@ early_param("reservelow", parse_reservelow);
 
 static void __init trim_low_memory_range(void)
 {
+	/*
+	 * A special case is the first 4Kb of memory;
+	 * This is a BIOS owned area, not kernel ram, but generally
+	 * not listed as such in the E820 table.
+	 *
+	 * This typically reserves additional memory (64KiB by default)
+	 * since some BIOSes are known to corrupt low memory.  See the
+	 * Kconfig help text for X86_RESERVE_LOW.
+	 */
 	memblock_reserve(0, ALIGN(reserve_low, PAGE_SIZE));
 }
 	
-- 
2.28.0

Re: [PATCH v3 1/2] x86/setup: don't remove E820_TYPE_RAM for pfn 0

[Oscar Salvador]

On Mon, Jan 11, 2021 at 09:40:16PM +0200, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> The first 4Kb of memory is a BIOS owned area and to avoid its allocation
> for the kernel it was not listed in e820 tables as memory. As the result,
> pfn 0 was never recognised by the generic memory management and it is not a
> part of neither node 0 nor ZONE_DMA.

So, since it never was added to memblock.memory structs, it was not
initialized by init_unavailable_mem, right?

-- 
Oscar Salvador
SUSE L3

Re: [PATCH v3 1/2] x86/setup: don't remove E820_TYPE_RAM for pfn 0

[Mike Rapoport]

On Wed, Jan 13, 2021 at 09:56:49AM +0100, Oscar Salvador wrote:
> On Mon, Jan 11, 2021 at 09:40:16PM +0200, Mike Rapoport wrote:
> > From: Mike Rapoport <rppt@linux.ibm.com>
> > 
> > The first 4Kb of memory is a BIOS owned area and to avoid its allocation
> > for the kernel it was not listed in e820 tables as memory. As the result,
> > pfn 0 was never recognised by the generic memory management and it is not a
> > part of neither node 0 nor ZONE_DMA.
> 
> So, since it never was added to memblock.memory structs, it was not
> initialized by init_unavailable_mem, right?

Actually it was initialized by init_unavailable_mem() and got zone=0 and
node=0, but the DMA zone started from pfn 1, so pfn 0 was never a part of
ZONE_DMA.
 
> -- 
> Oscar Salvador
> SUSE L3

-- 
Sincerely yours,
Mike.

Re: [PATCH v3 1/2] x86/setup: don't remove E820_TYPE_RAM for pfn 0

[David Hildenbrand]

On 11.01.21 20:40, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> The first 4Kb of memory is a BIOS owned area and to avoid its allocation
> for the kernel it was not listed in e820 tables as memory. As the result,
> pfn 0 was never recognised by the generic memory management and it is not a
> part of neither node 0 nor ZONE_DMA.
> 
> If set_pfnblock_flags_mask() would be ever called for the pageblock
> corresponding to the first 2Mbytes of memory, having pfn 0 outside of
> ZONE_DMA would trigger
> 
> 	VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
> 
> Along with reserving the first 4Kb in e820 tables, several first pages are
> reserved with memblock in several places during setup_arch(). These
> reservations are enough to ensure the kernel does not touch the BIOS area
> and it is not necessary to remove E820_TYPE_RAM for pfn 0.
> 
> Remove the update of e820 table that changes the type of pfn 0 and move the
> comment describing why it was done to trim_low_memory_range() that reserves
> the beginning of the memory.
> 
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
>  arch/x86/kernel/setup.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 740f3bdb3f61..3412c4595efd 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -660,17 +660,6 @@ static void __init trim_platform_memory_ranges(void)
>  
>  static void __init trim_bios_range(void)
>  {
> -	/*
> -	 * A special case is the first 4Kb of memory;
> -	 * This is a BIOS owned area, not kernel ram, but generally
> -	 * not listed as such in the E820 table.
> -	 *
> -	 * This typically reserves additional memory (64KiB by default)
> -	 * since some BIOSes are known to corrupt low memory.  See the
> -	 * Kconfig help text for X86_RESERVE_LOW.
> -	 */
> -	e820__range_update(0, PAGE_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> -
>  	/*
>  	 * special case: Some BIOSes report the PC BIOS
>  	 * area (640Kb -> 1Mb) as RAM even though it is not.
> @@ -728,6 +717,15 @@ early_param("reservelow", parse_reservelow);
>  
>  static void __init trim_low_memory_range(void)
>  {
> +	/*
> +	 * A special case is the first 4Kb of memory;
> +	 * This is a BIOS owned area, not kernel ram, but generally
> +	 * not listed as such in the E820 table.
> +	 *
> +	 * This typically reserves additional memory (64KiB by default)
> +	 * since some BIOSes are known to corrupt low memory.  See the
> +	 * Kconfig help text for X86_RESERVE_LOW.
> +	 */
>  	memblock_reserve(0, ALIGN(reserve_low, PAGE_SIZE));
>  }
>  	
> 

The only somewhat-confusing thing is that in-between
e820__memblock_setup() and trim_low_memory_range(), we already have
memblock allocations. So [0..4095] might look like ordinary memory until
we reserve it later on.

E.g., reserve_real_mode() does a

mem = memblock_find_in_range(0, 1<<20, size, PAGE_SIZE);
...
memblock_reserve(mem, size);
set_real_mode_mem(mem);

which looks kind of suspicious to me. Most probably I am missing
something, just wanted to point that out. We might want to do such
trimming/adjustments before any kind of allocations.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/2] x86/setup: don't remove E820_TYPE_RAM for pfn 0

[Mike Rapoport]

On Wed, Jan 13, 2021 at 01:56:45PM +0100, David Hildenbrand wrote:
> On 11.01.21 20:40, Mike Rapoport wrote:
> > From: Mike Rapoport <rppt@linux.ibm.com>
> > 
> > The first 4Kb of memory is a BIOS owned area and to avoid its allocation
> > for the kernel it was not listed in e820 tables as memory. As the result,
> > pfn 0 was never recognised by the generic memory management and it is not a
> > part of neither node 0 nor ZONE_DMA.
> > 
> > If set_pfnblock_flags_mask() would be ever called for the pageblock
> > corresponding to the first 2Mbytes of memory, having pfn 0 outside of
> > ZONE_DMA would trigger
> > 
> > 	VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
> > 
> > Along with reserving the first 4Kb in e820 tables, several first pages are
> > reserved with memblock in several places during setup_arch(). These
> > reservations are enough to ensure the kernel does not touch the BIOS area
> > and it is not necessary to remove E820_TYPE_RAM for pfn 0.
> > 
> > Remove the update of e820 table that changes the type of pfn 0 and move the
> > comment describing why it was done to trim_low_memory_range() that reserves
> > the beginning of the memory.
> > 
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > ---
> >  arch/x86/kernel/setup.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 740f3bdb3f61..3412c4595efd 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -660,17 +660,6 @@ static void __init trim_platform_memory_ranges(void)
> >  
> >  static void __init trim_bios_range(void)
> >  {
> > -	/*
> > -	 * A special case is the first 4Kb of memory;
> > -	 * This is a BIOS owned area, not kernel ram, but generally
> > -	 * not listed as such in the E820 table.
> > -	 *
> > -	 * This typically reserves additional memory (64KiB by default)
> > -	 * since some BIOSes are known to corrupt low memory.  See the
> > -	 * Kconfig help text for X86_RESERVE_LOW.
> > -	 */
> > -	e820__range_update(0, PAGE_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> > -
> >  	/*
> >  	 * special case: Some BIOSes report the PC BIOS
> >  	 * area (640Kb -> 1Mb) as RAM even though it is not.
> > @@ -728,6 +717,15 @@ early_param("reservelow", parse_reservelow);
> >  
> >  static void __init trim_low_memory_range(void)
> >  {
> > +	/*
> > +	 * A special case is the first 4Kb of memory;
> > +	 * This is a BIOS owned area, not kernel ram, but generally
> > +	 * not listed as such in the E820 table.
> > +	 *
> > +	 * This typically reserves additional memory (64KiB by default)
> > +	 * since some BIOSes are known to corrupt low memory.  See the
> > +	 * Kconfig help text for X86_RESERVE_LOW.
> > +	 */
> >  	memblock_reserve(0, ALIGN(reserve_low, PAGE_SIZE));
> >  }
> >  	
> > 
> 
> The only somewhat-confusing thing is that in-between
> e820__memblock_setup() and trim_low_memory_range(), we already have
> memblock allocations. So [0..4095] might look like ordinary memory until
> we reserve it later on.
> 
> E.g., reserve_real_mode() does a
> 
> mem = memblock_find_in_range(0, 1<<20, size, PAGE_SIZE);
> ...
> memblock_reserve(mem, size);
> set_real_mode_mem(mem);
> 
> which looks kind of suspicious to me. Most probably I am missing
> something, just wanted to point that out. We might want to do such
> trimming/adjustments before any kind of allocations.

You are right and it looks suspicious, but the first page is reserved at
the very beginning of x86::setup_arch() and, moreover, memblock never
allocates it (look at memblock::memblock_find_in_range_node()).

As for the range 0x1000 <-> reserve_low, we are unlikely to allocate it in
the default top-down mode. The bottom-up mode was only allocating memory
above the kernel so this would also prevent allocation of the lowest
memory, at least until the recent changes for CMA allocation:

https://lore.kernel.org/lkml/20201217201214.3414100-1-guro@fb.com

That said, we'd better consolidate all the trim_some_memory() and move it
closer to the beginning of setup_arch().
I'm going to take a look at it in the next few days.
 
> -- 
> Thanks,
> 
> David / dhildenb
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v3 1/2] x86/setup: don't remove E820_TYPE_RAM for pfn 0

[Borislav Petkov]

On Mon, Jan 11, 2021 at 09:40:16PM +0200, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> The first 4Kb of memory is a BIOS owned area and to avoid its allocation
> for the kernel it was not listed in e820 tables as memory. As the result,
> pfn 0 was never recognised by the generic memory management and it is not a
> part of neither node 0 nor ZONE_DMA.
> 
> If set_pfnblock_flags_mask() would be ever called for the pageblock
> corresponding to the first 2Mbytes of memory, having pfn 0 outside of
> ZONE_DMA would trigger
> 
> 	VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
> 
> Along with reserving the first 4Kb in e820 tables, several first pages are
> reserved with memblock in several places during setup_arch(). These
> reservations are enough to ensure the kernel does not touch the BIOS area
> and it is not necessary to remove E820_TYPE_RAM for pfn 0.
> 
> Remove the update of e820 table that changes the type of pfn 0 and move the
> comment describing why it was done to trim_low_memory_range() that reserves
> the beginning of the memory.
> 
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
>  arch/x86/kernel/setup.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)

FWIW,

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v3 2/2] mm: fix initialization of struct page for holes in memory layout

[Mike Rapoport]

From: Mike Rapoport <rppt@linux.ibm.com>

There could be struct pages that are not backed by actual physical memory.
This can happen when the actual memory bank is not a multiple of
SECTION_SIZE or when an architecture does not register memory holes
reserved by the firmware as memblock.memory.

Such pages are currently initialized using init_unavailable_mem() function
that iterates through PFNs in holes in memblock.memory and if there is a
struct page corresponding to a PFN, the fields if this page are set to
default values and the page is marked as Reserved.

init_unavailable_mem() does not take into account zone and node the page
belongs to and sets both zone and node links in struct page to zero.

On a system that has firmware reserved holes in a zone above ZONE_DMA, for
instance in a configuration below:

	# grep -A1 E820 /proc/iomem
	7a17b000-7a216fff : Unknown E820 type
	7a217000-7bffffff : System RAM

unset zone link in struct page will trigger

	VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);

because there are pages in both ZONE_DMA32 and ZONE_DMA (unset zone link in
struct page) in the same pageblock.

Update init_unavailable_mem() to use zone constraints defined by an
architecture to properly setup the zone link and use node ID of the
adjacent range in memblock.memory to set the node link.

Fixes: 73a6e474cb37 ("mm: memmap_init: iterate over memblock regions rather that check each PFN")
Reported-by: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
---
 mm/page_alloc.c | 84 +++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bdbec4c98173..0b56c3ca354e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7077,23 +7077,26 @@ void __init free_area_init_memoryless_node(int nid)
  * Initialize all valid struct pages in the range [spfn, epfn) and mark them
  * PageReserved(). Return the number of struct pages that were initialized.
  */
-static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn)
+static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn,
+					 int zone, int nid)
 {
-	unsigned long pfn;
+	unsigned long pfn, zone_spfn, zone_epfn;
 	u64 pgcnt = 0;
 
+	zone_spfn = arch_zone_lowest_possible_pfn[zone];
+	zone_epfn = arch_zone_highest_possible_pfn[zone];
+
+	spfn = clamp(spfn, zone_spfn, zone_epfn);
+	epfn = clamp(epfn, zone_spfn, zone_epfn);
+
 	for (pfn = spfn; pfn < epfn; pfn++) {
 		if (!pfn_valid(ALIGN_DOWN(pfn, pageblock_nr_pages))) {
 			pfn = ALIGN_DOWN(pfn, pageblock_nr_pages)
 				+ pageblock_nr_pages - 1;
 			continue;
 		}
-		/*
-		 * Use a fake node/zone (0) for now. Some of these pages
-		 * (in memblock.reserved but not in memblock.memory) will
-		 * get re-initialized via reserve_bootmem_region() later.
-		 */
-		__init_single_page(pfn_to_page(pfn), pfn, 0, 0);
+
+		__init_single_page(pfn_to_page(pfn), pfn, zone, nid);
 		__SetPageReserved(pfn_to_page(pfn));
 		pgcnt++;
 	}
@@ -7102,51 +7105,64 @@ static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn)
 }
 
 /*
- * Only struct pages that are backed by physical memory are zeroed and
- * initialized by going through __init_single_page(). But, there are some
- * struct pages which are reserved in memblock allocator and their fields
- * may be accessed (for example page_to_pfn() on some configuration accesses
- * flags). We must explicitly initialize those struct pages.
+ * Only struct pages that correspond to ranges defined by memblock.memory
+ * are zeroed and initialized by going through __init_single_page() during
+ * memmap_init().
+ *
+ * But, there could be struct pages that correspond to holes in
+ * memblock.memory. This can happen because of the following reasons:
+ * - phyiscal memory bank size is not necessarily the exact multiple of the
+ *   arbitrary section size
+ * - early reserved memory may not be listed in memblock.memory
+ * - memory layouts defined with memmap= kernel parameter may not align
+ *   nicely with memmap sections
  *
- * This function also addresses a similar issue where struct pages are left
- * uninitialized because the physical address range is not covered by
- * memblock.memory or memblock.reserved. That could happen when memblock
- * layout is manually configured via memmap=, or when the highest physical
- * address (max_pfn) does not end on a section boundary.
+ * Explicitly initialize those struct pages so that:
+ * - PG_Reserved is set
+ * - zone link is set accorging to the architecture constrains
+ * - node is set to node id of the next populated region except for the
+ *   trailing hole where last node id is used
  */
-static void __init init_unavailable_mem(void)
+static void __init init_zone_unavailable_mem(int zone)
 {
-	phys_addr_t start, end;
-	u64 i, pgcnt;
-	phys_addr_t next = 0;
+	unsigned long start, end;
+	int i, nid;
+	u64 pgcnt;
+	unsigned long next = 0;
 
 	/*
-	 * Loop through unavailable ranges not covered by memblock.memory.
+	 * Loop through holes in memblock.memory and initialize struct
+	 * pages corresponding to these holes
 	 */
 	pgcnt = 0;
-	for_each_mem_range(i, &start, &end) {
+	for_each_mem_pfn_range(i, MAX_NUMNODES, &start, &end, &nid) {
 		if (next < start)
-			pgcnt += init_unavailable_range(PFN_DOWN(next),
-							PFN_UP(start));
+			pgcnt += init_unavailable_range(next, start, zone, nid);
 		next = end;
 	}
 
 	/*
-	 * Early sections always have a fully populated memmap for the whole
-	 * section - see pfn_valid(). If the last section has holes at the
-	 * end and that section is marked "online", the memmap will be
-	 * considered initialized. Make sure that memmap has a well defined
-	 * state.
+	 * Last section may surpass the actual end of memory (e.g. we can
+	 * have 1Gb section and 512Mb of RAM pouplated).
+	 * Make sure that memmap has a well defined state in this case.
 	 */
-	pgcnt += init_unavailable_range(PFN_DOWN(next),
-					round_up(max_pfn, PAGES_PER_SECTION));
+	end = round_up(max_pfn, PAGES_PER_SECTION);
+	pgcnt += init_unavailable_range(next, end, zone, nid);
 
 	/*
 	 * Struct pages that do not have backing memory. This could be because
 	 * firmware is using some of this memory, or for some other reasons.
 	 */
 	if (pgcnt)
-		pr_info("Zeroed struct page in unavailable ranges: %lld pages", pgcnt);
+		pr_info("Zone %s: zeroed struct page in unavailable ranges: %lld pages", zone_names[zone], pgcnt);
+}
+
+static void __init init_unavailable_mem(void)
+{
+	int zone;
+
+	for (zone = 0; zone < ZONE_MOVABLE; zone++)
+		init_zone_unavailable_mem(zone);
 }
 #else
 static inline void __init init_unavailable_mem(void)
-- 
2.28.0

Re: [PATCH v3 2/2] mm: fix initialization of struct page for holes in memory layout

[David Hildenbrand]

On 11.01.21 20:40, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> There could be struct pages that are not backed by actual physical memory.
> This can happen when the actual memory bank is not a multiple of
> SECTION_SIZE or when an architecture does not register memory holes
> reserved by the firmware as memblock.memory.
> 
> Such pages are currently initialized using init_unavailable_mem() function
> that iterates through PFNs in holes in memblock.memory and if there is a
> struct page corresponding to a PFN, the fields if this page are set to
> default values and the page is marked as Reserved.
> 
> init_unavailable_mem() does not take into account zone and node the page
> belongs to and sets both zone and node links in struct page to zero.
> 
> On a system that has firmware reserved holes in a zone above ZONE_DMA, for
> instance in a configuration below:
> 
> 	# grep -A1 E820 /proc/iomem
> 	7a17b000-7a216fff : Unknown E820 type
> 	7a217000-7bffffff : System RAM
> 
> unset zone link in struct page will trigger
> 
> 	VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
> 
> because there are pages in both ZONE_DMA32 and ZONE_DMA (unset zone link in
> struct page) in the same pageblock.
> 
> Update init_unavailable_mem() to use zone constraints defined by an
> architecture to properly setup the zone link and use node ID of the
> adjacent range in memblock.memory to set the node link.
> 
> Fixes: 73a6e474cb37 ("mm: memmap_init: iterate over memblock regions rather that check each PFN")
> Reported-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
>   mm/page_alloc.c | 84 +++++++++++++++++++++++++++++--------------------
>   1 file changed, 50 insertions(+), 34 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index bdbec4c98173..0b56c3ca354e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7077,23 +7077,26 @@ void __init free_area_init_memoryless_node(int nid)
>    * Initialize all valid struct pages in the range [spfn, epfn) and mark them
>    * PageReserved(). Return the number of struct pages that were initialized.
>    */
> -static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn)
> +static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn,
> +					 int zone, int nid)
>   {
> -	unsigned long pfn;
> +	unsigned long pfn, zone_spfn, zone_epfn;
>   	u64 pgcnt = 0;
>   
> +	zone_spfn = arch_zone_lowest_possible_pfn[zone];
> +	zone_epfn = arch_zone_highest_possible_pfn[zone];
> +
> +	spfn = clamp(spfn, zone_spfn, zone_epfn);
> +	epfn = clamp(epfn, zone_spfn, zone_epfn);
> +
>   	for (pfn = spfn; pfn < epfn; pfn++) {
>   		if (!pfn_valid(ALIGN_DOWN(pfn, pageblock_nr_pages))) {
>   			pfn = ALIGN_DOWN(pfn, pageblock_nr_pages)
>   				+ pageblock_nr_pages - 1;
>   			continue;
>   		}
> -		/*
> -		 * Use a fake node/zone (0) for now. Some of these pages
> -		 * (in memblock.reserved but not in memblock.memory) will
> -		 * get re-initialized via reserve_bootmem_region() later.
> -		 */
> -		__init_single_page(pfn_to_page(pfn), pfn, 0, 0);
> +
> +		__init_single_page(pfn_to_page(pfn), pfn, zone, nid);
>   		__SetPageReserved(pfn_to_page(pfn));
>   		pgcnt++;
>   	}
> @@ -7102,51 +7105,64 @@ static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn)
>   }
>   
>   /*
> - * Only struct pages that are backed by physical memory are zeroed and
> - * initialized by going through __init_single_page(). But, there are some
> - * struct pages which are reserved in memblock allocator and their fields
> - * may be accessed (for example page_to_pfn() on some configuration accesses
> - * flags). We must explicitly initialize those struct pages.
> + * Only struct pages that correspond to ranges defined by memblock.memory
> + * are zeroed and initialized by going through __init_single_page() during
> + * memmap_init().
> + *
> + * But, there could be struct pages that correspond to holes in
> + * memblock.memory. This can happen because of the following reasons:
> + * - phyiscal memory bank size is not necessarily the exact multiple of the
> + *   arbitrary section size
> + * - early reserved memory may not be listed in memblock.memory
> + * - memory layouts defined with memmap= kernel parameter may not align
> + *   nicely with memmap sections
>    *
> - * This function also addresses a similar issue where struct pages are left
> - * uninitialized because the physical address range is not covered by
> - * memblock.memory or memblock.reserved. That could happen when memblock
> - * layout is manually configured via memmap=, or when the highest physical
> - * address (max_pfn) does not end on a section boundary.
> + * Explicitly initialize those struct pages so that:
> + * - PG_Reserved is set
> + * - zone link is set accorging to the architecture constrains
> + * - node is set to node id of the next populated region except for the
> + *   trailing hole where last node id is used
>    */
> -static void __init init_unavailable_mem(void)
> +static void __init init_zone_unavailable_mem(int zone)
>   {
> -	phys_addr_t start, end;
> -	u64 i, pgcnt;
> -	phys_addr_t next = 0;
> +	unsigned long start, end;
> +	int i, nid;
> +	u64 pgcnt;
> +	unsigned long next = 0;
>   
>   	/*
> -	 * Loop through unavailable ranges not covered by memblock.memory.
> +	 * Loop through holes in memblock.memory and initialize struct
> +	 * pages corresponding to these holes
>   	 */
>   	pgcnt = 0;
> -	for_each_mem_range(i, &start, &end) {
> +	for_each_mem_pfn_range(i, MAX_NUMNODES, &start, &end, &nid) {
>   		if (next < start)
> -			pgcnt += init_unavailable_range(PFN_DOWN(next),
> -							PFN_UP(start));
> +			pgcnt += init_unavailable_range(next, start, zone, nid);
>   		next = end;
>   	}
>   
>   	/*
> -	 * Early sections always have a fully populated memmap for the whole
> -	 * section - see pfn_valid(). If the last section has holes at the
> -	 * end and that section is marked "online", the memmap will be
> -	 * considered initialized. Make sure that memmap has a well defined
> -	 * state.
> +	 * Last section may surpass the actual end of memory (e.g. we can
> +	 * have 1Gb section and 512Mb of RAM pouplated).
> +	 * Make sure that memmap has a well defined state in this case.
>   	 */
> -	pgcnt += init_unavailable_range(PFN_DOWN(next),
> -					round_up(max_pfn, PAGES_PER_SECTION));
> +	end = round_up(max_pfn, PAGES_PER_SECTION);
> +	pgcnt += init_unavailable_range(next, end, zone, nid);
>   
>   	/*
>   	 * Struct pages that do not have backing memory. This could be because
>   	 * firmware is using some of this memory, or for some other reasons.
>   	 */
>   	if (pgcnt)
> -		pr_info("Zeroed struct page in unavailable ranges: %lld pages", pgcnt);
> +		pr_info("Zone %s: zeroed struct page in unavailable ranges: %lld pages", zone_names[zone], pgcnt);
> +}
> +
> +static void __init init_unavailable_mem(void)
> +{
> +	int zone;
> +
> +	for (zone = 0; zone < ZONE_MOVABLE; zone++)
> +		init_zone_unavailable_mem(zone);

Why < ZONE_MOVABLE?

I remember we can have memory holes inside the movable zone when messing 
with "movablecore" cmdline parameter.


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 2/2] mm: fix initialization of struct page for holes in memory layout

[Baoquan He]

On 02/01/21 at 10:14am, David Hildenbrand wrote:
> On 11.01.21 20:40, Mike Rapoport wrote:
> > From: Mike Rapoport <rppt@linux.ibm.com>
> > 
> > There could be struct pages that are not backed by actual physical memory.
> > This can happen when the actual memory bank is not a multiple of
> > SECTION_SIZE or when an architecture does not register memory holes
> > reserved by the firmware as memblock.memory.
> > 
> > Such pages are currently initialized using init_unavailable_mem() function
> > that iterates through PFNs in holes in memblock.memory and if there is a
> > struct page corresponding to a PFN, the fields if this page are set to
> > default values and the page is marked as Reserved.
> > 
> > init_unavailable_mem() does not take into account zone and node the page
> > belongs to and sets both zone and node links in struct page to zero.
> > 
> > On a system that has firmware reserved holes in a zone above ZONE_DMA, for
> > instance in a configuration below:
> > 
> > 	# grep -A1 E820 /proc/iomem
> > 	7a17b000-7a216fff : Unknown E820 type
> > 	7a217000-7bffffff : System RAM
> > 
> > unset zone link in struct page will trigger
> > 
> > 	VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
> > 
> > because there are pages in both ZONE_DMA32 and ZONE_DMA (unset zone link in
> > struct page) in the same pageblock.
> > 
> > Update init_unavailable_mem() to use zone constraints defined by an
> > architecture to properly setup the zone link and use node ID of the
> > adjacent range in memblock.memory to set the node link.
> > 
> > Fixes: 73a6e474cb37 ("mm: memmap_init: iterate over memblock regions rather that check each PFN")
> > Reported-by: Andrea Arcangeli <aarcange@redhat.com>
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > ---
> >   mm/page_alloc.c | 84 +++++++++++++++++++++++++++++--------------------
> >   1 file changed, 50 insertions(+), 34 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index bdbec4c98173..0b56c3ca354e 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -7077,23 +7077,26 @@ void __init free_area_init_memoryless_node(int nid)
> >    * Initialize all valid struct pages in the range [spfn, epfn) and mark them
> >    * PageReserved(). Return the number of struct pages that were initialized.
> >    */
> > -static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn)
> > +static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn,
> > +					 int zone, int nid)
> >   {
> > -	unsigned long pfn;
> > +	unsigned long pfn, zone_spfn, zone_epfn;
> >   	u64 pgcnt = 0;
> > +	zone_spfn = arch_zone_lowest_possible_pfn[zone];
> > +	zone_epfn = arch_zone_highest_possible_pfn[zone];
> > +
> > +	spfn = clamp(spfn, zone_spfn, zone_epfn);
> > +	epfn = clamp(epfn, zone_spfn, zone_epfn);
> > +
> >   	for (pfn = spfn; pfn < epfn; pfn++) {
> >   		if (!pfn_valid(ALIGN_DOWN(pfn, pageblock_nr_pages))) {
> >   			pfn = ALIGN_DOWN(pfn, pageblock_nr_pages)
> >   				+ pageblock_nr_pages - 1;
> >   			continue;
> >   		}
> > -		/*
> > -		 * Use a fake node/zone (0) for now. Some of these pages
> > -		 * (in memblock.reserved but not in memblock.memory) will
> > -		 * get re-initialized via reserve_bootmem_region() later.
> > -		 */
> > -		__init_single_page(pfn_to_page(pfn), pfn, 0, 0);
> > +
> > +		__init_single_page(pfn_to_page(pfn), pfn, zone, nid);
> >   		__SetPageReserved(pfn_to_page(pfn));
> >   		pgcnt++;
> >   	}
> > @@ -7102,51 +7105,64 @@ static u64 __init init_unavailable_range(unsigned long spfn, unsigned long epfn)
> >   }
> >   /*
> > - * Only struct pages that are backed by physical memory are zeroed and
> > - * initialized by going through __init_single_page(). But, there are some
> > - * struct pages which are reserved in memblock allocator and their fields
> > - * may be accessed (for example page_to_pfn() on some configuration accesses
> > - * flags). We must explicitly initialize those struct pages.
> > + * Only struct pages that correspond to ranges defined by memblock.memory
> > + * are zeroed and initialized by going through __init_single_page() during
> > + * memmap_init().
> > + *
> > + * But, there could be struct pages that correspond to holes in
> > + * memblock.memory. This can happen because of the following reasons:
> > + * - phyiscal memory bank size is not necessarily the exact multiple of the
> > + *   arbitrary section size
> > + * - early reserved memory may not be listed in memblock.memory
> > + * - memory layouts defined with memmap= kernel parameter may not align
> > + *   nicely with memmap sections
> >    *
> > - * This function also addresses a similar issue where struct pages are left
> > - * uninitialized because the physical address range is not covered by
> > - * memblock.memory or memblock.reserved. That could happen when memblock
> > - * layout is manually configured via memmap=, or when the highest physical
> > - * address (max_pfn) does not end on a section boundary.
> > + * Explicitly initialize those struct pages so that:
> > + * - PG_Reserved is set
> > + * - zone link is set accorging to the architecture constrains
> > + * - node is set to node id of the next populated region except for the
> > + *   trailing hole where last node id is used
> >    */
> > -static void __init init_unavailable_mem(void)
> > +static void __init init_zone_unavailable_mem(int zone)
> >   {
> > -	phys_addr_t start, end;
> > -	u64 i, pgcnt;
> > -	phys_addr_t next = 0;
> > +	unsigned long start, end;
> > +	int i, nid;
> > +	u64 pgcnt;
> > +	unsigned long next = 0;
> >   	/*
> > -	 * Loop through unavailable ranges not covered by memblock.memory.
> > +	 * Loop through holes in memblock.memory and initialize struct
> > +	 * pages corresponding to these holes
> >   	 */
> >   	pgcnt = 0;
> > -	for_each_mem_range(i, &start, &end) {
> > +	for_each_mem_pfn_range(i, MAX_NUMNODES, &start, &end, &nid) {
> >   		if (next < start)
> > -			pgcnt += init_unavailable_range(PFN_DOWN(next),
> > -							PFN_UP(start));
> > +			pgcnt += init_unavailable_range(next, start, zone, nid);
> >   		next = end;
> >   	}
> >   	/*
> > -	 * Early sections always have a fully populated memmap for the whole
> > -	 * section - see pfn_valid(). If the last section has holes at the
> > -	 * end and that section is marked "online", the memmap will be
> > -	 * considered initialized. Make sure that memmap has a well defined
> > -	 * state.
> > +	 * Last section may surpass the actual end of memory (e.g. we can
> > +	 * have 1Gb section and 512Mb of RAM pouplated).
> > +	 * Make sure that memmap has a well defined state in this case.
> >   	 */
> > -	pgcnt += init_unavailable_range(PFN_DOWN(next),
> > -					round_up(max_pfn, PAGES_PER_SECTION));
> > +	end = round_up(max_pfn, PAGES_PER_SECTION);
> > +	pgcnt += init_unavailable_range(next, end, zone, nid);
> >   	/*
> >   	 * Struct pages that do not have backing memory. This could be because
> >   	 * firmware is using some of this memory, or for some other reasons.
> >   	 */
> >   	if (pgcnt)
> > -		pr_info("Zeroed struct page in unavailable ranges: %lld pages", pgcnt);
> > +		pr_info("Zone %s: zeroed struct page in unavailable ranges: %lld pages", zone_names[zone], pgcnt);
> > +}
> > +
> > +static void __init init_unavailable_mem(void)
> > +{
> > +	int zone;
> > +
> > +	for (zone = 0; zone < ZONE_MOVABLE; zone++)
> > +		init_zone_unavailable_mem(zone);
> 
> Why < ZONE_MOVABLE?
> 
> I remember we can have memory holes inside the movable zone when messing
> with "movablecore" cmdline parameter.

Maybe because we haven't initialized MOABLE zone info at this time.

Re: [PATCH v3 2/2] mm: fix initialization of struct page for holes in memory layout

[Mike Rapoport]

On Mon, Feb 01, 2021 at 05:39:58PM +0800, Baoquan He wrote:
> On 02/01/21 at 10:14am, David Hildenbrand wrote:
> > On 11.01.21 20:40, Mike Rapoport wrote:
> > > +
> > > +static void __init init_unavailable_mem(void)
> > > +{
> > > +	int zone;
> > > +
> > > +	for (zone = 0; zone < ZONE_MOVABLE; zone++)
> > > +		init_zone_unavailable_mem(zone);
> > 
> > Why < ZONE_MOVABLE?
> > 
> > I remember we can have memory holes inside the movable zone when messing
> > with "movablecore" cmdline parameter.
> 
> Maybe because we haven't initialized MOABLE zone info at this time.

We already have zone_movable_pfn initialized at this point. 
So if there is a possibility for holes in the movable zone, we should take
care of it.

-- 
Sincerely yours,
Mike.

Re: [PATCH v3 0/2] mm: fix initialization of struct page for holes in memory layout

[Andrew Morton]

On Mon, 11 Jan 2021 21:40:15 +0200 Mike Rapoport <rppt@kernel.org> wrote:

> Commit 73a6e474cb37 ("mm: memmap_init: iterate over
> memblock regions rather that check each PFN") exposed several issues with
> the memory map initialization and these patches fix those issues.
> 
> Initially there were crashes during compaction that Qian Cai reported back
> in April [1]. It seemed back then that the problem was fixed, but a few
> weeks ago Andrea Arcangeli hit the same bug [2] and there was an additional
> discussion at [3].

The earlier version of these fixes had cc:stable.  Was the omission
this time deliberate?

Re: [PATCH v3 0/2] mm: fix initialization of struct page for holes in memory layout

[Mike Rapoport]

On Mon, Jan 11, 2021 at 04:58:01PM -0800, Andrew Morton wrote:
> On Mon, 11 Jan 2021 21:40:15 +0200 Mike Rapoport <rppt@kernel.org> wrote:
> 
> > Commit 73a6e474cb37 ("mm: memmap_init: iterate over
> > memblock regions rather that check each PFN") exposed several issues with
> > the memory map initialization and these patches fix those issues.
> > 
> > Initially there were crashes during compaction that Qian Cai reported back
> > in April [1]. It seemed back then that the problem was fixed, but a few
> > weeks ago Andrea Arcangeli hit the same bug [2] and there was an additional
> > discussion at [3].
> 
> The earlier version of these fixes had cc:stable.  Was the omission
> this time deliberate?

Nope, it was accidental. 
Thanks for noticing.

-- 
Sincerely yours,
Mike.