2020-04-17 15:04:47

by David Hildenbrand

[permalink] [raw]
Subject: [PATCH RFC 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(-)

diff --git a/drivers/s390/char/zcore.c b/drivers/s390/char/zcore.c
index 08f812475f5e..c40ac7d548d8 100644
--- a/drivers/s390/char/zcore.c
+++ b/drivers/s390/char/zcore.c
@@ -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;
}

--
2.25.1


2020-06-10 11:49:22

by Heiko Carstens

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

On Fri, Apr 17, 2020 at 05:01:50PM +0200, David Hildenbrand wrote:
> 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(-)

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 :)

2020-06-19 13:04:08

by David Hildenbrand

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

On 19.06.20 14:47, Philipp Rudo wrote:
> 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!

--
Thanks,

David / dhildenb

2020-06-19 18:25:32

by Philipp Rudo

[permalink] [raw]
Subject: 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

On Wed, 10 Jun 2020 13:45:23 +0200
Heiko Carstens <[email protected]> wrote:

> On Fri, Apr 17, 2020 at 05:01:50PM +0200, David Hildenbrand wrote:
> > 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(-)
>
> 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 :)