Patchwork [RFC,7/8] exec: Don't abort when we can't allocate guest memory

login
register
mail settings
Submitter Markus Armbruster
Date June 13, 2013, 7:02 a.m.
Message ID <1371106939-6968-8-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/250992/
State New
Headers show

Comments

Markus Armbruster - June 13, 2013, 7:02 a.m.
We abort() on memory allocation failure.  abort() is appropriate for
programming errors.  Maybe most memory allocation failures are
programming errors, maybe not.  But guest memory allocation failure
isn't, and aborting when the user asks for more memory than we can
provide is not nice.  exit(1) instead, and do it in just one place, so
the error message is consistent.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c             | 15 ++++++++++++++-
 target-s390x/kvm.c |  6 +-----
 util/oslib-posix.c |  4 +---
 util/oslib-win32.c |  5 +----
 4 files changed, 17 insertions(+), 13 deletions(-)
Andreas Färber - June 13, 2013, 8:33 a.m.
Am 13.06.2013 09:02, schrieb Markus Armbruster:
> We abort() on memory allocation failure.  abort() is appropriate for
> programming errors.  Maybe most memory allocation failures are
> programming errors, maybe not.  But guest memory allocation failure
> isn't, and aborting when the user asks for more memory than we can
> provide is not nice.  exit(1) instead, and do it in just one place, so
> the error message is consistent.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Andreas Färber <afaerber@suse.de>

Thanks for looking into this!

Andreas
Richard Henderson - June 13, 2013, 4:02 p.m.
On 06/13/2013 12:02 AM, Markus Armbruster wrote:
> @@ -945,7 +952,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>  #endif
>      if (area == MAP_FAILED) {
> -        perror("file_ram_alloc: can't mmap RAM pages");
> +        no_guest_mem(block);
>          close(fd);
>          return (NULL);
>      }

Dead code after no_guest_mem.


r~
Peter Maydell - June 13, 2013, 4:21 p.m.
On 13 June 2013 08:02, Markus Armbruster <armbru@redhat.com> wrote:
>  #ifdef __linux__
>
> +static void no_guest_mem(RAMBlock *block)
> +{
> +    fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
> +            block->mr->name, strerror(errno));
> +    exit(1);
> +}

This new error message is inside an #ifdef __linux__...

> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -112,9 +112,7 @@ void *qemu_anon_ram_alloc(size_t size)
>      size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
>
>      if (ptr == MAP_FAILED) {
> -        fprintf(stderr, "Failed to allocate %zu B: %s\n",
> -                size, strerror(errno));
> -        abort();
> +        return NULL;

...but this error message which got removed is not.
Shouldn't we be reporting errors in a non-host-specific
bit of code?

thanks
-- PMM
Markus Armbruster - June 13, 2013, 5:27 p.m.
Richard Henderson <rth@twiddle.net> writes:

> On 06/13/2013 12:02 AM, Markus Armbruster wrote:
>> @@ -945,7 +952,7 @@ static void *file_ram_alloc(RAMBlock *block,
>>      area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>>  #endif
>>      if (area == MAP_FAILED) {
>> -        perror("file_ram_alloc: can't mmap RAM pages");
>> +        no_guest_mem(block);
>>          close(fd);
>>          return (NULL);
>>      }
>
> Dead code after no_guest_mem.

Right you are.

Before: if allocation obeying -mem-path fails, we retry without
-mem-path.  Some failures are silent.

After: if mmap() fails, we give up.  Functional change not mentioned in
commit message.  I'll either revert it or document it.

I'm not sure falling back to non-mem-path allocation is a good idea in
all failure cases.

Thanks!
Markus Armbruster - June 13, 2013, 5:27 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 June 2013 08:02, Markus Armbruster <armbru@redhat.com> wrote:
>>  #ifdef __linux__
>>
>> +static void no_guest_mem(RAMBlock *block)
>> +{
>> +    fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
>> +            block->mr->name, strerror(errno));
>> +    exit(1);
>> +}
>
> This new error message is inside an #ifdef __linux__...

I'm afraid this won't compile on non-linux hosts.  Will respin.  Thanks!

[...]

Patch

diff --git a/exec.c b/exec.c
index 9f0355b..7432465 100644
--- a/exec.c
+++ b/exec.c
@@ -851,6 +851,13 @@  void qemu_mutex_unlock_ramlist(void)
 
 #ifdef __linux__
 
+static void no_guest_mem(RAMBlock *block)
+{
+    fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
+            block->mr->name, strerror(errno));
+    exit(1);
+}
+
 #include <sys/vfs.h>
 
 #define HUGETLBFS_MAGIC       0x958458f6
@@ -945,7 +952,7 @@  static void *file_ram_alloc(RAMBlock *block,
     area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 #endif
     if (area == MAP_FAILED) {
-        perror("file_ram_alloc: can't mmap RAM pages");
+        no_guest_mem(block);
         close(fd);
         return (NULL);
     }
@@ -1104,6 +1111,9 @@  ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
             exit(1);
         }
         new_block->host = kvm_arch_ram_alloc(size);
+        if (!new_block->host) {
+            no_guest_mem(new_block);
+        }
         memory_try_enable_merging(new_block->host, size);
     } else {
         if (mem_path) {
@@ -1111,6 +1121,9 @@  ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
         }
         if (!new_block->host) {
             new_block->host = qemu_anon_ram_alloc(size);
+            if (!new_block->host) {
+                no_guest_mem(new_block);
+            }
             memory_try_enable_merging(new_block->host, size);
         }
     }
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index aa45e06..472a66f 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -336,11 +336,7 @@  static void *legacy_s390_mmap(void *vaddr, ram_addr_t size)
 static void *legacy_s390_alloc(ram_addr_t size)
 {
     void *mem = legacy_s390_mmap((void *) 0x800000000ULL, size);
-    if (mem == MAP_FAILED) {
-        fprintf(stderr, "Allocating RAM failed\n");
-        abort();
-    }
-    return mem;
+    return mem == MAP_FAILED ? NULL : mem;
 }
 
 int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 3dc8b1b..253bc3d 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -112,9 +112,7 @@  void *qemu_anon_ram_alloc(size_t size)
     size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
 
     if (ptr == MAP_FAILED) {
-        fprintf(stderr, "Failed to allocate %zu B: %s\n",
-                size, strerror(errno));
-        abort();
+        return NULL;
     }
 
     ptr += offset;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 961fbf5..983b7a2 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -65,10 +65,7 @@  void *qemu_anon_ram_alloc(size_t size)
     /* FIXME: this is not exactly optimal solution since VirtualAlloc
        has 64Kb granularity, but at least it guarantees us that the
        memory is page aligned. */
-    if (!size) {
-        abort();
-    }
-    ptr = qemu_oom_check(VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE));
+    ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
     trace_qemu_anon_ram_alloc(size, ptr);
     return ptr;
 }