arm64: drop pfn_valid_within() and simplify pfn_valid()

From: Mike Rapoport <[email protected]>

Add comment describing the semantics of pfn_valid() that clarifies that pfn_valid() only checks for availability of a memory map entry (i.e. struct page) for a PFN rather than availability of usable memory backing that PFN.

The most "generic" version of pfn_valid() used by the configurations with SPARSEMEM enabled resides in include/linux/mmzone.h so this is the most suitable place for documentation about semantics of pfn_valid().

Suggested-by Anshuman Khandual <[email protected]>

Signed-off-by Mike Rapoport <[email protected]>

Reviewed-by Anshuman Khandual <[email protected]>

---
 include/linux/mmzone.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

include/linux/mmzone.h +11 -1

index 0d53eba1c383..e5945ca24df7 100644
@@ -1427,6 +1427,17 @@ static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
 #endif
 
 #ifndef CONFIG_HAVE_ARCH_PFN_VALID
+/**
+ * pfn_valid - check if there is a valid memory map entry for a PFN
+ * @pfn: the page frame number to check
+ *
+ * Check if there is a valid memory map entry aka struct page for the @pfn.
+ * Note, that availability of the memory map entry does not imply that
+ * there is actual usable memory at that @pfn. The struct page may
+ * represent a hole or an unusable page frame.
+ *
+ * Return: 1 for PFNs that have memory map entries and 0 otherwise
+ */
 static inline int pfn_valid(unsigned long pfn)
 {
        struct mem_section *ms;
--

2.28.0

From: Mike Rapoport <[email protected]>

The struct pages representing a reserved memory region are initialized using reserve_bootmem_range() function. This function is called for each reserved region just before the memory is freed from memblock to the buddy page allocator.

The struct pages for MEMBLOCK_NOMAP regions are kept with the default values set by the memory map initialization which makes it necessary to have a special treatment for such pages in pfn_valid() and pfn_valid_within().

Split out initialization of the reserved pages to a function with a meaningful name and treat the MEMBLOCK_NOMAP regions the same way as the reserved regions and mark struct pages for the NOMAP regions as PageReserved.

Signed-off-by Mike Rapoport <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Anshuman Khandual <[email protected]>

---
 include/linux/memblock.h |  4 +++-
 mm/memblock.c            | 28 ++++++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

include/linux/memblock.h +3 -1

index 5984fff3f175..1b4c97c151ae 100644
@@ -30,7 +30,9 @@ extern unsigned long long max_possible_pfn;
  * @MEMBLOCK_NONE: no special request
  * @MEMBLOCK_HOTPLUG: hotpluggable region
  * @MEMBLOCK_MIRROR: mirrored region
- * @MEMBLOCK_NOMAP: don't add to kernel direct mapping
+ * @MEMBLOCK_NOMAP: don't add to kernel direct mapping and treat as
+ * reserved in the memory map; refer to memblock_mark_nomap() description
+ * for further details
  */
 enum memblock_flags {
        MEMBLOCK_NONE           = 0x0,  /* No special request */

mm/memblock.c +26 -3

index afaefa8fc6ab..3abf2c3fea7f 100644
@@ -906,6 +906,11 @@ int __init_memblock memblock_mark_mirror(phys_addr_t base, phys_addr_t size)
  * @base: the base phys addr of the region
  * @size: the size of the region
  *
+ * The memory regions marked with %MEMBLOCK_NOMAP will not be added to the
+ * direct mapping of the physical memory. These regions will still be
+ * covered by the memory map. The struct page representing NOMAP memory
+ * frames in the memory map will be PageReserved()
+ *
  * Return: 0 on success, -errno on failure.
  */
 int __init_memblock memblock_mark_nomap(phys_addr_t base, phys_addr_t size)

@@ -2002,6 +2007,26 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
        return end_pfn - start_pfn;
 }
 
+static void __init memmap_init_reserved_pages(void)
+{
+       struct memblock_region *region;
+       phys_addr_t start, end;
+       u64 i;
+
+       /* initialize struct pages for the reserved regions */
+       for_each_reserved_mem_range(i, &start, &end)
+               reserve_bootmem_region(start, end);
+
+       /* and also treat struct pages for the NOMAP regions as PageReserved */
+       for_each_mem_region(region) {
+               if (memblock_is_nomap(region)) {
+                       start = region->base;
+                       end = start + region->size;
+                       reserve_bootmem_region(start, end);
+               }
+       }
+}
+
 static unsigned long __init free_low_memory_core_early(void)
 {
        unsigned long count = 0;

@@ -2010,8 +2035,7 @@ static unsigned long __init free_low_memory_core_early(void)
 
        memblock_clear_hotplug(0, -1);
 
-       for_each_reserved_mem_range(i, &start, &end)
-               reserve_bootmem_region(start, end);
+       memmap_init_reserved_pages();
 
        /*
         * We need to use NUMA_NO_NODE instead of NODE_DATA(0)->node_id
--

2.28.0

From: Mike Rapoport <[email protected]>

The struct pages representing a reserved memory region are initialized using reserve_bootmem_range() function. This function is called for each reserved region just before the memory is freed from memblock to the buddy page allocator.

The struct pages for MEMBLOCK_NOMAP regions are kept with the default values set by the memory map initialization which makes it necessary to have a special treatment for such pages in pfn_valid() and pfn_valid_within().

Split out initialization of the reserved pages to a function with a meaningful name and treat the MEMBLOCK_NOMAP regions the same way as the reserved regions and mark struct pages for the NOMAP regions as PageReserved.

Signed-off-by Mike Rapoport <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Anshuman Khandual <[email protected]>

---
 include/linux/memblock.h |  4 +++-
 mm/memblock.c            | 28 ++++++++++++++++++++++++++--

 2 files changed, 29 insertions(+), 3 deletions(-)

include/linux/memblock.h +3 -1

index 5984fff3f175..1b4c97c151ae 100644
@@ -30,7 +30,9 @@ extern unsigned long long max_possible_pfn;
   * @MEMBLOCK_NONE: no special request
   * @MEMBLOCK_HOTPLUG: hotpluggable region
   * @MEMBLOCK_MIRROR: mirrored region
- * @MEMBLOCK_NOMAP: don't add to kernel direct mapping
+ * @MEMBLOCK_NOMAP: don't add to kernel direct mapping and treat as
+ * reserved in the memory map; refer to memblock_mark_nomap() description
+ * for further details
   */
  enum memblock_flags {

  MEMBLOCK_NONE = 0x0, /* No special request */

mm/memblock.c +5 -0

index afaefa8fc6ab..3abf2c3fea7f 100644
@@ -906,6 +906,11 @@ int __init_memblock memblock_mark_mirror(phys_addr_t base, phys_addr_t size)
   * @base: the base phys addr of the region
   * @size: the size of the region
   *
+ * The memory regions marked with %MEMBLOCK_NOMAP will not be added to the
+ * direct mapping of the physical memory. These regions will still be
+ * covered by the memory map. The struct page representing NOMAP memory
+ * frames in the memory map will be PageReserved()
+ *
   * Return: 0 on success, -errno on failure.
   */
  int __init_memblock memblock_mark_nomap(phys_addr_t base, phys_addr_t size)

@@ -2002,6 +2007,26 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
  	return end_pfn - start_pfn;
  }

mm/memblock.c +10 -0

+static void __init memmap_init_reserved_pages(void)
+{
+	struct memblock_region *region;
+	phys_addr_t start, end;
+	u64 i;
+
+	/* initialize struct pages for the reserved regions */
+	for_each_reserved_mem_range(i, &start, &end)
+		reserve_bootmem_region(start, end);
+

Quoted 11 lines · Mike Rapoport

mm/memblock.c +10 -0

+static void __init memmap_init_reserved_pages(void)
+{
+	struct memblock_region *region;
+	phys_addr_t start, end;
+	u64 i;
+
+	/* initialize struct pages for the reserved regions */
+	for_each_reserved_mem_range(i, &start, &end)
+		reserve_bootmem_region(start, end);
+

mm/memblock.c static unsigned long __init __free_memory_core(phys_addr_t start, · line 2029

Quoted 10 lines

mm/memblock.c +10 -0

+	/* and also treat struct pages for the NOMAP regions as PageReserved */
+	for_each_mem_region(region) {
+		if (memblock_is_nomap(region)) {
+			start = region->base;
+			end = start + region->size;
+			reserve_bootmem_region(start, end);
+		}
+	}
+}
+

In some cases, that whole call to reserve_bootmem_region() may be a no- op because pfn_valid() is not true for *any* address in that range.

But reserve_bootmem_region() spends a long time iterating of them all, and eventually doing nothing:

void __meminit reserve_bootmem_region(phys_addr_t start,
phys_addr_t end, int nid)

{
        unsigned long start_pfn = PFN_DOWN(start);
        unsigned long end_pfn = PFN_UP(end);

        for (; start_pfn < end_pfn; start_pfn++) {
                if (pfn_valid(start_pfn)) {
                        struct page *page = pfn_to_page(start_pfn);

                        init_reserved_page(start_pfn, nid);

                        /*
                         * no need for atomic set_bit because the struct
                         * page is not visible yet so nobody should
                         * access it yet.
                         */
                        __SetPageReserved(page);
                }
        }
}

On platforms with large NOMAP regions (e.g. which are actually reserved for guest memory to keep it out of the Linux address map and allow for kexec-based live update of the hypervisor), this pointless loop ends up taking a significant amount of time which is visible as guest steal time during the live update.

Can reserve_bootmem_region() skip the loop *completely* if no PFN in the range from start to end is valid? Or tweak the loop itself to have an 'else' case which skips to the next valid PFN? Something like

 for(...) {
    if (pfn_valid(start_pfn)) {
       ...
    } else {
       start_pfn = next_valid_pfn(start_pfn);
    }

}

Quoted 61 lines · David Woodhouse

Quoted 11 lines · Mike Rapoport

(quoted diff) +10 -0

+static void __init memmap_init_reserved_pages(void)
+{
+	struct memblock_region *region;
+	phys_addr_t start, end;
+	u64 i;
+
+	/* initialize struct pages for the reserved regions */
+	for_each_reserved_mem_range(i, &start, &end)
+		reserve_bootmem_region(start, end);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	/* and also treat struct pages for the NOMAP regions as PageReserved */
+	for_each_mem_region(region) {
+		if (memblock_is_nomap(region)) {
+			start = region->base;
+			end = start + region->size;
+			reserve_bootmem_region(start, end);
+		}
+	}
+}
+

In some cases, that whole call to reserve_bootmem_region() may be a no- op because pfn_valid() is not true for *any* address in that range.

But reserve_bootmem_region() spends a long time iterating of them all, and eventually doing nothing:

void __meminit reserve_bootmem_region(phys_addr_t start, phys_addr_t end, int nid) {
        unsigned long start_pfn = PFN_DOWN(start); unsigned long end_pfn = PFN_UP(end);

        for (; start_pfn < end_pfn; start_pfn++) {
                if (pfn_valid(start_pfn)) {
                        struct page *page = pfn_to_page(start_pfn);

                        init_reserved_page(start_pfn, nid);

                        /*
                         * no need for atomic set_bit because the struct
                         * page is not visible yet so nobody should
                         * access it yet.
                         */
                        __SetPageReserved(page);
                }
        }
}

On platforms with large NOMAP regions (e.g. which are actually reserved for guest memory to keep it out of the Linux address map and allow for kexec-based live update of the hypervisor), this pointless loop ends up taking a significant amount of time which is visible as guest steal time during the live update.

Can reserve_bootmem_region() skip the loop *completely* if no PFN in the range from start to end is valid? Or tweak the loop itself to have an 'else' case which skips to the next valid PFN? Something like

Quoted 7 lines

 for(...) {
    if (pfn_valid(start_pfn)) {
       ...
    } else {
       start_pfn = next_valid_pfn(start_pfn);
    }
 }

My understanding is that you have large reserved NOMAP ranges that don't appear as memory at all, so no memory map for them is created and so pfn_valid() is false for pfns in those ranges.

If this is the case one way indeed would be to make reserve_bootmem_region() skip ranges with no valid pfns.

Another way could be to memblock_reserved_mark_noinit() such ranges and then reserve_bootmem_region() won't even get called, but that would require firmware to pass that information somehow.

Quoted 25 lines · Mike Rapoport

Quoted 12 lines · David Woodhouse

On Tue, 2021-05-11 at 13:05 +0300, Mike Rapoport wrote:

On platforms with large NOMAP regions (e.g. which are actually reserved for guest memory to keep it out of the Linux address map and allow for kexec-based live update of the hypervisor), this pointless loop ends up taking a significant amount of time which is visible as guest steal time during the live update.

Can reserve_bootmem_region() skip the loop *completely* if no PFN in the range from start to end is valid? Or tweak the loop itself to have an 'else' case which skips to the next valid PFN? Something like

Quoted 7 lines

 for(...) {
    if (pfn_valid(start_pfn)) {
       ...
    } else {
       start_pfn = next_valid_pfn(start_pfn);
    }
 }

My understanding is that you have large reserved NOMAP ranges that don't appear as memory at all, so no memory map for them is created and so pfn_valid() is false for pfns in those ranges.

Quoted 6 lines

If this is the case one way indeed would be to make reserve_bootmem_region() skip ranges with no valid pfns.

Another way could be to memblock_reserved_mark_noinit() such ranges and then reserve_bootmem_region() won't even get called, but that would require firmware to pass that information somehow.

I was thinking along these lines (not even build tested)...

I don't much like the (unsigned long)-1 part. I might make the helper 'static inline bool first_valid_pfn (unsigned long *pfn)' and return success or failure. But that's an implementation detail.

index 6d1fb6162ac1..edd27ba3e908 100644

include/asm-generic/memory_model.h +5 -0

@@ -29,8 +29,43 @@ static inline int pfn_valid(unsigned long pfn)
        return pfn >= pfn_offset && (pfn - pfn_offset) < max_mapnr;
 }
 #define pfn_valid pfn_valid
+
+static inline unsigned long first_valid_pfn(unsigned long pfn)
+{
+       /* avoid <linux/mm.h> include hell */
+       extern unsigned long max_mapnr;

Quoted 56 lines · David Woodhouse

Quoted 25 lines · Mike Rapoport

Quoted 12 lines · David Woodhouse

On Tue, 2021-05-11 at 13:05 +0300, Mike Rapoport wrote:

On platforms with large NOMAP regions (e.g. which are actually reserved for guest memory to keep it out of the Linux address map and allow for kexec-based live update of the hypervisor), this pointless loop ends up taking a significant amount of time which is visible as guest steal time during the live update.

Can reserve_bootmem_region() skip the loop *completely* if no PFN in the range from start to end is valid? Or tweak the loop itself to have an 'else' case which skips to the next valid PFN? Something like

Quoted 7 lines

 for(...) {
    if (pfn_valid(start_pfn)) {
       ...
    } else {
       start_pfn = next_valid_pfn(start_pfn);
    }
 }

My understanding is that you have large reserved NOMAP ranges that don't appear as memory at all, so no memory map for them is created and so pfn_valid() is false for pfns in those ranges.

Quoted 6 lines

If this is the case one way indeed would be to make reserve_bootmem_region() skip ranges with no valid pfns.

Another way could be to memblock_reserved_mark_noinit() such ranges and then reserve_bootmem_region() won't even get called, but that would require firmware to pass that information somehow.

I was thinking along these lines (not even build tested)...

I don't much like the (unsigned long)-1 part. I might make the helper 'static inline bool first_valid_pfn (unsigned long *pfn)' and return success or failure. But that's an implementation detail.

index 6d1fb6162ac1..edd27ba3e908 100644

include/asm-generic/memory_model.h +10 -0

@@ -29,8 +29,43 @@ static inline int pfn_valid(unsigned long pfn)
         return pfn >= pfn_offset && (pfn - pfn_offset) < max_mapnr;
  }
  #define pfn_valid pfn_valid
+
+static inline unsigned long first_valid_pfn(unsigned long pfn)
+{
+       /* avoid <linux/mm.h> include hell */
+       extern unsigned long max_mapnr;
+       unsigned long pfn_offset = ARCH_PFN_OFFSET;
+
+       if (pfn < pfn_offset)
+               return pfn_offset;
+

include/asm-generic/memory_model.h static inline int pfn_valid(unsigned long pfn) · line 46

Quoted 5 lines

include/asm-generic/memory_model.h +5 -0

+       if ((pfn - pfn_offset) < max_mapnr)
+               return pfn;
+
+       return (unsigned long)(-1);
+}

This seems about right for FLATMEM. For SPARSEMEM it would be something along these lines (I kept dubious -1):

static inline unsigned long first_valid_pfn(unsigned long pfn)

{
	unsigned long nr = pfn_to_section_nr(pfn);

	do {
		if (pfn_valid(pfn))
			return pfn;
		pfn = section_nr_to_pfn(nr++);
	} while (nr < NR_MEM_SECTIONS);

	return (unsigned long)-1;
}

Quoted 57 lines · Mike Rapoport

Quoted 56 lines · David Woodhouse

Quoted 25 lines · Mike Rapoport

Quoted 12 lines · David Woodhouse

On Tue, 2021-05-11 at 13:05 +0300, Mike Rapoport wrote:

On platforms with large NOMAP regions (e.g. which are actually reserved for guest memory to keep it out of the Linux address map and allow for kexec-based live update of the hypervisor), this pointless loop ends up taking a significant amount of time which is visible as guest steal time during the live update.

Can reserve_bootmem_region() skip the loop *completely* if no PFN in the range from start to end is valid? Or tweak the loop itself to have an 'else' case which skips to the next valid PFN? Something like

Quoted 7 lines

 for(...) {
    if (pfn_valid(start_pfn)) {
       ...
    } else {
       start_pfn = next_valid_pfn(start_pfn);
    }
 }

My understanding is that you have large reserved NOMAP ranges that don't appear as memory at all, so no memory map for them is created and so pfn_valid() is false for pfns in those ranges.

Quoted 6 lines

If this is the case one way indeed would be to make reserve_bootmem_region() skip ranges with no valid pfns.

Another way could be to memblock_reserved_mark_noinit() such ranges and then reserve_bootmem_region() won't even get called, but that would require firmware to pass that information somehow.

I was thinking along these lines (not even build tested)...

I don't much like the (unsigned long)-1 part. I might make the helper 'static inline bool first_valid_pfn (unsigned long *pfn)' and return success or failure. But that's an implementation detail.

index 6d1fb6162ac1..edd27ba3e908 100644

include/asm-generic/memory_model.h +10 -0

@@ -29,8 +29,43 @@ static inline int pfn_valid(unsigned long pfn)
         return pfn >= pfn_offset && (pfn - pfn_offset) < max_mapnr;
  }
  #define pfn_valid pfn_valid
+
+static inline unsigned long first_valid_pfn(unsigned long pfn)
+{
+       /* avoid <linux/mm.h> include hell */
+       extern unsigned long max_mapnr;
+       unsigned long pfn_offset = ARCH_PFN_OFFSET;
+
+       if (pfn < pfn_offset)
+               return pfn_offset;
+

Quoted 8 lines

include/asm-generic/memory_model.h static inline int pfn_valid(unsigned long pfn) · line 46

Quoted 5 lines

include/asm-generic/memory_model.h +5 -0

+       if ((pfn - pfn_offset) < max_mapnr)
+               return pfn;
+
+       return (unsigned long)(-1);
+}

This seems about right for FLATMEM. For SPARSEMEM it would be something along these lines (I kept dubious -1):

Thanks. Is that right even with CONFIG_SPARSEMEM_VMEMMAP? It seems that it's possible for pfn_valid() to be false for a given *page*, but there may still be valid pages in the remainder of the same section in that case?

I think it should only skip to the next section if the current section doesn't exist at all, not just when pfn_section_valid() return false?

I also wasn't sure how to cope with the rcu_read_lock_sched() that happens in pfn_valid(). What's that protecting against? Does it mean that by the time pfn_valid() returns true, that might not be the correct answer any more?

Quoted 12 lines

static inline unsigned long first_valid_pfn(unsigned long pfn)

{
	unsigned long nr = pfn_to_section_nr(pfn);

	do {
		if (pfn_valid(pfn))
			return pfn;
		pfn = section_nr_to_pfn(nr++);
	} while (nr < NR_MEM_SECTIONS);

	return (unsigned long)-1;
}

From: David Woodhouse <[email protected]>

In the FLATMEM case, the default pfn_valid() just checks that the PFN is within the range [ ARCH_PFN_OFFSET .. ARCH_PFN_OFFSET + max_mapnr ).

The for_each_valid_pfn() function can therefore be a simple for() loop using those as min/max respectively.

Signed-off-by David Woodhouse <[email protected]>

---
 include/asm-generic/memory_model.h | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

include/asm-generic/memory_model.h +25 -1

index a3b5029aebbd..4fe7dd3bc09c 100644
@@ -30,7 +30,31 @@ static inline int pfn_valid(unsigned long pfn)
 	return pfn >= pfn_offset && (pfn - pfn_offset) < max_mapnr;
 }
 #define pfn_valid pfn_valid
-#endif
+
+static inline bool first_valid_pfn(unsigned long *pfn)
+{
+	/* avoid <linux/mm.h> include hell */
+	extern unsigned long max_mapnr;
+	unsigned long pfn_offset = ARCH_PFN_OFFSET;
+
+	if (*pfn < pfn_offset) {
+		*pfn = pfn_offset;
+		return true;
+	}
+
+	if ((*pfn - pfn_offset) < max_mapnr)
+		return true;
+
+	return false;
+}
+
+#ifndef for_each_valid_pfn
+#define for_each_valid_pfn(pfn, start_pfn, end_pfn)			       \
+	for (pfn = max_t(unsigned long  start_pfn, ARCH_PFN_OFFSET);	       \
+	     pfn < min_t(unsigned long, end_pfn, ARCH_PFN_OFFSET + max_mapnr); \
+			 pfn++)
+#endif /* for_each_valid_pfn */
+#endif /* valid_pfn */
 
 #elif defined(CONFIG_SPARSEMEM_VMEMMAP)
 

Signature · 1 line

2.49.0

From: David Woodhouse <[email protected]>

Introduce a pfn_first_valid() helper which takes a pointer to the PFN and updates it to point to the first valid PFN starting from that point, and returns true if a valid PFN was found.

This largely mirrors pfn_valid(), calling into a pfn_section_first_valid() helper which is trivial for the !CONFIG_SPARSEMEM_VMEMMAP case, and in the VMEMMAP case will skip to the next subsection as needed.

Signed-off-by David Woodhouse <[email protected]>

include/linux/mmzone.h +26 -0

+		if (valid_section(ms) &&
+		    (early_section(ms) || pfn_section_first_valid(ms, &pfn))) {
+			ret = true;
+			break;
+		}
+
+		nr++;
+		if (nr > __highest_present_section_nr)
+			break;
+
+		pfn = section_nr_to_pfn(nr);
+		ms = __pfn_to_section(pfn);
+	}
+
+	rcu_read_unlock_sched();
+
+	*p_pfn = pfn;
+
+	return ret;
+}
+
+#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)	       \
+	for ((_pfn) = (_start_pfn);			       \
+	     first_valid_pfn(&(_pfn)) && (_pfn) < (_end_pfn);  \
+	     (_pfn)++)
+
 #endif
 
 static inline int pfn_in_present_section(unsigned long pfn)

Signature · 1 line

2.49.0

Quoted 14 lines · David Woodhouse

From: David Woodhouse <[email protected]>

Introduce a pfn_first_valid() helper which takes a pointer to the PFN and
updates it to point to the first valid PFN starting from that point, and
returns true if a valid PFN was found.

This largely mirrors pfn_valid(), calling into a pfn_section_first_valid()
helper which is trivial for the !CONFIG_SPARSEMEM_VMEMMAP case, and in
the VMEMMAP case will skip to the next subsection as needed.

Quoted 1 line

Signed-off-by David Woodhouse <[email protected]>

Reviewed-by Mike Rapoport (Microsoft) <[email protected]>

with a small nit below

include/linux/mmzone.h static inline int pfn_valid(unsigned long pfn) · line 2168

Quoted 12 lines

include/linux/mmzone.h +12 -0

+static inline bool first_valid_pfn(unsigned long *p_pfn)
+{
+	unsigned long pfn = *p_pfn;
+	unsigned long nr = pfn_to_section_nr(pfn);
+	struct mem_section *ms;
+	bool ret = false;
+
+	ms = __pfn_to_section(pfn);
+
+	rcu_read_lock_sched();
+
+	while (!ret && nr <= __highest_present_section_nr) {

This could be just for(;;), we anyway break when ret becomes true or we get past last present section.

Quoted 32 lines

include/linux/mmzone.h +26 -0

+		if (valid_section(ms) &&
+		    (early_section(ms) || pfn_section_first_valid(ms, &pfn))) {
+			ret = true;
+			break;
+		}
+
+		nr++;
+		if (nr > __highest_present_section_nr)
+			break;
+
+		pfn = section_nr_to_pfn(nr);
+		ms = __pfn_to_section(pfn);
+	}
+
+	rcu_read_unlock_sched();
+
+	*p_pfn = pfn;
+
+	return ret;
+}
+
+#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)	       \
+	for ((_pfn) = (_start_pfn);			       \
+	     first_valid_pfn(&(_pfn)) && (_pfn) < (_end_pfn);  \
+	     (_pfn)++)
+

 #endif

 static inline int pfn_in_present_section(unsigned long pfn)

Signature · 1 line

2.49.0

I'll see if I can make it neater. I may drop the 'ret' variable completely and just turn the match clause into unlock-and-return-true. I *like* having a single unlock site. But I think I like simpler loop code more than that.

Quoted 5 lines · David Woodhouse

I'll see if I can make it neater. I may drop the 'ret' variable completely and just turn the match clause into unlock-and-return-true. I *like* having a single unlock site. But I think I like simpler loop code more than that.

That's better (IMO).

And I note that pfn_valid() already doesn't follow the modern fetish for having only one unlock site even when it makes the surrounding code more complex to do so.

static inline bool first_valid_pfn(unsigned long *p_pfn)

{
	unsigned long pfn = *p_pfn;
	unsigned long nr = pfn_to_section_nr(pfn);
	struct mem_section *ms;

	rcu_read_lock_sched();

	while (nr <= __highest_present_section_nr) {
		ms = __pfn_to_section(pfn);

		if (valid_section(ms) &&
		    (early_section(ms) || pfn_section_first_valid(ms, &pfn))) {
			*p_pfn = pfn;
			rcu_read_unlock_sched();
			return true;
		}

		/* Nothing left in this section? Skip to next section */
		nr++;
		pfn = section_nr_to_pfn(nr);
	}

	rcu_read_unlock_sched();

	return false;
}

Quoted 6 lines

static inline bool first_valid_pfn(unsigned long *p_pfn) {
  unsigned long pfn = *p_pfn;
  unsigned long nr = pfn_to_section_nr(pfn); struct mem_section *ms;

Quoted 4 lines

  rcu_read_lock_sched();

  while (nr <= __highest_present_section_nr) { ms = __pfn_to_section(pfn);

Maybe move the declaration here:

  struct mem_section *ms = __pfn_to_section(pfn);

Quoted 12 lines · Mike Rapoport

Quoted 10 lines

static inline bool first_valid_pfn(unsigned long *p_pfn) {
  unsigned long pfn = *p_pfn;
  unsigned long nr = pfn_to_section_nr(pfn); struct mem_section *ms;

  rcu_read_lock_sched();

  while (nr <= __highest_present_section_nr) { ms = __pfn_to_section(pfn);

Quoted 3 lines

Maybe move the declaration here:

  struct mem_section *ms = __pfn_to_section(pfn);

Ack.

https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/for_each_valid_pfn

Quoted 17 lines · Mike Rapoport

with a small nit below

include/linux/mmzone.h static inline int pfn_valid(unsigned long pfn) · line 2168

Quoted 12 lines

include/linux/mmzone.h +12 -0

+static inline bool first_valid_pfn(unsigned long *p_pfn)
+{
+	unsigned long pfn = *p_pfn;
+	unsigned long nr = pfn_to_section_nr(pfn);
+	struct mem_section *ms;
+	bool ret = false;
+
+	ms = __pfn_to_section(pfn);
+
+	rcu_read_lock_sched();
+
+	while (!ret && nr <= __highest_present_section_nr) {

This could be just for(;;), we anyway break when ret becomes true or we get past last present section.

True for the 'ret' part but not *nicely* for the last present section. If the original pfn is higher than the last present section it could trigger that check before entering the loop.

Yes, in that case 'ms' will be NULL, valid_section(NULL) is false and you're right that it'll make it through to the check in the loop without crashing. So it would currently be harmless, but I didn't like it. It's relying on the loop not to do the wrong thing with an input which is arguably invalid.

I'll see if I can make it neater. I may drop the 'ret' variable completely and just turn the match clause into unlock-and-return-true. I *like* having a single unlock site. But I think I like simpler loop code more than that.

FWIW I think the check for (PHYS_PFN(PFN_PHYS(pfn)) != pfn) at the start of pfn_valid() a few lines above is similarly redundant. Because if the high bits are set in the PFN then pfn_to_section_nr(pfn) is surely going to be higher than NR_MEM_SECTIONS and it'll get thrown out at the very next check, won't it?

I care because I didn't bother to duplicate that 'redundant' check in my first_valid_pfn(), so if there's a reason for it that I'm missing, I should take a closer look.

I'm also missing the reason why the FLATMEM code in memory_model.h does 'unsigned long pfn_offset = ARCH_PFN_OFFSET' and then uses its local pfn_offset variable, instead of just using ARCH_PFN_OFFSET directly as I do in the FLATMEM for_each_valid_pfn() macro.

From: David Woodhouse <[email protected]>

Especially since commit 9092d4f7a1f8 ("memblock: update initialization of reserved pages"), the reserve_bootmem_region() function can spend a significant amount of time iterating over every 4KiB PFN in a range, calling pfn_valid() on each one, and ultimately doing absolutely nothing.

On a platform used for virtualization, with large NOMAP regions that eventually get used for guest RAM, this leads to a significant increase in steal time experienced during kexec for a live update.

Introduce for_each_valid_pfn() and use it from reserve_bootmem_region(). This implementation is precisely the same naïve loop that the function used to have, but subsequent commits will provide optimised versions for FLATMEM and SPARSEMEM, and this version will remain for those architectures which provide their own pfn_valid() implementation, until/unless they also provide a matching for_each_valid_pfn().

Signed-off-by David Woodhouse <[email protected]>

---
 include/linux/mmzone.h | 10 ++++++++++
 mm/mm_init.c           | 23 ++++++++++-------------
 2 files changed, 20 insertions(+), 13 deletions(-)

include/linux/mmzone.h +10 -0

index 25e80b2ca7f4..32ecb5cadbaf 100644
@@ -2176,6 +2176,16 @@ void sparse_init(void);
 #define subsection_map_init(_pfn, _nr_pages) do {} while (0)
 #endif /* CONFIG_SPARSEMEM */
 
+/*
+ * Fallback case for when the architecture provides its own pfn_valid() but
+ * not a corresponding for_each_valid_pfn().
+ */
+#ifndef for_each_valid_pfn
+#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)			\
+	for ((_pfn) = (_start_pfn); (_pfn) < (_end_pfn); (_pfn)++)	\
+		if (pfn_valid(_pfn))
+#endif
+
 #endif /* !__GENERATING_BOUNDS.H */
 #endif /* !__ASSEMBLY__ */
 #endif /* _LINUX_MMZONE_H */

mm/mm_init.c +10 -13

index a38a1909b407..7c699bad42ad 100644
@@ -777,22 +777,19 @@ static inline void init_deferred_page(unsigned long pfn, int nid)
 void __meminit reserve_bootmem_region(phys_addr_t start,
 				      phys_addr_t end, int nid)
 {
-	unsigned long start_pfn = PFN_DOWN(start);
-	unsigned long end_pfn = PFN_UP(end);
+	unsigned long pfn;
 
-	for (; start_pfn < end_pfn; start_pfn++) {
-		if (pfn_valid(start_pfn)) {
-			struct page *page = pfn_to_page(start_pfn);
+	for_each_valid_pfn (pfn, PFN_DOWN(start), PFN_UP(end)) {
+		struct page *page = pfn_to_page(pfn);
 
-			init_deferred_page(start_pfn, nid);
+		init_deferred_page(pfn, nid);
 
-			/*
-			 * no need for atomic set_bit because the struct
-			 * page is not visible yet so nobody should
-			 * access it yet.
-			 */
-			__SetPageReserved(page);
-		}
+		/*
+		 * no need for atomic set_bit because the struct
+		 * page is not visible yet so nobody should
+		 * access it yet.
+		 */
+		__SetPageReserved(page);
 	}
 }
 

Signature · 1 line

2.49.0

From: Mike Rapoport <[email protected]>

The intended semantics of pfn_valid() is to verify whether there is a struct page for the pfn in question and nothing else.

Yet, on arm64 it is used to distinguish memory areas that are mapped in the linear map vs those that require ioremap() to access them.

Introduce a dedicated pfn_is_map_memory() wrapper for memblock_is_map_memory() to perform such check and use it where appropriate.

Using a wrapper allows to avoid cyclic include dependencies.

While here also update style of pfn_valid() so that both pfn_valid() and pfn_is_map_memory() declarations will be consistent.

Signed-off-by Mike Rapoport <[email protected]>

Acked-by David Hildenbrand <[email protected]>

---
 arch/arm64/include/asm/memory.h |  2 +-
 arch/arm64/include/asm/page.h   |  3 ++-
 arch/arm64/kvm/mmu.c            |  2 +-
 arch/arm64/mm/init.c            | 12 ++++++++++++
 arch/arm64/mm/ioremap.c         |  4 ++--
 arch/arm64/mm/mmu.c             |  2 +-
 6 files changed, 19 insertions(+), 6 deletions(-)

arch/arm64/include/asm/memory.h +1 -1

index 87b90dc27a43..9027b7e16c4c 100644
@@ -369,7 +369,7 @@ static inline void *phys_to_virt(phys_addr_t x)
 
 #define virt_addr_valid(addr)  ({                                      \
        __typeof__(addr) __addr = __tag_reset(addr);                    \
-       __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr));      \
+       __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr));      \
 })
 
 void dump_mem_limit(void);

arch/arm64/include/asm/page.h +2 -1

index 012cffc574e8..75ddfe671393 100644
@@ -37,7 +37,8 @@ void copy_highpage(struct page *to, struct page *from);
 
 typedef struct page *pgtable_t;
 
-extern int pfn_valid(unsigned long);
+int pfn_valid(unsigned long pfn);
+int pfn_is_map_memory(unsigned long pfn);
 
 #include <asm/memory.h>
 

arch/arm64/kvm/mmu.c +1 -1

index c5d1f3c87dbd..470070073085 100644
@@ -85,7 +85,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm)
 
 static bool kvm_is_device_pfn(unsigned long pfn)
 {
-       return !pfn_valid(pfn);
+       return !pfn_is_map_memory(pfn);
 }
 
 static void *stage2_memcache_zalloc_page(void *arg)

arch/arm64/mm/init.c +12 -0

index 16a2b2b1c54d..798f74f501d5 100644
@@ -255,6 +255,18 @@ int pfn_valid(unsigned long pfn)
 }
 EXPORT_SYMBOL(pfn_valid);
 
+int pfn_is_map_memory(unsigned long pfn)
+{
+       phys_addr_t addr = PFN_PHYS(pfn);
+
+       /* avoid false positives for bogus PFNs, see comment in pfn_valid() */
+       if (PHYS_PFN(addr) != pfn)
+               return 0;
+
+       return memblock_is_map_memory(addr);
+}
+EXPORT_SYMBOL(pfn_is_map_memory);
+
 static phys_addr_t memory_limit = PHYS_ADDR_MAX;
 
 /*

arch/arm64/mm/ioremap.c +2 -2

index b5e83c46b23e..b7c81dacabf0 100644
@@ -43,7 +43,7 @@ static void __iomem *__ioremap_caller(phys_addr_t phys_addr, size_t size,
        /*
         * Don't allow RAM to be mapped.
         */
-       if (WARN_ON(pfn_valid(__phys_to_pfn(phys_addr))))
+       if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr))))
                return NULL;
 
        area = get_vm_area_caller(size, VM_IOREMAP, caller);

@@ -84,7 +84,7 @@ EXPORT_SYMBOL(iounmap);
 void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
 {
        /* For normal memory we already have a cacheable mapping. */
-       if (pfn_valid(__phys_to_pfn(phys_addr)))
+       if (pfn_is_map_memory(__phys_to_pfn(phys_addr)))
                return (void __iomem *)__phys_to_virt(phys_addr);
 
        return __ioremap_caller(phys_addr, size, __pgprot(PROT_NORMAL),

arch/arm64/mm/mmu.c +1 -2

index 6dd9369e3ea0..ab5914cebd3c 100644
@@ -82,7 +82,7 @@ void set_swapper_pgd(pgd_t *pgdp, pgd_t pgd)
 pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
                              unsigned long size, pgprot_t vma_prot)
 {
-       if (!pfn_valid(pfn))
+       if (!pfn_is_map_memory(pfn))
                return pgprot_noncached(vma_prot);
        else if (file->f_flags & O_SYNC)
                return pgprot_writecombine(vma_prot);
--

2.28.0

From: Mike Rapoport <[email protected]>

The arm64's version of pfn_valid() differs from the generic because of two reasons:

* Parts of the memory map are freed during boot. This makes it necessary to verify that there is actual physical memory that corresponds to a pfn which is done by querying memblock.

* There are NOMAP memory regions. These regions are not mapped in the linear map and until the previous commit the struct pages representing these areas had default values.

As the consequence of absence of the special treatment of NOMAP regions in the memory map it was necessary to use memblock_is_map_memory() in pfn_valid() and to have pfn_valid_within() aliased to pfn_valid() so that generic mm functionality would not treat a NOMAP page as a normal page.

Since the NOMAP regions are now marked as PageReserved(), pfn walkers and the rest of core mm will treat them as unusable memory and thus pfn_valid_within() is no longer required at all and can be disabled by removing CONFIG_HOLES_IN_ZONE on arm64.

pfn_valid() can be slightly simplified by replacing memblock_is_map_memory() with memblock_is_memory().

Signed-off-by Mike Rapoport <[email protected]>

Acked-by David Hildenbrand <[email protected]>

---
 arch/arm64/Kconfig   | 3 ---
 arch/arm64/mm/init.c | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

arch/arm64/Kconfig +0 -3

index 9f1d8566bbf9..d7dc8698cf8e 100644
@@ -1052,9 +1052,6 @@ config NEED_PER_CPU_EMBED_FIRST_CHUNK
        def_bool y
        depends on NUMA
 
-config HOLES_IN_ZONE
-       def_bool y
-
 source "kernel/Kconfig.hz"
 
 config ARCH_SPARSEMEM_ENABLE

arch/arm64/mm/init.c +1 -2

index 798f74f501d5..fb07218da2c0 100644
@@ -251,7 +251,7 @@ int pfn_valid(unsigned long pfn)
        if (!early_section(ms))
                return pfn_section_valid(ms, pfn);
 
-       return memblock_is_map_memory(addr);
+       return memblock_is_memory(addr);
 }
 EXPORT_SYMBOL(pfn_valid);
 
--

2.28.0

From: Mike Rapoport <[email protected]>

The arm64's version of pfn_valid() differs from the generic because of two reasons:

* Parts of the memory map are freed during boot. This makes it necessary to verify that there is actual physical memory that corresponds to a pfn which is done by querying memblock.

* There are NOMAP memory regions. These regions are not mapped in the linear map and until the previous commit the struct pages representing these areas had default values.

As the consequence of absence of the special treatment of NOMAP regions in the memory map it was necessary to use memblock_is_map_memory() in pfn_valid() and to have pfn_valid_within() aliased to pfn_valid() so that generic mm functionality would not treat a NOMAP page as a normal page.

Since the NOMAP regions are now marked as PageReserved(), pfn walkers and the rest of core mm will treat them as unusable memory and thus pfn_valid_within() is no longer required at all and can be disabled by removing CONFIG_HOLES_IN_ZONE on arm64.

pfn_valid() can be slightly simplified by replacing memblock_is_map_memory() with memblock_is_memory().

...

arch/arm64/mm/init.c +1 -1

index 798f74f501d5..fb07218da2c0 100644
@@ -251,7 +251,7 @@ int pfn_valid(unsigned long pfn)
 	if (!early_section(ms))
 		return pfn_section_valid(ms, pfn);
 
-	return memblock_is_map_memory(addr);
+	return memblock_is_memory(addr);
 }
 EXPORT_SYMBOL(pfn_valid);

Quoted 39 lines · On Tue, 11 May 2021 13:05:50 +0300 Mike Rapoport <[email protected]> wrote:

From: Mike Rapoport <[email protected]>

The arm64's version of pfn_valid() differs from the generic because of two reasons:

* Parts of the memory map are freed during boot. This makes it necessary to verify that there is actual physical memory that corresponds to a pfn which is done by querying memblock.

* There are NOMAP memory regions. These regions are not mapped in the linear map and until the previous commit the struct pages representing these areas had default values.

As the consequence of absence of the special treatment of NOMAP regions in the memory map it was necessary to use memblock_is_map_memory() in pfn_valid() and to have pfn_valid_within() aliased to pfn_valid() so that generic mm functionality would not treat a NOMAP page as a normal page.

Since the NOMAP regions are now marked as PageReserved(), pfn walkers and the rest of core mm will treat them as unusable memory and thus pfn_valid_within() is no longer required at all and can be disabled by removing CONFIG_HOLES_IN_ZONE on arm64.

pfn_valid() can be slightly simplified by replacing memblock_is_map_memory() with memblock_is_memory().

...

arch/arm64/Kconfig +0 -3

@@ -1052,9 +1052,6 @@ config NEED_PER_CPU_EMBED_FIRST_CHUNK
 	def_bool y
 	depends on NUMA
 
-config HOLES_IN_ZONE
-	def_bool y
-
 source "kernel/Kconfig.hz"
 

arch/arm64/Kconfig config NEED_PER_CPU_EMBED_FIRST_CHUNK · line 1057

Quoted 1 line

arch/arm64/Kconfig +0 -0

 config ARCH_SPARSEMEM_ENABLE

https://lkml.kernel.org/r/[email protected]
already did this, so I simply dropped that hunk? And I don't think the changelog needs updating for this?

And I don't think the changelog needs updating for this?

From: Mike Rapoport <[email protected]>

Hi,

These patches aim to remove CONFIG_HOLES_IN_ZONE and essentially hardwire pfn_valid_within() to 1.

The idea is to mark NOMAP pages as reserved in the memory map and restore the intended semantics of pfn_valid() to designate availability of struct page for a pfn.

With this the core mm will be able to cope with the fact that it cannot use NOMAP pages and the holes created by NOMAP ranges within MAX_ORDER blocks will be treated correctly even without the need for pfn_valid_within.

The patches are boot tested on qemu-system-aarch64.

I beleive it would be best to route these via mmotm tree.

From: Mike Rapoport <[email protected]>

Hi,

These patches aim to remove CONFIG_HOLES_IN_ZONE and essentially hardwire pfn_valid_within() to 1.

The idea is to mark NOMAP pages as reserved in the memory map and restore the intended semantics of pfn_valid() to designate availability of struct page for a pfn.

With this the core mm will be able to cope with the fact that it cannot use NOMAP pages and the holes created by NOMAP ranges within MAX_ORDER blocks will be treated correctly even without the need for pfn_valid_within.

The patches are boot tested on qemu-system-aarch64.

I beleive it would be best to route these via mmotm tree.

v4:
* rebase on v5.13-rc1

v3: Link: https://lore.kernel.org/lkml/[email protected]
* Fix minor issues found by Anshuman
* Freshen up the declaration of pfn_valid() to make it consistent with pfn_is_map_memory()
* Add more Acked-by and Reviewed-by tags, thanks Anshuman and David

v2: Link: https://lore.kernel.org/lkml/[email protected]
* Add check for PFN overflow in pfn_is_map_memory()
* Add Acked-by and Reviewed-by tags, thanks David.

v1: Link: https://lore.kernel.org/lkml/[email protected]
* Add comment about the semantics of pfn_valid() as Anshuman suggested
* Extend comments about MEMBLOCK_NOMAP, per Anshuman
* Use pfn_is_map_memory() name for the exported wrapper for memblock_is_map_memory(). It is still local to arch/arm64 in the end because of header dependency issues.

rfc: Link: https://lore.kernel.org/lkml/[email protected]

Mike Rapoport (4):
include/linux/mmzone.h: add documentation for pfn_valid()
memblock: update initialization of reserved pages
arm64: decouple check whether pfn is in linear map from pfn_valid()
arm64: drop pfn_valid_within() and simplify pfn_valid()

 arch/arm64/Kconfig              |  3 ---
 arch/arm64/include/asm/memory.h |  2 +-
 arch/arm64/include/asm/page.h   |  3 ++-
 arch/arm64/kvm/mmu.c            |  2 +-
 arch/arm64/mm/init.c            | 14 +++++++++++++-
 arch/arm64/mm/ioremap.c         |  4 ++--
 arch/arm64/mm/mmu.c             |  2 +-
 include/linux/memblock.h        |  4 +++-
 include/linux/mmzone.h          | 11 +++++++++++
 mm/memblock.c                   | 28 ++++++++++++++++++++++++++--
 10 files changed, 60 insertions(+), 13 deletions(-)

base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5 --
2.28.0

Quoted 16 lines · Mike Rapoport

From: Mike Rapoport <[email protected]>

Hi,

These patches aim to remove CONFIG_HOLES_IN_ZONE and essentially hardwire pfn_valid_within() to 1.

The idea is to mark NOMAP pages as reserved in the memory map and restore the intended semantics of pfn_valid() to designate availability of struct page for a pfn.

With this the core mm will be able to cope with the fact that it cannot use NOMAP pages and the holes created by NOMAP ranges within MAX_ORDER blocks will be treated correctly even without the need for pfn_valid_within.

Quoted 2 lines

The patches are boot tested on qemu-system-aarch64.

Did you use EFI boot when testing this? The memory map is much more fragmented in that case, so this would be a good data point.

Quoted 47 lines

I beleive it would be best to route these via mmotm tree.

v4:
* rebase on v5.13-rc1

v3: Link: https://lore.kernel.org/lkml/[email protected]
* Fix minor issues found by Anshuman
* Freshen up the declaration of pfn_valid() to make it consistent with pfn_is_map_memory()
* Add more Acked-by and Reviewed-by tags, thanks Anshuman and David

v2: Link: https://lore.kernel.org/lkml/[email protected]
* Add check for PFN overflow in pfn_is_map_memory()
* Add Acked-by and Reviewed-by tags, thanks David.

v1: Link: https://lore.kernel.org/lkml/[email protected]
* Add comment about the semantics of pfn_valid() as Anshuman suggested
* Extend comments about MEMBLOCK_NOMAP, per Anshuman
* Use pfn_is_map_memory() name for the exported wrapper for memblock_is_map_memory(). It is still local to arch/arm64 in the end because of header dependency issues.

rfc: Link: https://lore.kernel.org/lkml/[email protected]

Mike Rapoport (4):
include/linux/mmzone.h: add documentation for pfn_valid()
memblock: update initialization of reserved pages
arm64: decouple check whether pfn is in linear map from pfn_valid()
arm64: drop pfn_valid_within() and simplify pfn_valid()

 arch/arm64/Kconfig              |  3 ---
 arch/arm64/include/asm/memory.h |  2 +-
 arch/arm64/include/asm/page.h   |  3 ++-
 arch/arm64/kvm/mmu.c            |  2 +-
 arch/arm64/mm/init.c            | 14 +++++++++++++-
 arch/arm64/mm/ioremap.c         |  4 ++--
 arch/arm64/mm/mmu.c             |  2 +-
 include/linux/memblock.h        |  4 +++-
 include/linux/mmzone.h          | 11 +++++++++++
 mm/memblock.c                   | 28 ++++++++++++++++++++++++++--
 10 files changed, 60 insertions(+), 13 deletions(-)

base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5 --
2.28.0

Quoted 17 lines · Ard Biesheuvel

Quoted 16 lines · Mike Rapoport

From: Mike Rapoport <[email protected]>

Hi,

These patches aim to remove CONFIG_HOLES_IN_ZONE and essentially hardwire pfn_valid_within() to 1.

The idea is to mark NOMAP pages as reserved in the memory map and restore the intended semantics of pfn_valid() to designate availability of struct page for a pfn.

With this the core mm will be able to cope with the fact that it cannot use NOMAP pages and the holes created by NOMAP ranges within MAX_ORDER blocks will be treated correctly even without the need for pfn_valid_within.

Quoted 5 lines

Quoted 2 lines

The patches are boot tested on qemu-system-aarch64.

Did you use EFI boot when testing this? The memory map is much more fragmented in that case, so this would be a good data point.

Right, something like this:

[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000040000000-0x00000000ffffbfff]
[    0.000000]   node   0: [mem 0x00000000ffffc000-0x00000000ffffffff]

[    0.000000]   node   0: [mem 0x0000000100000000-0x00000004386fffff]
[    0.000000]   node   0: [mem 0x0000000438700000-0x000000043899ffff]
[    0.000000]   node   0: [mem 0x00000004389a0000-0x00000004389bffff]
[    0.000000]   node   0: [mem 0x00000004389c0000-0x0000000438b5ffff]
[    0.000000]   node   0: [mem 0x0000000438b60000-0x000000043be3ffff]
[    0.000000]   node   0: [mem 0x000000043be40000-0x000000043becffff]
[    0.000000]   node   0: [mem 0x000000043bed0000-0x000000043bedffff]
[    0.000000]   node   0: [mem 0x000000043bee0000-0x000000043bffffff]
[    0.000000]   node   0: [mem 0x000000043c000000-0x000000043fffffff]

This is a pity really, because I don't see a fundamental reason for those tiny holes all over the place.

I know that EFI/ACPI mandates "IO style" memory access for those regions, but I fail to get why...

Quoted 43 lines

Quoted 43 lines

I beleive it would be best to route these via mmotm tree.

v4:
* rebase on v5.13-rc1

v3: Link: https://lore.kernel.org/lkml/[email protected]
* Fix minor issues found by Anshuman
* Freshen up the declaration of pfn_valid() to make it consistent with pfn_is_map_memory()
* Add more Acked-by and Reviewed-by tags, thanks Anshuman and David

v2: Link: https://lore.kernel.org/lkml/[email protected]
* Add check for PFN overflow in pfn_is_map_memory()
* Add Acked-by and Reviewed-by tags, thanks David.

v1: Link: https://lore.kernel.org/lkml/[email protected]
* Add comment about the semantics of pfn_valid() as Anshuman suggested
* Extend comments about MEMBLOCK_NOMAP, per Anshuman
* Use pfn_is_map_memory() name for the exported wrapper for memblock_is_map_memory(). It is still local to arch/arm64 in the end because of header dependency issues.

rfc: Link: https://lore.kernel.org/lkml/[email protected]

Mike Rapoport (4):
include/linux/mmzone.h: add documentation for pfn_valid()
memblock: update initialization of reserved pages
arm64: decouple check whether pfn is in linear map from pfn_valid()
arm64: drop pfn_valid_within() and simplify pfn_valid()

 arch/arm64/Kconfig              |  3 ---
 arch/arm64/include/asm/memory.h |  2 +-
 arch/arm64/include/asm/page.h   |  3 ++-
 arch/arm64/kvm/mmu.c            |  2 +-
 arch/arm64/mm/init.c            | 14 +++++++++++++-
 arch/arm64/mm/ioremap.c         |  4 ++--
 arch/arm64/mm/mmu.c             |  2 +-
 include/linux/memblock.h        |  4 +++-
 include/linux/mmzone.h          | 11 +++++++++++
 mm/memblock.c                   | 28 ++++++++++++++++++++++++++--
 10 files changed, 60 insertions(+), 13 deletions(-)

Quoted 4 lines

Quoted 4 lines

base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5 --
2.28.0

--
Sincerely yours,
Mike.

Quoted 19 lines · Mike Rapoport

Quoted 17 lines · Ard Biesheuvel

Quoted 16 lines · Mike Rapoport

From: Mike Rapoport <[email protected]>

Hi,

These patches aim to remove CONFIG_HOLES_IN_ZONE and essentially hardwire pfn_valid_within() to 1.

The idea is to mark NOMAP pages as reserved in the memory map and restore the intended semantics of pfn_valid() to designate availability of struct page for a pfn.

With this the core mm will be able to cope with the fact that it cannot use NOMAP pages and the holes created by NOMAP ranges within MAX_ORDER blocks will be treated correctly even without the need for pfn_valid_within.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

The patches are boot tested on qemu-system-aarch64.

Did you use EFI boot when testing this? The memory map is much more fragmented in that case, so this would be a good data point.

Right, something like this:

Yes, although it is not always that bad.

Quoted 3 lines

[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000040000000-0x00000000ffffbfff]
[    0.000000]   node   0: [mem 0x00000000ffffc000-0x00000000ffffffff]

This is allocated below 4 GB by the firmware, for reasons that are only valid on x86 (where some of the early boot chain is IA32 only)

Quoted 10 lines

[    0.000000]   node   0: [mem 0x0000000100000000-0x00000004386fffff]
[    0.000000]   node   0: [mem 0x0000000438700000-0x000000043899ffff]
[    0.000000]   node   0: [mem 0x00000004389a0000-0x00000004389bffff]
[    0.000000]   node   0: [mem 0x00000004389c0000-0x0000000438b5ffff]
[    0.000000]   node   0: [mem 0x0000000438b60000-0x000000043be3ffff]
[    0.000000]   node   0: [mem 0x000000043be40000-0x000000043becffff]
[    0.000000]   node   0: [mem 0x000000043bed0000-0x000000043bedffff]
[    0.000000]   node   0: [mem 0x000000043bee0000-0x000000043bffffff]
[    0.000000]   node   0: [mem 0x000000043c000000-0x000000043fffffff]

Quoted 3 lines

This is a pity really, because I don't see a fundamental reason for those tiny holes all over the place.

There is a config option in the firmware build that allows these regions to be preallocated using larger windows, which greatly reduces the fragmentation.

Quoted 3 lines

I know that EFI/ACPI mandates "IO style" memory access for those regions, but I fail to get why...

Not sure what you mean by 'IO style memory access'.

Quoted 51 lines

Quoted 43 lines

Quoted 43 lines

I beleive it would be best to route these via mmotm tree.

v4:
* rebase on v5.13-rc1

v3: Link: https://lore.kernel.org/lkml/[email protected]
* Fix minor issues found by Anshuman
* Freshen up the declaration of pfn_valid() to make it consistent with pfn_is_map_memory()
* Add more Acked-by and Reviewed-by tags, thanks Anshuman and David

v2: Link: https://lore.kernel.org/lkml/[email protected]
* Add check for PFN overflow in pfn_is_map_memory()
* Add Acked-by and Reviewed-by tags, thanks David.

v1: Link: https://lore.kernel.org/lkml/[email protected]
* Add comment about the semantics of pfn_valid() as Anshuman suggested
* Extend comments about MEMBLOCK_NOMAP, per Anshuman
* Use pfn_is_map_memory() name for the exported wrapper for memblock_is_map_memory(). It is still local to arch/arm64 in the end because of header dependency issues.

rfc: Link: https://lore.kernel.org/lkml/[email protected]

Mike Rapoport (4):
include/linux/mmzone.h: add documentation for pfn_valid()
memblock: update initialization of reserved pages
arm64: decouple check whether pfn is in linear map from pfn_valid()
arm64: drop pfn_valid_within() and simplify pfn_valid()

 arch/arm64/Kconfig              |  3 ---
 arch/arm64/include/asm/memory.h |  2 +-
 arch/arm64/include/asm/page.h   |  3 ++-
 arch/arm64/kvm/mmu.c            |  2 +-
 arch/arm64/mm/init.c            | 14 +++++++++++++-
 arch/arm64/mm/ioremap.c         |  4 ++--
 arch/arm64/mm/mmu.c             |  2 +-
 include/linux/memblock.h        |  4 +++-
 include/linux/mmzone.h          | 11 +++++++++++
 mm/memblock.c                   | 28 ++++++++++++++++++++++++++--
 10 files changed, 60 insertions(+), 13 deletions(-)

Quoted 4 lines

Quoted 4 lines

base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5 --
2.28.0

--
Sincerely yours,
Mike.