Memory hotplug support for arm64 - complete patchset v2

Hi all,

this is a second round of patches to introduce memory hotplug and hotremove support for arm64. It builds on the work previously published at
[1] and it implements the feedback received in the first round of reviews.

The patchset applies and has been tested on commit bebc6082da0a ("Linux 4.14").

Due to a small regression introduced with commit 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports thp migration"), you will need to appy patch [2] first, until the fix is not upstreamed.

Comments and feedback are gold.

[1] https://lkml.org/lkml/2017/4/11/536
[2] https://lkml.org/lkml/2017/11/20/902

Changes v1->v2:
- swapper pgtable updated in place on hot add, avoiding unnecessary copy
- stop_machine used to updated swapper on hot add, avoiding races
- introduced check on offlining state before hot remove
- new memblock flag used to mark partially unused vmemmap pages, avoiding the nasty 0xFD hack used in the prev rev (and in x86 hot remove code)
- proper cleaning sequence for p[um]ds,ptes and related TLB management
- Removed macros that changed hot remove behavior based on number of pgtable levels. Now this is hidden in the pgtable traversal macros.
- Check on the corner case where P[UM]Ds would have to be split during hot remove: now this is forbidden.
- Minor fixes and refactoring.

Andrea Reale (4):
mm: memory_hotplug: Remove assumption on memory state before hotremove
mm: memory_hotplug: memblock to track partially removed vmemmap mem
mm: memory_hotplug: Add memory hotremove probe device
mm: memory-hotplug: Add memory hot remove support for arm64

Maciej Bielski (1):
mm: memory_hotplug: Memory hotplug (add) support for arm64

 arch/arm64/Kconfig             |  15 +
 arch/arm64/configs/defconfig   |   2 +
 arch/arm64/include/asm/mmu.h   |   7 +
 arch/arm64/mm/init.c           | 116 ++++++++
 arch/arm64/mm/mmu.c            | 609 ++++++++++++++++++++++++++++++++++++++++-
 drivers/acpi/acpi_memhotplug.c |   2 +-
 drivers/base/memory.c          |  34 ++-
 include/linux/memblock.h       |  12 +
 include/linux/memory_hotplug.h |   9 +-
 mm/memblock.c                  |  32 +++
 mm/memory_hotplug.c            |  13 +-
 11 files changed, 835 insertions(+), 16 deletions(-)

Signature · 1 line

2.7.4

I will try to have a look but I do not expect to understand any of arm64 specific changes so I will focus on the generic code but it would help a _lot_ if the cover letter provided some overview of what has been done from a higher level POV. What are the arch pieces and what is the generic code missing. A quick glance over patches suggests that changelogs for specific patches are modest as well. Could you give us more information please? Reviewing hundreds lines of code without context is a pain.

Quoted 38 lines

Changes v1->v2:
- swapper pgtable updated in place on hot add, avoiding unnecessary copy
- stop_machine used to updated swapper on hot add, avoiding races
- introduced check on offlining state before hot remove
- new memblock flag used to mark partially unused vmemmap pages, avoiding the nasty 0xFD hack used in the prev rev (and in x86 hot remove code)
- proper cleaning sequence for p[um]ds,ptes and related TLB management
- Removed macros that changed hot remove behavior based on number of pgtable levels. Now this is hidden in the pgtable traversal macros.
- Check on the corner case where P[UM]Ds would have to be split during hot remove: now this is forbidden.
- Minor fixes and refactoring.

Andrea Reale (4):
mm: memory_hotplug: Remove assumption on memory state before hotremove
mm: memory_hotplug: memblock to track partially removed vmemmap mem
mm: memory_hotplug: Add memory hotremove probe device
mm: memory-hotplug: Add memory hot remove support for arm64

Maciej Bielski (1):
mm: memory_hotplug: Memory hotplug (add) support for arm64

 arch/arm64/Kconfig             |  15 +
 arch/arm64/configs/defconfig   |   2 +
 arch/arm64/include/asm/mmu.h   |   7 +
 arch/arm64/mm/init.c           | 116 ++++++++
 arch/arm64/mm/mmu.c            | 609 ++++++++++++++++++++++++++++++++++++++++-
 drivers/acpi/acpi_memhotplug.c |   2 +-
 drivers/base/memory.c          |  34 ++-
 include/linux/memblock.h       |  12 +
 include/linux/memory_hotplug.h |   9 +-
 mm/memblock.c                  |  32 +++
 mm/memory_hotplug.c            |  13 +-
 11 files changed, 835 insertions(+), 16 deletions(-)

Signature · 1 line

2.7.4

Signature · 2 lines

Michal Hocko
SUSE Labs

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
  drivers/acpi/acpi_memhotplug.c |  2 +-
  include/linux/memory_hotplug.h |  9 ++++++---
  mm/memory_hotplug.c            | 13 +++++++++----
  3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +1 -1

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
  			nid = memory_add_physaddr_to_nid(info->start_addr);
 
  		acpi_unbind_memory_blocks(info);
-		remove_memory(nid, info->start_addr, info->length);
+		BUG_ON(remove_memory(nid, info->start_addr, info->length));
  		list_del(&info->list);
  		kfree(info);
  	}

include/linux/memory_hotplug.h +6 -3

index 58e110a..1a9c7b2 100644
@@ -295,7 +295,7 @@ static inline bool movable_node_is_enabled(void)
  extern bool is_mem_section_removable(unsigned long pfn, unsigned long nr_pages);
  extern void try_offline_node(int nid);
  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
-extern void remove_memory(int nid, u64 start, u64 size);
+extern int remove_memory(int nid, u64 start, u64 size);
 
  #else
  static inline bool is_mem_section_removable(unsigned long pfn,

@@ -311,7 +311,10 @@ static inline int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
  	return -EINVAL;
  }
 
-static inline void remove_memory(int nid, u64 start, u64 size) {}
+static inline int remove_memory(int nid, u64 start, u64 size)
+{
+	return -EINVAL;
+}
  #endif /* CONFIG_MEMORY_HOTREMOVE */
 
  extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,

@@ -323,7 +326,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
  		unsigned long nr_pages);
  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
  extern bool is_memblock_offlined(struct memory_block *mem);
-extern void remove_memory(int nid, u64 start, u64 size);
+extern int remove_memory(int nid, u64 start, u64 size);
  extern int sparse_add_one_section(struct pglist_data *pgdat, unsigned long start_pfn);
  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
  		unsigned long map_offset);

mm/memory_hotplug.c +9 -4

index d4b5f29..d5f15af 100644
@@ -1892,7 +1892,7 @@ EXPORT_SYMBOL(try_offline_node);
   * and online/offline operations before this call, as required by
   * try_offline_node().
   */
-void __ref remove_memory(int nid, u64 start, u64 size)
+int __ref remove_memory(int nid, u64 start, u64 size)
  {
  	int ret;
 

@@ -1908,18 +1908,23 @@ void __ref remove_memory(int nid, u64 start, u64 size)
  	ret = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
  				check_memblock_offlined_cb);
  	if (ret)
-		BUG();
+		goto end_remove;
+
+	ret = arch_remove_memory(start, size);
+
+	if (ret)
+		goto end_remove;
 
  	/* remove memmap entry */
  	firmware_map_remove(start, start + size, "System RAM");
  	memblock_free(start, size);
  	memblock_remove(start, size);
 
-	arch_remove_memory(start, size);
-
  	try_offline_node(nid);
 
+end_remove:
  	mem_hotplug_done();
+	return ret;
  }
  EXPORT_SYMBOL_GPL(remove_memory);
  #endif /* CONFIG_MEMORY_HOTREMOVE */

Introduces memory hotplug functionality (hot-add) for arm64.

Changes v1->v2:
- swapper pgtable updated in place on hot add, avoiding unnecessary copy:
all changes are additive and non destructive.

- stop_machine used to updated swapper on hot add, avoiding races

- checking if pagealloc is under debug to stay coherent with mem_map

Signed-off-by Maciej Bielski <[email protected]>

Signed-off-by Andrea Reale <[email protected]>

---
 arch/arm64/Kconfig           | 12 ++++++
 arch/arm64/configs/defconfig |  1 +
 arch/arm64/include/asm/mmu.h |  3 ++
 arch/arm64/mm/init.c         | 87 ++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c          | 39 ++++++++++++++++++++
 5 files changed, 142 insertions(+)

arch/arm64/Kconfig +12 -0

index 0df64a6..c736bba 100644
@@ -641,6 +641,14 @@ config HOTPLUG_CPU
          Say Y here to experiment with turning CPUs off and on.  CPUs
          can be controlled through /sys/devices/system/cpu.
 
+config ARCH_HAS_ADD_PAGES
+       def_bool y
+       depends on ARCH_ENABLE_MEMORY_HOTPLUG
+
+config ARCH_ENABLE_MEMORY_HOTPLUG
+       def_bool y
+    depends on !NUMA
+
 # Common NUMA Features
 config NUMA
        bool "Numa Memory Allocation and Scheduler Support"

@@ -715,6 +723,10 @@ config ARCH_HAS_CACHE_LINE_SIZE
 
 source "mm/Kconfig"
 
+config ARCH_MEMORY_PROBE
+       def_bool y
+       depends on MEMORY_HOTPLUG
+
 config SECCOMP
        bool "Enable seccomp to safely compute untrusted bytecode"
        ---help---

arch/arm64/configs/defconfig +1 -0

index 34480e9..5fc5656 100644
@@ -80,6 +80,7 @@ CONFIG_ARM64_VA_BITS_48=y
 CONFIG_SCHED_MC=y
 CONFIG_NUMA=y
 CONFIG_PREEMPT=y
+CONFIG_MEMORY_HOTPLUG=y
 CONFIG_KSM=y
 CONFIG_TRANSPARENT_HUGEPAGE=y
 CONFIG_CMA=y

arch/arm64/include/asm/mmu.h +3 -0

index 0d34bf0..2b3fa4d 100644
@@ -40,5 +40,8 @@ extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
                               pgprot_t prot, bool page_mappings_only);
 extern void *fixmap_remap_fdt(phys_addr_t dt_phys);
 extern void mark_linear_text_alias_ro(void);
+#ifdef CONFIG_MEMORY_HOTPLUG
+extern void hotplug_paging(phys_addr_t start, phys_addr_t size);
+#endif
 
 #endif

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

index 5960bef..e96e7d3 100644
@@ -722,3 +722,90 @@ static int __init register_mem_limit_dumper(void)
        return 0;
 }
 __initcall(register_mem_limit_dumper);
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+int add_pages(int nid, unsigned long start_pfn,
+               unsigned long nr_pages, bool want_memblock)
+{
+       int ret;
+       u64 start_addr = start_pfn << PAGE_SHIFT;
+       /*
+        * Mark the first page in the range as unusable. This is needed
+        * because __add_section (within __add_pages) wants pfn_valid
+        * of it to be false, and in arm64 pfn falid is implemented by
+        * just checking at the nomap flag for existing blocks.
+        *
+        * A small trick here is that __add_section() requires only
+        * phys_start_pfn (that is the first pfn of a section) to be
+        * invalid. Regardless of whether it was assumed (by the function
+        * author) that all pfns within a section are either all valid
+        * or all invalid, it allows to avoid looping twice (once here,
+        * second when memblock_clear_nomap() is called) through all
+        * pfns of the section and modify only one pfn. Thanks to that,
+        * further, in __add_zone() only this very first pfn is skipped
+        * and corresponding page is not flagged reserved. Therefore it
+        * is enough to correct this setup only for it.
+        *
+        * When arch_add_memory() returns the walk_memory_range() function
+        * is called and passed with online_memory_block() callback,
+        * which execution finally reaches the memory_block_action()
+        * function, where also only the first pfn of a memory block is
+        * checked to be reserved. Above, it was first pfn of a section,
+        * here it is a block but
+        * (drivers/base/memory.c):
+        *     sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
+        * (include/linux/memory.h):
+        *     #define MIN_MEMORY_BLOCK_SIZE     (1UL << SECTION_SIZE_BITS)
+        * so we can consider block and section equivalently
+        */
+       memblock_mark_nomap(start_addr, 1<<PAGE_SHIFT);
+       ret = __add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+       /*
+        * Make the pages usable after they have been added.
+        * This will make pfn_valid return true
+        */
+       memblock_clear_nomap(start_addr, 1<<PAGE_SHIFT);
+
+       /*
+        * This is a hack to avoid having to mix arch specific code
+        * into arch independent code. SetPageReserved is supposed
+        * to be called by __add_zone (within __add_section, within
+        * __add_pages). However, when it is called there, it assumes that
+        * pfn_valid returns true.  For the way pfn_valid is implemented
+        * in arm64 (a check on the nomap flag), the only way to make
+        * this evaluate true inside __add_zone is to clear the nomap
+        * flags of blocks in architecture independent code.
+        *
+        * To avoid this, we set the Reserved flag here after we cleared
+        * the nomap flag in the line above.
+        */
+       SetPageReserved(pfn_to_page(start_pfn));
+
+       return ret;
+}
+
+int arch_add_memory(int nid, u64 start, u64 size, bool want_memblock)
+{
+       int ret;
+       unsigned long start_pfn = start >> PAGE_SHIFT;
+       unsigned long nr_pages = size >> PAGE_SHIFT;
+       unsigned long end_pfn = start_pfn + nr_pages;
+       unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
+
+       if (end_pfn > max_sparsemem_pfn) {
+               pr_err("end_pfn too big");
+               return -1;
+       }
+       hotplug_paging(start, size);
+
+       ret = add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+       if (ret)
+               pr_warn("%s: Problem encountered in __add_pages() ret=%d\n",
+                       __func__, ret);
+
+       return ret;
+}
+
+#endif /* CONFIG_MEMORY_HOTPLUG */

arch/arm64/mm/mmu.c +14 -0

index f1eb15e..d93043d 100644
@@ -28,6 +28,7 @@
 #include <linux/mman.h>
 #include <linux/nodemask.h>
 #include <linux/memblock.h>
+#include <linux/stop_machine.h>
 #include <linux/fs.h>
 #include <linux/io.h>
 #include <linux/mm.h>

@@ -615,6 +616,44 @@ void __init paging_init(void)
                      SWAPPER_DIR_SIZE - PAGE_SIZE);
 }
 
+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables
+ * for hot added memory.
+ */
+
+struct mem_range {
+       phys_addr_t base;
+       phys_addr_t size;
+};
+
+static int __hotplug_paging(void *data)

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

+
+       return 0;
+}
+
+inline void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+       struct mem_range section = {
+               .base = start,
+               .size = size,
+       };
+
+       stop_machine(__hotplug_paging, &section, NULL);
+}
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
 /*
  * Check whether a kernel address is valid (derived from arch/x86/).
  */
--

2.7.4

Quoted 19 lines · Maciej Bielski

[ ...]
Introduces memory hotplug functionality (hot-add) for arm64.

arch/arm64/mm/mmu.c +13 -0

@@ -615,6 +616,44 @@ void __init paging_init(void)
                      SWAPPER_DIR_SIZE - PAGE_SIZE);
 }
 
+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables
+ * for hot added memory.
+ */
+
+struct mem_range {
+       phys_addr_t base;
+       phys_addr_t size;
+};
+
+static int __hotplug_paging(void *data)

arch/arm64/mm/mmu.c void __init paging_init(void) · line 641

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+{
+       int flags = 0;
+       struct mem_range *section = data;
+
+       if (debug_pagealloc_enabled())
+               flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
+       __create_pgd_mapping(swapper_pg_dir, section->base,
+                       __phys_to_virt(section->base), section->size,
+                       PAGE_KERNEL, pgd_pgtable_alloc, flags);

Hello Andrea,

__hotplug_paging runs on stop_machine context.
cpu stop callbacks must not sleep.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/stop_machine.c?h=v4.14#n479

__create_pgd_mapping uses pgd_pgtable_alloc. which does __get_free_page(PGALLOC_GFP)
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/mm/mmu.c?h=v4.14#n342

PGALLOC_GFP has GFP_KERNEL which inturn has __GFP_RECLAIM

#define PGALLOC_GFP (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO)
#define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS)

Now, prepare_alloc_pages() called by __alloc_pages_nodemask checks for

might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/page_alloc.c?h=v4.14#n4150

and then BUG()

I was testing on 4.4 kernel, but cross checked with 4.14 as well.

Regards,
Arun

Quoted 21 lines

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

+
+       return 0;
+}
+
+inline void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+       struct mem_range section = {
+               .base = start,
+               .size = size,
+       };
+
+       stop_machine(__hotplug_paging, &section, NULL);
+}
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
 /*
  * Check whether a kernel address is valid (derived from arch/x86/).
  */
--

2.7.4

Adding a "remove" sysfs handle that can be used to trigger memory hotremove manually, exactly simmetrically with what happens with the "probe" device for hot-add.

This is usueful for architecture that do not rely on ACPI for memory hot-remove.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/base/memory.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

drivers/base/memory.c +33 -1

index 1d60b58..8ccb67c 100644
@@ -530,7 +530,36 @@ memory_probe_store(struct device *dev, struct device_attribute *attr,
 }
 
 static DEVICE_ATTR(probe, S_IWUSR, NULL, memory_probe_store);
-#endif
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+static ssize_t
+memory_remove_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	u64 phys_addr;
+	int nid, ret;
+	unsigned long pages_per_block = PAGES_PER_SECTION * sections_per_block;
+
+	ret = kstrtoull(buf, 0, &phys_addr);
+	if (ret)
+		return ret;
+
+	if (phys_addr & ((pages_per_block << PAGE_SHIFT) - 1))
+		return -EINVAL;
+
+	nid = memory_add_physaddr_to_nid(phys_addr);
+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	remove_memory(nid, phys_addr,
+			 MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+	unlock_device_hotplug();
+	return count;
+}
+static DEVICE_ATTR(remove, S_IWUSR, NULL, memory_remove_store);
+#endif /* CONFIG_MEMORY_HOTREMOVE */
+#endif /* CONFIG_ARCH_MEMORY_PROBE */
 
 #ifdef CONFIG_MEMORY_FAILURE
 /*

@@ -790,6 +819,9 @@ bool is_memblock_offlined(struct memory_block *mem)
 static struct attribute *memory_root_attrs[] = {
 #ifdef CONFIG_ARCH_MEMORY_PROBE
 	&dev_attr_probe.attr,
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	&dev_attr_remove.attr,
+#endif
 #endif
 
 #ifdef CONFIG_MEMORY_FAILURE

HI, Andrea

I don't see "memory_add_physaddr_to_nid" in arch/arm64. Am I miss something?

Thnaks
zhongjiang

Quoted 65 lines · Andrea Reale

Adding a "remove" sysfs handle that can be used to trigger memory hotremove manually, exactly simmetrically with what happens with the "probe" device for hot-add.

This is usueful for architecture that do not rely on ACPI for memory hot-remove.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/base/memory.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

drivers/base/memory.c +33 -1

index 1d60b58..8ccb67c 100644
@@ -530,7 +530,36 @@ memory_probe_store(struct device *dev, struct device_attribute *attr,
 }
 
 static DEVICE_ATTR(probe, S_IWUSR, NULL, memory_probe_store);
-#endif
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+static ssize_t
+memory_remove_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	u64 phys_addr;
+	int nid, ret;
+	unsigned long pages_per_block = PAGES_PER_SECTION * sections_per_block;
+
+	ret = kstrtoull(buf, 0, &phys_addr);
+	if (ret)
+		return ret;
+
+	if (phys_addr & ((pages_per_block << PAGE_SHIFT) - 1))
+		return -EINVAL;
+
+	nid = memory_add_physaddr_to_nid(phys_addr);
+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	remove_memory(nid, phys_addr,
+			 MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+	unlock_device_hotplug();
+	return count;
+}
+static DEVICE_ATTR(remove, S_IWUSR, NULL, memory_remove_store);
+#endif /* CONFIG_MEMORY_HOTREMOVE */
+#endif /* CONFIG_ARCH_MEMORY_PROBE */
 
 #ifdef CONFIG_MEMORY_FAILURE
 /*

@@ -790,6 +819,9 @@ bool is_memblock_offlined(struct memory_block *mem)
 static struct attribute *memory_root_attrs[] = {
 #ifdef CONFIG_ARCH_MEMORY_PROBE
 	&dev_attr_probe.attr,
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	&dev_attr_remove.attr,
+#endif
 #endif
 
 #ifdef CONFIG_MEMORY_FAILURE

Hi Arun,

Quoted 47 lines · Arun KS

Quoted 19 lines · Maciej Bielski

[ ...]
Introduces memory hotplug functionality (hot-add) for arm64.

arch/arm64/mm/mmu.c +13 -0

@@ -615,6 +616,44 @@ void __init paging_init(void)
                      SWAPPER_DIR_SIZE - PAGE_SIZE);
 }
 
+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables
+ * for hot added memory.
+ */
+
+struct mem_range {
+       phys_addr_t base;
+       phys_addr_t size;
+};
+
+static int __hotplug_paging(void *data)

arch/arm64/mm/mmu.c void __init paging_init(void) · line 641

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+{
+       int flags = 0;
+       struct mem_range *section = data;
+
+       if (debug_pagealloc_enabled())
+               flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
+       __create_pgd_mapping(swapper_pg_dir, section->base,
+                       __phys_to_virt(section->base), section->size,
+                       PAGE_KERNEL, pgd_pgtable_alloc, flags);

Hello Andrea,

__hotplug_paging runs on stop_machine context.
cpu stop callbacks must not sleep.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/stop_machine.c?h=v4.14#n479

__create_pgd_mapping uses pgd_pgtable_alloc. which does __get_free_page(PGALLOC_GFP)
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/mm/mmu.c?h=v4.14#n342

PGALLOC_GFP has GFP_KERNEL which inturn has __GFP_RECLAIM

#define PGALLOC_GFP (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO)
#define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS)

Quoted 7 lines

Now, prepare_alloc_pages() called by __alloc_pages_nodemask checks for

might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/page_alloc.c?h=v4.14#n4150

and then BUG()

Well spotted, thanks for reporting the problem. One possible solution would be to revert back to building the updated page tables on a copy pgdir (as it was done in v1 of this patchset) and then replacing swapper atomically with stop_machine.

Actually, I am not sure if stop_machine is strictly needed, if we modify the swapper pgdir live: for example, in x86_64 kernel_physical_mapping_init, atomicity is ensured by spin-locking on init_mm.page_table_lock.
https://elixir.free-electrons.com/linux/v4.14/source/arch/x86/mm/init_64.c#L684
I'll spend some time investigating whoever else could be working concurrently on the swapper pgdir.

Any suggestion or pointer is very welcome.

Thanks,
Andrea

Quoted 28 lines

I was testing on 4.4 kernel, but cross checked with 4.14 as well.

Regards,
Arun

Quoted 21 lines

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

+
+       return 0;
+}
+
+inline void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+       struct mem_range section = {
+               .base = start,
+               .size = size,
+       };
+
+       stop_machine(__hotplug_paging, &section, NULL);
+}
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
 /*
  * Check whether a kernel address is valid (derived from arch/x86/).
  */
--

2.7.4

Hi zhongjiang,

Quoted 4 lines · zhong jiang

HI, Andrea

I don't see "memory_add_physaddr_to_nid" in arch/arm64. Am I miss something?

When !CONFIG_NUMA it is defined in include/linux/memory_hotplug.h as 0. In patch 1/5 of this series we require !NUMA to enable ARCH_ENABLE_MEMORY_HOTPLUG.

The reason for this simplification is simply that we would not know how to decide the correct node to which to add memory when NUMA is on. Any suggestion on that matter is welcome.

Thanks,
Andrea

Quoted 68 lines

Thnaks
zhongjiang

Quoted 64 lines · Andrea Reale

Adding a "remove" sysfs handle that can be used to trigger memory hotremove manually, exactly simmetrically with what happens with the "probe" device for hot-add.

This is usueful for architecture that do not rely on ACPI for memory hot-remove.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/base/memory.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

drivers/base/memory.c +33 -1

index 1d60b58..8ccb67c 100644
@@ -530,7 +530,36 @@ memory_probe_store(struct device *dev, struct device_attribute *attr,
 }
 
 static DEVICE_ATTR(probe, S_IWUSR, NULL, memory_probe_store);
-#endif
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+static ssize_t
+memory_remove_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	u64 phys_addr;
+	int nid, ret;
+	unsigned long pages_per_block = PAGES_PER_SECTION * sections_per_block;
+
+	ret = kstrtoull(buf, 0, &phys_addr);
+	if (ret)
+		return ret;
+
+	if (phys_addr & ((pages_per_block << PAGE_SHIFT) - 1))
+		return -EINVAL;
+
+	nid = memory_add_physaddr_to_nid(phys_addr);
+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	remove_memory(nid, phys_addr,
+			 MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+	unlock_device_hotplug();
+	return count;
+}
+static DEVICE_ATTR(remove, S_IWUSR, NULL, memory_remove_store);
+#endif /* CONFIG_MEMORY_HOTREMOVE */
+#endif /* CONFIG_ARCH_MEMORY_PROBE */
 
 #ifdef CONFIG_MEMORY_FAILURE
 /*

@@ -790,6 +819,9 @@ bool is_memblock_offlined(struct memory_block *mem)
 static struct attribute *memory_root_attrs[] = {
 #ifdef CONFIG_ARCH_MEMORY_PROBE
 	&dev_attr_probe.attr,
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	&dev_attr_remove.attr,
+#endif
 #endif
 

Quoted 2 lines

drivers/base/memory.c bool is_memblock_offlined(struct memory_block *mem) · line 827

Quoted 1 line

drivers/base/memory.c +0 -0

 #ifdef CONFIG_MEMORY_FAILURE

.

Hi, Andrea

most of server will benefit from NUMA ,it is best to sovle the issue without spcial restrictions.

At least we can obtain the numa information from dtb. therefore, The memory can online correctly.

Thanks
zhongjiang

Quoted 91 lines · Andrea Reale

Hi zhongjiang,

Quoted 4 lines · zhong jiang

HI, Andrea

I don't see "memory_add_physaddr_to_nid" in arch/arm64. Am I miss something?

When !CONFIG_NUMA it is defined in include/linux/memory_hotplug.h as 0. In patch 1/5 of this series we require !NUMA to enable ARCH_ENABLE_MEMORY_HOTPLUG.

The reason for this simplification is simply that we would not know how to decide the correct node to which to add memory when NUMA is on. Any suggestion on that matter is welcome.

Thanks,
Andrea

Quoted 68 lines

Thnaks
zhongjiang

Quoted 64 lines · Andrea Reale

Adding a "remove" sysfs handle that can be used to trigger memory hotremove manually, exactly simmetrically with what happens with the "probe" device for hot-add.

This is usueful for architecture that do not rely on ACPI for memory hot-remove.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/base/memory.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

drivers/base/memory.c +33 -1

index 1d60b58..8ccb67c 100644
@@ -530,7 +530,36 @@ memory_probe_store(struct device *dev, struct device_attribute *attr,
 }
 
 static DEVICE_ATTR(probe, S_IWUSR, NULL, memory_probe_store);
-#endif
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+static ssize_t
+memory_remove_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	u64 phys_addr;
+	int nid, ret;
+	unsigned long pages_per_block = PAGES_PER_SECTION * sections_per_block;
+
+	ret = kstrtoull(buf, 0, &phys_addr);
+	if (ret)
+		return ret;
+
+	if (phys_addr & ((pages_per_block << PAGE_SHIFT) - 1))
+		return -EINVAL;
+
+	nid = memory_add_physaddr_to_nid(phys_addr);
+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	remove_memory(nid, phys_addr,
+			 MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+	unlock_device_hotplug();
+	return count;
+}
+static DEVICE_ATTR(remove, S_IWUSR, NULL, memory_remove_store);
+#endif /* CONFIG_MEMORY_HOTREMOVE */
+#endif /* CONFIG_ARCH_MEMORY_PROBE */
 
 #ifdef CONFIG_MEMORY_FAILURE
 /*

@@ -790,6 +819,9 @@ bool is_memblock_offlined(struct memory_block *mem)
 static struct attribute *memory_root_attrs[] = {
 #ifdef CONFIG_ARCH_MEMORY_PROBE
 	&dev_attr_probe.attr,
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	&dev_attr_remove.attr,
+#endif
 #endif
 

Quoted 2 lines

drivers/base/memory.c bool is_memblock_offlined(struct memory_block *mem) · line 827

Quoted 1 line

drivers/base/memory.c +0 -0

 #ifdef CONFIG_MEMORY_FAILURE

.

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644

Quoted 32 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
                        nid = memory_add_physaddr_to_nid(info->start_addr);
 

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 285

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +1 -1

                acpi_unbind_memory_blocks(info);
-               remove_memory(nid, info->start_addr, info->length);
+               BUG_ON(remove_memory(nid, info->start_addr, info->length));

Why does this have to be BUG_ON()? Is it really necessary to kill the system here?

If it is, please add a comment describing why continuing is not an option here.

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 288

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +0 -0

                list_del(&info->list);
                kfree(info);
        }

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Rafael,

Quoted 33 lines · Rafael J. Wysocki

Quoted 32 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
                        nid = memory_add_physaddr_to_nid(info->start_addr);
 

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 285

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +1 -1

                acpi_unbind_memory_blocks(info);
-               remove_memory(nid, info->start_addr, info->length);
+               BUG_ON(remove_memory(nid, info->start_addr, info->length));

Why does this have to be BUG_ON()? Is it really necessary to kill the system here?

Actually, I hoped you would help me understand that: that BUG() call was introduced by yourself in Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") in memory_hoptlug.c:remove_memory()).

Just reading at that commit my understanding was that you were assuming that acpi_memory_remove_memory() have already done the job of offlining the target memory, so there would be a bug if that wasn't the case.

In my case, that assumption did not hold and I found that it might not hold for other platforms that do not use ACPI. In fact, the purpose of this patch is to move this assumption out of the generic hotplug code and move it to ACPI code where it originated.

Quoted 47 lines · Andrea Reale

Hi Rafael,

Quoted 33 lines · Rafael J. Wysocki

Quoted 32 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
                        nid = memory_add_physaddr_to_nid(info->start_addr);
 

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 285

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +1 -1

                acpi_unbind_memory_blocks(info);
-               remove_memory(nid, info->start_addr, info->length);
+               BUG_ON(remove_memory(nid, info->start_addr, info->length));

Why does this have to be BUG_ON()? Is it really necessary to kill the system here?

Actually, I hoped you would help me understand that: that BUG() call was introduced by yourself in Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") in memory_hoptlug.c:remove_memory()).

Quoted 8 lines

Just reading at that commit my understanding was that you were assuming that acpi_memory_remove_memory() have already done the job of offlining the target memory, so there would be a bug if that wasn't the case.

In my case, that assumption did not hold and I found that it might not hold for other platforms that do not use ACPI. In fact, the purpose of this patch is to move this assumption out of the generic hotplug code and move it to ACPI code where it originated.

remove_memory failure is basically impossible to handle AFAIR. The original code to BUG in remove_memory is ugly as hell and we do not want to spread that out of that function. Instead we really want to get rid of it.

Signature · 6 lines

Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 57 lines · Michal Hocko

Quoted 55 lines · Andrea Reale

Hi Rafael,

Quoted 33 lines · Rafael J. Wysocki

Quoted 32 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
                        nid = memory_add_physaddr_to_nid(info->start_addr);
 

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 285

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +1 -1

                acpi_unbind_memory_blocks(info);
-               remove_memory(nid, info->start_addr, info->length);
+               BUG_ON(remove_memory(nid, info->start_addr, info->length));

Why does this have to be BUG_ON()? Is it really necessary to kill the system here?

Actually, I hoped you would help me understand that: that BUG() call was introduced by yourself in Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") in memory_hoptlug.c:remove_memory()).

Just reading at that commit my understanding was that you were assuming that acpi_memory_remove_memory() have already done the job of offlining the target memory, so there would be a bug if that wasn't the case.

In my case, that assumption did not hold and I found that it might not hold for other platforms that do not use ACPI. In fact, the purpose of this patch is to move this assumption out of the generic hotplug code and move it to ACPI code where it originated.

Quoted 4 lines

remove_memory failure is basically impossible to handle AFAIR. The original code to BUG in remove_memory is ugly as hell and we do not want to spread that out of that function. Instead we really want to get rid of it.

Today, BUG() is called even in the simple case where remove fails because the section we are removing is not offline.

I cannot see any need to
BUG() in such a case: an error code seems more than sufficient to me.

This is why this patch removes the BUG() call when the "offline" check fails from the generic code.

It moves it back to the ACPI call, where the assumption originated. Honestlly, I cannot tell if it makes sense to BUG() there:
I have nothing against removing it from ACPI hotplug too, but I don't know enough to feel free to change the acpi semantics myself, so I moved it there to keep the original behavior unchanged for x86 code.

In this arm64 hot-remove port, offline and remove are done in two separate steps, and is conceivable that an user tries erroneusly to remove some section that he forgot to offline first: in that case, with the patch, remove will just report an erro without BUGing.

Is my reasoning flawed?

Quoted 72 lines · Andrea Reale

Hi Arun,

Quoted 47 lines · Arun KS

Quoted 19 lines · Maciej Bielski

[ ...]
Introduces memory hotplug functionality (hot-add) for arm64.

arch/arm64/mm/mmu.c +13 -0

@@ -615,6 +616,44 @@ void __init paging_init(void)
                      SWAPPER_DIR_SIZE - PAGE_SIZE);
 }
 
+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables
+ * for hot added memory.
+ */
+
+struct mem_range {
+       phys_addr_t base;
+       phys_addr_t size;
+};
+
+static int __hotplug_paging(void *data)

arch/arm64/mm/mmu.c void __init paging_init(void) · line 641

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+{
+       int flags = 0;
+       struct mem_range *section = data;
+
+       if (debug_pagealloc_enabled())
+               flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
+       __create_pgd_mapping(swapper_pg_dir, section->base,
+                       __phys_to_virt(section->base), section->size,
+                       PAGE_KERNEL, pgd_pgtable_alloc, flags);

Hello Andrea,

__hotplug_paging runs on stop_machine context.
cpu stop callbacks must not sleep.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/stop_machine.c?h=v4.14#n479

__create_pgd_mapping uses pgd_pgtable_alloc. which does __get_free_page(PGALLOC_GFP)
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/mm/mmu.c?h=v4.14#n342

PGALLOC_GFP has GFP_KERNEL which inturn has __GFP_RECLAIM

#define PGALLOC_GFP (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO)
#define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS)

Quoted 7 lines

Now, prepare_alloc_pages() called by __alloc_pages_nodemask checks for

might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/page_alloc.c?h=v4.14#n4150

and then BUG()

Well spotted, thanks for reporting the problem. One possible solution would be to revert back to building the updated page tables on a copy pgdir (as it was done in v1 of this patchset) and then replacing swapper atomically with stop_machine.

Actually, I am not sure if stop_machine is strictly needed, if we modify the swapper pgdir live: for example, in x86_64 kernel_physical_mapping_init, atomicity is ensured by spin-locking on init_mm.page_table_lock.
https://elixir.free-electrons.com/linux/v4.14/source/arch/x86/mm/init_64.c#L684
I'll spend some time investigating whoever else could be working concurrently on the swapper pgdir.

Quoted 1 line

Any suggestion or pointer is very welcome.

Hi Andrea, Arun,

Alternative approach could be implementing pgd_pgtable_alloc_nosleep() and pointing this to hotplug_paging(). Subsequently, it could use different flags, eg:

#define PGALLOC_GFP_NORECLAIM (__GFP_IO | __GFP_FS | __GFP_NOTRACK | __GFP_ZERO)

Is this unefficient approach in any way?
Do we like the fact that the memory-attaching thread can go to sleep?

BR,

Quoted 33 lines

Thanks,
Andrea

Quoted 28 lines

I was testing on 4.4 kernel, but cross checked with 4.14 as well.

Regards,
Arun

Quoted 21 lines

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

+
+       return 0;
+}
+
+inline void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+       struct mem_range section = {
+               .base = start,
+               .size = size,
+       };
+
+       stop_machine(__hotplug_paging, &section, NULL);
+}
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
 /*
  * Check whether a kernel address is valid (derived from arch/x86/).
  */
--

2.7.4

--
Maciej Bielski

Introduces memory hotplug functionality (hot-add) for arm64.

Changes v1->v2:
- swapper pgtable updated in place on hot add, avoiding unnecessary copy:
all changes are additive and non destructive.

- stop_machine used to updated swapper on hot add, avoiding races

- checking if pagealloc is under debug to stay coherent with mem_map

Signed-off-by Maciej Bielski <[email protected]>

Signed-off-by Andrea Reale <[email protected]>

---
  arch/arm64/Kconfig           | 12 ++++++
  arch/arm64/configs/defconfig |  1 +
  arch/arm64/include/asm/mmu.h |  3 ++
  arch/arm64/mm/init.c         | 87 ++++++++++++++++++++++++++++++++++++++++++++
  arch/arm64/mm/mmu.c          | 39 ++++++++++++++++++++
  5 files changed, 142 insertions(+)

arch/arm64/Kconfig +0 -0

index 0df64a6..c736bba 100644

arch/arm64/include/asm/mmu.h +1 -0

+#endif
 
  #endif

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

index 5960bef..e96e7d3 100644
@@ -722,3 +722,90 @@ static int __init register_mem_limit_dumper(void)
  	return 0;
  }
  __initcall(register_mem_limit_dumper);
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+int add_pages(int nid, unsigned long start_pfn,
+		unsigned long nr_pages, bool want_memblock)
+{
+	int ret;
+	u64 start_addr = start_pfn << PAGE_SHIFT;
+	/*
+	 * Mark the first page in the range as unusable. This is needed
+	 * because __add_section (within __add_pages) wants pfn_valid
+	 * of it to be false, and in arm64 pfn falid is implemented by
+	 * just checking at the nomap flag for existing blocks.
+	 *
+	 * A small trick here is that __add_section() requires only
+	 * phys_start_pfn (that is the first pfn of a section) to be
+	 * invalid. Regardless of whether it was assumed (by the function
+	 * author) that all pfns within a section are either all valid
+	 * or all invalid, it allows to avoid looping twice (once here,
+	 * second when memblock_clear_nomap() is called) through all
+	 * pfns of the section and modify only one pfn. Thanks to that,
+	 * further, in __add_zone() only this very first pfn is skipped
+	 * and corresponding page is not flagged reserved. Therefore it
+	 * is enough to correct this setup only for it.
+	 *
+	 * When arch_add_memory() returns the walk_memory_range() function
+	 * is called and passed with online_memory_block() callback,
+	 * which execution finally reaches the memory_block_action()
+	 * function, where also only the first pfn of a memory block is
+	 * checked to be reserved. Above, it was first pfn of a section,
+	 * here it is a block but
+	 * (drivers/base/memory.c):
+	 *     sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
+	 * (include/linux/memory.h):
+	 *     #define MIN_MEMORY_BLOCK_SIZE     (1UL << SECTION_SIZE_BITS)
+	 * so we can consider block and section equivalently
+	 */
+	memblock_mark_nomap(start_addr, 1<<PAGE_SHIFT);
+	ret = __add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+	/*
+	 * Make the pages usable after they have been added.
+	 * This will make pfn_valid return true
+	 */
+	memblock_clear_nomap(start_addr, 1<<PAGE_SHIFT);
+
+	/*
+	 * This is a hack to avoid having to mix arch specific code
+	 * into arch independent code. SetPageReserved is supposed
+	 * to be called by __add_zone (within __add_section, within

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

+
+	return ret;
+}
+
+int arch_add_memory(int nid, u64 start, u64 size, bool want_memblock)
+{
+	int ret;
+	unsigned long start_pfn = start >> PAGE_SHIFT;
+	unsigned long nr_pages = size >> PAGE_SHIFT;
+	unsigned long end_pfn = start_pfn + nr_pages;
+	unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
+
+	if (end_pfn > max_sparsemem_pfn) {
+		pr_err("end_pfn too big");
+		return -1;
+	}
+	hotplug_paging(start, size);
+
+	ret = add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+	if (ret)
+		pr_warn("%s: Problem encountered in __add_pages() ret=%d\n",
+			__func__, ret);
+
+	return ret;
+}
+
+#endif /* CONFIG_MEMORY_HOTPLUG */

arch/arm64/mm/mmu.c +26 -0

index f1eb15e..d93043d 100644
@@ -28,6 +28,7 @@
  #include <linux/mman.h>
  #include <linux/nodemask.h>
  #include <linux/memblock.h>
+#include <linux/stop_machine.h>
  #include <linux/fs.h>
  #include <linux/io.h>
  #include <linux/mm.h>

@@ -615,6 +616,44 @@ void __init paging_init(void)
  		      SWAPPER_DIR_SIZE - PAGE_SIZE);
  }
 
+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables
+ * for hot added memory.
+ */
+
+struct mem_range {
+	phys_addr_t base;
+	phys_addr_t size;
+};
+
+static int __hotplug_paging(void *data)
+{
+	int flags = 0;
+	struct mem_range *section = data;
+
+	if (debug_pagealloc_enabled())
+		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
+	__create_pgd_mapping(swapper_pg_dir, section->base,
+			__phys_to_virt(section->base), section->size,
+			PAGE_KERNEL, pgd_pgtable_alloc, flags);
+
+	return 0;

arch/arm64/mm/mmu.c +3 -0

+}
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
  /*
   * Check whether a kernel address is valid (derived from arch/x86/).
   */

Quoted 57 lines · Michal Hocko

Quoted 47 lines · Andrea Reale

Hi Rafael,

Quoted 33 lines · Rafael J. Wysocki

Quoted 32 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
  drivers/acpi/acpi_memhotplug.c |  2 +-
  include/linux/memory_hotplug.h |  9 ++++++---
  mm/memory_hotplug.c            | 13 +++++++++----
  3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
                         nid = memory_add_physaddr_to_nid(info->start_addr);
 

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 285

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +1 -1

                 acpi_unbind_memory_blocks(info);
-               remove_memory(nid, info->start_addr, info->length);
+               BUG_ON(remove_memory(nid, info->start_addr, info->length));

Why does this have to be BUG_ON()? Is it really necessary to kill the system here?

Actually, I hoped you would help me understand that: that BUG() call was introduced by yourself in Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") in memory_hoptlug.c:remove_memory()).

Quoted 8 lines

Just reading at that commit my understanding was that you were assuming that acpi_memory_remove_memory() have already done the job of offlining the target memory, so there would be a bug if that wasn't the case.

In my case, that assumption did not hold and I found that it might not hold for other platforms that do not use ACPI. In fact, the purpose of this patch is to move this assumption out of the generic hotplug code and move it to ACPI code where it originated.

Quoted 4 lines

remove_memory failure is basically impossible to handle AFAIR. The original code to BUG in remove_memory is ugly as hell and we do not want to spread that out of that function. Instead we really want to get rid of it.

Today, BUG() is called even in the simple case where remove fails because the section we are removing is not offline. I cannot see any need to BUG() in such a case: an error code seems more than sufficient to me. This is why this patch removes the BUG() call when the "offline" check fails from the generic code.
It moves it back to the ACPI call, where the assumption originated. Honestlly, I cannot tell if it makes sense to BUG() there:
I have nothing against removing it from ACPI hotplug too, but
I don't know enough to feel free to change the acpi semantics myself, so I moved it there to keep the original behavior unchanged for x86 code.

In this arm64 hot-remove port, offline and remove are done in two separate steps, and is conceivable that an user tries erroneusly to remove some section that he forgot to offline first: in that case, with the patch, remove will just report an erro without BUGing.

Is my reasoning flawed?

Cheers,
Andrea

Quoted 8 lines

Signature · 6 lines

Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

When hot-removing memory we need to free vmemmap memory.

However, depending on the memory is being removed, it might not be always possible to free a full vmemmap page / huge-page because part of it might still be used.

Commit ae9aae9eda2d ("memory-hotplug: common APIs to support page tables hot-remove") introduced a workaround for x86
hot-remove, by which partially unused areas are filled with the 0xFD constant. Full pages are only removed when fully filled by 0xFDs.

This commit introduces a MEMBLOCK_UNUSED_VMEMMAP memblock flag, with the goal of using it in place of 0xFDs. For now, this will be used for the arm64 port of memory hot remove, but the idea is to eventually use the same mechanism for x86 as well.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
  include/linux/memblock.h | 12 ++++++++++++
  mm/memblock.c            | 32 ++++++++++++++++++++++++++++++++
  2 files changed, 44 insertions(+)

include/linux/memblock.h +0 -0

index bae11c7..0daec05 100644

include/linux/memblock.h +10 -0

+#endif
  };
 
  struct memblock_region {

@@ -90,6 +93,10 @@ int memblock_mark_mirror(phys_addr_t base, phys_addr_t size);
  int memblock_mark_nomap(phys_addr_t base, phys_addr_t size);
  int memblock_clear_nomap(phys_addr_t base, phys_addr_t size);
  ulong choose_memblock_flags(void);
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int memblock_mark_unused_vmemmap(phys_addr_t base, phys_addr_t size);
+int memblock_clear_unused_vmemmap(phys_addr_t base, phys_addr_t size);
+#endif
 
  /* Low level functions */
  int memblock_add_range(struct memblock_type *type,

@@ -182,6 +189,11 @@ static inline bool memblock_is_nomap(struct memblock_region *m)
  	return m->flags & MEMBLOCK_NOMAP;
  }
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+bool memblock_is_vmemmap_unused_range(struct memblock_type *mt,
+		phys_addr_t start, phys_addr_t end);
+#endif
+
  #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
  int memblock_search_pfn_nid(unsigned long pfn, unsigned long *start_pfn,
  			    unsigned long  *end_pfn);

mm/memblock.c +32 -0

index 9120578..30d5aa4 100644
@@ -809,6 +809,18 @@ int __init_memblock memblock_clear_nomap(phys_addr_t base, phys_addr_t size)
  	return memblock_setclr_flag(base, size, 0, MEMBLOCK_NOMAP);
  }
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int __init_memblock memblock_mark_unused_vmemmap(phys_addr_t base,
+		phys_addr_t size)
+{
+	return memblock_setclr_flag(base, size, 1, MEMBLOCK_UNUSED_VMEMMAP);
+}
+int __init_memblock memblock_clear_unused_vmemmap(phys_addr_t base,
+		phys_addr_t size)
+{
+	return memblock_setclr_flag(base, size, 0, MEMBLOCK_UNUSED_VMEMMAP);
+}
+#endif
  /**
   * __next_reserved_mem_region - next function for for_each_reserved_region()
   * @idx: pointer to u64 loop variable

@@ -1696,6 +1708,26 @@ void __init_memblock memblock_trim_memory(phys_addr_t align)
  	}
  }
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+bool __init_memblock memblock_is_vmemmap_unused_range(struct memblock_type *mt,
+		phys_addr_t start, phys_addr_t end)
+{
+	u64 i;
+	struct memblock_region *r;
+
+	i = memblock_search(mt, start);
+	r = &(mt->regions[i]);
+	while (r->base < end) {
+		if (!(r->flags & MEMBLOCK_UNUSED_VMEMMAP))
+			return 0;
+
+		r = &(memblock.memory.regions[++i]);
+	}
+
+	return 1;
+}
+#endif
+
  void __init_memblock memblock_set_current_limit(phys_addr_t limit)
  {
  	memblock.current_limit = limit;

Adding a "remove" sysfs handle that can be used to trigger memory hotremove manually, exactly simmetrically with what happens with the "probe" device for hot-add.

This is usueful for architecture that do not rely on ACPI for memory hot-remove.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
  drivers/base/memory.c | 34 +++++++++++++++++++++++++++++++++-
  1 file changed, 33 insertions(+), 1 deletion(-)

drivers/base/memory.c +8 -1

index 1d60b58..8ccb67c 100644
@@ -530,7 +530,36 @@ memory_probe_store(struct device *dev, struct device_attribute *attr,
  }
 
  static DEVICE_ATTR(probe, S_IWUSR, NULL, memory_probe_store);
-#endif
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+static ssize_t
+memory_remove_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	u64 phys_addr;
+	int nid, ret;

drivers/base/memory.c +15 -0

+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	remove_memory(nid, phys_addr,
+			 MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+	unlock_device_hotplug();
+	return count;
+}
+static DEVICE_ATTR(remove, S_IWUSR, NULL, memory_remove_store);
+#endif /* CONFIG_MEMORY_HOTREMOVE */
+#endif /* CONFIG_ARCH_MEMORY_PROBE */
 
  #ifdef CONFIG_MEMORY_FAILURE
  /*

@@ -790,6 +819,9 @@ bool is_memblock_offlined(struct memory_block *mem)
  static struct attribute *memory_root_attrs[] = {
  #ifdef CONFIG_ARCH_MEMORY_PROBE
  	&dev_attr_probe.attr,
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	&dev_attr_remove.attr,
+#endif
  #endif
 
  #ifdef CONFIG_MEMORY_FAILURE

Hi Andrea,

I've also been looking at memory hotplug for arm64, from the perspective of enabling ZONE_DEVICE for pmem. May I ask what your use-case for this series is? AFAICS the real demand will be coming from server systems, which in practice means both ACPI and NUMA, both of which are being resoundingly ignored here.

Further review comments inline.

Quoted 25 lines · Maciej Bielski

Introduces memory hotplug functionality (hot-add) for arm64.

Changes v1->v2:
- swapper pgtable updated in place on hot add, avoiding unnecessary copy:
all changes are additive and non destructive.

- stop_machine used to updated swapper on hot add, avoiding races

- checking if pagealloc is under debug to stay coherent with mem_map

Signed-off-by Maciej Bielski <[email protected]>

Signed-off-by Andrea Reale <[email protected]>

---
 arch/arm64/Kconfig           | 12 ++++++
 arch/arm64/configs/defconfig |  1 +
 arch/arm64/include/asm/mmu.h |  3 ++
 arch/arm64/mm/init.c         | 87 ++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c          | 39 ++++++++++++++++++++
 5 files changed, 142 insertions(+)

arch/arm64/Kconfig +0 -0

index 0df64a6..c736bba 100644

arch/arm64/Kconfig config HOTPLUG_CPU · line 649

Quoted 9 lines

arch/arm64/Kconfig +7 -0

@@ -641,6 +641,14 @@ config HOTPLUG_CPU
 	  Say Y here to experiment with turning CPUs off and on.  CPUs
 	  can be controlled through /sys/devices/system/cpu.
+config ARCH_HAS_ADD_PAGES
+	def_bool y
+	depends on ARCH_ENABLE_MEMORY_HOTPLUG
+
+config ARCH_ENABLE_MEMORY_HOTPLUG
+	def_bool y
+    depends on !NUMA

As above, realistically this seems too limiting to be useful.

arch/arm64/Kconfig config ARCH_HAS_CACHE_LINE_SIZE · line 726

Quoted 9 lines

arch/arm64/Kconfig +4 -0

+
 # Common NUMA Features
 config NUMA
 	bool "Numa Memory Allocation and Scheduler Support"

@@ -715,6 +723,10 @@ config ARCH_HAS_CACHE_LINE_SIZE
 source "mm/Kconfig"
+config ARCH_MEMORY_PROBE
+	def_bool y
+	depends on MEMORY_HOTPLUG

I'm particularly dubious about enabling this by default - it's useful for development and testing, yes, but I think it's the kind of feature where the onus should be on interested developers to turn it on, rather than production configs to have to turn it off.

arch/arm64/configs/defconfig CONFIG_ARM64_VA_BITS_48=y · line 83

Quoted 13 lines

arch/arm64/Kconfig +1 -0

+
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	---help---

arch/arm64/configs/defconfig +1 -0

index 34480e9..5fc5656 100644
@@ -80,6 +80,7 @@ CONFIG_ARM64_VA_BITS_48=y
 CONFIG_SCHED_MC=y
 CONFIG_NUMA=y
 CONFIG_PREEMPT=y
+CONFIG_MEMORY_HOTPLUG=y

Note that this is effectively pointless, given two lines above...

arch/arm64/include/asm/mmu.h extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys, · line 44

Quoted 13 lines

arch/arm64/configs/defconfig +0 -0

 CONFIG_KSM=y
 CONFIG_TRANSPARENT_HUGEPAGE=y
 CONFIG_CMA=y

arch/arm64/include/asm/mmu.h +2 -0

index 0d34bf0..2b3fa4d 100644
@@ -40,5 +40,8 @@ extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
 			       pgprot_t prot, bool page_mappings_only);
 extern void *fixmap_remap_fdt(phys_addr_t dt_phys);
 extern void mark_linear_text_alias_ro(void);
+#ifdef CONFIG_MEMORY_HOTPLUG
+extern void hotplug_paging(phys_addr_t start, phys_addr_t size);

Is there any reason for not just implementing all the hotplug code self-contained in mmu.c?

Quoted 59 lines

arch/arm64/include/asm/mmu.h +1 -0

+#endif
 #endif

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

index 5960bef..e96e7d3 100644
@@ -722,3 +722,90 @@ static int __init register_mem_limit_dumper(void)
 	return 0;
 }
 __initcall(register_mem_limit_dumper);
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+int add_pages(int nid, unsigned long start_pfn,
+		unsigned long nr_pages, bool want_memblock)
+{
+	int ret;
+	u64 start_addr = start_pfn << PAGE_SHIFT;
+	/*
+	 * Mark the first page in the range as unusable. This is needed
+	 * because __add_section (within __add_pages) wants pfn_valid
+	 * of it to be false, and in arm64 pfn falid is implemented by
+	 * just checking at the nomap flag for existing blocks.
+	 *
+	 * A small trick here is that __add_section() requires only
+	 * phys_start_pfn (that is the first pfn of a section) to be
+	 * invalid. Regardless of whether it was assumed (by the function
+	 * author) that all pfns within a section are either all valid
+	 * or all invalid, it allows to avoid looping twice (once here,
+	 * second when memblock_clear_nomap() is called) through all
+	 * pfns of the section and modify only one pfn. Thanks to that,
+	 * further, in __add_zone() only this very first pfn is skipped
+	 * and corresponding page is not flagged reserved. Therefore it
+	 * is enough to correct this setup only for it.
+	 *
+	 * When arch_add_memory() returns the walk_memory_range() function
+	 * is called and passed with online_memory_block() callback,
+	 * which execution finally reaches the memory_block_action()
+	 * function, where also only the first pfn of a memory block is
+	 * checked to be reserved. Above, it was first pfn of a section,
+	 * here it is a block but
+	 * (drivers/base/memory.c):
+	 *     sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
+	 * (include/linux/memory.h):
+	 *     #define MIN_MEMORY_BLOCK_SIZE     (1UL << SECTION_SIZE_BITS)
+	 * so we can consider block and section equivalently
+	 */
+	memblock_mark_nomap(start_addr, 1<<PAGE_SHIFT);
+	ret = __add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+	/*
+	 * Make the pages usable after they have been added.
+	 * This will make pfn_valid return true
+	 */
+	memblock_clear_nomap(start_addr, 1<<PAGE_SHIFT);
+
+	/*
+	 * This is a hack to avoid having to mix arch specific code
+	 * into arch independent code. SetPageReserved is supposed
+	 * to be called by __add_zone (within __add_section, within

arch/arm64/mm/init.c static int __init register_mem_limit_dumper(void) · line 783

Quoted 10 lines

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

+	 * __add_pages). However, when it is called there, it assumes that
+	 * pfn_valid returns true.  For the way pfn_valid is implemented
+	 * in arm64 (a check on the nomap flag), the only way to make
+	 * this evaluate true inside __add_zone is to clear the nomap
+	 * flags of blocks in architecture independent code.
+	 *
+	 * To avoid this, we set the Reserved flag here after we cleared
+	 * the nomap flag in the line above.
+	 */
+	SetPageReserved(pfn_to_page(start_pfn));

This whole business is utterly horrible. I really think we need to revisit why arm64 isn't using the normal sparsemem pfn_valid() implementation. If there are callers misusing pfn_valid() where they really want page_is_ram() or similar, or missing further pfn_valid_within() checks, then it's surely time to fix those at the source rather than adding to the Jenga pile of hacks in this area. I've started digging into it myself, but don't have any answers yet.

Quoted 76 lines

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

+
+	return ret;
+}
+
+int arch_add_memory(int nid, u64 start, u64 size, bool want_memblock)
+{
+	int ret;
+	unsigned long start_pfn = start >> PAGE_SHIFT;
+	unsigned long nr_pages = size >> PAGE_SHIFT;
+	unsigned long end_pfn = start_pfn + nr_pages;
+	unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
+
+	if (end_pfn > max_sparsemem_pfn) {
+		pr_err("end_pfn too big");
+		return -1;
+	}
+	hotplug_paging(start, size);
+
+	ret = add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+	if (ret)
+		pr_warn("%s: Problem encountered in __add_pages() ret=%d\n",
+			__func__, ret);
+
+	return ret;
+}
+
+#endif /* CONFIG_MEMORY_HOTPLUG */

arch/arm64/mm/mmu.c +34 -0

index f1eb15e..d93043d 100644
@@ -28,6 +28,7 @@
 #include <linux/mman.h>
 #include <linux/nodemask.h>
 #include <linux/memblock.h>
+#include <linux/stop_machine.h>
 #include <linux/fs.h>
 #include <linux/io.h>
 #include <linux/mm.h>

@@ -615,6 +616,44 @@ void __init paging_init(void)
 		      SWAPPER_DIR_SIZE - PAGE_SIZE);
 }
+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables
+ * for hot added memory.
+ */
+
+struct mem_range {
+	phys_addr_t base;
+	phys_addr_t size;
+};
+
+static int __hotplug_paging(void *data)
+{
+	int flags = 0;
+	struct mem_range *section = data;
+
+	if (debug_pagealloc_enabled())
+		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
+	__create_pgd_mapping(swapper_pg_dir, section->base,
+			__phys_to_virt(section->base), section->size,
+			PAGE_KERNEL, pgd_pgtable_alloc, flags);
+
+	return 0;
+}
+
+inline void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+	struct mem_range section = {
+		.base = start,
+		.size = size,
+	};

arch/arm64/mm/mmu.c void __init paging_init(void) · line 652

Quoted 2 lines

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

+
+	stop_machine(__hotplug_paging, &section, NULL);

Why exactly do we need to swing the stop_machine() hammer here? I appreciate that separate hotplug events for adjacent sections could potentially affect the same top-level entry in swapper_pg_dir, but those should already be serialised by the hotplug lock - who else has cause to modify non-leaf entries for the linear map at runtime in a manner which might conflict?

Robin.

arch/arm64/mm/mmu.c void __init paging_init(void)

Quoted 7 lines

arch/arm64/mm/mmu.c +3 -0

+}
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
 /*
  * Check whether a kernel address is valid (derived from arch/x86/).
  */
 

Hi Robin,

Thank you for your feedback, its highly appreciated. I let myself to add some comments.

Our primary goal was to have hotplug working even in the basic setup and publish first working results. Then we want to improve the code building on top of community comments. This is a general answer for questions about configuration flags. The working setup is presented, a bit as a hint, and we do not deem it to be ultimately best at all. The questions about configuration, IMHO, falls into category of making an agreement on a proper setup (defaults, dependencies) and, therefore, we strongly rely on the community experience to advise us how it should be. So, shortly, for some questions "why this is setup in such a way" the simple anser is that it worked as a first approximation. Then, I totally agree that for a server-grade system it should be different and thanks a lot for sharing your opinion on that.

Quoted 2 lines · Robin Murphy

Hi Andrea,

Quoted 6 lines

I've also been looking at memory hotplug for arm64, from the perspective of enabling ZONE_DEVICE for pmem. May I ask what your use-case for this series is? AFAICS the real demand will be coming from server systems, which in practice means both ACPI and NUMA, both of which are being resoundingly ignored here.

Eventually we aim for aarch64 server system.

Quoted 55 lines

Further review comments inline.

Quoted 25 lines · Maciej Bielski

Introduces memory hotplug functionality (hot-add) for arm64.

Changes v1->v2:
- swapper pgtable updated in place on hot add, avoiding unnecessary copy:
all changes are additive and non destructive.

- stop_machine used to updated swapper on hot add, avoiding races

- checking if pagealloc is under debug to stay coherent with mem_map

Signed-off-by Maciej Bielski <[email protected]>

Signed-off-by Andrea Reale <[email protected]>

---
 arch/arm64/Kconfig           | 12 ++++++
 arch/arm64/configs/defconfig |  1 +
 arch/arm64/include/asm/mmu.h |  3 ++
 arch/arm64/mm/init.c         | 87 ++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c          | 39 ++++++++++++++++++++
 5 files changed, 142 insertions(+)

arch/arm64/Kconfig +0 -0

index 0df64a6..c736bba 100644

arch/arm64/Kconfig config HOTPLUG_CPU · line 649

Quoted 9 lines

arch/arm64/Kconfig +7 -0

@@ -641,6 +641,14 @@ config HOTPLUG_CPU
 	  Say Y here to experiment with turning CPUs off and on.  CPUs
 	  can be controlled through /sys/devices/system/cpu.
+config ARCH_HAS_ADD_PAGES
+	def_bool y
+	depends on ARCH_ENABLE_MEMORY_HOTPLUG
+
+config ARCH_ENABLE_MEMORY_HOTPLUG
+	def_bool y
+    depends on !NUMA

As above, realistically this seems too limiting to be useful.

arch/arm64/Kconfig config ARCH_HAS_CACHE_LINE_SIZE · line 726

Quoted 9 lines

arch/arm64/Kconfig +4 -0

+
 # Common NUMA Features
 config NUMA
 	bool "Numa Memory Allocation and Scheduler Support"

@@ -715,6 +723,10 @@ config ARCH_HAS_CACHE_LINE_SIZE
 source "mm/Kconfig"
+config ARCH_MEMORY_PROBE
+	def_bool y
+	depends on MEMORY_HOTPLUG

I'm particularly dubious about enabling this by default - it's useful for development and testing, yes, but I think it's the kind of feature where the onus should be on interested developers to turn it on, rather than production configs to have to turn it off.

Quoted 7 lines

Quoted 7 lines

arch/arm64/Kconfig +1 -0

+
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	---help---

arch/arm64/configs/defconfig +0 -1

index 34480e9..5fc5656 100644
--- a/arch/arm64/configs/defconfig

Quoted 9 lines

arch/arm64/configs/defconfig CONFIG_ARM64_VA_BITS_48=y · line 83

Quoted 6 lines

+++ b/arch/arm64/configs/defconfig

arch/arm64/configs/defconfig +1 -0

@@ -80,6 +80,7 @@ CONFIG_ARM64_VA_BITS_48=y
 CONFIG_SCHED_MC=y
 CONFIG_NUMA=y
 CONFIG_PREEMPT=y
+CONFIG_MEMORY_HOTPLUG=y

Note that this is effectively pointless, given two lines above...

Quoted 8 lines

Quoted 8 lines

arch/arm64/configs/defconfig +0 -0

 CONFIG_KSM=y
 CONFIG_TRANSPARENT_HUGEPAGE=y
 CONFIG_CMA=y

arch/arm64/include/asm/mmu.h +0 -0

index 0d34bf0..2b3fa4d 100644
@@ -40,5 +40,8 @@ extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,

Quoted 9 lines

arch/arm64/include/asm/mmu.h extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys, · line 44

Quoted 5 lines

arch/arm64/include/asm/mmu.h +2 -0

 			       pgprot_t prot, bool page_mappings_only);
 extern void *fixmap_remap_fdt(phys_addr_t dt_phys);
 extern void mark_linear_text_alias_ro(void);
+#ifdef CONFIG_MEMORY_HOTPLUG
+extern void hotplug_paging(phys_addr_t start, phys_addr_t size);

Is there any reason for not just implementing all the hotplug code self-contained in mmu.c?

Simply, in the first version we were supposed to built on top of the patch by Scott Branden, who put a mock implementation of arch_add_memory() in arch/arm64/mm/init.c, this is why hotplug_paging() needed to be announced outside. Quickly looking on the code now I agree that it would be more clean to put everything in arch/arm64/mm/mmu.c. I will test that.

Quoted 70 lines

Quoted 59 lines

arch/arm64/include/asm/mmu.h +1 -0

+#endif
 #endif

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

index 5960bef..e96e7d3 100644
@@ -722,3 +722,90 @@ static int __init register_mem_limit_dumper(void)
 	return 0;
 }
 __initcall(register_mem_limit_dumper);
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+int add_pages(int nid, unsigned long start_pfn,
+		unsigned long nr_pages, bool want_memblock)
+{
+	int ret;
+	u64 start_addr = start_pfn << PAGE_SHIFT;
+	/*
+	 * Mark the first page in the range as unusable. This is needed
+	 * because __add_section (within __add_pages) wants pfn_valid
+	 * of it to be false, and in arm64 pfn falid is implemented by
+	 * just checking at the nomap flag for existing blocks.
+	 *
+	 * A small trick here is that __add_section() requires only
+	 * phys_start_pfn (that is the first pfn of a section) to be
+	 * invalid. Regardless of whether it was assumed (by the function
+	 * author) that all pfns within a section are either all valid
+	 * or all invalid, it allows to avoid looping twice (once here,
+	 * second when memblock_clear_nomap() is called) through all
+	 * pfns of the section and modify only one pfn. Thanks to that,
+	 * further, in __add_zone() only this very first pfn is skipped
+	 * and corresponding page is not flagged reserved. Therefore it
+	 * is enough to correct this setup only for it.
+	 *
+	 * When arch_add_memory() returns the walk_memory_range() function
+	 * is called and passed with online_memory_block() callback,
+	 * which execution finally reaches the memory_block_action()
+	 * function, where also only the first pfn of a memory block is
+	 * checked to be reserved. Above, it was first pfn of a section,
+	 * here it is a block but
+	 * (drivers/base/memory.c):
+	 *     sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
+	 * (include/linux/memory.h):
+	 *     #define MIN_MEMORY_BLOCK_SIZE     (1UL << SECTION_SIZE_BITS)
+	 * so we can consider block and section equivalently
+	 */
+	memblock_mark_nomap(start_addr, 1<<PAGE_SHIFT);
+	ret = __add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+	/*
+	 * Make the pages usable after they have been added.
+	 * This will make pfn_valid return true
+	 */
+	memblock_clear_nomap(start_addr, 1<<PAGE_SHIFT);
+
+	/*
+	 * This is a hack to avoid having to mix arch specific code
+	 * into arch independent code. SetPageReserved is supposed
+	 * to be called by __add_zone (within __add_section, within

arch/arm64/mm/init.c static int __init register_mem_limit_dumper(void) · line 783

Quoted 10 lines

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

+	 * __add_pages). However, when it is called there, it assumes that
+	 * pfn_valid returns true.  For the way pfn_valid is implemented
+	 * in arm64 (a check on the nomap flag), the only way to make
+	 * this evaluate true inside __add_zone is to clear the nomap
+	 * flags of blocks in architecture independent code.
+	 *
+	 * To avoid this, we set the Reserved flag here after we cleared
+	 * the nomap flag in the line above.
+	 */
+	SetPageReserved(pfn_to_page(start_pfn));

Quoted 8 lines

This whole business is utterly horrible. I really think we need to revisit why arm64 isn't using the normal sparsemem pfn_valid() implementation. If there are callers misusing pfn_valid() where they really want page_is_ram() or similar, or missing further pfn_valid_within() checks, then it's surely time to fix those at the source rather than adding to the Jenga pile of hacks in this area. I've started digging into it myself, but don't have any answers yet.

I fully agree and this is the exact reaction we hoped for. We just decided to avoid opening too many fronts at the same time, also that we were not completely sure what exactly the pfn_valid() is supposed to serve for and what we can potentially break. We are looking for your findings here.

Quoted 76 lines

Quoted 76 lines

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

+
+	return ret;
+}
+
+int arch_add_memory(int nid, u64 start, u64 size, bool want_memblock)
+{
+	int ret;
+	unsigned long start_pfn = start >> PAGE_SHIFT;
+	unsigned long nr_pages = size >> PAGE_SHIFT;
+	unsigned long end_pfn = start_pfn + nr_pages;
+	unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
+
+	if (end_pfn > max_sparsemem_pfn) {
+		pr_err("end_pfn too big");
+		return -1;
+	}
+	hotplug_paging(start, size);
+
+	ret = add_pages(nid, start_pfn, nr_pages, want_memblock);
+
+	if (ret)
+		pr_warn("%s: Problem encountered in __add_pages() ret=%d\n",
+			__func__, ret);
+
+	return ret;
+}
+
+#endif /* CONFIG_MEMORY_HOTPLUG */

arch/arm64/mm/mmu.c +34 -0

index f1eb15e..d93043d 100644
@@ -28,6 +28,7 @@
 #include <linux/mman.h>
 #include <linux/nodemask.h>
 #include <linux/memblock.h>
+#include <linux/stop_machine.h>
 #include <linux/fs.h>
 #include <linux/io.h>
 #include <linux/mm.h>

@@ -615,6 +616,44 @@ void __init paging_init(void)
 		      SWAPPER_DIR_SIZE - PAGE_SIZE);
 }
+#ifdef CONFIG_MEMORY_HOTPLUG
+
+/*
+ * hotplug_paging() is used by memory hotplug to build new page tables
+ * for hot added memory.
+ */
+
+struct mem_range {
+	phys_addr_t base;
+	phys_addr_t size;
+};
+
+static int __hotplug_paging(void *data)
+{
+	int flags = 0;
+	struct mem_range *section = data;
+
+	if (debug_pagealloc_enabled())
+		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
+	__create_pgd_mapping(swapper_pg_dir, section->base,
+			__phys_to_virt(section->base), section->size,
+			PAGE_KERNEL, pgd_pgtable_alloc, flags);
+
+	return 0;
+}
+
+inline void hotplug_paging(phys_addr_t start, phys_addr_t size)
+{
+	struct mem_range section = {
+		.base = start,
+		.size = size,
+	};

Quoted 9 lines

arch/arm64/mm/mmu.c void __init paging_init(void) · line 652

Quoted 2 lines

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

+
+	stop_machine(__hotplug_paging, &section, NULL);

Why exactly do we need to swing the stop_machine() hammer here? I appreciate that separate hotplug events for adjacent sections could potentially affect the same top-level entry in swapper_pg_dir, but those should already be serialised by the hotplug lock - who else has cause to modify non-leaf entries for the linear map at runtime in a manner which might conflict?

The reason for this has been mentioned by Mark Rutland in the previous spin
(https://lkml.org/lkml/2017/4/11/582), please let us know if you have different point of view.

BR,
Maciej Bielski

Quoted 9 lines

Robin.

Quoted 7 lines

arch/arm64/mm/mmu.c +3 -0

+}
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
 /*
  * Check whether a kernel address is valid (derived from arch/x86/).
  */
 

Quoted 6 lines · Andrea Reale

Adding a "remove" sysfs handle that can be used to trigger memory hotremove manually, exactly simmetrically with what happens with the "probe" device for hot-add.

This is usueful for architecture that do not rely on ACPI for memory hot-remove.

Is there a real-world use-case for this, or is it mostly just a handy development feature?

Quoted 30 lines

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/base/memory.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

drivers/base/memory.c +16 -1

index 1d60b58..8ccb67c 100644
@@ -530,7 +530,36 @@ memory_probe_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR(probe, S_IWUSR, NULL, memory_probe_store);
-#endif
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+static ssize_t
+memory_remove_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	u64 phys_addr;
+	int nid, ret;
+	unsigned long pages_per_block = PAGES_PER_SECTION * sections_per_block;
+
+	ret = kstrtoull(buf, 0, &phys_addr);
+	if (ret)
+		return ret;
+
+	if (phys_addr & ((pages_per_block << PAGE_SHIFT) - 1))
+		return -EINVAL;

drivers/base/memory.c memory_probe_store(struct device *dev, struct device_attribute *attr, · line 549

Quoted 2 lines

drivers/base/memory.c +2 -0

+
+	nid = memory_add_physaddr_to_nid(phys_addr);

This call looks a bit odd, since you're not doing a memory add. In fact, any memory being removed should already be fully known-about, so AFAICS it should be simple to get everything you need to know (including potentially the online status as mentioned earlier), through 'normal' methods, e.g. page_to_nid() or similar.

Robin.

Quoted 24 lines

drivers/base/memory.c +15 -0

+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	remove_memory(nid, phys_addr,
+			 MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+	unlock_device_hotplug();
+	return count;
+}
+static DEVICE_ATTR(remove, S_IWUSR, NULL, memory_remove_store);
+#endif /* CONFIG_MEMORY_HOTREMOVE */
+#endif /* CONFIG_ARCH_MEMORY_PROBE */
 #ifdef CONFIG_MEMORY_FAILURE
 /*

@@ -790,6 +819,9 @@ bool is_memblock_offlined(struct memory_block *mem)
 static struct attribute *memory_root_attrs[] = {
 #ifdef CONFIG_ARCH_MEMORY_PROBE
 	&dev_attr_probe.attr,
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	&dev_attr_remove.attr,
+#endif
 #endif
 #ifdef CONFIG_MEMORY_FAILURE
 

Quoted 1 line · Andrea Reale

When hot-removing memory we need to free vmemmap memory.

What problems arise if we don't? Is it only for the sake of freeing up some pages here and there, or is there something more fundamental?

Quoted 27 lines

However, depending on the memory is being removed, it might not be always possible to free a full vmemmap page / huge-page because part of it might still be used.

Commit ae9aae9eda2d ("memory-hotplug: common APIs to support page tables hot-remove") introduced a workaround for x86
hot-remove, by which partially unused areas are filled with the 0xFD constant. Full pages are only removed when fully filled by 0xFDs.

This commit introduces a MEMBLOCK_UNUSED_VMEMMAP memblock flag, with the goal of using it in place of 0xFDs. For now, this will be used for the arm64 port of memory hot remove, but the idea is to eventually use the same mechanism for x86 as well.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 include/linux/memblock.h | 12 ++++++++++++
 mm/memblock.c            | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

include/linux/memblock.h +0 -0

index bae11c7..0daec05 100644

include/linux/memblock.h enum { · line 30

Quoted 5 lines

include/linux/memblock.h +2 -0

@@ -26,6 +26,9 @@ enum {
 	MEMBLOCK_HOTPLUG	= 0x1,	/* hotpluggable region */
 	MEMBLOCK_MIRROR		= 0x2,	/* mirrored region */
 	MEMBLOCK_NOMAP		= 0x4,	/* don't add to kernel direct mapping */
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	MEMBLOCK_UNUSED_VMEMMAP	= 0x8,  /* Mark VMEMAP blocks as dirty */

I'm not sure I get what "dirty" is supposed to mean in this context. Also, this appears to be specific to CONFIG_SPARSEMEM_VMEMMAP, whilst only tangentially related to CONFIG_MEMORY_HOTREMOVE, so the dependencies look a bit off.

In fact, now that I think about it, why does this need to be in memblock at all? If it is specific to sparsemem, shouldn't the section map already be enough to tell us what's supposed to be present or not?

Robin.

Quoted 73 lines

include/linux/memblock.h +10 -0

+#endif
 };
 struct memblock_region {

@@ -90,6 +93,10 @@ int memblock_mark_mirror(phys_addr_t base, phys_addr_t size);
 int memblock_mark_nomap(phys_addr_t base, phys_addr_t size);
 int memblock_clear_nomap(phys_addr_t base, phys_addr_t size);
 ulong choose_memblock_flags(void);
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int memblock_mark_unused_vmemmap(phys_addr_t base, phys_addr_t size);
+int memblock_clear_unused_vmemmap(phys_addr_t base, phys_addr_t size);
+#endif
 /* Low level functions */
 int memblock_add_range(struct memblock_type *type,

@@ -182,6 +189,11 @@ static inline bool memblock_is_nomap(struct memblock_region *m)
 	return m->flags & MEMBLOCK_NOMAP;
 }
+#ifdef CONFIG_MEMORY_HOTREMOVE
+bool memblock_is_vmemmap_unused_range(struct memblock_type *mt,
+		phys_addr_t start, phys_addr_t end);
+#endif
+
 #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
 int memblock_search_pfn_nid(unsigned long pfn, unsigned long *start_pfn,
 			    unsigned long  *end_pfn);

mm/memblock.c +32 -0

index 9120578..30d5aa4 100644
@@ -809,6 +809,18 @@ int __init_memblock memblock_clear_nomap(phys_addr_t base, phys_addr_t size)
 	return memblock_setclr_flag(base, size, 0, MEMBLOCK_NOMAP);
 }
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int __init_memblock memblock_mark_unused_vmemmap(phys_addr_t base,
+		phys_addr_t size)
+{
+	return memblock_setclr_flag(base, size, 1, MEMBLOCK_UNUSED_VMEMMAP);
+}
+int __init_memblock memblock_clear_unused_vmemmap(phys_addr_t base,
+		phys_addr_t size)
+{
+	return memblock_setclr_flag(base, size, 0, MEMBLOCK_UNUSED_VMEMMAP);
+}
+#endif
 /**
  * __next_reserved_mem_region - next function for for_each_reserved_region()
  * @idx: pointer to u64 loop variable

@@ -1696,6 +1708,26 @@ void __init_memblock memblock_trim_memory(phys_addr_t align)
 	}
 }
+#ifdef CONFIG_MEMORY_HOTREMOVE
+bool __init_memblock memblock_is_vmemmap_unused_range(struct memblock_type *mt,
+		phys_addr_t start, phys_addr_t end)
+{
+	u64 i;
+	struct memblock_region *r;
+
+	i = memblock_search(mt, start);
+	r = &(mt->regions[i]);
+	while (r->base < end) {
+		if (!(r->flags & MEMBLOCK_UNUSED_VMEMMAP))
+			return 0;
+
+		r = &(memblock.memory.regions[++i]);
+	}
+
+	return 1;
+}
+#endif
+
 void __init_memblock memblock_set_current_limit(phys_addr_t limit)
 {
 	memblock.current_limit = limit;
 

Quoted 74 lines · Andrea Reale

Quoted 57 lines · Michal Hocko

Quoted 47 lines · Andrea Reale

Hi Rafael,

Quoted 33 lines · Rafael J. Wysocki

Quoted 32 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
                        nid = memory_add_physaddr_to_nid(info->start_addr);
 

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 285

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +1 -1

                acpi_unbind_memory_blocks(info);
-               remove_memory(nid, info->start_addr, info->length);
+               BUG_ON(remove_memory(nid, info->start_addr, info->length));

Why does this have to be BUG_ON()? Is it really necessary to kill the system here?

Actually, I hoped you would help me understand that: that BUG() call was introduced by yourself in Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") in memory_hoptlug.c:remove_memory()).

Quoted 8 lines

Just reading at that commit my understanding was that you were assuming that acpi_memory_remove_memory() have already done the job of offlining the target memory, so there would be a bug if that wasn't the case.

In my case, that assumption did not hold and I found that it might not hold for other platforms that do not use ACPI. In fact, the purpose of this patch is to move this assumption out of the generic hotplug code and move it to ACPI code where it originated.

Quoted 4 lines

remove_memory failure is basically impossible to handle AFAIR. The original code to BUG in remove_memory is ugly as hell and we do not want to spread that out of that function. Instead we really want to get rid of it.

Today, BUG() is called even in the simple case where remove fails because the section we are removing is not offline. I cannot see any need to BUG() in such a case: an error code seems more than sufficient to me. This is why this patch removes the BUG() call when the "offline" check fails from the generic code.
It moves it back to the ACPI call, where the assumption originated. Honestlly, I cannot tell if it makes sense to BUG() there:
I have nothing against removing it from ACPI hotplug too, but
I don't know enough to feel free to change the acpi semantics myself, so I moved it there to keep the original behavior unchanged for x86 code.

Quoted 4 lines

In this arm64 hot-remove port, offline and remove are done in two separate steps, and is conceivable that an user tries erroneusly to remove some section that he forgot to offline first: in that case, with the patch, remove will just report an erro without BUGing.

The user can already kill the system by misusing the sysfs probe driver; should similar theoretical misuse of your sysfs remove driver really need to be all that different?

Quoted 1 line

Is my reasoning flawed?

Furthermore, even if your driver does want to enforce this, I don't see why it can't just do the equivalent of memory_subsys_offline() itself before even trying to call remove_memory().

Robin.

Quoted 14 lines

Cheers,
Andrea

Quoted 8 lines

Signature · 6 lines

Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +1 -1

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 			nid = memory_add_physaddr_to_nid(info->start_addr);
 
 		acpi_unbind_memory_blocks(info);
-		remove_memory(nid, info->start_addr, info->length);
+		BUG_ON(remove_memory(nid, info->start_addr, info->length));
 		list_del(&info->list);
 		kfree(info);
 	}

include/linux/memory_hotplug.h +6 -3

index 58e110a..1a9c7b2 100644
@@ -295,7 +295,7 @@ static inline bool movable_node_is_enabled(void)
 extern bool is_mem_section_removable(unsigned long pfn, unsigned long nr_pages);
 extern void try_offline_node(int nid);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
-extern void remove_memory(int nid, u64 start, u64 size);
+extern int remove_memory(int nid, u64 start, u64 size);
 
 #else
 static inline bool is_mem_section_removable(unsigned long pfn,

@@ -311,7 +311,10 @@ static inline int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 	return -EINVAL;
 }
 
-static inline void remove_memory(int nid, u64 start, u64 size) {}
+static inline int remove_memory(int nid, u64 start, u64 size)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,

@@ -323,7 +326,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 		unsigned long nr_pages);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
 extern bool is_memblock_offlined(struct memory_block *mem);
-extern void remove_memory(int nid, u64 start, u64 size);
+extern int remove_memory(int nid, u64 start, u64 size);
 extern int sparse_add_one_section(struct pglist_data *pgdat, unsigned long start_pfn);
 extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset);

mm/memory_hotplug.c +1 -1

index d4b5f29..d5f15af 100644
@@ -1892,7 +1892,7 @@ EXPORT_SYMBOL(try_offline_node);
  * and online/offline operations before this call, as required by
  * try_offline_node().
  */
-void __ref remove_memory(int nid, u64 start, u64 size)
+int __ref remove_memory(int nid, u64 start, u64 size)
 {
 	int ret;
 

mm/memory_hotplug.c +2 -2

 	/* remove memmap entry */
 	firmware_map_remove(start, start + size, "System RAM");
 	memblock_free(start, size);
 	memblock_remove(start, size);
 
-	arch_remove_memory(start, size);
-
 	try_offline_node(nid);
 
+end_remove:
 	mem_hotplug_done();
+	return ret;
 }
 EXPORT_SYMBOL_GPL(remove_memory);
 #endif /* CONFIG_MEMORY_HOTREMOVE */

Signature · 6 lines

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 63 lines · Andrea Reale

Quoted 61 lines · Michal Hocko

Quoted 47 lines · Andrea Reale

Hi Rafael,

Quoted 33 lines · Rafael J. Wysocki

Quoted 32 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +0 -0

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
                        nid = memory_add_physaddr_to_nid(info->start_addr);
 

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) · line 285

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c +1 -1

                acpi_unbind_memory_blocks(info);
-               remove_memory(nid, info->start_addr, info->length);
+               BUG_ON(remove_memory(nid, info->start_addr, info->length));

Why does this have to be BUG_ON()? Is it really necessary to kill the system here?

Actually, I hoped you would help me understand that: that BUG() call was introduced by yourself in Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") in memory_hoptlug.c:remove_memory()).

Quoted 8 lines

Just reading at that commit my understanding was that you were assuming that acpi_memory_remove_memory() have already done the job of offlining the target memory, so there would be a bug if that wasn't the case.

In my case, that assumption did not hold and I found that it might not hold for other platforms that do not use ACPI. In fact, the purpose of this patch is to move this assumption out of the generic hotplug code and move it to ACPI code where it originated.

remove_memory failure is basically impossible to handle AFAIR. The original code to BUG in remove_memory is ugly as hell and we do not want to spread that out of that function. Instead we really want to get rid of it.

Quoted 2 lines

Today, BUG() is called even in the simple case where remove fails because the section we are removing is not offline.

You cannot hotremove memory which is still online. This is what caller should enforce. This is too late to handle the failure. At least for ACPI.

Quoted 2 lines

I cannot see any need to
BUG() in such a case: an error code seems more than sufficient to me.

I do not rememeber details but AFAIR ACPI is in a deferred (kworker) context here and cannot simply communicate error code down the road. I agree that we should be able to simply return an error but what is the actual error condition that might happen here?

Quoted 2 lines

This is why this patch removes the BUG() call when the "offline" check fails from the generic code.

As I've said we should simply get rid of BUG rather than move it around.

Hi Andrea,

Quoted 85 lines · Andrea Reale

Resending the patch adding linux-acpi in CC, as suggested by Rafael. Everyone else: apologies for the noise.

Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") introduced an assumption whereas when control
reaches remove_memory the corresponding memory has been already offlined. In that case, the acpi_memhotplug was making sure that the assumption held.
This assumption, however, is not necessarily true if offlining and removal are not done by the same "controller" (for example, when first offlining via sysfs).

Removing this assumption for the generic remove_memory code and moving it in the specific acpi_memhotplug code. This is a dependency for the software-aided arm64 offlining and removal process.

Signed-off-by Andrea Reale <[email protected]>

Signed-off-by Maciej Bielski <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |  2 +-
 include/linux/memory_hotplug.h |  9 ++++++---
 mm/memory_hotplug.c            | 13 +++++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

drivers/acpi/acpi_memhotplug.c +1 -1

index 6b0d3ef..b0126a0 100644
@@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 			nid = memory_add_physaddr_to_nid(info->start_addr);
 
 		acpi_unbind_memory_blocks(info);
-		remove_memory(nid, info->start_addr, info->length);
+		BUG_ON(remove_memory(nid, info->start_addr, info->length));
 		list_del(&info->list);
 		kfree(info);
 	}

include/linux/memory_hotplug.h +6 -3

index 58e110a..1a9c7b2 100644
@@ -295,7 +295,7 @@ static inline bool movable_node_is_enabled(void)
 extern bool is_mem_section_removable(unsigned long pfn, unsigned long nr_pages);
 extern void try_offline_node(int nid);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
-extern void remove_memory(int nid, u64 start, u64 size);
+extern int remove_memory(int nid, u64 start, u64 size);
 
 #else
 static inline bool is_mem_section_removable(unsigned long pfn,

@@ -311,7 +311,10 @@ static inline int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 	return -EINVAL;
 }
 
-static inline void remove_memory(int nid, u64 start, u64 size) {}
+static inline int remove_memory(int nid, u64 start, u64 size)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,

@@ -323,7 +326,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 		unsigned long nr_pages);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
 extern bool is_memblock_offlined(struct memory_block *mem);
-extern void remove_memory(int nid, u64 start, u64 size);
+extern int remove_memory(int nid, u64 start, u64 size);
 extern int sparse_add_one_section(struct pglist_data *pgdat, unsigned long start_pfn);
 extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset);

mm/memory_hotplug.c +1 -1

index d4b5f29..d5f15af 100644
@@ -1892,7 +1892,7 @@ EXPORT_SYMBOL(try_offline_node);
  * and online/offline operations before this call, as required by
  * try_offline_node().
  */
-void __ref remove_memory(int nid, u64 start, u64 size)
+int __ref remove_memory(int nid, u64 start, u64 size)
 {
 	int ret;