s390/mm: don't set ARCH_KEEP_MEMBLOCK

RFC PATCH Applied David Hildenbrand 3 participants 10 messages Apr 17, 2020 – Jun 29, 2020

Subsystems (2)

Memory management s390

lore

Share & tools

lore

David Hildenbrand

author

2020-04-17 15:02 UTC #0 lore raw

cover s390/mm: don't set ARCH_KEEP_MEMBLOCK

Looking into why we still create memblocks for hotplugged memory (via add_memory()), I discovered that we might not need ARCH_KEEP_MEMBLOCK on s390x after all.

After [1] we will no longer create memblocks for hotplugged memory in case of !CONFIG_ARCH_KEEP_MEMBLOCK. With this series, the same will apply to standby memory on s390x, added via add_memory().

[1] https://lkml.kernel.org/r/[email protected]

David Hildenbrand (2):
s390/zcore: traverse resources instead of memblocks s390/mm: don't set ARCH_KEEP_MEMBLOCK

 arch/s390/Kconfig         |  1 -
 drivers/s390/char/zcore.c | 61 ++++++++++++++++++++++++++++++---------
 2 files changed, 48 insertions(+), 14 deletions(-)

Signature · 1 line

2.25.1

David Hildenbrand

author

2020-04-17 15:01 UTC #1 lore raw

RFC PATCH 1/2 s390/zcore: traverse resources instead of memblocks

The zcore memmap basically contains the first level of all system RAM from /proc/iomem. We want to disable CONFIG_ARCH_KEEP_MEMBLOCK (e.g., to not create memblocks for hotplugged/standby memory and save space), switch to traversing system ram resources instead. During early boot, we create resources for all early memblocks (including the crash kernel area). When adding standby memory, we currently create both, memblocks and resources.

Note: As we don't have memory hotplug after boot (standby memory is added via sclp during boot), we don't have to worry about races.

I am only able to test under KVM (where I hacked up zcore to still create the memmap file)

root@vm0:~# cat /proc/iomem
00000000-2fffffff : System RAM
  10424000-10ec6fff : Kernel code
  10ec7000-1139a0e3 : Kernel data
  1177a000-11850fff : Kernel bss
30000000-3fffffff : Crash kernel

Result without this patch:

root@vm0:~# cat /sys/kernel/debug/zcore/memmap
0000000000000000 0000000040000000

Result with this patch:

root@vm0:~# cat /sys/kernel/debug/zcore/memmap
0000000000000000 0000000030000000 0000000030000000 0000000010000000

The difference is due to memblocks getting merged, resources (currently) not. So we might have some more entries, but they describe the same memory map.

Cc Heiko Carstens <[email protected]>

Cc Vasily Gorbik <[email protected]>

Cc Christian Borntraeger <[email protected]>

Cc Philipp Rudo <[email protected]>

Cc Kirill Smelkov <[email protected]>

Cc Michael Holzheu <[email protected]>

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

---
 drivers/s390/char/zcore.c | 61 ++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 13 deletions(-)

drivers/s390/char/zcore.c +48 ~~-13~~

index 08f812475f5e..c40ac7d548d8 100644
@@ -16,7 +16,7 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/debugfs.h>
-#include <linux/memblock.h>
+#include <linux/ioport.h>
 
 #include <asm/asm-offsets.h>
 #include <asm/ipl.h>

@@ -139,35 +139,70 @@ static void release_hsa(void)
 	hsa_available = 0;
 }
 
+struct zcore_memmap_info {
+	char *buf;
+	size_t length;
+};
+
 static ssize_t zcore_memmap_read(struct file *filp, char __user *buf,
 				 size_t count, loff_t *ppos)
 {
-	return simple_read_from_buffer(buf, count, ppos, filp->private_data,
-				       memblock.memory.cnt * CHUNK_INFO_SIZE);
+	struct zcore_memmap_info *info = filp->private_data;
+
+	return simple_read_from_buffer(buf, count, ppos, info->buf,
+				       info->length);
+}
+
+static int zcore_count_ram_resource(struct resource *res, void *arg)
+{
+	size_t *count = arg;
+
+	*count += 1;
+	return 0;
+}
+
+static int zcore_process_ram_resource(struct resource *res, void *arg)
+{
+	char **buf = arg;
+
+	sprintf(*buf, "%016lx %016lx ", (unsigned long) res->start,
+		(unsigned long) resource_size(res));
+
+	*buf += CHUNK_INFO_SIZE;
+	return 0;
 }
 
 static int zcore_memmap_open(struct inode *inode, struct file *filp)
 {
-	struct memblock_region *reg;
+	struct zcore_memmap_info *info;
+	size_t count = 0;
 	char *buf;
-	int i = 0;
 
-	buf = kcalloc(memblock.memory.cnt, CHUNK_INFO_SIZE, GFP_KERNEL);
+	walk_system_ram_res(0, ULONG_MAX, &count, zcore_count_ram_resource);
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	buf = kcalloc(count, CHUNK_INFO_SIZE, GFP_KERNEL);
 	if (!buf) {
+		kfree(info);
 		return -ENOMEM;
 	}
-	for_each_memblock(memory, reg) {
-		sprintf(buf + (i++ * CHUNK_INFO_SIZE), "%016llx %016llx ",
-			(unsigned long long) reg->base,
-			(unsigned long long) reg->size);
-	}
-	filp->private_data = buf;
+	info->length = count * CHUNK_INFO_SIZE;
+	info->buf = buf;
+
+	walk_system_ram_res(0, ULONG_MAX, &buf, zcore_process_ram_resource);
+
+	filp->private_data = info;
 	return nonseekable_open(inode, filp);
 }
 
 static int zcore_memmap_release(struct inode *inode, struct file *filp)
 {
-	kfree(filp->private_data);
+	struct zcore_memmap_info *info = filp->private_data;
+
+	kfree(info->buf);
+	kfree(info);
 	return 0;
 }

Signature · 1 line

2.25.1

Heiko Carstens

2020-06-10 11:46 UTC #2 lore raw

Re: [PATCH RFC 1/2] s390/zcore: traverse resources instead of memblocks

Quoted 39 lines · David Hildenbrand

The zcore memmap basically contains the first level of all system RAM from /proc/iomem. We want to disable CONFIG_ARCH_KEEP_MEMBLOCK (e.g., to not create memblocks for hotplugged/standby memory and save space), switch to traversing system ram resources instead. During early boot, we create resources for all early memblocks (including the crash kernel area). When adding standby memory, we currently create both, memblocks and resources.
Note: As we don't have memory hotplug after boot (standby memory is added via sclp during boot), we don't have to worry about races.
I am only able to test under KVM (where I hacked up zcore to still create the memmap file)
root@vm0:~# cat /proc/iomem
00000000-2fffffff : System RAM
  10424000-10ec6fff : Kernel code
  10ec7000-1139a0e3 : Kernel data
  1177a000-11850fff : Kernel bss
30000000-3fffffff : Crash kernel
Result without this patch:
root@vm0:~# cat /sys/kernel/debug/zcore/memmap
0000000000000000 0000000040000000
Result with this patch:
root@vm0:~# cat /sys/kernel/debug/zcore/memmap
0000000000000000 0000000030000000 0000000030000000 0000000010000000
The difference is due to memblocks getting merged, resources (currently) not. So we might have some more entries, but they describe the same memory map.
Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Philipp Rudo <[email protected]>
Cc Kirill Smelkov <[email protected]>
Cc Michael Holzheu <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>

Quoted 3 lines

---
 drivers/s390/char/zcore.c | 61 ++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 13 deletions(-)

I'm having a hard time to find any code that ever made use of this file. After all this was only meant for our zfcp dumper, but as far as I can tell there was never code out there that read the memmap file.

So the pragmatic approach would be to just change its contents, or a more progressive variant would be to remove the file completely. But maybe I'm entirely wrong...

I'm leaving this up to Philipp and Alexander :)

Philipp Rudo

2020-06-19 12:47 UTC #3 lore raw

Re: [PATCH RFC 1/2] s390/zcore: traverse resources instead of memblocks

Hi David,

zcore/memmap is no longer needed and Alexander is preparing a patch to remove it. You can drop this patch.

Thanks
Philipp

Quoted 53 lines · On Wed, 10 Jun 2020 13:45:23 +0200 Heiko Carstens <[email protected]>…

Quoted 39 lines · David Hildenbrand
The zcore memmap basically contains the first level of all system RAM from /proc/iomem. We want to disable CONFIG_ARCH_KEEP_MEMBLOCK (e.g., to not create memblocks for hotplugged/standby memory and save space), switch to traversing system ram resources instead. During early boot, we create resources for all early memblocks (including the crash kernel area). When adding standby memory, we currently create both, memblocks and resources.
Note: As we don't have memory hotplug after boot (standby memory is added via sclp during boot), we don't have to worry about races.
I am only able to test under KVM (where I hacked up zcore to still create the memmap file)
root@vm0:~# cat /proc/iomem
00000000-2fffffff : System RAM
  10424000-10ec6fff : Kernel code
  10ec7000-1139a0e3 : Kernel data
  1177a000-11850fff : Kernel bss
30000000-3fffffff : Crash kernel
Result without this patch:
root@vm0:~# cat /sys/kernel/debug/zcore/memmap
0000000000000000 0000000040000000
Result with this patch:
root@vm0:~# cat /sys/kernel/debug/zcore/memmap
0000000000000000 0000000030000000 0000000030000000 0000000010000000
The difference is due to memblocks getting merged, resources (currently) not. So we might have some more entries, but they describe the same memory map.
Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Philipp Rudo <[email protected]>
Cc Kirill Smelkov <[email protected]>
Cc Michael Holzheu <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
Quoted 3 lines
---
 drivers/s390/char/zcore.c | 61 ++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 13 deletions(-)  
I'm having a hard time to find any code that ever made use of this file. After all this was only meant for our zfcp dumper, but as far as I can tell there was never code out there that read the memmap file.
So the pragmatic approach would be to just change its contents, or a more progressive variant would be to remove the file completely. But maybe I'm entirely wrong...
I'm leaving this up to Philipp and Alexander :)

David Hildenbrand

author

2020-06-19 12:49 UTC #4 lore raw

Re: [PATCH RFC 1/2] s390/zcore: traverse resources instead of memblocks

Quoted 4 lines · Philipp Rudo

Hi David,
zcore/memmap is no longer needed and Alexander is preparing a patch to remove it. You can drop this patch.

Awesome, once that's done, can you consider patch #2? Thanks!

Signature · 3 lines

Thanks,

David / dhildenb

David Hildenbrand

author

2020-04-17 15:02 UTC #5 lore raw

RFC PATCH 2/2 s390/mm: don't set ARCH_KEEP_MEMBLOCK

In reply to David Hildenbrand

Commit 50be63450728 ("s390/mm: Convert bootmem to memblock") mentions
"The original bootmem allocator is getting replaced by memblock. To cover the needs of the s390 kdump implementation the physical memory list is used."

zcore was converted to use resources instead of memblocks. memblock_discard() will *not* mess with "physmem", only with "memory" and "reserved" memblocks. So, that data will stay after early boot, to be used in arch/s390/kernel/crash_dump.c to create the ELF header from inside the 2nd (a.k.a. dumping) kernel.

We no longer need ARCH_KEEP_MEMBLOCK.

Cc Heiko Carstens <[email protected]>

Cc Vasily Gorbik <[email protected]>

Cc Christian Borntraeger <[email protected]>

Cc Philipp Rudo <[email protected]>

Cc Michael Holzheu <[email protected]>

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

---
 arch/s390/Kconfig | 1 -
 1 file changed, 1 deletion(-)

arch/s390/Kconfig +0 -1

index 8206b2c19aa8..122bf4cad8a5 100644
@@ -102,7 +102,6 @@ config S390
 	select ARCH_INLINE_WRITE_UNLOCK_BH
 	select ARCH_INLINE_WRITE_UNLOCK_IRQ
 	select ARCH_INLINE_WRITE_UNLOCK_IRQRESTORE
-	select ARCH_KEEP_MEMBLOCK
 	select ARCH_STACKWALK
 	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_SUPPORTS_NUMA_BALANCING

Signature · 1 line

2.25.1

Heiko Carstens

Applied

2020-06-26 16:32 UTC #6 lore raw

Quoted 19 lines · David Hildenbrand

Commit 50be63450728 ("s390/mm: Convert bootmem to memblock") mentions
"The original bootmem allocator is getting replaced by memblock. To cover the needs of the s390 kdump implementation the physical memory list is used."
zcore was converted to use resources instead of memblocks. memblock_discard() will *not* mess with "physmem", only with "memory" and "reserved" memblocks. So, that data will stay after early boot, to be used in arch/s390/kernel/crash_dump.c to create the ELF header from inside the 2nd (a.k.a. dumping) kernel.
We no longer need ARCH_KEEP_MEMBLOCK.
Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Philipp Rudo <[email protected]>
Cc Michael Holzheu <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>

Quoted 3 lines

---
 arch/s390/Kconfig | 1 -
 1 file changed, 1 deletion(-)

Applied, thanks!

Heiko Carstens

2020-06-29 12:44 UTC #7 lore raw

Quoted 24 lines · Heiko Carstens

Quoted 19 lines · David Hildenbrand
Commit 50be63450728 ("s390/mm: Convert bootmem to memblock") mentions
"The original bootmem allocator is getting replaced by memblock. To cover the needs of the s390 kdump implementation the physical memory list is used."
zcore was converted to use resources instead of memblocks. memblock_discard() will *not* mess with "physmem", only with "memory" and "reserved" memblocks. So, that data will stay after early boot, to be used in arch/s390/kernel/crash_dump.c to create the ELF header from inside the 2nd (a.k.a. dumping) kernel.
We no longer need ARCH_KEEP_MEMBLOCK.
Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Philipp Rudo <[email protected]>
Cc Michael Holzheu <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
Quoted 3 lines
---
 arch/s390/Kconfig | 1 -
 1 file changed, 1 deletion(-)

Quoted 1 line

Applied, thanks!

Hmm, this triggers:

WARNING: modpost: vmlinux.o(.text+0x1e7c2): Section mismatch in reference from the function elfcorehdr_alloc() to the variable .meminit.data:memblock The function elfcorehdr_alloc() references
the variable __meminitdata memblock.
This is often because elfcorehdr_alloc lacks a __meminitdata
annotation or the annotation of memblock is wrong.

WARNING: modpost: vmlinux.o(.text+0x1e800): Section mismatch in reference from the function elfcorehdr_alloc() to the function .meminit.text:__next_mem_range() The function elfcorehdr_alloc() references
the function __meminit __next_mem_range().
This is often because elfcorehdr_alloc lacks a __meminit
annotation or the annotation of __next_mem_range is wrong.

WARNING: modpost: vmlinux.o(.text+0x1e844): Section mismatch in reference from the function elfcorehdr_alloc() to the function .meminit.text:__next_mem_range() The function elfcorehdr_alloc() references
the function __meminit __next_mem_range().
This is often because elfcorehdr_alloc lacks a __meminit
annotation or the annotation of __next_mem_range is wrong.

So, reverted again ;)

David Hildenbrand

author

2020-06-29 13:04 UTC #8 lore raw

Quoted 25 lines · Heiko Carstens

Quoted 24 lines · Heiko Carstens
Quoted 22 lines · David Hildenbrand
Commit 50be63450728 ("s390/mm: Convert bootmem to memblock") mentions
"The original bootmem allocator is getting replaced by memblock. To cover the needs of the s390 kdump implementation the physical memory list is used."
zcore was converted to use resources instead of memblocks. memblock_discard() will *not* mess with "physmem", only with "memory" and "reserved" memblocks. So, that data will stay after early boot, to be used in arch/s390/kernel/crash_dump.c to create the ELF header from inside the 2nd (a.k.a. dumping) kernel.
We no longer need ARCH_KEEP_MEMBLOCK.
Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Philipp Rudo <[email protected]>
Cc Michael Holzheu <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/Kconfig | 1 -
 1 file changed, 1 deletion(-)

Quoted 3 lines

Quoted 1 line
Applied, thanks!
Hmm, this triggers:

Ah, I see, will have a look. Weird I didn't notice that ...

Thanks!

Signature · 3 lines

Thanks,

David / dhildenb

David Hildenbrand

author

2020-04-29 14:56 UTC #9 lore raw

In reply to David Hildenbrand

Quoted 10 lines · David Hildenbrand

Looking into why we still create memblocks for hotplugged memory (via add_memory()), I discovered that we might not need ARCH_KEEP_MEMBLOCK on s390x after all.
After [1] we will no longer create memblocks for hotplugged memory in case of !CONFIG_ARCH_KEEP_MEMBLOCK. With this series, the same will apply to standby memory on s390x, added via add_memory().
[1] https://lkml.kernel.org/r/[email protected]

Quoted 8 lines

David Hildenbrand (2):
s390/zcore: traverse resources instead of memblocks s390/mm: don't set ARCH_KEEP_MEMBLOCK
 arch/s390/Kconfig         |  1 -
 drivers/s390/char/zcore.c | 61 ++++++++++++++++++++++++++++++---------
 2 files changed, 48 insertions(+), 14 deletions(-)

Ping.

Signature · 3 lines

Thanks,

David / dhildenb