Kexec_file: Load kernel at top of system ram

From: AKASHI Takahiro <[email protected]>

This function, being a variant of walk_system_ram_res() introduced in commit 8c86e70acead ("resource: provide new functions to walk through resources"), walks through a list of all the resources of System RAM in reversed order, i.e., from higher to lower.

It will be used in kexec_file code.

...

kernel/resource.c +2 -0

@@ -23,6 +23,8 @@
 #include <linux/pfn.h>
 #include <linux/mm.h>
 #include <linux/resource_ext.h>
+#include <linux/string.h>
+#include <linux/vmalloc.h>
 #include <asm/io.h>
 
 

kernel/resource.c +39 -0

+int walk_system_ram_res_rev(u64 start, u64 end, void *arg,
+				int (*func)(struct resource *, void *))
+{
+	struct resource res, *rams;
+	int rams_size = 16, i;
+	int ret = -1;
+
+	/* create a list */
+	rams = vmalloc(sizeof(struct resource) * rams_size);
+	if (!rams)
+		return ret;
+
+	res.start = start;
+	res.end = end;
+	res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
+	i = 0;
+	while ((res.start < res.end) &&
+		(!find_next_iomem_res(&res, IORES_DESC_NONE, true))) {
+		if (i >= rams_size) {
+			/* re-alloc */
+			struct resource *rams_new;
+			int rams_new_size;
+
+			rams_new_size = rams_size + 16;
+			rams_new = vmalloc(sizeof(struct resource)
+							* rams_new_size);
+			if (!rams_new)
+				goto out;
+
+			memcpy(rams_new, rams,
+					sizeof(struct resource) * rams_size);
+			vfree(rams);
+			rams = rams_new;
+			rams_size = rams_new_size;
+		}
+
+		rams[i].start = res.start;
+		rams[i++].end = res.end;
+

kernel/resource.c +4 -0

+out:
+	vfree(rams);
+	return ret;
+}

Quoted 6 lines

/*
+ * This function, being a variant of walk_system_ram_res(), calls the @func
+ * callback against all memory ranges of type System RAM which are marked as
+ * IORESOURCE_SYSTEM_RAM and IORESOUCE_BUSY in reversed order, i.e., from
+ * higher to lower.
+ */

This should document the return value, as should walk_system_ram_res(). Why does it return -1 on error rather than an errno (ENOMEM)?

Quoted 44 lines

kernel/resource.c +43 -0

+int walk_system_ram_res_rev(u64 start, u64 end, void *arg,
+				int (*func)(struct resource *, void *))
+{
+	struct resource res, *rams;
+	int rams_size = 16, i;
+	int ret = -1;
+
+	/* create a list */
+	rams = vmalloc(sizeof(struct resource) * rams_size);
+	if (!rams)
+		return ret;
+
+	res.start = start;
+	res.end = end;
+	res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
+	i = 0;
+	while ((res.start < res.end) &&
+		(!find_next_iomem_res(&res, IORES_DESC_NONE, true))) {
+		if (i >= rams_size) {
+			/* re-alloc */
+			struct resource *rams_new;
+			int rams_new_size;
+
+			rams_new_size = rams_size + 16;
+			rams_new = vmalloc(sizeof(struct resource)
+							* rams_new_size);
+			if (!rams_new)
+				goto out;
+
+			memcpy(rams_new, rams,
+					sizeof(struct resource) * rams_size);
+			vfree(rams);
+			rams = rams_new;
+			rams_size = rams_new_size;
+		}
+
+		rams[i].start = res.start;
+		rams[i++].end = res.end;
+
+		res.start = res.end + 1;
+		res.end = end;
+	}
+

kernel/resource.c int walk_system_ram_res(u64 start, u64 end, void *arg, · line 528

Quoted 6 lines

kernel/resource.c +6 -0

+	/* go reverse */
+	for (i--; i >= 0; i--) {
+		ret = (*func)(&rams[i], arg);
+		if (ret)
+			break;
+	}

erk, this is pretty nasty. Isn't there a better way :(

Quoted 5 lines

kernel/resource.c +4 -0

+out:
+	vfree(rams);
+	return ret;
+}

Quoted 1 line

erk, this is pretty nasty. Isn't there a better way :(

Yes, this is not efficient.

In struct resource{}, ->sibling list is a singly linked list. I ever thought about changing it to doubly linked list, yet not very sure if it will have effect since struct resource is a core data structure.

AKASHI's method is more acceptable, and currently only kexec has this requirement.

Quoted 4 lines · On Fri, 23 Mar 2018 08:58:45 +0800 Baoquan He <[email protected]> wrote:

Quoted 1 line

erk, this is pretty nasty. Isn't there a better way :(

Yes, this is not efficient.

Quoted 3 lines

In struct resource{}, ->sibling list is a singly linked list. I ever thought about changing it to doubly linked list, yet not very sure if it will have effect since struct resource is a core data structure.

Switching to a list_head sounds OK. The only issue really is memory consumption and surely we don't have tens of thousands of struct resources floating about(?). Or if we do have a lot, the machine is presumably huge (hope?).

Quoted 3 lines

AKASHI's method is more acceptable, and currently only kexec has this requirement.

What method is that?

Quoted 6 lines · Andrew Morton

Quoted 4 lines · On Fri, 23 Mar 2018 08:58:45 +0800 Baoquan He <[email protected]> wrote:

Quoted 1 line

erk, this is pretty nasty. Isn't there a better way :(

Yes, this is not efficient.

Quoted 8 lines

Quoted 3 lines

In struct resource{}, ->sibling list is a singly linked list. I ever thought about changing it to doubly linked list, yet not very sure if it will have effect since struct resource is a core data structure.

Switching to a list_head sounds OK. The only issue really is memory consumption and surely we don't have tens of thousands of struct resources floating about(?). Or if we do have a lot, the machine is presumably huge (hope?).

Yes. It doubles the memory consumption.

AFAIK, the biggest number of resrouces I heard of possibly is mentioned in this user space kexec_tools commit. In this commit, Xunlei told on SGI system with 64TB RAM, the array which we have been using to store "System RAM"|"Reserved"|"ACPI **" regions is not big enough. In that case, we need extra 8Byte*2048=16KB at most. With my understanding, this increase is system wide, since each resource instance only needs its own list_head member, right?

Quoted 18 lines · On Fri, 23 Mar 2018 11:10:13 +0800 Baoquan He <[email protected]> wrote:

Quoted 6 lines · Andrew Morton

Quoted 4 lines · On Fri, 23 Mar 2018 08:58:45 +0800 Baoquan He <[email protected]> wrote:

Quoted 1 line

erk, this is pretty nasty. Isn't there a better way :(

Yes, this is not efficient.

Quoted 8 lines

Quoted 3 lines

In struct resource{}, ->sibling list is a singly linked list. I ever thought about changing it to doubly linked list, yet not very sure if it will have effect since struct resource is a core data structure.

Switching to a list_head sounds OK. The only issue really is memory consumption and surely we don't have tens of thousands of struct resources floating about(?). Or if we do have a lot, the machine is presumably huge (hope?).

Yes. It doubles the memory consumption.

Quoted 7 lines

AFAIK, the biggest number of resrouces I heard of possibly is mentioned in this user space kexec_tools commit. In this commit, Xunlei told on SGI system with 64TB RAM, the array which we have been using to store "System RAM"|"Reserved"|"ACPI **" regions is not big enough. In that case, we need extra 8Byte*2048=16KB at most. With my understanding, this increase is system wide, since each resource instance only needs its own list_head member, right?

Yes. That sounds perfectly acceptable.

It would be interesting to see what this approach looks like, if you have time to toss something together?

Quoted 5 lines

Yes. That sounds perfectly acceptable.

It would be interesting to see what this approach looks like, if you have time to toss something together?

OK, will make patches for reviewing. Thanks!

From b36d50487f1d4e4d6a5103965a27101b3121e0ea Mon Sep 17 00:00:00 2001

Quoted 6 lines · Baoquan He

Quoted 5 lines

Yes. That sounds perfectly acceptable.

It would be interesting to see what this approach looks like, if you have time to toss something together?

Quoted 1 line

OK, will make patches for reviewing. Thanks!

Hi, Baoquan, Andrew

I have come up with an implementation for top-down search the ram resources. Hope this would meet your need.

From b36d50487f1d4e4d6a5103965a27101b3121e0ea Mon Sep 17 00:00:00 2001
From: Wei Yang <[email protected]>
Date: Sat, 24 Mar 2018 23:25:46 +0800
Subject: [PATCH] kernel/resource: add walk_system_ram_res_rev()

As discussed on https://patchwork.kernel.org/patch/10300819/, this patch comes up with a variant implementation of walk_system_ram_res_rev(), which uses iteration instead of allocating array to store those resources.

Signed-off-by Wei Yang <[email protected]>

---
 include/linux/ioport.h |   3 ++
 kernel/resource.c      | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

include/linux/ioport.h +3 -0

index da0ebaec25f0..473f1d9cb97e 100644
@@ -277,6 +277,9 @@ extern int
 walk_system_ram_res(u64 start, u64 end, void *arg,
 		    int (*func)(struct resource *, void *));
 extern int
+walk_system_ram_res_rev(u64 start, u64 end, void *arg,
+		    int (*func)(struct resource *, void *));
+extern int
 walk_iomem_res_desc(unsigned long desc, unsigned long flags, u64 start, u64 end,
 		    void *arg, int (*func)(struct resource *, void *));
 

kernel/resource.c +113 -0

index 769109f20fb7..ddf6b4c41498 100644
@@ -73,6 +73,38 @@ static struct resource *next_resource(struct resource *p, bool sibling_only)
 	return p->sibling;
 }
 
+static struct resource *prev_resource(struct resource *p, bool sibling_only)
+{
+	struct resource *prev;
+	if (NULL == iomem_resource.child)
+		return NULL;
+
+	if (p == NULL) {
+		prev = iomem_resource.child;
+		while (prev->sibling)
+			prev = prev->sibling;
+	} else {
+		if (p->parent->child == p) {
+			return p->parent;
+		}
+
+		for (prev = p->parent->child; prev->sibling != p;
+			prev = prev->sibling) {}
+	}
+
+	/* Caller wants to traverse through siblings only */
+	if (sibling_only)
+		return prev;
+
+	for (;prev->child;) {
+		prev = prev->child;
+
+		while (prev->sibling)
+			prev = prev->sibling;
+	}
+	return prev;
+}
+
 static void *r_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct resource *p = v;

@@ -401,6 +433,47 @@ static int find_next_iomem_res(struct resource *res, unsigned long desc,
 	return 0;
 }
 
+/*
+ * Finds the highest iomem resource existing within [res->start.res->end).
+ * The caller must specify res->start, res->end, res->flags, and optionally
+ * desc.  If found, returns 0, res is overwritten, if not found, returns -1.
+ * This function walks the whole tree and not just first level children until
+ * and unless first_level_children_only is true.
+ */
+static int find_prev_iomem_res(struct resource *res, unsigned long desc,
+			       bool first_level_children_only)
+{
+	struct resource *p;
+
+	BUG_ON(!res);
+	BUG_ON(res->start >= res->end);
+
+	read_lock(&resource_lock);
+
+	for (p = prev_resource(NULL, first_level_children_only); p;
+		p = prev_resource(p, first_level_children_only)) {
+		if ((p->flags & res->flags) != res->flags)
+			continue;
+		if ((desc != IORES_DESC_NONE) && (desc != p->desc))
+			continue;
+		if (p->end < res->start) {
+			p = NULL;
+			break;
+		}
+		if ((p->end >= res->start) && (p->start < res->end))
+			break;
+	}
+
+	read_unlock(&resource_lock);
+	if (!p)
+		return -1;
+	/* copy data */
+	resource_clip(res, p->start, p->end);
+	res->flags = p->flags;
+	res->desc = p->desc;
+	return 0;
+}
+
 static int __walk_iomem_res_desc(struct resource *res, unsigned long desc,
 				 bool first_level_children_only,
 				 void *arg,

@@ -422,6 +495,27 @@ static int __walk_iomem_res_desc(struct resource *res, unsigned long desc,
 	return ret;
 }
 
+static int __walk_iomem_res_rev_desc(struct resource *res, unsigned long desc,
+				 bool first_level_children_only,
+				 void *arg,
+				 int (*func)(struct resource *, void *))
+{
+	u64 orig_start = res->start;
+	int ret = -1;
+
+	while ((res->start < res->end) &&
+	       !find_prev_iomem_res(res, desc, first_level_children_only)) {
+		ret = (*func)(res, arg);
+		if (ret)
+			break;
+
+		res->end = res->start - 1;
+		res->start = orig_start;
+	}
+
+	return ret;
+}
+
 /*
  * Walks through iomem resources and calls func() with matching resource
  * ranges. This walks through whole tree and not just first level children.

@@ -468,6 +562,25 @@ int walk_system_ram_res(u64 start, u64 end, void *arg,
 				     arg, func);
 }
 
+/*
+ * This function, being a variant of walk_system_ram_res(), calls the @func
+ * callback against all memory ranges of type System RAM which are marked as
+ * IORESOURCE_SYSTEM_RAM and IORESOUCE_BUSY in reversed order, i.e., from
+ * higher to lower.
+ */
+int walk_system_ram_res_rev(u64 start, u64 end, void *arg,
+				int (*func)(struct resource *, void *))
+{
+	struct resource res;
+
+	res.start = start;
+	res.end = end;
+	res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
+
+	return __walk_iomem_res_rev_desc(&res, IORES_DESC_NONE, true,
+				     arg, func);
+}
+
 /*
  * This function calls the @func callback against all memory ranges, which
  * are ranges marked as IORESOURCE_MEM and IORESOUCE_BUSY.

Signature · 6 lines

2.15.1


-- 
Wei Yang
Help you, Help me

Hi Wei Yang,

Quoted 7 lines · Wei Yang

Quoted 6 lines · Baoquan He

Quoted 5 lines

Yes. That sounds perfectly acceptable.

It would be interesting to see what this approach looks like, if you have time to toss something together?

Quoted 6 lines

Quoted 1 line

OK, will make patches for reviewing. Thanks!

Hi, Baoquan, Andrew

I have come up with an implementation for top-down search the ram resources. Hope this would meet your need.

Thanks for telling and your effort. Glad to know
I am not the only buyer of walk_system_ram_res_rev. I am fine with other ways to make it, people can compare them and know which one is better. I am working to use the list_head instead, the doubly linked list way as Andrew suggested. Andrew and other people can help make a choice. It won't be long.

Thanks
Baoquan

The current kexec_file ignores kexec_buf.top_down value when call arch_kexec_walk_mem() to allocate memory for loading kernel/initrd stuffs. This is not supposed to be what kexec_buf.top_down is used for.

Quoted 4 lines · On Thu, 22 Mar 2018 11:37:20 +0800 Baoquan He <[email protected]> wrote:

The current kexec_file ignores kexec_buf.top_down value when call arch_kexec_walk_mem() to allocate memory for loading kernel/initrd stuffs. This is not supposed to be what kexec_buf.top_down is used for.

OK, but why is this a problem? When fixing a bug, please fully describe the user-visible effects of that bug. That helps others understand the importance of the fix, whether it should be backported into various kernels, etc.