Memory hotplug support for arm64 - complete patchset v2

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

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

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/).
   */

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/).
  */
 

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 */

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 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.

On Fri, Nov 24, 2017 at 07:17:41PM +0100, Michal Hocko wrote:

Quoted 4 lines

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.

The logic in acpi_scan_hot_remove() calls memory_subsys_offline(). If there doesn't have any error returns by memory_subsys_offline, then ACPI assumes all devices are offlined by subsystem (memory subsystem in this case).

Then system moves to remove stage, ACPI calls acpi_memory_device_remove(). Here

Quoted 8 lines

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?

Currently acpi_bus_trim() didn't handle any return error. If subsystem returns error, then ACPI can only interrupt hot-remove process.

Quoted 5 lines

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.

As I remember that the original BUG() helped us to find out a bug about the offline state doesn't sync between memblock device with memory state. Something likes:

	mem->dev.offline != (mem->state == MEM_OFFLINE)

So, the BUG() is useful to capture bug about state sync between device object and subsystem object.

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

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

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;
 

mm/memory_hotplug.c void __ref remove_memory(int nid, u64 start, u64 size) · line 1913

Quoted 8 lines

mm/memory_hotplug.c +3 -1

@@ -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);

mm/memory_hotplug.c void __ref remove_memory(int nid, u64 start, u64 size) · line 1916

Quoted 3 lines

mm/memory_hotplug.c +3 -0

+
+	if (ret)
+		goto end_remove;

The original code triggers BUG() when any memblock is not offlined. Why the new logic includes the result of arch_remove_memory()?

But I agreed the we don't need BUG(). Returning a error is better.

Quoted 88 lines · joeyli

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;
 

Quoted 8 lines

mm/memory_hotplug.c void __ref remove_memory(int nid, u64 start, u64 size) · line 1913

Quoted 8 lines

mm/memory_hotplug.c +3 -1

@@ -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);

Should not include arch_remove_memory() to BUG().

Quoted 8 lines

mm/memory_hotplug.c void __ref remove_memory(int nid, u64 start, u64 size) · line 1916

Quoted 3 lines

mm/memory_hotplug.c +3 -0

+
+	if (ret)
+		goto end_remove;

The original code triggers BUG() when any memblock is not offlined. Why the new logic includes the result of arch_remove_memory()?

But I agreed the we don't need BUG(). Returning a error is better.

Actually, I lost one thing.

The BUG() have caught a issue about the offline state doesn't sync between memory_block and device object. like:

        mem->dev.offline != (mem->state == MEM_OFFLINE)

So, the BUG() is useful to capture state issue in memory subsystem. But, I understood your concern about the two steps offline/remove from userland.

Maybe we should move the BUG() to somewhere but not just remove it. Or if we think that the BUG() is too intense, at least we should print out a error message, and ACPI should checks the return value from subsystem to interrupt memory-hotplug process.

Thanks a lot!
Joey Lee

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

Hi Joey,

and thanks for your comments. Response inline:

Quoted 8 lines

So, the BUG() is useful to capture state issue in memory subsystem. But, I understood your concern about the two steps offline/remove from userland.

Maybe we should move the BUG() to somewhere but not just remove it. Or if we think that the BUG() is too intense, at least we should print out a error message, and ACPI should checks the return value from subsystem to interrupt memory-hotplug process.

In this patchset, BUG() is moved to acpi_memory_remove_memory(), the caller of arch_remove_memory(). However, I agree with Michal, that we should not BUG() here but rather halt the hotremove process and print some errors.
Is there any state in ACPI that should be undone in case of hotremove errors or we can just stop the process "halfway"?

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;

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;
 

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 +12 -0

index bae11c7..0daec05 100644
@@ -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 */
+#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;

Signature · 7 lines

2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo at kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> email at kvack.org </a>

Quoted 11 lines · Andrea Reale

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.

Quoted 4 lines

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.

Why cannot you use the same approach as x86 have? Have a look at the vmemmap_free at al.

Quoted 97 lines

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 +12 -0

index bae11c7..0daec05 100644
@@ -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 */
+#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;

Signature · 1 line

2.7.4

Quoted 5 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo at kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> email at kvack.org </a>

Signature · 3 lines

-- 
Michal Hocko
SUSE Labs

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Andrea Reale

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.

Quoted 8 lines

Quoted 4 lines

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.

Why cannot you use the same approach as x86 have? Have a look at the vmemmap_free at al.

This arm64 hot-remove version (including vmemmap_free) is indeed an almost 1-to-1 port of the x86 approach.

If you look at the first version of the patchset we submitted a while ago (https://lkml.org/lkml/2017/4/11/540), we were initially using the x86 approach of filling unsued page structs with 0xFDs. Commenting on that, Mark suggested (and, indeed, I agree with him) that relying on a magic constant for marking some portions of physical memory was quite ugly. That is why we have used memblock for the purpose in this revised patchset.

If you have a different view and any concrete suggestion on how to improve this, it is definitely very well welcome.

Quoted 33 lines · Andrea Reale

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Andrea Reale

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.

Quoted 8 lines

Quoted 4 lines

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.

Why cannot you use the same approach as x86 have? Have a look at the vmemmap_free at al.

This arm64 hot-remove version (including vmemmap_free) is indeed an almost 1-to-1 port of the x86 approach.

If you look at the first version of the patchset we submitted a while ago (https://lkml.org/lkml/2017/4/11/540), we were initially using the x86 approach of filling unsued page structs with 0xFDs. Commenting on that, Mark suggested (and, indeed, I agree with him) that relying on a magic constant for marking some portions of physical memory was quite ugly. That is why we have used memblock for the purpose in this revised patchset.

Quoted 2 lines

If you have a different view and any concrete suggestion on how to improve this, it is definitely very well welcome.

I would really prefer if those archictectues shared the code (and concept) as much as possible. It is really a PITA to wrap your head around each architectures for reasons which are not inherent to that specific architecture. If you find the way how x86 is implemented ugly, then all right, but making arm64 special just for the matter of taste is far from ideal IMHO.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 37 lines · Michal Hocko

Quoted 33 lines · Andrea Reale

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Andrea Reale

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.

Quoted 8 lines

Quoted 4 lines

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.

Why cannot you use the same approach as x86 have? Have a look at the vmemmap_free at al.

This arm64 hot-remove version (including vmemmap_free) is indeed an almost 1-to-1 port of the x86 approach.

If you look at the first version of the patchset we submitted a while ago (https://lkml.org/lkml/2017/4/11/540), we were initially using the x86 approach of filling unsued page structs with 0xFDs. Commenting on that, Mark suggested (and, indeed, I agree with him) that relying on a magic constant for marking some portions of physical memory was quite ugly. That is why we have used memblock for the purpose in this revised patchset.

Quoted 2 lines

If you have a different view and any concrete suggestion on how to improve this, it is definitely very well welcome.

Quoted 6 lines

I would really prefer if those archictectues shared the code (and concept) as much as possible. It is really a PITA to wrap your head around each architectures for reasons which are not inherent to that specific architecture. If you find the way how x86 is implemented ugly, then all right, but making arm64 special just for the matter of taste is far from ideal IMHO.

The plan is indeed to use this memblock flag in x86 hot remove as well, in place of the 0xFDs. The change is quite straightforward and we could push it in a next patchset release. Our rationale was to first use it in the new architecture and then, once proven stable, back port it to x86.

However, I am not in principle against of pushing it right now.

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