[PATCH V4 0/4] arm64/mm: Enable memory hot remove

[PATCH V4 0/4] arm64/mm: Enable memory hot remove

[Anshuman Khandual]

This series enables memory hot remove on arm64 after fixing a memblock
removal ordering problem in generic __remove_memory() and two possible
arm64 platform specific kernel page table race conditions. This series
is based on latest v5.2-rc1 tag.

Testing:

Memory hot remove has been tested on arm64 for 4K, 16K, 64K page config
options with all possible CONFIG_ARM64_VA_BITS and CONFIG_PGTABLE_LEVELS
combinations. Its only build tested on non-arm64 platforms.

Changes in V4:

- Implemented most of the suggestions from Mark Rutland
- Interchanged patch [PATCH 2/4] <---> [PATCH 3/4] and updated commit message
- Moved CONFIG_PGTABLE_LEVELS inside free_[pud|pmd]_table()
- Used READ_ONCE() in missing instances while accessing page table entries
- s/p???_present()/p???_none() for checking valid kernel page table entries
- WARN_ON() when an entry is !p???_none() and !p???_present() at the same time
- Updated memory hot-remove commit message with additional details as suggested
- Rebased the series on 5.2-rc1 with hotplug changes from David and Michal Hocko
- Collected all new Acked-by tags

Changes in V3: (https://lkml.org/lkml/2019/5/14/197)
 
- Implemented most of the suggestions from Mark Rutland for remove_pagetable()
- Fixed applicable PGTABLE_LEVEL wrappers around pgtable page freeing functions
- Replaced 'direct' with 'sparse_vmap' in remove_pagetable() with inverted polarity
- Changed pointer names ('p' at end) and removed tmp from iterations
- Perform intermediate TLB invalidation while clearing pgtable entries
- Dropped flush_tlb_kernel_range() in remove_pagetable()
- Added flush_tlb_kernel_range() in remove_pte_table() instead
- Renamed page freeing functions for pgtable page and mapped pages
- Used page range size instead of order while freeing mapped or pgtable pages
- Removed all PageReserved() handling while freeing mapped or pgtable pages
- Replaced XXX_index() with XXX_offset() while walking the kernel page table
- Used READ_ONCE() while fetching individual pgtable entries
- Taken overall init_mm.page_table_lock instead of just while changing an entry
- Dropped previously added [pmd|pud]_index() which are not required anymore

- Added a new patch to protect kernel page table race condition for ptdump
- Added a new patch from Mark Rutland to prevent huge-vmap with ptdump

Changes in V2: (https://lkml.org/lkml/2019/4/14/5)

- Added all received review and ack tags
- Split the series from ZONE_DEVICE enablement for better review
- Moved memblock re-order patch to the front as per Robin Murphy
- Updated commit message on memblock re-order patch per Michal Hocko
- Dropped [pmd|pud]_large() definitions
- Used existing [pmd|pud]_sect() instead of earlier [pmd|pud]_large()
- Removed __meminit and __ref tags as per Oscar Salvador
- Dropped unnecessary 'ret' init in arch_add_memory() per Robin Murphy
- Skipped calling into pgtable_page_dtor() for linear mapping page table
  pages and updated all relevant functions

Changes in V1: (https://lkml.org/lkml/2019/4/3/28)

Anshuman Khandual (3):
  mm/hotplug: Reorder arch_remove_memory() call in __remove_memory()
  arm64/mm: Hold memory hotplug lock while walking for kernel page table dump
  arm64/mm: Enable memory hot remove

Mark Rutland (1):
  arm64/mm: Inhibit huge-vmap with ptdump

 arch/arm64/Kconfig             |   3 +
 arch/arm64/mm/mmu.c            | 223 ++++++++++++++++++++++++++++++++++++++++-
 arch/arm64/mm/ptdump_debugfs.c |   3 +
 mm/memory_hotplug.c            |   2 +-
 4 files changed, 225 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH V4 1/4] mm/hotplug: Reorder arch_remove_memory() call in __remove_memory()

[Anshuman Khandual]

Memory hot remove uses get_nid_for_pfn() while tearing down linked sysfs
entries between memory block and node. It first checks pfn validity with
pfn_valid_within() before fetching nid. With CONFIG_HOLES_IN_ZONE config
(arm64 has this enabled) pfn_valid_within() calls pfn_valid().

pfn_valid() is an arch implementation on arm64 (CONFIG_HAVE_ARCH_PFN_VALID)
which scans all mapped memblock regions with memblock_is_map_memory(). This
creates a problem in memory hot remove path which has already removed given
memory range from memory block with memblock_[remove|free] before arriving
at unregister_mem_sect_under_nodes(). Hence get_nid_for_pfn() returns -1
skipping subsequent sysfs_remove_link() calls leaving node <-> memory block
sysfs entries as is. Subsequent memory add operation hits BUG_ON() because
of existing sysfs entries.

[   62.007176] NUMA: Unknown node for memory at 0x680000000, assuming node 0
[   62.052517] ------------[ cut here ]------------
[   62.053211] kernel BUG at mm/memory_hotplug.c:1143!
[   62.053868] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[   62.054589] Modules linked in:
[   62.054999] CPU: 19 PID: 3275 Comm: bash Not tainted 5.1.0-rc2-00004-g28cea40b2683 #41
[   62.056274] Hardware name: linux,dummy-virt (DT)
[   62.057166] pstate: 40400005 (nZcv daif +PAN -UAO)
[   62.058083] pc : add_memory_resource+0x1cc/0x1d8
[   62.058961] lr : add_memory_resource+0x10c/0x1d8
[   62.059842] sp : ffff0000168b3ce0
[   62.060477] x29: ffff0000168b3ce0 x28: ffff8005db546c00
[   62.061501] x27: 0000000000000000 x26: 0000000000000000
[   62.062509] x25: ffff0000111ef000 x24: ffff0000111ef5d0
[   62.063520] x23: 0000000000000000 x22: 00000006bfffffff
[   62.064540] x21: 00000000ffffffef x20: 00000000006c0000
[   62.065558] x19: 0000000000680000 x18: 0000000000000024
[   62.066566] x17: 0000000000000000 x16: 0000000000000000
[   62.067579] x15: ffffffffffffffff x14: ffff8005e412e890
[   62.068588] x13: ffff8005d6b105d8 x12: 0000000000000000
[   62.069610] x11: ffff8005d6b10490 x10: 0000000000000040
[   62.070615] x9 : ffff8005e412e898 x8 : ffff8005e412e890
[   62.071631] x7 : ffff8005d6b105d8 x6 : ffff8005db546c00
[   62.072640] x5 : 0000000000000001 x4 : 0000000000000002
[   62.073654] x3 : ffff8005d7049480 x2 : 0000000000000002
[   62.074666] x1 : 0000000000000003 x0 : 00000000ffffffef
[   62.075685] Process bash (pid: 3275, stack limit = 0x00000000d754280f)
[   62.076930] Call trace:
[   62.077411]  add_memory_resource+0x1cc/0x1d8
[   62.078227]  __add_memory+0x70/0xa8
[   62.078901]  probe_store+0xa4/0xc8
[   62.079561]  dev_attr_store+0x18/0x28
[   62.080270]  sysfs_kf_write+0x40/0x58
[   62.080992]  kernfs_fop_write+0xcc/0x1d8
[   62.081744]  __vfs_write+0x18/0x40
[   62.082400]  vfs_write+0xa4/0x1b0
[   62.083037]  ksys_write+0x5c/0xc0
[   62.083681]  __arm64_sys_write+0x18/0x20
[   62.084432]  el0_svc_handler+0x88/0x100
[   62.085177]  el0_svc+0x8/0xc

Re-ordering arch_remove_memory() with memblock_[free|remove] solves the
problem on arm64 as pfn_valid() behaves correctly and returns positive
as memblock for the address range still exists. arch_remove_memory()
removes applicable memory sections from zone with __remove_pages() and
tears down kernel linear mapping. Removing memblock regions afterwards
is safe because there is no other memblock (bootmem) allocator user that
late. So nobody is going to allocate from the removed range just to blow
up later. Also nobody should be using the bootmem allocated range else
we wouldn't allow to remove it. So reordering is indeed safe.

Acked-by: Michal Hocko <mhocko@suse.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 328878b..1dbda48 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1850,10 +1850,10 @@ void __ref __remove_memory(int nid, u64 start, u64 size)
 
 	/* remove memmap entry */
 	firmware_map_remove(start, start + size, "System RAM");
+	arch_remove_memory(nid, start, size, NULL);
 	memblock_free(start, size);
 	memblock_remove(start, size);
 
-	arch_remove_memory(nid, start, size, NULL);
 	__release_memory_resource(start, size);
 
 	try_offline_node(nid);
-- 
2.7.4

[PATCH V4 2/4] arm64/mm: Inhibit huge-vmap with ptdump

[Anshuman Khandual]

From: Mark Rutland <mark.rutland@arm.com>

The arm64 ptdump code can race with concurrent modification of the
kernel page tables. At the time this was added, this was sound as:

* Modifications to leaf entries could result in stale information being
  logged, but would not result in a functional problem.

* Boot time modifications to non-leaf entries (e.g. freeing of initmem)
  were performed when the ptdump code cannot be invoked.

* At runtime, modifications to non-leaf entries only occurred in the
  vmalloc region, and these were strictly additive, as intermediate
  entries were never freed.

However, since commit:

  commit 324420bf91f6 ("arm64: add support for ioremap() block mappings")

... it has been possible to create huge mappings in the vmalloc area at
runtime, and as part of this existing intermediate levels of table my be
removed and freed.

It's possible for the ptdump code to race with this, and continue to
walk tables which have been freed (and potentially poisoned or
reallocated). As a result of this, the ptdump code may dereference bogus
addresses, which could be fatal.

Since huge-vmap is a TLB and memory optimization, we can disable it when
the runtime ptdump code is in use to avoid this problem.

Fixes: 324420bf91f60582 ("arm64: add support for ioremap() block mappings")
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/mm/mmu.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index a170c63..a1bfc44 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -955,13 +955,18 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
 
 int __init arch_ioremap_pud_supported(void)
 {
-	/* only 4k granule supports level 1 block mappings */
-	return IS_ENABLED(CONFIG_ARM64_4K_PAGES);
+	/*
+	 * Only 4k granule supports level 1 block mappings.
+	 * SW table walks can't handle removal of intermediate entries.
+	 */
+	return IS_ENABLED(CONFIG_ARM64_4K_PAGES) &&
+	       !IS_ENABLED(CONFIG_ARM64_PTDUMP_DEBUGFS);
 }
 
 int __init arch_ioremap_pmd_supported(void)
 {
-	return 1;
+	/* See arch_ioremap_pud_supported() */
+	return !IS_ENABLED(CONFIG_ARM64_PTDUMP_DEBUGFS);
 }
 
 int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
-- 
2.7.4

[PATCH V4 3/4] arm64/mm: Hold memory hotplug lock while walking for kernel page table dump

[Anshuman Khandual]

The arm64 page table dump code can race with concurrent modification of the
kernel page tables. When a leaf entries are modified concurrently, the dump
code may log stale or inconsistent information for a VA range, but this is
otherwise not harmful.

When intermediate levels of table are freed, the dump code will continue to
use memory which has been freed and potentially reallocated for another
purpose. In such cases, the dump code may dereference bogus addresses,
leading to a number of potential problems.

Intermediate levels of table may by freed during memory hot-remove,
which will be enabled by a subsequent patch. To avoid racing with
this, take the memory hotplug lock when walking the kernel page table.

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/mm/ptdump_debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c
index 064163f..80171d1 100644
--- a/arch/arm64/mm/ptdump_debugfs.c
+++ b/arch/arm64/mm/ptdump_debugfs.c
@@ -7,7 +7,10 @@
 static int ptdump_show(struct seq_file *m, void *v)
 {
 	struct ptdump_info *info = m->private;
+
+	get_online_mems();
 	ptdump_walk_pgd(m, info);
+	put_online_mems();
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(ptdump);
-- 
2.7.4

Re: [PATCH V4 3/4] arm64/mm: Hold memory hotplug lock while walking for kernel page table dump

[Michal Hocko]

On Mon 20-05-19 10:48:35, Anshuman Khandual wrote:
> The arm64 page table dump code can race with concurrent modification of the
> kernel page tables. When a leaf entries are modified concurrently, the dump
> code may log stale or inconsistent information for a VA range, but this is
> otherwise not harmful.
> 
> When intermediate levels of table are freed, the dump code will continue to
> use memory which has been freed and potentially reallocated for another
> purpose. In such cases, the dump code may dereference bogus addresses,
> leading to a number of potential problems.
> 
> Intermediate levels of table may by freed during memory hot-remove,
> which will be enabled by a subsequent patch. To avoid racing with
> this, take the memory hotplug lock when walking the kernel page table.

I've had a comment on this patch in the previous version which didn't
get answered completely AFAICS. If you really insist then please make
sure to describe why does this really matter because this will make
any further changes to the hotplug locking harder and I would to see
that it is worth the additional trouble.

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH V4 3/4] arm64/mm: Hold memory hotplug lock while walking for kernel page table dump

[Anshuman Khandual]

On 05/21/2019 03:44 PM, Michal Hocko wrote:
> On Mon 20-05-19 10:48:35, Anshuman Khandual wrote:
>> The arm64 page table dump code can race with concurrent modification of the
>> kernel page tables. When a leaf entries are modified concurrently, the dump
>> code may log stale or inconsistent information for a VA range, but this is
>> otherwise not harmful.
>>
>> When intermediate levels of table are freed, the dump code will continue to
>> use memory which has been freed and potentially reallocated for another
>> purpose. In such cases, the dump code may dereference bogus addresses,
>> leading to a number of potential problems.
>>
>> Intermediate levels of table may by freed during memory hot-remove,
>> which will be enabled by a subsequent patch. To avoid racing with
>> this, take the memory hotplug lock when walking the kernel page table.
> 
> I've had a comment on this patch in the previous version which didn't
> get answered completely AFAICS. If you really insist then please make
> sure to describe why does this really matter because this will make
> any further changes to the hotplug locking harder and I would to see
> that it is worth the additional trouble.

Hello Michal,

I was under the impression (seems wrongful now) that the previous discussion
was complete. Nonetheless we can still discuss it further. Mark has responded
on the previous V3 thread [1] and because this particular patch does not have
any changes from last time, we can continue discussing this in that thread.

[1] https://lkml.org/lkml/2019/5/22/613   

- Anshuman

[PATCH V4 4/4] arm64/mm: Enable memory hot remove

[Anshuman Khandual]

The arch code for hot-remove must tear down portions of the linear map and
vmemmap corresponding to memory being removed. In both cases the page
tables mapping these regions must be freed, and when sparse vmemmap is in
use the memory backing the vmemmap must also be freed.

This patch adds a new remove_pagetable() helper which can be used to tear
down either region, and calls it from vmemmap_free() and
___remove_pgd_mapping(). The sparse_vmap argument determines whether the
backing memory will be freed.

While freeing intermediate level page table pages bail out if any of it's
entries are still valid. This can happen for partially filled kernel page
table either from a previously attempted failed memory hot add or while
removing an address range which does not span the entire page table page
range.

The vmemmap region may share levels of table with the vmalloc region. Take
the kernel ptl so that we can safely free potentially-shared tables.

While here update arch_add_memory() to handle __add_pages() failures by
just unmapping recently added kernel linear mapping. Now enable memory hot
remove on arm64 platforms by default with ARCH_ENABLE_MEMORY_HOTREMOVE.

This implementation is overall inspired from kernel page table tear down
procedure on X86 architecture.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/Kconfig  |   3 +
 arch/arm64/mm/mmu.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 213 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4780eb7..ce24427 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -267,6 +267,9 @@ config HAVE_GENERIC_GUP
 config ARCH_ENABLE_MEMORY_HOTPLUG
 	def_bool y
 
+config ARCH_ENABLE_MEMORY_HOTREMOVE
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index a1bfc44..0cf0d41 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -733,6 +733,187 @@ int kern_addr_valid(unsigned long addr)
 
 	return pfn_valid(pte_pfn(pte));
 }
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+static void free_hotplug_page_range(struct page *page, ssize_t size)
+{
+	WARN_ON(PageReserved(page));
+	free_pages((unsigned long)page_address(page), get_order(size));
+}
+
+static void free_hotplug_pgtable_page(struct page *page)
+{
+	free_hotplug_page_range(page, PAGE_SIZE);
+}
+
+static void free_pte_table(pte_t *ptep, pmd_t *pmdp, unsigned long addr)
+{
+	struct page *page;
+	int i;
+
+	for (i = 0; i < PTRS_PER_PTE; i++) {
+		if (!pte_none(ptep[i]))
+			return;
+	}
+
+	page = pmd_page(READ_ONCE(*pmdp));
+	pmd_clear(pmdp);
+	__flush_tlb_kernel_pgtable(addr);
+	free_hotplug_pgtable_page(page);
+}
+
+static void free_pmd_table(pmd_t *pmdp, pud_t *pudp, unsigned long addr)
+{
+	struct page *page;
+	int i;
+
+	if (CONFIG_PGTABLE_LEVELS <= 2)
+		return;
+
+	for (i = 0; i < PTRS_PER_PMD; i++) {
+		if (!pmd_none(pmdp[i]))
+			return;
+	}
+
+	page = pud_page(READ_ONCE(*pudp));
+	pud_clear(pudp);
+	__flush_tlb_kernel_pgtable(addr);
+	free_hotplug_pgtable_page(page);
+}
+
+static void free_pud_table(pud_t *pudp, pgd_t *pgdp, unsigned long addr)
+{
+	struct page *page;
+	int i;
+
+	if (CONFIG_PGTABLE_LEVELS <= 3)
+		return;
+
+	for (i = 0; i < PTRS_PER_PUD; i++) {
+		if (!pud_none(pudp[i]))
+			return;
+	}
+
+	page = pgd_page(READ_ONCE(*pgdp));
+	pgd_clear(pgdp);
+	__flush_tlb_kernel_pgtable(addr);
+	free_hotplug_pgtable_page(page);
+}
+
+static void
+remove_pte_table(pmd_t *pmdp, unsigned long addr,
+			unsigned long end, bool sparse_vmap)
+{
+	struct page *page;
+	pte_t *ptep, pte;
+	unsigned long start = addr;
+
+	for (; addr < end; addr += PAGE_SIZE) {
+		ptep = pte_offset_kernel(pmdp, addr);
+		pte = READ_ONCE(*ptep);
+
+		if (pte_none(pte))
+			continue;
+
+		WARN_ON(!pte_present(pte));
+		if (sparse_vmap) {
+			page = pte_page(pte);
+			free_hotplug_page_range(page, PAGE_SIZE);
+		}
+		pte_clear(&init_mm, addr, ptep);
+	}
+	flush_tlb_kernel_range(start, end);
+}
+
+static void
+remove_pmd_table(pud_t *pudp, unsigned long addr,
+			unsigned long end, bool sparse_vmap)
+{
+	unsigned long next;
+	struct page *page;
+	pte_t *ptep_base;
+	pmd_t *pmdp, pmd;
+
+	for (; addr < end; addr = next) {
+		next = pmd_addr_end(addr, end);
+		pmdp = pmd_offset(pudp, addr);
+		pmd = READ_ONCE(*pmdp);
+
+		if (pmd_none(pmd))
+			continue;
+
+		WARN_ON(!pmd_present(pmd));
+		if (pmd_sect(pmd)) {
+			if (sparse_vmap) {
+				page = pmd_page(pmd);
+				free_hotplug_page_range(page, PMD_SIZE);
+			}
+			pmd_clear(pmdp);
+			continue;
+		}
+		ptep_base = pte_offset_kernel(pmdp, 0UL);
+		remove_pte_table(pmdp, addr, next, sparse_vmap);
+		free_pte_table(ptep_base, pmdp, addr);
+	}
+}
+
+static void
+remove_pud_table(pgd_t *pgdp, unsigned long addr,
+			unsigned long end, bool sparse_vmap)
+{
+	unsigned long next;
+	struct page *page;
+	pmd_t *pmdp_base;
+	pud_t *pudp, pud;
+
+	for (; addr < end; addr = next) {
+		next = pud_addr_end(addr, end);
+		pudp = pud_offset(pgdp, addr);
+		pud = READ_ONCE(*pudp);
+
+		if (pud_none(pud))
+			continue;
+
+		WARN_ON(!pud_present(pud));
+		if (pud_sect(pud)) {
+			if (sparse_vmap) {
+				page = pud_page(pud);
+				free_hotplug_page_range(page, PUD_SIZE);
+			}
+			pud_clear(pudp);
+			continue;
+		}
+		pmdp_base = pmd_offset(pudp, 0UL);
+		remove_pmd_table(pudp, addr, next, sparse_vmap);
+		free_pmd_table(pmdp_base, pudp, addr);
+	}
+}
+
+static void
+remove_pagetable(unsigned long start, unsigned long end, bool sparse_vmap)
+{
+	unsigned long addr, next;
+	pud_t *pudp_base;
+	pgd_t *pgdp, pgd;
+
+	spin_lock(&init_mm.page_table_lock);
+	for (addr = start; addr < end; addr = next) {
+		next = pgd_addr_end(addr, end);
+		pgdp = pgd_offset_k(addr);
+		pgd = READ_ONCE(*pgdp);
+
+		if (pgd_none(pgd))
+			continue;
+
+		WARN_ON(!pgd_present(pgd));
+		pudp_base = pud_offset(pgdp, 0UL);
+		remove_pud_table(pgdp, addr, next, sparse_vmap);
+		free_pud_table(pudp_base, pgdp, addr);
+	}
+	spin_unlock(&init_mm.page_table_lock);
+}
+#endif
+
 #ifdef CONFIG_SPARSEMEM_VMEMMAP
 #if !ARM64_SWAPPER_USES_SECTION_MAPS
 int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
@@ -780,6 +961,9 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 void vmemmap_free(unsigned long start, unsigned long end,
 		struct vmem_altmap *altmap)
 {
+#ifdef CONFIG_MEMORY_HOTPLUG
+	remove_pagetable(start, end, true);
+#endif
 }
 #endif	/* CONFIG_SPARSEMEM_VMEMMAP */
 
@@ -1070,10 +1254,16 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr)
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG
+static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
+{
+	WARN_ON(pgdir != init_mm.pgd);
+	remove_pagetable(start, start + size, false);
+}
+
 int arch_add_memory(int nid, u64 start, u64 size,
 			struct mhp_restrictions *restrictions)
 {
-	int flags = 0;
+	int ret, flags = 0;
 
 	if (rodata_full || debug_pagealloc_enabled())
 		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
@@ -1081,7 +1271,25 @@ int arch_add_memory(int nid, u64 start, u64 size,
 	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
 			     size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
 
-	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
+	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
 			   restrictions);
+	if (ret)
+		__remove_pgd_mapping(swapper_pg_dir,
+					__phys_to_virt(start), size);
+	return ret;
+}
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+void arch_remove_memory(int nid, u64 start, u64 size,
+				struct vmem_altmap *altmap)
+{
+	unsigned long start_pfn = start >> PAGE_SHIFT;
+	unsigned long nr_pages = size >> PAGE_SHIFT;
+	struct zone *zone = page_zone(pfn_to_page(start_pfn));
+
+	__remove_pages(zone, start_pfn, nr_pages, altmap);
+	__remove_pgd_mapping(swapper_pg_dir,
+					__phys_to_virt(start), size);
 }
 #endif
+#endif
-- 
2.7.4

Re: [PATCH V4 4/4] arm64/mm: Enable memory hot remove

[David Hildenbrand]

On 20.05.19 07:18, Anshuman Khandual wrote:
> The arch code for hot-remove must tear down portions of the linear map and
> vmemmap corresponding to memory being removed. In both cases the page
> tables mapping these regions must be freed, and when sparse vmemmap is in
> use the memory backing the vmemmap must also be freed.
> 
> This patch adds a new remove_pagetable() helper which can be used to tear
> down either region, and calls it from vmemmap_free() and
> ___remove_pgd_mapping(). The sparse_vmap argument determines whether the
> backing memory will be freed.
> 
> While freeing intermediate level page table pages bail out if any of it's
> entries are still valid. This can happen for partially filled kernel page
> table either from a previously attempted failed memory hot add or while
> removing an address range which does not span the entire page table page
> range.
> 
> The vmemmap region may share levels of table with the vmalloc region. Take
> the kernel ptl so that we can safely free potentially-shared tables.
> 
> While here update arch_add_memory() to handle __add_pages() failures by
> just unmapping recently added kernel linear mapping. Now enable memory hot
> remove on arm64 platforms by default with ARCH_ENABLE_MEMORY_HOTREMOVE.
> 
> This implementation is overall inspired from kernel page table tear down
> procedure on X86 architecture.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/Kconfig  |   3 +
>  arch/arm64/mm/mmu.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 213 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 4780eb7..ce24427 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -267,6 +267,9 @@ config HAVE_GENERIC_GUP
>  config ARCH_ENABLE_MEMORY_HOTPLUG
>  	def_bool y
>  
> +config ARCH_ENABLE_MEMORY_HOTREMOVE
> +	def_bool y
> +
>  config SMP
>  	def_bool y
>  
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index a1bfc44..0cf0d41 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -733,6 +733,187 @@ int kern_addr_valid(unsigned long addr)
>  
>  	return pfn_valid(pte_pfn(pte));
>  }
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +static void free_hotplug_page_range(struct page *page, ssize_t size)
> +{
> +	WARN_ON(PageReserved(page));
> +	free_pages((unsigned long)page_address(page), get_order(size));
> +}
> +
> +static void free_hotplug_pgtable_page(struct page *page)
> +{
> +	free_hotplug_page_range(page, PAGE_SIZE);
> +}
> +
> +static void free_pte_table(pte_t *ptep, pmd_t *pmdp, unsigned long addr)
> +{
> +	struct page *page;
> +	int i;
> +
> +	for (i = 0; i < PTRS_PER_PTE; i++) {
> +		if (!pte_none(ptep[i]))
> +			return;
> +	}
> +
> +	page = pmd_page(READ_ONCE(*pmdp));
> +	pmd_clear(pmdp);
> +	__flush_tlb_kernel_pgtable(addr);
> +	free_hotplug_pgtable_page(page);
> +}
> +
> +static void free_pmd_table(pmd_t *pmdp, pud_t *pudp, unsigned long addr)
> +{
> +	struct page *page;
> +	int i;
> +
> +	if (CONFIG_PGTABLE_LEVELS <= 2)
> +		return;
> +
> +	for (i = 0; i < PTRS_PER_PMD; i++) {
> +		if (!pmd_none(pmdp[i]))
> +			return;
> +	}
> +
> +	page = pud_page(READ_ONCE(*pudp));
> +	pud_clear(pudp);
> +	__flush_tlb_kernel_pgtable(addr);
> +	free_hotplug_pgtable_page(page);
> +}
> +
> +static void free_pud_table(pud_t *pudp, pgd_t *pgdp, unsigned long addr)
> +{
> +	struct page *page;
> +	int i;
> +
> +	if (CONFIG_PGTABLE_LEVELS <= 3)
> +		return;
> +
> +	for (i = 0; i < PTRS_PER_PUD; i++) {
> +		if (!pud_none(pudp[i]))
> +			return;
> +	}
> +
> +	page = pgd_page(READ_ONCE(*pgdp));
> +	pgd_clear(pgdp);
> +	__flush_tlb_kernel_pgtable(addr);
> +	free_hotplug_pgtable_page(page);
> +}
> +
> +static void
> +remove_pte_table(pmd_t *pmdp, unsigned long addr,
> +			unsigned long end, bool sparse_vmap)
> +{
> +	struct page *page;
> +	pte_t *ptep, pte;
> +	unsigned long start = addr;
> +
> +	for (; addr < end; addr += PAGE_SIZE) {
> +		ptep = pte_offset_kernel(pmdp, addr);
> +		pte = READ_ONCE(*ptep);
> +
> +		if (pte_none(pte))
> +			continue;
> +
> +		WARN_ON(!pte_present(pte));
> +		if (sparse_vmap) {
> +			page = pte_page(pte);
> +			free_hotplug_page_range(page, PAGE_SIZE);
> +		}
> +		pte_clear(&init_mm, addr, ptep);
> +	}
> +	flush_tlb_kernel_range(start, end);
> +}
> +
> +static void
> +remove_pmd_table(pud_t *pudp, unsigned long addr,
> +			unsigned long end, bool sparse_vmap)
> +{
> +	unsigned long next;
> +	struct page *page;
> +	pte_t *ptep_base;
> +	pmd_t *pmdp, pmd;
> +
> +	for (; addr < end; addr = next) {
> +		next = pmd_addr_end(addr, end);
> +		pmdp = pmd_offset(pudp, addr);
> +		pmd = READ_ONCE(*pmdp);
> +
> +		if (pmd_none(pmd))
> +			continue;
> +
> +		WARN_ON(!pmd_present(pmd));
> +		if (pmd_sect(pmd)) {
> +			if (sparse_vmap) {
> +				page = pmd_page(pmd);
> +				free_hotplug_page_range(page, PMD_SIZE);
> +			}
> +			pmd_clear(pmdp);
> +			continue;
> +		}
> +		ptep_base = pte_offset_kernel(pmdp, 0UL);
> +		remove_pte_table(pmdp, addr, next, sparse_vmap);
> +		free_pte_table(ptep_base, pmdp, addr);
> +	}
> +}
> +
> +static void
> +remove_pud_table(pgd_t *pgdp, unsigned long addr,
> +			unsigned long end, bool sparse_vmap)
> +{
> +	unsigned long next;
> +	struct page *page;
> +	pmd_t *pmdp_base;
> +	pud_t *pudp, pud;
> +
> +	for (; addr < end; addr = next) {
> +		next = pud_addr_end(addr, end);
> +		pudp = pud_offset(pgdp, addr);
> +		pud = READ_ONCE(*pudp);
> +
> +		if (pud_none(pud))
> +			continue;
> +
> +		WARN_ON(!pud_present(pud));
> +		if (pud_sect(pud)) {
> +			if (sparse_vmap) {
> +				page = pud_page(pud);
> +				free_hotplug_page_range(page, PUD_SIZE);
> +			}
> +			pud_clear(pudp);
> +			continue;
> +		}
> +		pmdp_base = pmd_offset(pudp, 0UL);
> +		remove_pmd_table(pudp, addr, next, sparse_vmap);
> +		free_pmd_table(pmdp_base, pudp, addr);
> +	}
> +}
> +
> +static void
> +remove_pagetable(unsigned long start, unsigned long end, bool sparse_vmap)
> +{
> +	unsigned long addr, next;
> +	pud_t *pudp_base;
> +	pgd_t *pgdp, pgd;
> +
> +	spin_lock(&init_mm.page_table_lock);
> +	for (addr = start; addr < end; addr = next) {
> +		next = pgd_addr_end(addr, end);
> +		pgdp = pgd_offset_k(addr);
> +		pgd = READ_ONCE(*pgdp);
> +
> +		if (pgd_none(pgd))
> +			continue;
> +
> +		WARN_ON(!pgd_present(pgd));
> +		pudp_base = pud_offset(pgdp, 0UL);
> +		remove_pud_table(pgdp, addr, next, sparse_vmap);
> +		free_pud_table(pudp_base, pgdp, addr);
> +	}
> +	spin_unlock(&init_mm.page_table_lock);
> +}
> +#endif
> +
>  #ifdef CONFIG_SPARSEMEM_VMEMMAP
>  #if !ARM64_SWAPPER_USES_SECTION_MAPS
>  int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
> @@ -780,6 +961,9 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>  void vmemmap_free(unsigned long start, unsigned long end,
>  		struct vmem_altmap *altmap)
>  {
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	remove_pagetable(start, end, true);
> +#endif
>  }
>  #endif	/* CONFIG_SPARSEMEM_VMEMMAP */
>  
> @@ -1070,10 +1254,16 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr)
>  }
>  
>  #ifdef CONFIG_MEMORY_HOTPLUG
> +static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
> +{
> +	WARN_ON(pgdir != init_mm.pgd);
> +	remove_pagetable(start, start + size, false);
> +}
> +
>  int arch_add_memory(int nid, u64 start, u64 size,
>  			struct mhp_restrictions *restrictions)
>  {
> -	int flags = 0;
> +	int ret, flags = 0;
>  
>  	if (rodata_full || debug_pagealloc_enabled())
>  		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> @@ -1081,7 +1271,25 @@ int arch_add_memory(int nid, u64 start, u64 size,
>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>  			     size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
>  
> -	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> +	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>  			   restrictions);
> +	if (ret)
> +		__remove_pgd_mapping(swapper_pg_dir,
> +					__phys_to_virt(start), size);

Nit: Indentation of the parameters looks really weird.

> +	return ret;
> +}
> +
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +void arch_remove_memory(int nid, u64 start, u64 size,
> +				struct vmem_altmap *altmap)
> +{
> +	unsigned long start_pfn = start >> PAGE_SHIFT;
> +	unsigned long nr_pages = size >> PAGE_SHIFT;
> +	struct zone *zone = page_zone(pfn_to_page(start_pfn));
> +
> +	__remove_pages(zone, start_pfn, nr_pages, altmap);
> +	__remove_pgd_mapping(swapper_pg_dir,
> +					__phys_to_virt(start), size);

Dito, indentation of the parameters.

For these two changes (arch_*_memory)

Acked-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH V4 4/4] arm64/mm: Enable memory hot remove

[Anshuman Khandual]

On 05/21/2019 03:50 PM, David Hildenbrand wrote:
> On 20.05.19 07:18, Anshuman Khandual wrote:
>> The arch code for hot-remove must tear down portions of the linear map and
>> vmemmap corresponding to memory being removed. In both cases the page
>> tables mapping these regions must be freed, and when sparse vmemmap is in
>> use the memory backing the vmemmap must also be freed.
>>
>> This patch adds a new remove_pagetable() helper which can be used to tear
>> down either region, and calls it from vmemmap_free() and
>> ___remove_pgd_mapping(). The sparse_vmap argument determines whether the
>> backing memory will be freed.
>>
>> While freeing intermediate level page table pages bail out if any of it's
>> entries are still valid. This can happen for partially filled kernel page
>> table either from a previously attempted failed memory hot add or while
>> removing an address range which does not span the entire page table page
>> range.
>>
>> The vmemmap region may share levels of table with the vmalloc region. Take
>> the kernel ptl so that we can safely free potentially-shared tables.
>>
>> While here update arch_add_memory() to handle __add_pages() failures by
>> just unmapping recently added kernel linear mapping. Now enable memory hot
>> remove on arm64 platforms by default with ARCH_ENABLE_MEMORY_HOTREMOVE.
>>
>> This implementation is overall inspired from kernel page table tear down
>> procedure on X86 architecture.
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/Kconfig  |   3 +
>>  arch/arm64/mm/mmu.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 213 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 4780eb7..ce24427 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -267,6 +267,9 @@ config HAVE_GENERIC_GUP
>>  config ARCH_ENABLE_MEMORY_HOTPLUG
>>  	def_bool y
>>  
>> +config ARCH_ENABLE_MEMORY_HOTREMOVE
>> +	def_bool y
>> +
>>  config SMP
>>  	def_bool y
>>  
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index a1bfc44..0cf0d41 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -733,6 +733,187 @@ int kern_addr_valid(unsigned long addr)
>>  
>>  	return pfn_valid(pte_pfn(pte));
>>  }
>> +
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +static void free_hotplug_page_range(struct page *page, ssize_t size)
>> +{
>> +	WARN_ON(PageReserved(page));
>> +	free_pages((unsigned long)page_address(page), get_order(size));
>> +}
>> +
>> +static void free_hotplug_pgtable_page(struct page *page)
>> +{
>> +	free_hotplug_page_range(page, PAGE_SIZE);
>> +}
>> +
>> +static void free_pte_table(pte_t *ptep, pmd_t *pmdp, unsigned long addr)
>> +{
>> +	struct page *page;
>> +	int i;
>> +
>> +	for (i = 0; i < PTRS_PER_PTE; i++) {
>> +		if (!pte_none(ptep[i]))
>> +			return;
>> +	}
>> +
>> +	page = pmd_page(READ_ONCE(*pmdp));
>> +	pmd_clear(pmdp);
>> +	__flush_tlb_kernel_pgtable(addr);
>> +	free_hotplug_pgtable_page(page);
>> +}
>> +
>> +static void free_pmd_table(pmd_t *pmdp, pud_t *pudp, unsigned long addr)
>> +{
>> +	struct page *page;
>> +	int i;
>> +
>> +	if (CONFIG_PGTABLE_LEVELS <= 2)
>> +		return;
>> +
>> +	for (i = 0; i < PTRS_PER_PMD; i++) {
>> +		if (!pmd_none(pmdp[i]))
>> +			return;
>> +	}
>> +
>> +	page = pud_page(READ_ONCE(*pudp));
>> +	pud_clear(pudp);
>> +	__flush_tlb_kernel_pgtable(addr);
>> +	free_hotplug_pgtable_page(page);
>> +}
>> +
>> +static void free_pud_table(pud_t *pudp, pgd_t *pgdp, unsigned long addr)
>> +{
>> +	struct page *page;
>> +	int i;
>> +
>> +	if (CONFIG_PGTABLE_LEVELS <= 3)
>> +		return;
>> +
>> +	for (i = 0; i < PTRS_PER_PUD; i++) {
>> +		if (!pud_none(pudp[i]))
>> +			return;
>> +	}
>> +
>> +	page = pgd_page(READ_ONCE(*pgdp));
>> +	pgd_clear(pgdp);
>> +	__flush_tlb_kernel_pgtable(addr);
>> +	free_hotplug_pgtable_page(page);
>> +}
>> +
>> +static void
>> +remove_pte_table(pmd_t *pmdp, unsigned long addr,
>> +			unsigned long end, bool sparse_vmap)
>> +{
>> +	struct page *page;
>> +	pte_t *ptep, pte;
>> +	unsigned long start = addr;
>> +
>> +	for (; addr < end; addr += PAGE_SIZE) {
>> +		ptep = pte_offset_kernel(pmdp, addr);
>> +		pte = READ_ONCE(*ptep);
>> +
>> +		if (pte_none(pte))
>> +			continue;
>> +
>> +		WARN_ON(!pte_present(pte));
>> +		if (sparse_vmap) {
>> +			page = pte_page(pte);
>> +			free_hotplug_page_range(page, PAGE_SIZE);
>> +		}
>> +		pte_clear(&init_mm, addr, ptep);
>> +	}
>> +	flush_tlb_kernel_range(start, end);
>> +}
>> +
>> +static void
>> +remove_pmd_table(pud_t *pudp, unsigned long addr,
>> +			unsigned long end, bool sparse_vmap)
>> +{
>> +	unsigned long next;
>> +	struct page *page;
>> +	pte_t *ptep_base;
>> +	pmd_t *pmdp, pmd;
>> +
>> +	for (; addr < end; addr = next) {
>> +		next = pmd_addr_end(addr, end);
>> +		pmdp = pmd_offset(pudp, addr);
>> +		pmd = READ_ONCE(*pmdp);
>> +
>> +		if (pmd_none(pmd))
>> +			continue;
>> +
>> +		WARN_ON(!pmd_present(pmd));
>> +		if (pmd_sect(pmd)) {
>> +			if (sparse_vmap) {
>> +				page = pmd_page(pmd);
>> +				free_hotplug_page_range(page, PMD_SIZE);
>> +			}
>> +			pmd_clear(pmdp);
>> +			continue;
>> +		}
>> +		ptep_base = pte_offset_kernel(pmdp, 0UL);
>> +		remove_pte_table(pmdp, addr, next, sparse_vmap);
>> +		free_pte_table(ptep_base, pmdp, addr);
>> +	}
>> +}
>> +
>> +static void
>> +remove_pud_table(pgd_t *pgdp, unsigned long addr,
>> +			unsigned long end, bool sparse_vmap)
>> +{
>> +	unsigned long next;
>> +	struct page *page;
>> +	pmd_t *pmdp_base;
>> +	pud_t *pudp, pud;
>> +
>> +	for (; addr < end; addr = next) {
>> +		next = pud_addr_end(addr, end);
>> +		pudp = pud_offset(pgdp, addr);
>> +		pud = READ_ONCE(*pudp);
>> +
>> +		if (pud_none(pud))
>> +			continue;
>> +
>> +		WARN_ON(!pud_present(pud));
>> +		if (pud_sect(pud)) {
>> +			if (sparse_vmap) {
>> +				page = pud_page(pud);
>> +				free_hotplug_page_range(page, PUD_SIZE);
>> +			}
>> +			pud_clear(pudp);
>> +			continue;
>> +		}
>> +		pmdp_base = pmd_offset(pudp, 0UL);
>> +		remove_pmd_table(pudp, addr, next, sparse_vmap);
>> +		free_pmd_table(pmdp_base, pudp, addr);
>> +	}
>> +}
>> +
>> +static void
>> +remove_pagetable(unsigned long start, unsigned long end, bool sparse_vmap)
>> +{
>> +	unsigned long addr, next;
>> +	pud_t *pudp_base;
>> +	pgd_t *pgdp, pgd;
>> +
>> +	spin_lock(&init_mm.page_table_lock);
>> +	for (addr = start; addr < end; addr = next) {
>> +		next = pgd_addr_end(addr, end);
>> +		pgdp = pgd_offset_k(addr);
>> +		pgd = READ_ONCE(*pgdp);
>> +
>> +		if (pgd_none(pgd))
>> +			continue;
>> +
>> +		WARN_ON(!pgd_present(pgd));
>> +		pudp_base = pud_offset(pgdp, 0UL);
>> +		remove_pud_table(pgdp, addr, next, sparse_vmap);
>> +		free_pud_table(pudp_base, pgdp, addr);
>> +	}
>> +	spin_unlock(&init_mm.page_table_lock);
>> +}
>> +#endif
>> +
>>  #ifdef CONFIG_SPARSEMEM_VMEMMAP
>>  #if !ARM64_SWAPPER_USES_SECTION_MAPS
>>  int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>> @@ -780,6 +961,9 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>  void vmemmap_free(unsigned long start, unsigned long end,
>>  		struct vmem_altmap *altmap)
>>  {
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +	remove_pagetable(start, end, true);
>> +#endif
>>  }
>>  #endif	/* CONFIG_SPARSEMEM_VMEMMAP */
>>  
>> @@ -1070,10 +1254,16 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr)
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTPLUG
>> +static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>> +{
>> +	WARN_ON(pgdir != init_mm.pgd);
>> +	remove_pagetable(start, start + size, false);
>> +}
>> +
>>  int arch_add_memory(int nid, u64 start, u64 size,
>>  			struct mhp_restrictions *restrictions)
>>  {
>> -	int flags = 0;
>> +	int ret, flags = 0;
>>  
>>  	if (rodata_full || debug_pagealloc_enabled())
>>  		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>> @@ -1081,7 +1271,25 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>>  			     size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
>>  
>> -	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>> +	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>>  			   restrictions);
>> +	if (ret)
>> +		__remove_pgd_mapping(swapper_pg_dir,
>> +					__phys_to_virt(start), size);
> 
> Nit: Indentation of the parameters looks really weird.
> 
>> +	return ret;
>> +}
>> +
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +void arch_remove_memory(int nid, u64 start, u64 size,
>> +				struct vmem_altmap *altmap)
>> +{
>> +	unsigned long start_pfn = start >> PAGE_SHIFT;
>> +	unsigned long nr_pages = size >> PAGE_SHIFT;
>> +	struct zone *zone = page_zone(pfn_to_page(start_pfn));
>> +
>> +	__remove_pages(zone, start_pfn, nr_pages, altmap);
>> +	__remove_pgd_mapping(swapper_pg_dir,
>> +					__phys_to_virt(start), size);
> 
> Dito, indentation of the parameters.
> 
> For these two changes (arch_*_memory)
> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks David. The following change on this patch should fix the indentation
problem. If there are no other comments I will incorporate this and re-spin
the series once more.

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 0cf0d41..a87ba18 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1273,9 +1273,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
 
 	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
 			   restrictions);
-	if (ret)
-		__remove_pgd_mapping(swapper_pg_dir,
-					__phys_to_virt(start), size);
+	if (!ret)
+		return ret;
+
+	__remove_pgd_mapping(swapper_pg_dir, __phys_to_virt(start), size);
 	return ret;
 }
 
@@ -1288,8 +1289,7 @@ void arch_remove_memory(int nid, u64 start, u64 size,
 	struct zone *zone = page_zone(pfn_to_page(start_pfn));
 
 	__remove_pages(zone, start_pfn, nr_pages, altmap);
-	__remove_pgd_mapping(swapper_pg_dir,
-					__phys_to_virt(start), size);
+	__remove_pgd_mapping(swapper_pg_dir, __phys_to_virt(start), size);
 }
 #endif
 #endif

Re: [PATCH V4 4/4] arm64/mm: Enable memory hot remove

[David Hildenbrand]

On 27.05.19 10:09, Anshuman Khandual wrote:
> 
> 
> On 05/21/2019 03:50 PM, David Hildenbrand wrote:
>> On 20.05.19 07:18, Anshuman Khandual wrote:
>>> The arch code for hot-remove must tear down portions of the linear map and
>>> vmemmap corresponding to memory being removed. In both cases the page
>>> tables mapping these regions must be freed, and when sparse vmemmap is in
>>> use the memory backing the vmemmap must also be freed.
>>>
>>> This patch adds a new remove_pagetable() helper which can be used to tear
>>> down either region, and calls it from vmemmap_free() and
>>> ___remove_pgd_mapping(). The sparse_vmap argument determines whether the
>>> backing memory will be freed.
>>>
>>> While freeing intermediate level page table pages bail out if any of it's
>>> entries are still valid. This can happen for partially filled kernel page
>>> table either from a previously attempted failed memory hot add or while
>>> removing an address range which does not span the entire page table page
>>> range.
>>>
>>> The vmemmap region may share levels of table with the vmalloc region. Take
>>> the kernel ptl so that we can safely free potentially-shared tables.
>>>
>>> While here update arch_add_memory() to handle __add_pages() failures by
>>> just unmapping recently added kernel linear mapping. Now enable memory hot
>>> remove on arm64 platforms by default with ARCH_ENABLE_MEMORY_HOTREMOVE.
>>>
>>> This implementation is overall inspired from kernel page table tear down
>>> procedure on X86 architecture.
>>>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>>  arch/arm64/Kconfig  |   3 +
>>>  arch/arm64/mm/mmu.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 213 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index 4780eb7..ce24427 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -267,6 +267,9 @@ config HAVE_GENERIC_GUP
>>>  config ARCH_ENABLE_MEMORY_HOTPLUG
>>>  	def_bool y
>>>  
>>> +config ARCH_ENABLE_MEMORY_HOTREMOVE
>>> +	def_bool y
>>> +
>>>  config SMP
>>>  	def_bool y
>>>  
>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>> index a1bfc44..0cf0d41 100644
>>> --- a/arch/arm64/mm/mmu.c
>>> +++ b/arch/arm64/mm/mmu.c
>>> @@ -733,6 +733,187 @@ int kern_addr_valid(unsigned long addr)
>>>  
>>>  	return pfn_valid(pte_pfn(pte));
>>>  }
>>> +
>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>> +static void free_hotplug_page_range(struct page *page, ssize_t size)
>>> +{
>>> +	WARN_ON(PageReserved(page));
>>> +	free_pages((unsigned long)page_address(page), get_order(size));
>>> +}
>>> +
>>> +static void free_hotplug_pgtable_page(struct page *page)
>>> +{
>>> +	free_hotplug_page_range(page, PAGE_SIZE);
>>> +}
>>> +
>>> +static void free_pte_table(pte_t *ptep, pmd_t *pmdp, unsigned long addr)
>>> +{
>>> +	struct page *page;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < PTRS_PER_PTE; i++) {
>>> +		if (!pte_none(ptep[i]))
>>> +			return;
>>> +	}
>>> +
>>> +	page = pmd_page(READ_ONCE(*pmdp));
>>> +	pmd_clear(pmdp);
>>> +	__flush_tlb_kernel_pgtable(addr);
>>> +	free_hotplug_pgtable_page(page);
>>> +}
>>> +
>>> +static void free_pmd_table(pmd_t *pmdp, pud_t *pudp, unsigned long addr)
>>> +{
>>> +	struct page *page;
>>> +	int i;
>>> +
>>> +	if (CONFIG_PGTABLE_LEVELS <= 2)
>>> +		return;
>>> +
>>> +	for (i = 0; i < PTRS_PER_PMD; i++) {
>>> +		if (!pmd_none(pmdp[i]))
>>> +			return;
>>> +	}
>>> +
>>> +	page = pud_page(READ_ONCE(*pudp));
>>> +	pud_clear(pudp);
>>> +	__flush_tlb_kernel_pgtable(addr);
>>> +	free_hotplug_pgtable_page(page);
>>> +}
>>> +
>>> +static void free_pud_table(pud_t *pudp, pgd_t *pgdp, unsigned long addr)
>>> +{
>>> +	struct page *page;
>>> +	int i;
>>> +
>>> +	if (CONFIG_PGTABLE_LEVELS <= 3)
>>> +		return;
>>> +
>>> +	for (i = 0; i < PTRS_PER_PUD; i++) {
>>> +		if (!pud_none(pudp[i]))
>>> +			return;
>>> +	}
>>> +
>>> +	page = pgd_page(READ_ONCE(*pgdp));
>>> +	pgd_clear(pgdp);
>>> +	__flush_tlb_kernel_pgtable(addr);
>>> +	free_hotplug_pgtable_page(page);
>>> +}
>>> +
>>> +static void
>>> +remove_pte_table(pmd_t *pmdp, unsigned long addr,
>>> +			unsigned long end, bool sparse_vmap)
>>> +{
>>> +	struct page *page;
>>> +	pte_t *ptep, pte;
>>> +	unsigned long start = addr;
>>> +
>>> +	for (; addr < end; addr += PAGE_SIZE) {
>>> +		ptep = pte_offset_kernel(pmdp, addr);
>>> +		pte = READ_ONCE(*ptep);
>>> +
>>> +		if (pte_none(pte))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pte_present(pte));
>>> +		if (sparse_vmap) {
>>> +			page = pte_page(pte);
>>> +			free_hotplug_page_range(page, PAGE_SIZE);
>>> +		}
>>> +		pte_clear(&init_mm, addr, ptep);
>>> +	}
>>> +	flush_tlb_kernel_range(start, end);
>>> +}
>>> +
>>> +static void
>>> +remove_pmd_table(pud_t *pudp, unsigned long addr,
>>> +			unsigned long end, bool sparse_vmap)
>>> +{
>>> +	unsigned long next;
>>> +	struct page *page;
>>> +	pte_t *ptep_base;
>>> +	pmd_t *pmdp, pmd;
>>> +
>>> +	for (; addr < end; addr = next) {
>>> +		next = pmd_addr_end(addr, end);
>>> +		pmdp = pmd_offset(pudp, addr);
>>> +		pmd = READ_ONCE(*pmdp);
>>> +
>>> +		if (pmd_none(pmd))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pmd_present(pmd));
>>> +		if (pmd_sect(pmd)) {
>>> +			if (sparse_vmap) {
>>> +				page = pmd_page(pmd);
>>> +				free_hotplug_page_range(page, PMD_SIZE);
>>> +			}
>>> +			pmd_clear(pmdp);
>>> +			continue;
>>> +		}
>>> +		ptep_base = pte_offset_kernel(pmdp, 0UL);
>>> +		remove_pte_table(pmdp, addr, next, sparse_vmap);
>>> +		free_pte_table(ptep_base, pmdp, addr);
>>> +	}
>>> +}
>>> +
>>> +static void
>>> +remove_pud_table(pgd_t *pgdp, unsigned long addr,
>>> +			unsigned long end, bool sparse_vmap)
>>> +{
>>> +	unsigned long next;
>>> +	struct page *page;
>>> +	pmd_t *pmdp_base;
>>> +	pud_t *pudp, pud;
>>> +
>>> +	for (; addr < end; addr = next) {
>>> +		next = pud_addr_end(addr, end);
>>> +		pudp = pud_offset(pgdp, addr);
>>> +		pud = READ_ONCE(*pudp);
>>> +
>>> +		if (pud_none(pud))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pud_present(pud));
>>> +		if (pud_sect(pud)) {
>>> +			if (sparse_vmap) {
>>> +				page = pud_page(pud);
>>> +				free_hotplug_page_range(page, PUD_SIZE);
>>> +			}
>>> +			pud_clear(pudp);
>>> +			continue;
>>> +		}
>>> +		pmdp_base = pmd_offset(pudp, 0UL);
>>> +		remove_pmd_table(pudp, addr, next, sparse_vmap);
>>> +		free_pmd_table(pmdp_base, pudp, addr);
>>> +	}
>>> +}
>>> +
>>> +static void
>>> +remove_pagetable(unsigned long start, unsigned long end, bool sparse_vmap)
>>> +{
>>> +	unsigned long addr, next;
>>> +	pud_t *pudp_base;
>>> +	pgd_t *pgdp, pgd;
>>> +
>>> +	spin_lock(&init_mm.page_table_lock);
>>> +	for (addr = start; addr < end; addr = next) {
>>> +		next = pgd_addr_end(addr, end);
>>> +		pgdp = pgd_offset_k(addr);
>>> +		pgd = READ_ONCE(*pgdp);
>>> +
>>> +		if (pgd_none(pgd))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pgd_present(pgd));
>>> +		pudp_base = pud_offset(pgdp, 0UL);
>>> +		remove_pud_table(pgdp, addr, next, sparse_vmap);
>>> +		free_pud_table(pudp_base, pgdp, addr);
>>> +	}
>>> +	spin_unlock(&init_mm.page_table_lock);
>>> +}
>>> +#endif
>>> +
>>>  #ifdef CONFIG_SPARSEMEM_VMEMMAP
>>>  #if !ARM64_SWAPPER_USES_SECTION_MAPS
>>>  int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>> @@ -780,6 +961,9 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>>  void vmemmap_free(unsigned long start, unsigned long end,
>>>  		struct vmem_altmap *altmap)
>>>  {
>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>> +	remove_pagetable(start, end, true);
>>> +#endif
>>>  }
>>>  #endif	/* CONFIG_SPARSEMEM_VMEMMAP */
>>>  
>>> @@ -1070,10 +1254,16 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr)
>>>  }
>>>  
>>>  #ifdef CONFIG_MEMORY_HOTPLUG
>>> +static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>>> +{
>>> +	WARN_ON(pgdir != init_mm.pgd);
>>> +	remove_pagetable(start, start + size, false);
>>> +}
>>> +
>>>  int arch_add_memory(int nid, u64 start, u64 size,
>>>  			struct mhp_restrictions *restrictions)
>>>  {
>>> -	int flags = 0;
>>> +	int ret, flags = 0;
>>>  
>>>  	if (rodata_full || debug_pagealloc_enabled())
>>>  		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>> @@ -1081,7 +1271,25 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>>>  			     size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
>>>  
>>> -	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>>> +	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>>>  			   restrictions);
>>> +	if (ret)
>>> +		__remove_pgd_mapping(swapper_pg_dir,
>>> +					__phys_to_virt(start), size);
>>
>> Nit: Indentation of the parameters looks really weird.
>>
>>> +	return ret;
>>> +}
>>> +
>>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>>> +void arch_remove_memory(int nid, u64 start, u64 size,
>>> +				struct vmem_altmap *altmap)
>>> +{
>>> +	unsigned long start_pfn = start >> PAGE_SHIFT;
>>> +	unsigned long nr_pages = size >> PAGE_SHIFT;
>>> +	struct zone *zone = page_zone(pfn_to_page(start_pfn));
>>> +
>>> +	__remove_pages(zone, start_pfn, nr_pages, altmap);
>>> +	__remove_pgd_mapping(swapper_pg_dir,
>>> +					__phys_to_virt(start), size);
>>
>> Dito, indentation of the parameters.
>>
>> For these two changes (arch_*_memory)
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
> 
> Thanks David. The following change on this patch should fix the indentation
> problem. If there are no other comments I will incorporate this and re-spin
> the series once more.
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 0cf0d41..a87ba18 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1273,9 +1273,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
>  
>  	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>  			   restrictions);
> -	if (ret)
> -		__remove_pgd_mapping(swapper_pg_dir,
> -					__phys_to_virt(start), size);
> +	if (!ret)
> +		return ret;
> +
> +	__remove_pgd_mapping(swapper_pg_dir, __phys_to_virt(start), size);


you can leave the old code, just make sure the start of all parameters
on new lines are aligned, that seemed to be not the case

if (ret)
	__remove_pgd_mapping(swapper_pg_dir,
			     __phys_to_virt(start), size);
............................^

>  	return ret;
>  }
>  
> @@ -1288,8 +1289,7 @@ void arch_remove_memory(int nid, u64 start, u64 size,
>  	struct zone *zone = page_zone(pfn_to_page(start_pfn));
>  
>  	__remove_pages(zone, start_pfn, nr_pages, altmap);
> -	__remove_pgd_mapping(swapper_pg_dir,
> -					__phys_to_virt(start), size);
> +	__remove_pgd_mapping(swapper_pg_dir, __phys_to_virt(start), size);
>  }
>  #endif
>  #endif
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH V4 4/4] arm64/mm: Enable memory hot remove

[David Hildenbrand]

On 27.05.19 10:09, Anshuman Khandual wrote:
> 
> 
> On 05/21/2019 03:50 PM, David Hildenbrand wrote:
>> On 20.05.19 07:18, Anshuman Khandual wrote:
>>> The arch code for hot-remove must tear down portions of the linear map and
>>> vmemmap corresponding to memory being removed. In both cases the page
>>> tables mapping these regions must be freed, and when sparse vmemmap is in
>>> use the memory backing the vmemmap must also be freed.
>>>
>>> This patch adds a new remove_pagetable() helper which can be used to tear
>>> down either region, and calls it from vmemmap_free() and
>>> ___remove_pgd_mapping(). The sparse_vmap argument determines whether the
>>> backing memory will be freed.
>>>
>>> While freeing intermediate level page table pages bail out if any of it's
>>> entries are still valid. This can happen for partially filled kernel page
>>> table either from a previously attempted failed memory hot add or while
>>> removing an address range which does not span the entire page table page
>>> range.
>>>
>>> The vmemmap region may share levels of table with the vmalloc region. Take
>>> the kernel ptl so that we can safely free potentially-shared tables.
>>>
>>> While here update arch_add_memory() to handle __add_pages() failures by
>>> just unmapping recently added kernel linear mapping. Now enable memory hot
>>> remove on arm64 platforms by default with ARCH_ENABLE_MEMORY_HOTREMOVE.
>>>
>>> This implementation is overall inspired from kernel page table tear down
>>> procedure on X86 architecture.
>>>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>>  arch/arm64/Kconfig  |   3 +
>>>  arch/arm64/mm/mmu.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 213 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index 4780eb7..ce24427 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -267,6 +267,9 @@ config HAVE_GENERIC_GUP
>>>  config ARCH_ENABLE_MEMORY_HOTPLUG
>>>  	def_bool y
>>>  
>>> +config ARCH_ENABLE_MEMORY_HOTREMOVE
>>> +	def_bool y
>>> +
>>>  config SMP
>>>  	def_bool y
>>>  
>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>> index a1bfc44..0cf0d41 100644
>>> --- a/arch/arm64/mm/mmu.c
>>> +++ b/arch/arm64/mm/mmu.c
>>> @@ -733,6 +733,187 @@ int kern_addr_valid(unsigned long addr)
>>>  
>>>  	return pfn_valid(pte_pfn(pte));
>>>  }
>>> +
>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>> +static void free_hotplug_page_range(struct page *page, ssize_t size)
>>> +{
>>> +	WARN_ON(PageReserved(page));
>>> +	free_pages((unsigned long)page_address(page), get_order(size));
>>> +}
>>> +
>>> +static void free_hotplug_pgtable_page(struct page *page)
>>> +{
>>> +	free_hotplug_page_range(page, PAGE_SIZE);
>>> +}
>>> +
>>> +static void free_pte_table(pte_t *ptep, pmd_t *pmdp, unsigned long addr)
>>> +{
>>> +	struct page *page;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < PTRS_PER_PTE; i++) {
>>> +		if (!pte_none(ptep[i]))
>>> +			return;
>>> +	}
>>> +
>>> +	page = pmd_page(READ_ONCE(*pmdp));
>>> +	pmd_clear(pmdp);
>>> +	__flush_tlb_kernel_pgtable(addr);
>>> +	free_hotplug_pgtable_page(page);
>>> +}
>>> +
>>> +static void free_pmd_table(pmd_t *pmdp, pud_t *pudp, unsigned long addr)
>>> +{
>>> +	struct page *page;
>>> +	int i;
>>> +
>>> +	if (CONFIG_PGTABLE_LEVELS <= 2)
>>> +		return;
>>> +
>>> +	for (i = 0; i < PTRS_PER_PMD; i++) {
>>> +		if (!pmd_none(pmdp[i]))
>>> +			return;
>>> +	}
>>> +
>>> +	page = pud_page(READ_ONCE(*pudp));
>>> +	pud_clear(pudp);
>>> +	__flush_tlb_kernel_pgtable(addr);
>>> +	free_hotplug_pgtable_page(page);
>>> +}
>>> +
>>> +static void free_pud_table(pud_t *pudp, pgd_t *pgdp, unsigned long addr)
>>> +{
>>> +	struct page *page;
>>> +	int i;
>>> +
>>> +	if (CONFIG_PGTABLE_LEVELS <= 3)
>>> +		return;
>>> +
>>> +	for (i = 0; i < PTRS_PER_PUD; i++) {
>>> +		if (!pud_none(pudp[i]))
>>> +			return;
>>> +	}
>>> +
>>> +	page = pgd_page(READ_ONCE(*pgdp));
>>> +	pgd_clear(pgdp);
>>> +	__flush_tlb_kernel_pgtable(addr);
>>> +	free_hotplug_pgtable_page(page);
>>> +}
>>> +
>>> +static void
>>> +remove_pte_table(pmd_t *pmdp, unsigned long addr,
>>> +			unsigned long end, bool sparse_vmap)
>>> +{
>>> +	struct page *page;
>>> +	pte_t *ptep, pte;
>>> +	unsigned long start = addr;
>>> +
>>> +	for (; addr < end; addr += PAGE_SIZE) {
>>> +		ptep = pte_offset_kernel(pmdp, addr);
>>> +		pte = READ_ONCE(*ptep);
>>> +
>>> +		if (pte_none(pte))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pte_present(pte));
>>> +		if (sparse_vmap) {
>>> +			page = pte_page(pte);
>>> +			free_hotplug_page_range(page, PAGE_SIZE);
>>> +		}
>>> +		pte_clear(&init_mm, addr, ptep);
>>> +	}
>>> +	flush_tlb_kernel_range(start, end);
>>> +}
>>> +
>>> +static void
>>> +remove_pmd_table(pud_t *pudp, unsigned long addr,
>>> +			unsigned long end, bool sparse_vmap)
>>> +{
>>> +	unsigned long next;
>>> +	struct page *page;
>>> +	pte_t *ptep_base;
>>> +	pmd_t *pmdp, pmd;
>>> +
>>> +	for (; addr < end; addr = next) {
>>> +		next = pmd_addr_end(addr, end);
>>> +		pmdp = pmd_offset(pudp, addr);
>>> +		pmd = READ_ONCE(*pmdp);
>>> +
>>> +		if (pmd_none(pmd))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pmd_present(pmd));
>>> +		if (pmd_sect(pmd)) {
>>> +			if (sparse_vmap) {
>>> +				page = pmd_page(pmd);
>>> +				free_hotplug_page_range(page, PMD_SIZE);
>>> +			}
>>> +			pmd_clear(pmdp);
>>> +			continue;
>>> +		}
>>> +		ptep_base = pte_offset_kernel(pmdp, 0UL);
>>> +		remove_pte_table(pmdp, addr, next, sparse_vmap);
>>> +		free_pte_table(ptep_base, pmdp, addr);
>>> +	}
>>> +}
>>> +
>>> +static void
>>> +remove_pud_table(pgd_t *pgdp, unsigned long addr,
>>> +			unsigned long end, bool sparse_vmap)
>>> +{
>>> +	unsigned long next;
>>> +	struct page *page;
>>> +	pmd_t *pmdp_base;
>>> +	pud_t *pudp, pud;
>>> +
>>> +	for (; addr < end; addr = next) {
>>> +		next = pud_addr_end(addr, end);
>>> +		pudp = pud_offset(pgdp, addr);
>>> +		pud = READ_ONCE(*pudp);
>>> +
>>> +		if (pud_none(pud))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pud_present(pud));
>>> +		if (pud_sect(pud)) {
>>> +			if (sparse_vmap) {
>>> +				page = pud_page(pud);
>>> +				free_hotplug_page_range(page, PUD_SIZE);
>>> +			}
>>> +			pud_clear(pudp);
>>> +			continue;
>>> +		}
>>> +		pmdp_base = pmd_offset(pudp, 0UL);
>>> +		remove_pmd_table(pudp, addr, next, sparse_vmap);
>>> +		free_pmd_table(pmdp_base, pudp, addr);
>>> +	}
>>> +}
>>> +
>>> +static void
>>> +remove_pagetable(unsigned long start, unsigned long end, bool sparse_vmap)
>>> +{
>>> +	unsigned long addr, next;
>>> +	pud_t *pudp_base;
>>> +	pgd_t *pgdp, pgd;
>>> +
>>> +	spin_lock(&init_mm.page_table_lock);
>>> +	for (addr = start; addr < end; addr = next) {
>>> +		next = pgd_addr_end(addr, end);
>>> +		pgdp = pgd_offset_k(addr);
>>> +		pgd = READ_ONCE(*pgdp);
>>> +
>>> +		if (pgd_none(pgd))
>>> +			continue;
>>> +
>>> +		WARN_ON(!pgd_present(pgd));
>>> +		pudp_base = pud_offset(pgdp, 0UL);
>>> +		remove_pud_table(pgdp, addr, next, sparse_vmap);
>>> +		free_pud_table(pudp_base, pgdp, addr);
>>> +	}
>>> +	spin_unlock(&init_mm.page_table_lock);
>>> +}
>>> +#endif
>>> +
>>>  #ifdef CONFIG_SPARSEMEM_VMEMMAP
>>>  #if !ARM64_SWAPPER_USES_SECTION_MAPS
>>>  int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>> @@ -780,6 +961,9 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>>  void vmemmap_free(unsigned long start, unsigned long end,
>>>  		struct vmem_altmap *altmap)
>>>  {
>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>> +	remove_pagetable(start, end, true);
>>> +#endif
>>>  }
>>>  #endif	/* CONFIG_SPARSEMEM_VMEMMAP */
>>>  
>>> @@ -1070,10 +1254,16 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr)
>>>  }
>>>  
>>>  #ifdef CONFIG_MEMORY_HOTPLUG
>>> +static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>>> +{
>>> +	WARN_ON(pgdir != init_mm.pgd);
>>> +	remove_pagetable(start, start + size, false);
>>> +}
>>> +
>>>  int arch_add_memory(int nid, u64 start, u64 size,
>>>  			struct mhp_restrictions *restrictions)
>>>  {
>>> -	int flags = 0;
>>> +	int ret, flags = 0;
>>>  
>>>  	if (rodata_full || debug_pagealloc_enabled())
>>>  		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>> @@ -1081,7 +1271,25 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>>>  			     size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
>>>  
>>> -	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>>> +	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>>>  			   restrictions);
>>> +	if (ret)
>>> +		__remove_pgd_mapping(swapper_pg_dir,
>>> +					__phys_to_virt(start), size);
>>
>> Nit: Indentation of the parameters looks really weird.
>>
>>> +	return ret;
>>> +}
>>> +
>>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>>> +void arch_remove_memory(int nid, u64 start, u64 size,
>>> +				struct vmem_altmap *altmap)
>>> +{
>>> +	unsigned long start_pfn = start >> PAGE_SHIFT;
>>> +	unsigned long nr_pages = size >> PAGE_SHIFT;
>>> +	struct zone *zone = page_zone(pfn_to_page(start_pfn));
>>> +
>>> +	__remove_pages(zone, start_pfn, nr_pages, altmap);
>>> +	__remove_pgd_mapping(swapper_pg_dir,
>>> +					__phys_to_virt(start), size);
>>
>> Dito, indentation of the parameters.
>>
>> For these two changes (arch_*_memory)
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
> 
> Thanks David. The following change on this patch should fix the indentation
> problem. If there are no other comments I will incorporate this and re-spin
> the series once more.
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 0cf0d41..a87ba18 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1273,9 +1273,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
>  
>  	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>  			   restrictions);
> -	if (ret)
> -		__remove_pgd_mapping(swapper_pg_dir,
> -					__phys_to_virt(start), size);
> +	if (!ret)
> +		return ret;
> +
> +	__remove_pgd_mapping(swapper_pg_dir, __phys_to_virt(start), size);


you can leave the old code, just make sure the start of all parameters
on new lines are aligned, that seemed to be not the case

if (ret)
	__remove_pgd_mapping(swapper_pg_dir,
			     __phys_to_virt(start), size);
............................^

>  	return ret;
>  }
>  
> @@ -1288,8 +1289,7 @@ void arch_remove_memory(int nid, u64 start, u64 size,
>  	struct zone *zone = page_zone(pfn_to_page(start_pfn));
>  
>  	__remove_pages(zone, start_pfn, nr_pages, altmap);
> -	__remove_pgd_mapping(swapper_pg_dir,
> -					__phys_to_virt(start), size);
> +	__remove_pgd_mapping(swapper_pg_dir, __phys_to_virt(start), size);
>  }
>  #endif
>  #endif
> 


-- 

Thanks,

David / dhildenb