diff mbox series

[4/5] elf2dmp: use Linux mmap with MAP_NORESERVE when possible

Message ID 20230913224657.11606-5-viktor@daynix.com
State New
Headers show
Series elf2dmp: improve Win2022, Win11 and large dumps | expand

Commit Message

Viktor Prutyanov Sept. 13, 2023, 10:46 p.m. UTC 
Glib's g_mapped_file_new maps file with PROT_READ|PROT_WRITE and
MAP_PRIVATE. This leads to premature physical memory allocation of dump
file size on Linux hosts and may fail. On Linux, mapping the file with
MAP_NORESERVE limits the allocation by available memory.

Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
---
 contrib/elf2dmp/qemu_elf.c | 66 +++++++++++++++++++++++++++++++-------
 contrib/elf2dmp/qemu_elf.h |  4 +++
 2 files changed, 58 insertions(+), 12 deletions(-)

Comments

Akihiko Odaki Sept. 14, 2023, 7:42 a.m. UTC | #1 
On 2023/09/14 7:46, Viktor Prutyanov wrote:
> Glib's g_mapped_file_new maps file with PROT_READ|PROT_WRITE and
> MAP_PRIVATE. This leads to premature physical memory allocation of dump
> file size on Linux hosts and may fail. On Linux, mapping the file with
> MAP_NORESERVE limits the allocation by available memory.
> 
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> ---
>   contrib/elf2dmp/qemu_elf.c | 66 +++++++++++++++++++++++++++++++-------
>   contrib/elf2dmp/qemu_elf.h |  4 +++
>   2 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
> index ebda60dcb8..94a8c3ad15 100644
> --- a/contrib/elf2dmp/qemu_elf.c
> +++ b/contrib/elf2dmp/qemu_elf.c
> @@ -165,10 +165,37 @@ static bool check_ehdr(QEMU_Elf *qe)
>       return true;
>   }
>   
> -int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
> +static int QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
>   {
> +#ifdef CONFIG_LINUX

Here CONFIG_LINUX is used while qemu_elf.h uses CONFIG_POSIX.
I also wonder if GLib implementation is really necessary.

> +    struct stat st;
> +
> +    printf("Using Linux's mmap\n");
> +
> +    qe->fd = open(filename, O_RDONLY, 0);
> +    if (qe->fd == -1) {
> +        eprintf("Failed to open ELF dump file \'%s\'\n", filename);
> +        return 1;
> +    }
> +
> +    if (fstat(qe->fd, &st)) {
> +        eprintf("Failed to get size of ELF dump file\n");
> +        close(qe->fd);
> +        return 1;
> +    }
> +    qe->size = st.st_size;
> +
> +    qe->map = mmap(NULL, qe->size, PROT_READ | PROT_WRITE,
> +            MAP_PRIVATE | MAP_NORESERVE, qe->fd, 0);

It should be possible to close the file immediately after mmap().
Viktor Prutyanov Sept. 15, 2023, 4:55 p.m. UTC | #2 
> On 2023/09/14 7:46, Viktor Prutyanov wrote:
> 
>> Glib's g_mapped_file_new maps file with PROT_READ|PROT_WRITE and
>> MAP_PRIVATE. This leads to premature physical memory allocation of dump
>> file size on Linux hosts and may fail. On Linux, mapping the file with
>> MAP_NORESERVE limits the allocation by available memory.
>>
>> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
>> ---
>> contrib/elf2dmp/qemu_elf.c | 66 +++++++++++++++++++++++++++++++-------
>> contrib/elf2dmp/qemu_elf.h | 4 +++
>> 2 files changed, 58 insertions(+), 12 deletions(-)
>>
>> diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
>> index ebda60dcb8..94a8c3ad15 100644
>> --- a/contrib/elf2dmp/qemu_elf.c
>> +++ b/contrib/elf2dmp/qemu_elf.c
>> @@ -165,10 +165,37 @@ static bool check_ehdr(QEMU_Elf *qe)
>> return true;
>> }
>>
>> -int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
>> +static int QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
>> {
>> +#ifdef CONFIG_LINUX
> 
> Here CONFIG_LINUX is used while qemu_elf.h uses CONFIG_POSIX.

Thank you, the right one is CONFIG_LINUX.

> I also wonder if GLib implementation is really necessary.

GLib implementation is for non-Linux OS. Some of them have mmap,
but don't have MAP_NORESERVE. Some other, such as Windows, don't have
POSIX calls at all.

> 
>> + struct stat st;
>> +
>> + printf("Using Linux's mmap\n");
>> +
>> + qe->fd = open(filename, O_RDONLY, 0);
>> + if (qe->fd == -1) {
>> + eprintf("Failed to open ELF dump file \'%s\'\n", filename);
>> + return 1;
>> + }
>> +
>> + if (fstat(qe->fd, &st)) {
>> + eprintf("Failed to get size of ELF dump file\n");
>> + close(qe->fd);
>> + return 1;
>> + }
>> + qe->size = st.st_size;
>> +
>> + qe->map = mmap(NULL, qe->size, PROT_READ | PROT_WRITE,
>> + MAP_PRIVATE | MAP_NORESERVE, qe->fd, 0);
> 
> It should be possible to close the file immediately after mmap().
diff mbox series

Patch

diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
index ebda60dcb8..94a8c3ad15 100644
--- a/contrib/elf2dmp/qemu_elf.c
+++ b/contrib/elf2dmp/qemu_elf.c
@@ -165,10 +165,37 @@  static bool check_ehdr(QEMU_Elf *qe)
     return true;
 }
 
-int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
+static int QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
 {
+#ifdef CONFIG_LINUX
+    struct stat st;
+
+    printf("Using Linux's mmap\n");
+
+    qe->fd = open(filename, O_RDONLY, 0);
+    if (qe->fd == -1) {
+        eprintf("Failed to open ELF dump file \'%s\'\n", filename);
+        return 1;
+    }
+
+    if (fstat(qe->fd, &st)) {
+        eprintf("Failed to get size of ELF dump file\n");
+        close(qe->fd);
+        return 1;
+    }
+    qe->size = st.st_size;
+
+    qe->map = mmap(NULL, qe->size, PROT_READ | PROT_WRITE,
+            MAP_PRIVATE | MAP_NORESERVE, qe->fd, 0);
+    if (qe->map == MAP_FAILED) {
+        eprintf("Failed to map ELF file\n");
+        close(qe->fd);
+        return 1;
+    }
+#else
     GError *gerr = NULL;
-    int err = 0;
+
+    printf("Using GLib's mmap\n");
 
     qe->gmf = g_mapped_file_new(filename, TRUE, &gerr);
     if (gerr) {
@@ -179,29 +206,44 @@  int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
 
     qe->map = g_mapped_file_get_contents(qe->gmf);
     qe->size = g_mapped_file_get_length(qe->gmf);
+#endif
+
+    return 0;
+}
+
+static void QEMU_Elf_unmap(QEMU_Elf *qe)
+{
+#ifdef CONFIG_LINUX
+    munmap(qe->map, qe->size);
+    close(qe->fd);
+#else
+    g_mapped_file_unref(qe->gmf);
+#endif
+}
+
+int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
+{
+    if (QEMU_Elf_map(qe, filename)) {
+        return 1;
+    }
 
     if (!check_ehdr(qe)) {
         eprintf("Input file has the wrong format\n");
-        err = 1;
-        goto out_unmap;
+        QEMU_Elf_unmap(qe);
+        return 1;
     }
 
     if (init_states(qe)) {
         eprintf("Failed to extract QEMU CPU states\n");
-        err = 1;
-        goto out_unmap;
+        QEMU_Elf_unmap(qe);
+        return 1;
     }
 
     return 0;
-
-out_unmap:
-    g_mapped_file_unref(qe->gmf);
-
-    return err;
 }
 
 void QEMU_Elf_exit(QEMU_Elf *qe)
 {
     exit_states(qe);
-    g_mapped_file_unref(qe->gmf);
+    QEMU_Elf_unmap(qe);
 }
diff --git a/contrib/elf2dmp/qemu_elf.h b/contrib/elf2dmp/qemu_elf.h
index b2f0d9cbc9..2a71beca8e 100644
--- a/contrib/elf2dmp/qemu_elf.h
+++ b/contrib/elf2dmp/qemu_elf.h
@@ -32,7 +32,11 @@  typedef struct QEMUCPUState {
 int is_system(QEMUCPUState *s);
 
 typedef struct QEMU_Elf {
+#ifdef CONFIG_POSIX
+    int fd;
+#else
     GMappedFile *gmf;
+#endif
     size_t size;
     void *map;
     QEMUCPUState **state;