arm64: Do not defer reserve_crashkernel() for platforms with no DMA memory zones

PATCH Superseded Vijay Balakrishna 2 participants 4 messages Feb 17, 2022 arm64

Share & tools

Vijay Balakrishna

author

2022-02-17 00:05 UTC #0 lore raw

PATCH 1/1 arm64: Do not defer reserve_crashkernel() for platforms with no DMA memory zones

The following patches resulted in deferring crash kernel reservation to mem_init(), mainly aimed at platforms with DMA memory zones (no IOMMU), in particular Raspberry Pi 4.

commit 1a8e1cef7603 ("arm64: use both ZONE_DMA and ZONE_DMA32")
commit 8424ecdde7df ("arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges") commit 0a30c53573b0 ("arm64: mm: Move reserve_crashkernel() into mem_init()") commit 2687275a5843 ("arm64: Force NO_BLOCK_MAPPINGS if crashkernel reservation is required")

Above changes introduced boot slowdown due to linear map creation for all the memory banks with NO_BLOCK_MAPPINGS, see discussion[1]. The proposed changes restore crash kernel reservation to earlier behavior thus avoids slow boot, particularly for platforms with IOMMU (no DMA memory zones).

[1] https://lore.kernel.org/all/[email protected]/

Signed-off-by Vijay Balakrishna <[email protected]>

Cc [email protected]

---
Tested changes to confirm no ~150ms boot slowdown on our SoC with IOMMU and 8GB memory. Also tested with ZONE_DMA and/or ZONE_DMA32 configs to confirm no regression to deferring scheme of crash kernel memory reservation. In both cases successfully collected kernel crash dump.

---
 arch/arm64/mm/init.c | 14 +++++++++++---
 arch/arm64/mm/mmu.c  | 24 +++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 4 deletions(-)

arch/arm64/mm/init.c +11 -3

index db63cc885771..f2a982c19b75 100644
@@ -62,7 +62,11 @@ EXPORT_SYMBOL(memstart_addr);
  * In such case, ZONE_DMA32 covers the rest of the 32-bit addressable memory,
  * otherwise it is empty.
  */
-phys_addr_t arm64_dma_phys_limit __ro_after_init;
+#if !defined(CONFIG_ZONE_DMA) && !defined(CONFIG_ZONE_DMA32)
+phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
+#else
+phys_addr_t __ro_after_init arm64_dma_phys_limit;
+#endif
 
 #ifdef CONFIG_KEXEC_CORE
 /*

@@ -153,8 +157,6 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max)
 	if (!arm64_dma_phys_limit)
 		arm64_dma_phys_limit = dma32_phys_limit;
 #endif
-	if (!arm64_dma_phys_limit)
-		arm64_dma_phys_limit = PHYS_MASK + 1;
 	max_zone_pfns[ZONE_NORMAL] = max;
 
 	free_area_init(max_zone_pfns);

@@ -315,6 +317,10 @@ void __init arm64_memblock_init(void)
 
 	early_init_fdt_scan_reserved_mem();
 
+#if !defined(CONFIG_ZONE_DMA) && !defined(CONFIG_ZONE_DMA32)
+	reserve_crashkernel();
+#endif
+
 	high_memory = __va(memblock_end_of_DRAM() - 1) + 1;
 }

@@ -357,11 +363,13 @@ void __init bootmem_init(void)
 	 */
 	dma_contiguous_reserve(arm64_dma_phys_limit);
 
+#if defined(CONFIG_ZONE_DMA) || defined(CONFIG_ZONE_DMA32)
 	/*
 	 * request_standard_resources() depends on crashkernel's memory being
 	 * reserved, so do it here.
 	 */
 	reserve_crashkernel();
+#endif
 
 	memblock_dump_all();
 }

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

index acfae9b41cc8..e7faf5edccfc 100644
@@ -517,7 +517,7 @@ static void __init map_mem(pgd_t *pgdp)
 	 */
 	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
 
-	if (can_set_direct_map() || crash_mem_map || IS_ENABLED(CONFIG_KFENCE))
+	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
 		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
 	/*

@@ -528,6 +528,14 @@ static void __init map_mem(pgd_t *pgdp)
 	 */
 	memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
 
+#ifdef CONFIG_KEXEC_CORE
+	if (crash_mem_map && !crashk_res.end)
+		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+
+	if (crashk_res.end)
+		memblock_mark_nomap(crashk_res.start,
+				    resource_size(&crashk_res));
+#endif
 	/* map all the memory banks */
 	for_each_mem_range(i, &start, &end) {
 		if (start >= end)

@@ -554,6 +562,20 @@ static void __init map_mem(pgd_t *pgdp)
 	__map_memblock(pgdp, kernel_start, kernel_end,
 		       PAGE_KERNEL, NO_CONT_MAPPINGS);
 	memblock_clear_nomap(kernel_start, kernel_end - kernel_start);
+#ifdef CONFIG_KEXEC_CORE
+	/*
+	 * Use page-level mappings here so that we can shrink the region
+	 * in page granularity and put back unused memory to buddy system
+	 * through /sys/kernel/kexec_crash_size interface.
+	 */
+	if (crashk_res.end) {
+		__map_memblock(pgdp, crashk_res.start, crashk_res.end + 1,
+			       PAGE_KERNEL,
+			       NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
+		memblock_clear_nomap(crashk_res.start,
+				     resource_size(&crashk_res));
+	}
+#endif
 }
 
 void mark_rodata_ro(void)

Signature · 1 line

2.35.1

nicolas saenz julienne

2022-02-17 10:50 UTC #1 lore raw

Quoted 93 lines · Vijay Balakrishna

The following patches resulted in deferring crash kernel reservation to mem_init(), mainly aimed at platforms with DMA memory zones (no IOMMU), in particular Raspberry Pi 4.
commit 1a8e1cef7603 ("arm64: use both ZONE_DMA and ZONE_DMA32")
commit 8424ecdde7df ("arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges") commit 0a30c53573b0 ("arm64: mm: Move reserve_crashkernel() into mem_init()") commit 2687275a5843 ("arm64: Force NO_BLOCK_MAPPINGS if crashkernel reservation is required")
Above changes introduced boot slowdown due to linear map creation for all the memory banks with NO_BLOCK_MAPPINGS, see discussion[1]. The proposed changes restore crash kernel reservation to earlier behavior thus avoids slow boot, particularly for platforms with IOMMU (no DMA memory zones).
[1] https://lore.kernel.org/all/[email protected]/
Signed-off-by Vijay Balakrishna <[email protected]>
Cc [email protected]
---
Tested changes to confirm no ~150ms boot slowdown on our SoC with IOMMU and 8GB memory. Also tested with ZONE_DMA and/or ZONE_DMA32 configs to confirm no regression to deferring scheme of crash kernel memory reservation. In both cases successfully collected kernel crash dump.
---
 arch/arm64/mm/init.c | 14 +++++++++++---
 arch/arm64/mm/mmu.c  | 24 +++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 4 deletions(-)
arch/arm64/mm/init.c +11 -3
index db63cc885771..f2a982c19b75 100644
@@ -62,7 +62,11 @@ EXPORT_SYMBOL(memstart_addr);
  * In such case, ZONE_DMA32 covers the rest of the 32-bit addressable memory,
  * otherwise it is empty.
  */
-phys_addr_t arm64_dma_phys_limit __ro_after_init;
+#if !defined(CONFIG_ZONE_DMA) && !defined(CONFIG_ZONE_DMA32)
+phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
+#else
+phys_addr_t __ro_after_init arm64_dma_phys_limit;
+#endif
 
 #ifdef CONFIG_KEXEC_CORE
 /*
@@ -153,8 +157,6 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max)
 	if (!arm64_dma_phys_limit)
 		arm64_dma_phys_limit = dma32_phys_limit;
 #endif
-	if (!arm64_dma_phys_limit)
-		arm64_dma_phys_limit = PHYS_MASK + 1;
 	max_zone_pfns[ZONE_NORMAL] = max;
 
 	free_area_init(max_zone_pfns);
@@ -315,6 +317,10 @@ void __init arm64_memblock_init(void)
 
 	early_init_fdt_scan_reserved_mem();
 
+#if !defined(CONFIG_ZONE_DMA) && !defined(CONFIG_ZONE_DMA32)
+	reserve_crashkernel();
+#endif
+
 	high_memory = __va(memblock_end_of_DRAM() - 1) + 1;
 }
 
@@ -357,11 +363,13 @@ void __init bootmem_init(void)
 	 */
 	dma_contiguous_reserve(arm64_dma_phys_limit);
 
+#if defined(CONFIG_ZONE_DMA) || defined(CONFIG_ZONE_DMA32)
 	/*
 	 * request_standard_resources() depends on crashkernel's memory being
 	 * reserved, so do it here.
 	 */
 	reserve_crashkernel();
+#endif
 
 	memblock_dump_all();
 }
arch/arm64/mm/mmu.c +1 -1
index acfae9b41cc8..e7faf5edccfc 100644
@@ -517,7 +517,7 @@ static void __init map_mem(pgd_t *pgdp)
 	 */
 	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
 
-	if (can_set_direct_map() || crash_mem_map || IS_ENABLED(CONFIG_KFENCE))
+	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
 		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
 	/*

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgdp) · line 533

Quoted 6 lines

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

@@ -528,6 +528,14 @@ static void __init map_mem(pgd_t *pgdp)
 	 */
 	memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
 
+#ifdef CONFIG_KEXEC_CORE
+	if (crash_mem_map && !crashk_res.end)
+		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;

Using IS_ENABLED(ZONE_DMA/DMA32) instead of '!crashk_res.end' would be more efficient and a bit more explicit IMO.

Quoted 4 lines

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

 	/* map all the memory banks */
 	for_each_mem_range(i, &start, &end) {
 		if (start >= end)

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgdp) · line 571

Quoted 10 lines

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

@@ -554,6 +562,20 @@ static void __init map_mem(pgd_t *pgdp)
 	__map_memblock(pgdp, kernel_start, kernel_end,
 		       PAGE_KERNEL, NO_CONT_MAPPINGS);
 	memblock_clear_nomap(kernel_start, kernel_end - kernel_start);
+#ifdef CONFIG_KEXEC_CORE
+	/*
+	 * Use page-level mappings here so that we can shrink the region
+	 * in page granularity and put back unused memory to buddy system
+	 * through /sys/kernel/kexec_crash_size interface.
+	 */
+	if (crashk_res.end) {

Same here.

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgdp) · line 578

Quoted 7 lines

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

+		__map_memblock(pgdp, crashk_res.start, crashk_res.end + 1,
+			       PAGE_KERNEL,
+			       NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
+		memblock_clear_nomap(crashk_res.start,
+				     resource_size(&crashk_res));
+	}
+#endif

Now, I carefully reviewed the patch and it seems to be doing the right thing. But even while knowlegable on the topic, it took a good amount of effort to untangle the possible code paths. I suspect it's going to be painful to maintain. I'd suggest at least introducing a comment explaining the situation.

If there approach if deemed acceptable, I'll test is on the RPi4.

Regards,
Nicolas

nicolas saenz julienne

2022-02-17 10:54 UTC #2 lore raw

Sorry, typos:

Quoted 1 line · nicolas saenz julienne

If there approach if deemed acceptable, I'll test is on the RPi4.

"If the approach is deemed acceptable,"

Regards,
Nicoals

Vijay Balakrishna

author

2022-02-17 18:26 UTC #3 lore raw

In reply to nicolas saenz julienne

Quoted 8 lines · nicolas saenz julienne

Quoted 7 lines · Vijay Balakrishna
The following patches resulted in deferring crash kernel reservation to mem_init(), mainly aimed at platforms with DMA memory zones (no IOMMU), in particular Raspberry Pi 4.
commit 1a8e1cef7603 ("arm64: use both ZONE_DMA and ZONE_DMA32")
commit 8424ecdde7df ("arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges") commit 0a30c53573b0 ("arm64: mm: Move reserve_crashkernel() into mem_init()")

Quoted 17 lines

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

index acfae9b41cc8..e7faf5edccfc 100644
@@ -517,7 +517,7 @@ static void __init map_mem(pgd_t *pgdp)
  	 */
  	BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end));
 
-	if (can_set_direct_map() || crash_mem_map || IS_ENABLED(CONFIG_KFENCE))
+	if (can_set_direct_map() || IS_ENABLED(CONFIG_KFENCE))
  		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
  	/*

@@ -528,6 +528,14 @@ static void __init map_mem(pgd_t *pgdp)
  	 */
  	memblock_mark_nomap(kernel_start, kernel_end - kernel_start);

Quoted 6 lines

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgdp) · line 533
Quoted 3 lines
arch/arm64/mm/mmu.c +3 -0
+#ifdef CONFIG_KEXEC_CORE
+	if (crash_mem_map && !crashk_res.end)
+		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
Using IS_ENABLED(ZONE_DMA/DMA32) instead of '!crashk_res.end' would be more efficient and a bit more explicit IMO.

Sure, I will make change in a follow up submission.

Quoted 17 lines

Quoted 5 lines

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

  	/* map all the memory banks */
  	for_each_mem_range(i, &start, &end) {
  		if (start >= end)

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgdp) · line 571

Quoted 10 lines

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

@@ -554,6 +562,20 @@ static void __init map_mem(pgd_t *pgdp)
  	__map_memblock(pgdp, kernel_start, kernel_end,
  		       PAGE_KERNEL, NO_CONT_MAPPINGS);
  	memblock_clear_nomap(kernel_start, kernel_end - kernel_start);
+#ifdef CONFIG_KEXEC_CORE
+	/*
+	 * Use page-level mappings here so that we can shrink the region
+	 * in page granularity and put back unused memory to buddy system
+	 * through /sys/kernel/kexec_crash_size interface.
+	 */
+	if (crashk_res.end) {

Same here.

Yes.

Quoted 13 lines

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgdp) · line 578
Quoted 8 lines
arch/arm64/mm/mmu.c +7 -0
+		__map_memblock(pgdp, crashk_res.start, crashk_res.end + 1,
+			       PAGE_KERNEL,
+			       NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
+		memblock_clear_nomap(crashk_res.start,
+				     resource_size(&crashk_res));
+	}
+#endif
Now, I carefully reviewed the patch and it seems to be doing the right thing. But even while knowlegable on the topic, it took a good amount of effort to untangle the possible code paths. I suspect it's going to be painful to maintain. I'd suggest at least introducing a comment explaining the situation.

I appreciate your review. Yes, it took a good amount of time for me (new here) too and glad for your notice. Let me take a shot at explaining in my next revision.

Quoted 2 lines

If there approach if deemed acceptable, I'll test is on the RPi4.

Please, your testing on RPi4 would be valuable.

Thanks,
Vijay

Quoted 3 lines

Regards,
Nicolas