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

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

[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.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 kexec/kexec.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

--- kexec-tools.orig/kexec/kexec.c
+++ kexec-tools/kexec/kexec.c
@@ -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 {
-		size = nread = stats.st_size;
-		buf = mmap(NULL, size,
-			   PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+		size = stats.st_size;
 	}
+
+	buf = slurp_fd(fd, filename, size, &nread);
 	if (!buf)
 		die("Cannot read %s", filename);
 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()"

[Michael Holzheu]

On Fri, 23 Oct 2015 11:10:00 +0800
Dave Young <dyoung@redhat.com> 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()?
- ...

Michael


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()"

[Simon Horman]

On Fri, Oct 23, 2015 at 11:10:00AM +0800, Dave Young 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.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>

Thanks, applied.

> ---
>  kexec/kexec.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> --- kexec-tools.orig/kexec/kexec.c
> +++ kexec-tools/kexec/kexec.c
> @@ -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 {
> -		size = nread = stats.st_size;
> -		buf = mmap(NULL, size,
> -			   PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> +		size = stats.st_size;
>  	}
> +
> +	buf = slurp_fd(fd, filename, size, &nread);
>  	if (!buf)
>  		die("Cannot read %s", filename);
>  
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()"

[Dave Young]

Hi, Michael

On 10/23/15 at 05:09pm, Michael Holzheu wrote:
> On Fri, 23 Oct 2015 11:10:00 +0800
> Dave Young <dyoung@redhat.com> 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.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Michael Holzheu]

On Mon, 26 Oct 2015 15:31:39 +0800
Dave Young <dyoung@redhat.com> wrote:

[snip]

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

Ok, what about the following patch:
---
[PATCH] kexec/s390x: use mmap instead of read for slurp_file()

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

Therefore use mmap() to reduce the runtime memory consumption of the kexec tool.

The following use case illustrates the usefulness of this patch a bit more:

 1) Boot a 4 GB Linux system
 2) Read kernel and 1,5 GB ramdisk from external source into local
    tmpfs (ram)
 3) kexec the kernel and ramdisk

 Without this patch for the kexec runtime we need:

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

Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
---
 kexec/arch/s390/kexec-image.c |    2 +-
 kexec/kexec.c                 |   40 ++++++++++++++++++++++++++++++++++------
 kexec/kexec.h                 |    1 +
 3 files changed, 36 insertions(+), 7 deletions(-)

--- a/kexec/arch/s390/kexec-image.c
+++ b/kexec/arch/s390/kexec-image.c
@@ -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;
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -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
@@ -481,13 +482,19 @@ static int add_backup_segments(struct ke
 	return 0;
 }
 
-static char *slurp_fd(int fd, const char *filename, off_t size, off_t *nread)
+static char *slurp_fd(int fd, const char *filename, off_t size, off_t *nread,
+		      int use_mmap)
 {
 	char *buf;
 	off_t progress;
 	ssize_t result;
 
-	buf = xmalloc(size);
+	if (use_mmap) {
+		buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
+			   MAP_ANON|MAP_PRIVATE, -1, 0);
+	} else {
+		buf = xmalloc(size);
+	}
 	progress = 0;
 	while (progress < size) {
 		result = read(fd, buf + progress, size - progress);
@@ -496,7 +503,10 @@ static char *slurp_fd(int fd, const char
 				continue;
 			fprintf(stderr, "Read on %s failed: %s\n", filename,
 				strerror(errno));
-			free(buf);
+			if (use_mmap)
+				munmap(buf, size);
+			else
+				free(buf);
 			close(fd);
 			return NULL;
 		}
@@ -514,7 +524,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 +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);
 	} 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);
+		}
 	}
 
-	buf = slurp_fd(fd, filename, size, &nread);
 	if (!buf)
 		die("Cannot read %s", filename);
 
@@ -567,6 +585,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. */
 
@@ -583,7 +611,7 @@ char *slurp_file_len(const char *filenam
 		return 0;
 	}
 
-	return slurp_fd(fd, filename, size, nread);
+	return slurp_fd(fd, filename, size, nread, 0);
 }
 
 char *slurp_decompress_file(const char *filename, off_t *r_size)
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
 extern void *xmalloc(size_t size);
 extern void *xrealloc(void *ptr, size_t size);
 extern char *slurp_file(const char *filename, off_t *r_size);
+extern char *slurp_file_mmap(const char *filename, off_t *r_size);
 extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
 extern char *slurp_decompress_file(const char *filename, off_t *r_size);
 extern unsigned long virt_to_phys(unsigned long addr);


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Dave Young]

Hi, Michael

> @@ -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);
>  	} 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?

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

Otherwise I'm fine with the patch.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Michael Holzheu]

On Wed, 28 Oct 2015 14:46:23 +0800
Dave Young <dyoung@redhat.com> wrote:

> Hi, Michael
> 
> > @@ -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);
> >  	} 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?
> 
> -	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().

The change in slurp_fd() to use anonymous mmap in case of use_mmap=true is
not really necessary. I did it nevertheless for consistency. This ensures
that the slrup_file_mmap() functions *always* returns mmaped memory.

Michael


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Dave Young]

On 10/28/15 at 10:57am, Michael Holzheu wrote:
> On Wed, 28 Oct 2015 14:46:23 +0800
> Dave Young <dyoung@redhat.com> wrote:
> 
> > Hi, Michael
> > 
> > > @@ -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);
> > >  	} 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?
> > 
> > -	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().

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..

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Michael Holzheu]

On Thu, 29 Oct 2015 14:37:10 +0800
Dave Young <dyoung@redhat.com> wrote:

> On 10/28/15 at 10:57am, Michael Holzheu wrote:
> > On Wed, 28 Oct 2015 14:46:23 +0800
> > Dave Young <dyoung@redhat.com> wrote:
> > 
> > > Hi, Michael
> > > 
> > > > @@ -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);
> > > >  	} 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?
> > > 
> > > -	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().
> 
> 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(-)

--- a/kexec/arch/s390/kexec-image.c
+++ b/kexec/arch/s390/kexec-image.c
@@ -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;
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -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,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. */
 
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
 extern void *xmalloc(size_t size);
 extern void *xrealloc(void *ptr, size_t size);
 extern char *slurp_file(const char *filename, off_t *r_size);
+extern char *slurp_file_mmap(const char *filename, off_t *r_size);
 extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
 extern char *slurp_decompress_file(const char *filename, off_t *r_size);
 extern unsigned long virt_to_phys(unsigned long addr);


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Dave Young]

Hi,

On 10/29/15 at 04:26pm, Michael Holzheu wrote:
> On Thu, 29 Oct 2015 14:37:10 +0800
> Dave Young <dyoung@redhat.com> wrote:
> 
> > On 10/28/15 at 10:57am, Michael Holzheu wrote:
> > > On Wed, 28 Oct 2015 14:46:23 +0800
> > > Dave Young <dyoung@redhat.com> wrote:
> > > 
> > > > Hi, Michael
> > > > 
> > > > > @@ -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);
> > > > >  	} 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?
> > > > 
> > > > -	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().
> > 
> > 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.

I understand it now. But it looks odd in code..

> 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?

Two nitpicks see the comments inline, otherwise I think it is much better to me.

> 
> Michael
> ---
>  kexec/arch/s390/kexec-image.c |    2 +-
>  kexec/kexec.c                 |   24 +++++++++++++++++++++---
>  kexec/kexec.h                 |    1 +
>  3 files changed, 23 insertions(+), 4 deletions(-)
> 
> --- a/kexec/arch/s390/kexec-image.c
> +++ b/kexec/arch/s390/kexec-image.c
> @@ -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;
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -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)

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

>  {
>  	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);

Probably map it as readonly is enough.

> +			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. */
>  
> --- a/kexec/kexec.h
> +++ b/kexec/kexec.h
> @@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
>  extern void *xmalloc(size_t size);
>  extern void *xrealloc(void *ptr, size_t size);
>  extern char *slurp_file(const char *filename, off_t *r_size);
> +extern char *slurp_file_mmap(const char *filename, off_t *r_size);
>  extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
>  extern char *slurp_decompress_file(const char *filename, off_t *r_size);
>  extern unsigned long virt_to_phys(unsigned long addr);
> 

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Michael Holzheu]

On Fri, 30 Oct 2015 10:03:22 +0800
Dave Young <dyoung@redhat.com> wrote:

> Hi,
> 

[snip]

> > --- a/kexec/arch/s390/kexec-image.c
> > +++ b/kexec/arch/s390/kexec-image.c
> > @@ -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;
> > --- a/kexec/kexec.c
> > +++ b/kexec/kexec.c
> > @@ -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)
> 
> Add a function comment about the argument use_mmap so that one knows
> that it will be not used for character devices?#

Good idea, I put comments before slurp_file() and slurp_file_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);
> 
> 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(-)

--- a/kexec/arch/s390/kexec-image.c
+++ b/kexec/arch/s390/kexec-image.c
@@ -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;
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -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. */
 
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
 extern void *xmalloc(size_t size);
 extern void *xrealloc(void *ptr, size_t size);
 extern char *slurp_file(const char *filename, off_t *r_size);
+extern char *slurp_file_mmap(const char *filename, off_t *r_size);
 extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
 extern char *slurp_decompress_file(const char *filename, off_t *r_size);
 extern unsigned long virt_to_phys(unsigned long addr);


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Dave Young]

Hi,
[snip]

> > >  		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, it looks good to me.
Reviewed-by: Dave Young <dyoung@redhat.com>

Thanks a lot.

> 
> Michael
> ---
>  kexec/arch/s390/kexec-image.c |    2 +-
>  kexec/kexec.c                 |   31 ++++++++++++++++++++++++++++---
>  kexec/kexec.h                 |    1 +
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> --- a/kexec/arch/s390/kexec-image.c
> +++ b/kexec/arch/s390/kexec-image.c
> @@ -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;
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -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. */
>  
> --- a/kexec/kexec.h
> +++ b/kexec/kexec.h
> @@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
>  extern void *xmalloc(size_t size);
>  extern void *xrealloc(void *ptr, size_t size);
>  extern char *slurp_file(const char *filename, off_t *r_size);
> +extern char *slurp_file_mmap(const char *filename, off_t *r_size);
>  extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
>  extern char *slurp_decompress_file(const char *filename, off_t *r_size);
>  extern unsigned long virt_to_phys(unsigned long addr);
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v3] kexec/s390x: use mmap instead of read for slurp_file()

[Michael Holzheu]

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 <holzheu@linux.vnet.ibm.com>
Reviewed-by: Dave Young <dyoung@redhat.com>
---
 kexec/arch/s390/kexec-image.c |    2 +-
 kexec/kexec.c                 |   31 ++++++++++++++++++++++++++++---
 kexec/kexec.h                 |    1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

--- a/kexec/arch/s390/kexec-image.c
+++ b/kexec/arch/s390/kexec-image.c
@@ -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;
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -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. for 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. */
 
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
 extern void *xmalloc(size_t size);
 extern void *xrealloc(void *ptr, size_t size);
 extern char *slurp_file(const char *filename, off_t *r_size);
+extern char *slurp_file_mmap(const char *filename, off_t *r_size);
 extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
 extern char *slurp_decompress_file(const char *filename, off_t *r_size);
 extern unsigned long virt_to_phys(unsigned long addr);


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()"

[Geoff Levand]

Hi,

On Fri, 2015-10-23 at 11:10 +0800, Dave Young 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.

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

-Geoff

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()"

[Dave Young]

Hi, Geoff

On 11/02/15 at 04:11pm, Geoff Levand wrote:
> Hi,
> 
> On Fri, 2015-10-23 at 11:10 +0800, Dave Young 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.
> 
> Could you say more about the cause of the segfault?  Where is the dtb
> reallocated?

See kexec-zImage-arm.c:
slurp_file --> setup_dtb_prop --> xrealloc

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3] kexec/s390x: use mmap instead of read for slurp_file()

[Simon Horman]

On Fri, Oct 30, 2015 at 04:02:04PM +0100, Michael Holzheu wrote:
> 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 <holzheu@linux.vnet.ibm.com>
> Reviewed-by: Dave Young <dyoung@redhat.com>

Thanks, applied.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec