* [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" @ 2015-10-23 3:10 Dave Young 2015-10-23 15:09 ` Michael Holzheu ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Dave Young @ 2015-10-23 3:10 UTC (permalink / raw) To: Simon Horman; +Cc: Michael Holzheu, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" 2015-10-23 3:10 [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" Dave Young @ 2015-10-23 15:09 ` Michael Holzheu 2015-10-26 7:31 ` Dave Young 2015-10-26 4:30 ` [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" Simon Horman 2015-11-03 0:11 ` Geoff Levand 2 siblings, 1 reply; 16+ messages in thread From: Michael Holzheu @ 2015-10-23 15:09 UTC (permalink / raw) To: Dave Young; +Cc: Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" 2015-10-23 15:09 ` Michael Holzheu @ 2015-10-26 7:31 ` Dave Young 2015-10-27 12:35 ` [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() Michael Holzheu 0 siblings, 1 reply; 16+ messages in thread From: Dave Young @ 2015-10-26 7:31 UTC (permalink / raw) To: Michael Holzheu; +Cc: Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-26 7:31 ` Dave Young @ 2015-10-27 12:35 ` Michael Holzheu 2015-10-28 6:46 ` Dave Young 0 siblings, 1 reply; 16+ messages in thread From: Michael Holzheu @ 2015-10-27 12:35 UTC (permalink / raw) To: Dave Young, Simon Horman; +Cc: stefan.roscher, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-27 12:35 ` [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() Michael Holzheu @ 2015-10-28 6:46 ` Dave Young 2015-10-28 9:57 ` Michael Holzheu 0 siblings, 1 reply; 16+ messages in thread From: Dave Young @ 2015-10-28 6:46 UTC (permalink / raw) To: Michael Holzheu; +Cc: stefan.roscher, Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-28 6:46 ` Dave Young @ 2015-10-28 9:57 ` Michael Holzheu 2015-10-29 6:37 ` Dave Young 0 siblings, 1 reply; 16+ messages in thread From: Michael Holzheu @ 2015-10-28 9:57 UTC (permalink / raw) To: Dave Young; +Cc: stefan.roscher, Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-28 9:57 ` Michael Holzheu @ 2015-10-29 6:37 ` Dave Young 2015-10-29 15:26 ` Michael Holzheu 0 siblings, 1 reply; 16+ messages in thread From: Dave Young @ 2015-10-29 6:37 UTC (permalink / raw) To: Michael Holzheu; +Cc: stefan.roscher, Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-29 6:37 ` Dave Young @ 2015-10-29 15:26 ` Michael Holzheu 2015-10-30 2:03 ` Dave Young 0 siblings, 1 reply; 16+ messages in thread From: Michael Holzheu @ 2015-10-29 15:26 UTC (permalink / raw) To: Dave Young; +Cc: stefan.roscher, Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-29 15:26 ` Michael Holzheu @ 2015-10-30 2:03 ` Dave Young 2015-10-30 10:13 ` Michael Holzheu 0 siblings, 1 reply; 16+ messages in thread From: Dave Young @ 2015-10-30 2:03 UTC (permalink / raw) To: Michael Holzheu; +Cc: stefan.roscher, Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-30 2:03 ` Dave Young @ 2015-10-30 10:13 ` Michael Holzheu 2015-10-30 13:39 ` Dave Young 0 siblings, 1 reply; 16+ messages in thread From: Michael Holzheu @ 2015-10-30 10:13 UTC (permalink / raw) To: Dave Young; +Cc: stefan.roscher, Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-30 10:13 ` Michael Holzheu @ 2015-10-30 13:39 ` Dave Young 2015-10-30 15:02 ` [PATCH v3] " Michael Holzheu 0 siblings, 1 reply; 16+ messages in thread From: Dave Young @ 2015-10-30 13:39 UTC (permalink / raw) To: Michael Holzheu; +Cc: stefan.roscher, Simon Horman, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-30 13:39 ` Dave Young @ 2015-10-30 15:02 ` Michael Holzheu 2015-11-09 1:59 ` Simon Horman 0 siblings, 1 reply; 16+ messages in thread From: Michael Holzheu @ 2015-10-30 15:02 UTC (permalink / raw) To: Simon Horman; +Cc: stefan.roscher, kexec, Dave Young 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] kexec/s390x: use mmap instead of read for slurp_file() 2015-10-30 15:02 ` [PATCH v3] " Michael Holzheu @ 2015-11-09 1:59 ` Simon Horman 0 siblings, 0 replies; 16+ messages in thread From: Simon Horman @ 2015-11-09 1:59 UTC (permalink / raw) To: Michael Holzheu; +Cc: stefan.roscher, kexec, Dave Young 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" 2015-10-23 3:10 [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" Dave Young 2015-10-23 15:09 ` Michael Holzheu @ 2015-10-26 4:30 ` Simon Horman 2015-11-03 0:11 ` Geoff Levand 2 siblings, 0 replies; 16+ messages in thread From: Simon Horman @ 2015-10-26 4:30 UTC (permalink / raw) To: Dave Young; +Cc: Michael Holzheu, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" 2015-10-23 3:10 [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" Dave Young 2015-10-23 15:09 ` Michael Holzheu 2015-10-26 4:30 ` [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" Simon Horman @ 2015-11-03 0:11 ` Geoff Levand 2015-11-03 1:15 ` Dave Young 2 siblings, 1 reply; 16+ messages in thread From: Geoff Levand @ 2015-11-03 0:11 UTC (permalink / raw) To: Dave Young, Simon Horman; +Cc: Michael Holzheu, kexec 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" 2015-11-03 0:11 ` Geoff Levand @ 2015-11-03 1:15 ` Dave Young 0 siblings, 0 replies; 16+ messages in thread From: Dave Young @ 2015-11-03 1:15 UTC (permalink / raw) To: Geoff Levand; +Cc: Simon Horman, kexec, Michael Holzheu 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-11-09 1:59 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-23 3:10 [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" Dave Young 2015-10-23 15:09 ` Michael Holzheu 2015-10-26 7:31 ` Dave Young 2015-10-27 12:35 ` [PATCH v2] kexec/s390x: use mmap instead of read for slurp_file() Michael Holzheu 2015-10-28 6:46 ` Dave Young 2015-10-28 9:57 ` Michael Holzheu 2015-10-29 6:37 ` Dave Young 2015-10-29 15:26 ` Michael Holzheu 2015-10-30 2:03 ` Dave Young 2015-10-30 10:13 ` Michael Holzheu 2015-10-30 13:39 ` Dave Young 2015-10-30 15:02 ` [PATCH v3] " Michael Holzheu 2015-11-09 1:59 ` Simon Horman 2015-10-26 4:30 ` [PATCH] Revert "kexec: use mmap instead of read for slurp_file()" Simon Horman 2015-11-03 0:11 ` Geoff Levand 2015-11-03 1:15 ` Dave Young
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.