Revert "kexec: use mmap instead of read for slurp_file()"

This reverts commit 7ab842d8a004f6cd75a9d7b3528e4a70819ce4ef.

using mmap by default in slurp_file cause segment fault while later reallocing dtb_buf during my arm kexec test.

Quoted 4 lines · On Fri, 23 Oct 2015 11:10:00 +0800 Dave Young <[email protected]> wrote:

This reverts commit 7ab842d8a004f6cd75a9d7b3528e4a70819ce4ef.

using mmap by default in slurp_file cause segment fault while later reallocing dtb_buf during my arm kexec test.

Sorry, I obviously missed that part.

How can we fix that:

- Create a separate function slurp_file_mmap() that is called by s390x?
- Rework xmalloc/xrealloc to always use mmap() and mremap()?
- ...

IMHO adding a slurp_file_mmap for s390x use is a better way since the huge initramfs is not a general case.

kexec/kexec.c +1 -0

@@ -552,11 +563,18 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread, use_mmap);

Hi, Michael

Quoted 5 lines

kexec/kexec.c +1 -0

@@ -552,11 +563,18 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread, use_mmap);

kexec/kexec.c char *slurp_file(const char *filename, o · line 576

Quoted 10 lines

kexec/kexec.c +7 -0

 	} else {
 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ | PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);
+			nread = stats.st_size;
+		} else {
+			buf = slurp_fd(fd, filename, size, &nread, 0);
+		}
 	}

Drop above changes and replace below lines with an extra use_mmap argument should be enough?

Quoted 21 lines · On Wed, 28 Oct 2015 14:46:23 +0800 Dave Young <[email protected]> wrote:

Hi, Michael

Quoted 5 lines

kexec/kexec.c +1 -0

@@ -552,11 +563,18 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread, use_mmap);

kexec/kexec.c char *slurp_file(const char *filename, o · line 576

Quoted 10 lines

kexec/kexec.c +7 -0

 	} else {
 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ | PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);
+			nread = stats.st_size;
+		} else {
+			buf = slurp_fd(fd, filename, size, &nread, 0);
+		}
 	}

Drop above changes and replace below lines with an extra use_mmap argument should be enough?

kexec/kexec.c char *slurp_file(const char *filename, o · line 559

Quoted 2 lines

kexec/kexec.c +0 -1

-	buf = slurp_fd(fd, filename, size, &nread);
[snip]

Hmm, I don't think so.

In case of non-character devices I either mmap the file directly (use_mmap=true) or use "slurp_fd()" (use_mmap=false). So I can't unconditionaly use slurp_fd().

Quoted 29 lines · Michael Holzheu

Quoted 21 lines · On Wed, 28 Oct 2015 14:46:23 +0800 Dave Young <[email protected]> wrote:

Hi, Michael

Quoted 5 lines

(quoted diff) +1 -0

@@ -552,11 +563,18 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread, use_mmap);

Review char *slurp_file(const char *filename, o · line 576

Quoted 10 lines

(quoted diff) +7 -0

 	} else {
 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ | PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);
+			nread = stats.st_size;
+		} else {
+			buf = slurp_fd(fd, filename, size, &nread, 0);
+		}
 	}

Drop above changes and replace below lines with an extra use_mmap argument should be enough?

Review

Quoted 2 lines

(quoted diff) +0 -1

-	buf = slurp_fd(fd, filename, size, &nread);
[snip]

Hmm, I don't think so.

Quoted 2 lines

In case of non-character devices I either mmap the file directly (use_mmap=true) or use "slurp_fd()" (use_mmap=false). So I can't unconditionaly use slurp_fd().

How about handle these in slurp_fd only? Directly return mmapped buf in case use_mmap=1 there. I do not understand why use_mmap=1 but you still call read syscall to read data into the mmapped buffer..

Quoted 33 lines · On Thu, 29 Oct 2015 14:37:10 +0800 Dave Young <[email protected]> wrote:

Quoted 31 lines · Michael Holzheu

Quoted 21 lines · On Wed, 28 Oct 2015 14:46:23 +0800 Dave Young <[email protected]> wrote:

Hi, Michael

Quoted 5 lines

kexec/kexec.c +1 -0

@@ -552,11 +563,18 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread, use_mmap);

kexec/kexec.c char *slurp_file(const char *filename, o · line 576

Quoted 10 lines

kexec/kexec.c +7 -0

 	} else {
 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ | PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);
+			nread = stats.st_size;
+		} else {
+			buf = slurp_fd(fd, filename, size, &nread, 0);
+		}
 	}

Drop above changes and replace below lines with an extra use_mmap argument should be enough?

kexec/kexec.c char *slurp_file(const char *filename, o · line 559

Quoted 2 lines

kexec/kexec.c +0 -1

-	buf = slurp_fd(fd, filename, size, &nread);
[snip]

Hmm, I don't think so.

In case of non-character devices I either mmap the file directly (use_mmap=true) or use "slurp_fd()" (use_mmap=false). So I can't unconditionaly use slurp_fd().

Quoted 3 lines

How about handle these in slurp_fd only? Directly return mmapped buf in case use_mmap=1 there. I do not understand why use_mmap=1 but you still call read syscall to read data into the mmapped buffer..

For the character device case (S_ISCHR(stats.st_mode)) we have to use the read() syscall path. With my patch I wanted to ensure that when calling slurp_file_mmap() we always return mmaped storage.

Otherwise slurp_file_mmap() would return mmaped storage for files and malloced memory for character devices.

As already noted this is only to be consistent and is not really required for our use case.

So would you prefer the patch below?

Michael

---
 kexec/arch/s390/kexec-image.c |    2 +-
 kexec/kexec.c                 |   24 +++++++++++++++++++++---
 kexec/kexec.h                 |    1 +
 3 files changed, 23 insertions(+), 4 deletions(-)

kexec/arch/s390/kexec-image.c +1 -1

@@ -101,7 +101,7 @@ image_s390_load(int argc, char **argv, c
 	 * we load the ramdisk directly behind the image with 1 MiB alignment.
 	 */
 	if (ramdisk) {
-		rd_buffer = slurp_file(ramdisk, &ramdisk_len);
+		rd_buffer = slurp_file_mmap(ramdisk, &ramdisk_len);
 		if (rd_buffer == NULL) {
 			fprintf(stderr, "Could not read ramdisk.\n");
 			return -1;

kexec/kexec.c +1 -0

@@ -29,6 +29,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/reboot.h>
+#include <sys/mman.h>
 #include <unistd.h>
 #include <fcntl.h>
 #ifndef _O_BINARY

kexec/kexec.c +14 -2

+			nread = size;
+		} else {
+			buf = slurp_fd(fd, filename, size, &nread);
+		}
 	}
-
-	buf = slurp_fd(fd, filename, size, &nread);
 	if (!buf)
 		die("Cannot read %s", filename);
 

@@ -567,6 +575,16 @@ char *slurp_file(const char *filename, o
 	return buf;
 }
 
+char *slurp_file(const char *filename, off_t *r_size)
+{
+	return slurp_file_generic(filename, r_size, 0);
+}
+
+char *slurp_file_mmap(const char *filename, off_t *r_size)
+{
+	return slurp_file_generic(filename, r_size, 1);
+}
+
 /* This functions reads either specified number of bytes from the file or
    lesser if EOF is met. */
 

Hi,

Quoted 25 lines

kexec/arch/s390/kexec-image.c +1 -1

@@ -101,7 +101,7 @@ image_s390_load(int argc, char **argv, c
 	 * we load the ramdisk directly behind the image with 1 MiB alignment.
 	 */
 	if (ramdisk) {
-		rd_buffer = slurp_file(ramdisk, &ramdisk_len);
+		rd_buffer = slurp_file_mmap(ramdisk, &ramdisk_len);
 		if (rd_buffer == NULL) {
 			fprintf(stderr, "Could not read ramdisk.\n");
 			return -1;

kexec/kexec.c +1 -0

@@ -29,6 +29,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/reboot.h>
+#include <sys/mman.h>
 #include <unistd.h>
 #include <fcntl.h>
 #ifndef _O_BINARY

@@ -514,7 +515,8 @@ static char *slurp_fd(int fd, const char
 	return buf;
 }
 

kexec/kexec.c static char *slurp_fd(int fd, const char · line 519

Quoted 3 lines

kexec/kexec.c +2 -1

-char *slurp_file(const char *filename, off_t *r_size)
+static char *slurp_file_generic(const char *filename, off_t *r_size,
+				int use_mmap)

Add a function comment about the argument use_mmap so that one knows that it will be not used for character devices?#

Quoted 5 lines

kexec/kexec.c +0 -0

 {
 	int fd;
 	char *buf;

kexec/kexec.c char *slurp_file(const char *filename, o · line 562

Quoted 9 lines

kexec/kexec.c +4 -0

@@ -552,11 +554,17 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread);
 	} else {
 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);

Probably map it as readonly is enough.

Quoted 6 lines

kexec/kexec.c char *slurp_file(const char *filename, o · line 562

Quoted 4 lines

kexec/kexec.c +3 -0

 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);

Probably map it as readonly is enough.

Although I agree with you for the ramdisk case, I nevertheless would prefer to keep it writable to have the same semantics as for the malloc case.

So do you agree with the patch below?

Michael

---
 kexec/arch/s390/kexec-image.c |    2 +-
 kexec/kexec.c                 |   31 ++++++++++++++++++++++++++++---
 kexec/kexec.h                 |    1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

kexec/arch/s390/kexec-image.c +1 -1

@@ -101,7 +101,7 @@ image_s390_load(int argc, char **argv, c
 	 * we load the ramdisk directly behind the image with 1 MiB alignment.
 	 */
 	if (ramdisk) {
-		rd_buffer = slurp_file(ramdisk, &ramdisk_len);
+		rd_buffer = slurp_file_mmap(ramdisk, &ramdisk_len);
 		if (rd_buffer == NULL) {
 			fprintf(stderr, "Could not read ramdisk.\n");
 			return -1;

kexec/kexec.c +28 -3

@@ -29,6 +29,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/reboot.h>
+#include <sys/mman.h>
 #include <unistd.h>
 #include <fcntl.h>
 #ifndef _O_BINARY

@@ -514,7 +515,8 @@ static char *slurp_fd(int fd, const char
 	return buf;
 }
 
-char *slurp_file(const char *filename, off_t *r_size)
+static char *slurp_file_generic(const char *filename, off_t *r_size,
+				int use_mmap)
 {
 	int fd;
 	char *buf;

@@ -552,11 +554,17 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread);
 	} else {
 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);
+			nread = size;
+		} else {
+			buf = slurp_fd(fd, filename, size, &nread);
+		}
 	}
-
-	buf = slurp_fd(fd, filename, size, &nread);
 	if (!buf)
 		die("Cannot read %s", filename);
 

@@ -567,6 +575,23 @@ char *slurp_file(const char *filename, o
 	return buf;
 }
 
+/*
+ * Read file into malloced buffer
+ */
+char *slurp_file(const char *filename, off_t *r_size)
+{
+	return slurp_file_generic(filename, r_size, 0);
+}
+
+/*
+ * Map "normal" file or read "character device" into malloced buffer.
+ * You must not use free, realloc, etc. with the returned buffer.
+ */
+char *slurp_file_mmap(const char *filename, off_t *r_size)
+{
+	return slurp_file_generic(filename, r_size, 1);
+}
+
 /* This functions reads either specified number of bytes from the file or
    lesser if EOF is met. */
 

The slurp_fd() function allocates memory and uses the read() system call. This results in double memory consumption for image and initrd:

1) Memory allocated in user space by the kexec tool
2) Memory allocated in kernel by the kexec() system call

The following illustrates the use case that we have on s390x:

1) Boot a 4 GB Linux system
2) Copy kernel and 1,5 GB ramdisk from external source into tmpfs (ram)
3) Use kexec to boot kernel with ramdisk

Therefore for kexec runtime we need:

1,5 GB (tmpfs) + 1,5 GB (kexec malloc) + 1,5 GB (kernel memory) = 4,5 GB

This patch introduces slurp_file_mmap() which for "normal" files uses mmap() instead of malloc()/read(). This reduces the runtime memory consumption of the kexec tool as follows:

1,5 GB (tmpfs) + 1,5 GB (kernel memory) = 3 GB

Signed-off-by Michael Holzheu <[email protected]>

Reviewed-by Dave Young <[email protected]>

This reverts commit 7ab842d8a004f6cd75a9d7b3528e4a70819ce4ef.

using mmap by default in slurp_file cause segment fault while later reallocing dtb_buf during my arm kexec test.

Signed-off-by Dave Young <[email protected]>

---
 kexec/kexec.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

kexec/kexec.c +0 -2

@@ -26,7 +26,6 @@
 #include <stdlib.h>
 #include <errno.h>
 #include <limits.h>
-#include <sys/mman.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/reboot.h>

@@ -553,12 +552,11 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
-		buf = slurp_fd(fd, filename, size, &nread);
 	} else {

This reverts commit 7ab842d8a004f6cd75a9d7b3528e4a70819ce4ef.

using mmap by default in slurp_file cause segment fault while later reallocing dtb_buf during my arm kexec test.

Hi,

Quoted 4 lines · Dave Young

This reverts commit 7ab842d8a004f6cd75a9d7b3528e4a70819ce4ef.

using mmap by default in slurp_file cause segment fault while later reallocing dtb_buf during my arm kexec test.

Could you say more about the cause of the segfault? Where is the dtb reallocated?